False positives with testing-library/prefer-presence-queries

[domarmstrong]

Plugin version

v5.0.0

ESLint version

v8.2.0

Node.js version

14.18.0

npm/yarn version

1.22.17

Operating system

Big Sur

Bug description

Generally I like this rule but we have to disable it in quite a few places due to false positives with toBeNull.

Steps to reproduce

Examples:

expect(queryTooltip(screen.getByText('Some text'))).toBeNull();

expect(within(screen.getByRole('dialog')).queryByText('Some text')).toBeNull();

expect(screen.getByText('Some text').getAttribute('disabled')).toBeNull();

Error output/screenshots

No response

ESLint configuration

    {
      files: ['src/**/*.test.[tj]s?(x)', 'src/test/**/*.[tj]s?(x)'],
      env: {
        jest: true,
        browser: true,
      },
      plugins: ['eslint-plugin-jest', 'eslint-plugin-testing-library'],
      extends: ['plugin:jest/recommended', 'plugin:testing-library/react'],
      rules: {
        // warning are under migration
        'testing-library/prefer-screen-queries': 'warn',
        'testing-library/no-render-in-setup': 'warn',
        'testing-library/render-result-naming-convention': 'warn',
        'testing-library/no-wait-for-multiple-assertions': 'warn',
        'testing-library/no-node-access': 'off'
      },
    },

Rule(s) affected

testing-library/prefer-presence-queries

Anything else?

No response

Do you want to submit a pull request to fix this bug?

No

[Belco90]

Thanks for reporting this @domarmstrong. Interesting issue, it seems it's reporting false positives when the Testing Library query is nested into something else. We will have to take a look at this at some point.

[josias-r]

Another case of a false positive for await-async-query:
facebook/react#23093

@Belco90 Does this relate enough, or should I open a new issue? 🤔

[Belco90]

Hey @josias-r, this is a different problem. Could you move it to a different issue? Thanks.

[themagickoala]

I'm having the same problem as @domarmstrong - at the very least it would be nice to make this rule configurable so that we could disable the "Use queryBy*" version until this is fixed?

[Belco90]

@themagickoala Introducing some config to the rule would be nice. We would be more than happy to receive some PR to address that, or fixing the original issue!

[themagickoala]

I will try and make time for this if I can, but it's unlikely as I've come down with Covid and still having to look after my 2 kids/try and work when I can! I've done a little investigation and I'm going to make some notes here in case somebody else finds it and wants to have a go (this is from the perspective of somebody who has never written anything to do with eslint before):

1. If you want to make a rule to disable the absence query version of this, then you'd need to supply the options to the schema in the presence query rule. For an example of a rule using options you could refer to e.g. the consistent data-testid rule;
2. To properly fix this, you'd need to know whether you were using within, which the rule currently isn't checking (and for which there's no helper). However, there's another rule checking for within: the prefer screen queries rule - so you should be able to get a good idea of how to implement that from there.

Like I said, I'll try and make time for this, but it's unlikely to be for a few weeks if that.

[themagickoala]

I've had a quick stab at the config version (part 1 above) here: #557

[Belco90]

@themagickoala thanks for your contribution! I'll take a look at the PR in a bit, then we can take care of fixing the actual bug around within usage.

[themagickoala]

@Belco90 has any work been done to look into fixing the false positive here? Just wanted to check in on it all!

[Belco90]

I'm afraid not 😞. Hopefully, I can take a look during this month.

[sjarva]

@Belco90: I started looking into this issue, and there's something wrong with getDeepestIdentifierNode function, either the JS Doc comment is wrong, or the implementation is buggy.

For example...

// This is just pseudo code, I'm getting this result from running rule unit tests and printing out this stuff
const a = rtl.within('foo').getByRole('button')
const rtlNode = ... // get this somehow
getDeepestIdentifierNode(rtlNode).name // returns 'rtl', not 'getRoleButton' as the JS Doc says

Any ideas what is happening?
And, since the node-utils functions aren't unit tested, I was thinking would that they catch stuff like this in the future 🤔 💭

[Belco90]

@Belco90: I started looking into this issue, and there's something wrong with getDeepestIdentifierNode function, either the JS Doc comment is wrong, or the implementation is buggy.

For example...

// This is just pseudo code, I'm getting this result from running rule unit tests and printing out this stuff
const a = rtl.within('foo').getByRole('button')
const rtlNode = ... // get this somehow
getDeepestIdentifierNode(rtlNode).name // returns 'rtl', not 'getRoleButton' as the JS Doc says

Not sure if I understood your example correctly. I'm confused about the const rtlNode = ... // get this somehow line, could you elaborate on this?

The JSDoc description is correct, so you should get the identifier for the last chained node. If that's not the case, we have a bug, but I didn't fully understand your example.

since the node-utils functions aren't unit tested, I was thinking would that they catch stuff like this in the future

They are! It's difficult to spot that. If you go to the tests/create-testing-library-rule.test.ts file, you'll see tests for a fake rule. We use this as some sort of unit tests for our internal utils, mainly.

[sjarva]

Sorry about vagueness and/or unclear example. I meant that when I call getDeepestIdentifierNode(rtlNode), I get rtlNode, and not the last chained node.

The JSDoc description is correct, so you should get the identifier for the last chained node. If that's not the case, we have a bug, but I didn't fully understand your example.

So we have a bug 🐛

They are! It's difficult to spot that. If you go to the tests/create-testing-library-rule.test.ts file, you'll see tests for a fake rule. We use this as some sort of unit tests for our internal utils, mainly.

Yay for unit testing helpers! Hmmn, I think I'll need some time to dug deeper into that test file, and probably write some notes (to myself and others) how to unit test helper functions with this file. I'll ask if I get stuck 😅

[Belco90]

But what is rtlNode in your example? Is not defined, so I can't say if it's a bug or not.

[Belco90]

If you are obtaining the rtl identifier in your rtlNode variable, then yes the util will give you the node itself and it's not a bug (although it can be clarified in the JSDocs).

That util is supposed to be used with call expressions and that kind of nodes, not the identifiers themselves.

[github-actions]

🎉 This issue has been resolved in version 5.11.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 6.0.0-alpha.15 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀