fix: add findby variable declaration to prefer-find-by when auto fixing

[gndelia]

This PR fixes #167

The following scenario

// incorrect
const { getByText } = render(<Foo />)
await waitFor(() => getByText('foo'))

now becomes

// correct - and findByText is defined
const { getByText, findByText } = render(<Foo />)
await findByText('foo')

this should even work if they are in different levels. For instance

// incorrect
const { getByText } = render(<Foo />)
it('tests', async () => {
   await waitFor(() => getByText('foo'))
})

should be converted to

// correct - findByText is defined
// as long as the customRender is in any parent scope from the original code, this works
const { getByText, findByText } = customRender(<Foo />)
it('tests', async () => {
     await findByText('foo')
})

there are some scenarios that I assume they are out of scope, and I am seriously not sure how to fix them. For instance, if the query is passed from a function, like

function commonCode(getByText) {
  await waitFor(() => getByText(baz))
// do stuff
}

// later, in a test
commonCode(render().getByText)

or similar, I can't fix it.. but I guess that sounds like a non-realistic scenario.

Another that might be slightly more realistic (or actually, less complex) might be something like

const utils= render(baz)
const getByText = utils.getByText
await waitFor(() => getByText('foo'))

I am not fixing that either. I'd assume if they are storing the result from the render, then they are using that variable. These snippets seem mostly hacks to avoid the rule, rather than code that someone would write on its own.

and a demo gif!

(The spaces are due to other eslint rules I've configured on the project I tested this 👀 )

P.S: I'm not using the fork 🎉

[timdeschryver]

This would turn runnable code into breaking code, right?
Because findByRole isn't defined.

If that's the case I would consider this a bad experience as it could potentially break a lot of code.

[gndelia]

it would.
but it's was already hapenning before this PR. There are a couple of scenarios where the fix breaks.
This PR fixes one of them. This test you've commented is still unfixed. Not quite sure if we can't fix it, as it seems there are multiple scenarios to fix (like I've mentioned in the PR description) and the fix might become way complex. I think fixes should be for simple scenarios.

Maybe we went too far by making this rule autofixable? Not sure.

[gndelia]

In addition to that, I am not sure how often that would be a realistic scenario. I'd think that devs use destructuring or call the function from screen or the object returned from the render function. But not quite sure. I'm open to debate it.

I still want to enforce this PR does not introduce this scenario, but rather, fixes one of the many possible scenarios where findBy* method is added and might not be defined. the scenarios were there already, and the bug mentioned one of them (the one I've fixed)

[timdeschryver]

Got it, thanks for the clarification.
For the record, I'm not against it.

I do agree that most devs will use screen, especially since we're recommended it.

[Belco90]

I hope no one uses Testing Library this way to be honest 😅 I think we are fine by covering all the other cases.

[Belco90]

Nice one, thank you!

[Belco90]

I'm not a fan of doing this kind of "importing the function calculating something to check the same in the tests". This can lead to silent errors, so I prefer to hardcode or reimplement this kind of behaviors in testing side. Anyway, I think for this case it's fine.

[gndelia]

I generally, agree, but I was using that function everywhere and it was so small... I couldn't resist the temptation (?)

[Belco90]

🎉 This PR is included in version 3.3.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀