Shouldn't prefer-screen-queries takes into account querySelector?

[lourenci]

prefer-screen-queries is not taken into account that the user can do this:

const utils = render(<Component />);
utils.container.querySelector('div');

This kind of selector can lead the user to a bad pattern of tests according to the testing-library.

I don't understand eslint under the hood, but I propose to change the prefer-screen-queries to take the snippet above into account as a bad pattern.

What do you guys think about that?

[gndelia]

Hi! Thanks for opening this issue 😃

This rule is supposed to enforce the usage of screen over values returned from render. Nothing says about using or not container and querySelector, or other properties the library exposes. I think that should be covered in another rule. Partially related ticket should be #62

Having said that, this rule should fail because it's using the returned value from a render function.

In that sense, I think

// **incorrect**
const utils = render(<Component />);
utils.container.querySelector('div');

// **correct**
render(<Component />);
screen.container.querySelector('div');

Does that make sense?

So I would consider this a bug, as a scenario of not correctly detecting the lack of usage of screen. For disabling the usage of DOM selectors, we should probably go for another rule. I think that one will be an important addition of this plugin

[timdeschryver]

I think this can be taken broader, and that we can add a new rule that prohibits the usage of querySelector (from screen or render).

[lourenci]

I got it and it makes sense for me, though I think we should make this broader and force the user to use the best-practices selectors somehow.

Can I take a look at this bug?

Thanks for that.

[lourenci]

I can't simulate this bug anymore. Probably, the test I was running earlier was not using the screen.container.querySelector.

I'll close this issue, though I think we should make this broader.

Sorry for that.

[gndelia]

Hey, @lourenci , I'm reopening your issue.

despite the discussion about whether we should enforce or not using the querySelector and all that, this code in your first comment

const utils = render(<Component />);
utils.container.querySelector('div');

is an invalid scenario.

This rule (prefer-screen-queries) should detect that screen is not used and enforce it to fail, as the correct scenario should be

render(<Component />);
screen.container.querySelector('div');

Then, another rule should probably enforce whether to use or not container.querySelector - but this probably is a bug of prefer-screen-queries

Good find btw 😃

If you like, feel free to take a look at this bug and open a PR.

[lourenci]

Yes, there is a bug. I made some mistake earlier trying to reproduce this, I can reproduce now.

I'll try to solve this.

[Belco90]

Thanks for the PR! Agree with @gndelia we should disallow container or querySelector in a different rule (we had some discussion around it in #62)

[gndelia]

Continuing with the discussion of this bug, we should also consider (besides container and debug) the other properties that are available in RenderResult https://github.com/testing-library/react-testing-library/blob/master/types/index.d.ts#L9

[Belco90]

🎉 This issue has been resolved in version 3.2.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[gndelia]

Reverted due to container not being exposed in screen object as we originally thought - so this one

const utils = render(<Component />);
utils.container.querySelector('div');

is a valid scenario now (will make another PR to add it as a valid one)