testing-library/eslint-plugin-testing-library

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

Belco90 opened this issue ยท 10 comments

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

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 ๐ŸŽ‰

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.

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

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!

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

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.

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