Open Belco90 opened 2 years ago
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).
@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.
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 😅
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?
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.
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');
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 whenwaitFor
options are provided, or there is an assertion involved.Steps to reproduce
Error output/screenshots
These are fixed as:
ESLint configuration
N/A
Rule(s) affected
prefer-find-by
Anything else?
They should be fixed as:
Do you want to submit a pull request to fix this bug?
Yes