prefer-find-by autofix doesn't respect extra code

[Belco90]

Plugin version

v5.3.1

ESLint version

v8.14.0

Node.js version

v16.3.0

npm/yarn version

npm v8.9.0

Operating system

macOS v12.3.1

Bug description

The autofix of the rule prefer-find-by doesn't work correctly when waitFor options are provided, or there is an assertion involved.

Steps to reproduce

1. Providing waitFor options (✅ fixed):

   const button = await waitFor(() => screen.getByText('Count is: 0'), {
       timeout: 100,
     })

2. There is an assertion involved

   await waitFor(() =>
       expect(
         screen.getByRole('button', { name: 'Count is: 0' }),
       ).toBeInTheDocument(),
     )

Error output/screenshots

These are fixed as:

1. Providing waitFor options:

   const button = await screen.findByText('Count is: 0')

2. There is an assertion involved

   await screen.findByRole('button', { name: 'Count is: 0' })

ESLint configuration

N/A

Rule(s) affected

prefer-find-by

Anything else?

They should be fixed as:

1. Providing waitFor options:

   const button = await screen.findByText('Count is: 0', { timeout: 100 })

2. There is an assertion involved

     expect(
       await screen.findByRole('button', { name: 'Count is: 0' }),
     ).toBeInTheDocument(),

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

Yes

[sjarva]

I started looking into this, and... I think that this issue's options part might be unnecessary 😅 Yes, the waitFor can be passed options that are in the shape of

  options?: {
    container?: HTMLElement
    timeout?: number
    interval?: number
    onTimeout?: (error: Error) => Error
    mutationObserverOptions?: MutationObserverInit
  }

However, the different queries (ByText, ByLabelText, etc) can also be passed options, but they are of totally different shape (this varies by the query, but most common case is):

  options?: {
    exact?: boolean = true,
    normalizer?: NormalizerFn,
  }

Our unit tests for prefer-find-by already test that query options are being passed correctly after auto-fixing 🎉

[Belco90]

That's right, but this is referring to findBy* queries, which is a wrapper over waitFor util + getBy* queries. This is mentioned in the official docs (I should have linked this in the ticket description, my bad):

findBy methods are a combination of getBy queries and waitFor. They accept the waitFor options as the last argument (e.g. await screen.findByText('text', queryOptions, waitForOptions)).

So based on this, we should keep the options found in the waitFor util, and pass them as the last argument to findBy* queries. You can reproduce this with the examples I left in the description: the options disappear.

[Belco90]

Additionally, this issue describes another issue when the waitFor is wrapping an assertion. Perhaps we can split these two bugs in separate issues.

[sjarva]

Aaah, thank you for clarifying @Belco90! I learned something new today 🔖 🤗 Then this issue is totally valid, and I know how to fix the options part!

[Belco90]

@sjarva that's amazing! Thanks for your efforts!

[sjarva]

Okay, #679 fixes the part/scenario 1 of this issue. I'll continue working on the second part too, so I'm assigning this to myself.

[Belco90]

Reopening since only the first half is done (GitHub automation closed it, my bad).

[reinrl]

@sjarva Any movement on the second part? This bit me at work today - especially after I just got done convincing my team that they should be writing expect(await screen.findBy*()).toBeInTheDocument() instead of await screen.findBy*(), in part because of this suggestion. An already uphill battle became almost insurmountable when we decided to starting doing some linting of our tests using this plugin...and the automatic fix for some of our old code was changing to the way that a few of them wanted to write anyway.

[sjarva]

Thanks for asking @reinrl. Anything didn't happen before you asked, but I took some time to continue this during the weekend and I have some more questions. Pinging @Belco90 for answers and brainstorming 😅

1. Is this part of our documentation outdated? 🤔

Our rule documentation says:

Examples of correct code for this rule:
...
// using expects inside waitFor
await waitFor(() => expect(screen.getByText('bar')).toBeDisabled());
await waitFor(() => expect(getAllByText('bar')).toBeDisabled());

Should this be updated also?

2. Should the auto fixer be changed for query methods not called by screen? 💻

@reinrl's examples are about queries that have screen.queryMethod, should the auto fixer keep the expect and the assertion when the query method is just called synchronously, like this:

const { getByRole } = render();

await waitFor(() =>
    expect(
     getByRole('button', { name: 'Count is: 0' }),
    ).toBeInTheDocument(),
  );

I'm guessing that the answer to both questions is yes, but asking to prevent this discussion on the actual PR itself.

[bhr-dorian-farrimond]

I am also seeing the same issue where assertions are removed, when letting auto fix run:

From this:

		await waitFor(() =>
			expect(screen.getByTestId('some-id')).toBeInTheDocument()
		);

To this:

		await screen.findByTestId('some-id');