fix(prefer-screen-queries): false positives when using within method

[gndelia]

I am pretty new to writing/fixing ESlint rules, so I might be doing something wrong 😄

This should fix #116 so this code

import { render, within, screen } from 'react-testing-library';

// ...
render(<SomeComponent />);
const section = screen.getByTestId('some-id');

// this is fine but the rule complains about it.
const submit = within(section).getByText(/submit/i);

should now be valid

I think this scenario is still pending

const { getByText } = within(foo)
getByText(/submit/)

perhaps I could do another PR for that one. I am still new to AST so there's probably a better way to do this one.

Thank you very much!

[Belco90]

Hey Gonzalo, thanks for your PR!

The implementation and tests seem fine. You are right about the other scenario: it should cover destructuring queries from within too. I think it should be addressed here, so take your time to investigate that. There are similar behaviors around the plugin code, you can probably spot how to do something similar in other rules! If you struggle with this, let me know so I can guide you to proper way of checking that.

Additionally, there is something weird in your branch. Commits are from "unknown", and it's causing issues as travis-ci stopping pipelines because it's not secure or something. Could you take a look at that?

[gndelia]

sure, I will take a look. I will fix that problem about my user (probably a git config error 🤔 ) and I will try to address the destructuring scenario during the week. Thanks for the feedback!

[gndelia]

Fixed the author issue 😁

edit: Now that my email is properly set I'm getting the CI errors - I Will fix it. I will increase the coverage

[gndelia]

Pushed an update

the following scenario should now work

const { getByText } = within(screen.getByText('foo'));
getByText('foo');

the only pending scenario is

const myWithinVariable = within(foo)
myWithinVariable.getByText('baz')

and after that, the PR should be ready

[Belco90]

@gndelia Awesome, nice work! We are about to release a migration to TypeScript tho, maybe you prefer to wait until that's released to keep working on it. Just FYI.

[gndelia]

thanks!

I see. I will rebase my work once that one is released - I have experience working with TS so it should be straightforward to adapt my changes to it. thanks for the FYI

[Belco90]

TS version of the package moved. Let us know if you need help rebasing your changes or adapting the code to TS!

[gndelia]

thank you! I will try to do it during the week, worst-case scenario in the weekend.

[gndelia]

I added that last case - PR is ready! My only doubt is that coverage is failing and I am not sure why. It states that this line is not being covered in the tests, but that does not make sense, as the following line is... any ideas?

Thanks!

[Belco90]

Our current coverage check it's too picky, isn't it? I'd like to migrate from forced 100% coverage threshold form jest to codecov checking the current coverage isn't decreased. Meanwhile, I'm afraid we need to try to cover that line. I'll try to check it later.

However, the PR itself is looking fine so far! I'll try to do deeper review later too. Thanks.

[Belco90]

Btw I still see all the pipeline executions from this PR as "abuse detected" on travis: https://travis-ci.org/github/testing-library/eslint-plugin-testing-library/requests

This is the first time I see something like that.

[gndelia]

Btw I still see all the pipeline executions from this PR as "abuse detected" on travis: https://travis-ci.org/github/testing-library/eslint-plugin-testing-library/requests

This is the first time I see something like that.

really not sure what's that about 🤷 I sent an email to travis support to see if they can help.

As of the build, here's a screenshot of the coverage

as you can see, line 69 does not seem covered, though I don't know why 😞

[gndelia]

after talking with Travis support, the builds fail normally and my account does not appear as "abuse detected" anymore 😄

now I need to figure out why that line appear as not tested in coverage lol

[Belco90]

@gndelia Cool, I see the CI working fine now! I hope I can check the coverage threshold soon, I'll let you know when I find something.

[Belco90]

Ok, mystery solved 🔍

If you open the generated coverage html report in the browser, you can see why it's complaining about that particular line:

There is no test for checking that if statement when the condition evaluates to false. But I can't think about a particular case to test this from an ESLint rule test to be honest.

What we are gonna do instead is relaxing the threshold for branches, as I don't see the point of having 100% there for now. Could you do that for me in your PR please? You need to set branches to 90 rather than 100 in jest.config.js.

Sorry to bother you with all that coverage threshold stuff 😅 . After that should be ready to go!

[gndelia]

I see, thanks for checking !

my first thought was about that else, but I thought the following scenario

const [myVariable] = render()
myVariable.getByText(baz)

was verifying it. Now I see it doesn't.

I added another test, perhaps not so valid as within does not return an array response 😆 but it gets coverage 100% now

const [myVariable] = within()
myVariable.${queryMethod}(baz)

I'd rather keep the coverage for the rules as the maximum, as it's reading all kinds of code, so at least we should cover a wide variety of examples. Having said that, if you still consider the threshold should be lowered, I can just do a separated PR, no problem 😃

the PR now should be ready, the build passed 🎉

[Belco90]

That's even better! Thank you so much for your effort. Approving and merging 🚀

[Belco90]

🎉 This PR is included in version 3.1.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[Belco90]

@all-contributors please add @gndelia for code, tests and doc

[allcontributors]

@Belco90

I've put up a pull request to add @gndelia! 🎉