-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flaky tests: Asynchronous queries can return unmounted elements #865
Comments
I hope I'm getting this correctly but IMO the problem here is these two lines:
I do recall something about passing an element instead of a callback to |
This is not really an issue with const content = await screen.findByText("Content");
console.log(content.outerHTML); // <span id="test-id">Content</span>
console.log(content.parentElement); // null <-- Unmounted from DOM Test with the suggested approach:
This can also be reproduced without React. Here's example of DTL. Just change the import of https://codesandbox.io/s/dom-testing-libraryissues876-2-fe3fi?file=/src/Dom.test.js:100-308
|
If const content = await screen.findByText("Content");
expect(content).toBeInTheDocument();
|
Does this still happen if you make the timeout longer than the React batch time (16ms)? |
With this setup: const TIMEOUT = 5; // Adjusting this doesnt matter
const DIFF = 5;
...
useEffect(() => {
const timer1 = setTimeout(() => setVisible(true), TIMEOUT);
const timer2 = setTimeout(() => setVisible(false), TIMEOUT + DIFF); const TIMEOUT = 5;
const DIFF = 1; // Tests: 316 failed, 684 passed, 1000 total
const DIFF = 2; // Tests: 9 failed, 991 passed, 1000 total
const DIFF = 3; // Tests: 1 failed, 999 passed, 1000 total
const DIFF = 4; // Tests: 1 failed, 999 passed, 1000 total
const DIFF = 5; // Tests: 1 failed, 999 passed, 1000 total
const DIFF = 6; // Tests: 1 failed, 999 passed, 1000 total
const DIFF = 7; // Tests: 1000 passed, 1000 total So async queries are unstable if content updates are done fast enough. As said earlier my real use case utilizes MSW behind the queries. If I'm looking at this correctly it looks like MSW is using 5ms delay by default. https://github.com/mswjs/msw/blob/9b667c74f635735e1b941baf61db9f1b77c5a5e3/src/context/delay.ts#L7-L12 But this is just my use case. I think RTL should work with any delay values. |
I had some time to debug this further. Looks like this is caused by nested awaits in react-testing-library/src/pure.js Lines 12 to 18 in a241cb8
More debugging info, narrows the issue to asyncWrapperReplace diff --git a/src/pure.js b/src/pure.js
index 75098f7..c02003f 100644
--- a/src/pure.js
+++ b/src/pure.js
@@ -5,15 +5,19 @@ import {
prettyDOM,
configure as configureDTL,
} from '@testing-library/dom'
-import act, {asyncAct} from './act-compat'
+import act from './act-compat'
import {fireEvent} from './fire-event'
+import * as testUtils from 'react-dom/test-utils'
configureDTL({
asyncWrapper: async cb => {
let result
- await asyncAct(async () => {
+ await testUtils.act(async () => {
result = await cb()
+ console.log('callback resolved', prettyDOM());
})
+
+ console.log('asyncWrapper done', prettyDOM());
return result
},
eventWrapper: cb => {
A quick fix is to resolve the outer scope immediately when callback resolves: diff --git a/src/pure.js b/src/pure.js
index 75098f7..61110d7 100644
--- a/src/pure.js
+++ b/src/pure.js
@@ -9,12 +9,13 @@ import act, {asyncAct} from './act-compat'
import {fireEvent} from './fire-event'
configureDTL({
- asyncWrapper: async cb => {
- let result
- await asyncAct(async () => {
- result = await cb()
+ asyncWrapper: cb => {
+ return new Promise(resolve => {
+ asyncAct(async () => {
+ const result = await cb()
+ resolve(result)
+ })
})
- return result
},
eventWrapper: cb => {
let result Now I can run 2000 tests with 1ms timeout difference and they all pass. There is however a new error thrown by Clearly this fix isn't perfect. I'll have to study the |
React's act will flush micro tasks once given callback resolves. In practice this means that once callback created by Repro setup in react repository with minimal git clone git@github.com:AriPerkkio/react.git
cd react
yarn # Install deps
yarn test packages/react-dom/src/__tests__/AsyncAct-test.js --watchAll I see two possible solutions for this issue: 1. Skip
|
Trying to recap: The way |
And... I've seen this one in the wild now. Basically this:
The expectation throws an error but not the findBy. |
Faced same kind of issue: |
So it is happening like this: If you have a flag with value In my case, async call is await new Promise((resolve) => setTimeout(resolve, 1000)); // 1000 is the time that i know my `msw` should be finished. At that point of delay, I'm sure that async call is finished and component is stable with no That solved my issue so far, but I lost the check for the loading indicator because I don't know how to predect the loading element. As a question, Is there aways to detect that react component has async call is going on and? Maybe a wrapper? Does |
I'm experiencing this with
|
@vadeneev and @josh-biddick, it sounds like you have run exactly into this same issue. I'm not sure about @AhmedBHameed's case. I've found one tricky way to identify such unstable tests in local environment. By running multiple CI=true npm run test & \
CI=true npm run test & \
CI=true npm run test & \
CI=true npm run test & \
CI=true npm run test & \
wait I wonder if this will be fixed by #937 in future since it's removing the |
Hello @AriPerkkio , thanks for this post, I thought I was going nuts. I've followed your advice on the parallel local tests to see which ones were failing, but how did you actually solved them? I can't seem to find a way to make them run correctly every single time, some of them keep failing I am using the same setup as you, with MSW for mocking the API. When I try to test a failing API call (e.g. 400), for some reason with the parallel tests the expectations keep on failing. As an example:
|
@alessandrocapra without seeing the whole test case + component, or a minimal repro of your specific problem it is difficult to give any accurate instructions. Based on your code snippet I'm not 100% sure this issue is its root cause. But in general: If you query elements which can change their state/attributes after some time has passed, do not make any assumptions of their state. For example, if a "Loading" text is visible only when fetch request is active, do not assume the queried element is in the DOM after You can see this in practice at https://codesandbox.io/s/dom-testing-libraryissues876-2-fe3fi?file=/src/React.test.js. render(<Component />);
const content = await screen.findByText("Content");
await waitForElementToBeRemoved(content);
// ^^ The element(s) given to waitForElementToBeRemoved are already removed.
// waitForElementToBeRemoved requires that the element(s) exist(s) before waiting for removal. By changing the render(<Component />);
const content = await screen.findByText("Content");
- await waitForElementToBeRemoved(content);
+ await waitFor(() => {
+ expect(content).not.toBeInTheDocument(); // Does not care if findByText returns element without parentElement (= unmounted DOM node)
+ });
With this parallel running I've found these flaky tests from many projects. We were simply making assertions on elements returned by asynchronous queries. By refactoring the test case a bit like shown above we were able to fix these issues. In all cases in the end we were able to run projects containing hundreds of tests parallel with 100% success. |
Is this ever going to be fixed? We just ran into the same issue. |
encounter such flaky issues with the following dependencies: issue is not easily reproducible on the local machine but prone to fail on CI/CD. since most of us run jest on CI/CD with the --silent option. I then reproduce it locally much more frequently with the --silent option (seems the output matters? 🤔️). the error message is "element could not be found in the document" which means the element is indeed found/exist but not located in the current dom tree when expecting it to be in the document. In my case I don't have to use findBy* actually, after replace findBy with getBy my issues seems fixed. |
Still encountering this issue in CI/CD |
Edit: Skip comments about debugging and jump to the root cause.
This is related to asynchronous queries (
findBy*
) but the issue is not ondom-testing-library
. See testing-library/dom-testing-library#876.@testing-library/react
version:"^11.1.0"
jest@26.6.0
jest-environment-jsdom@26.6.2
jsdom@16.4.0
Relevant code or config:
Also available in codesandbox below.
This issue comes up randomly so below we are generating 200 tests:
Above is only a minimal repro. My real use case includes querying "Loading" text which is visible while MSW is handling the request. It does not use setTimeout at all. Something like:
What you did:
Queried element with asynchronous query
findBy*
. I would expect element returned to be present in the DOM. If it wasn't the query should throw error.Passed the queried element to
waitForElementToBeRemoved
.What happened:
Element returned from
findByText
was not in DOM whenwaitForElementToBeRemoved
started handling it.Most of the time these issues come up really randomly. We might have successful builds for months on CI and then this error occurs. Triggering new build usually fixes this. 😄
Reproduction:
The codesandbox is still showing incorrect error message
Cannot read property 'parentElement' of null
but this was fixed in testing-library/dom-testing-library#871.React: https://codesandbox.io/s/dom-testing-libraryissues876-2-fe3fi?file=/src/React.test.js
DOM: https://codesandbox.io/s/dom-testing-libraryissues876-2-fe3fi?file=/src/Dom.test.js
Problem description:
Asynchronous queries can return elements which have unmounted the DOM.
I was hoping the
initialCheck
done bywaitForElementToBeRemoved
would have been the cause but it looks like theparentElement
is not even present before element is given to it.By setting the
isAsyncActSupported
tofalse
fromdist/act-compat.js
this issue disappears. However errors fill stdout but tests are passing. I have no idea what I'm doing here but this works 😃 .react-testing-library/src/act-compat.js
Line 20 in deafd51
I think
act-compact.js
is causing asynchronous queries to run additional cycles/ticks/micro-queues instead of returning the element instantly. This causes setTimeouts and/or Promises to run before the element is returned. In practice the queried element can unmount before asynchronous query is resolved.Suggested solution:
I'm not familiar with the
act
wrapper of RTL but I think the fix should be applied there.There is a workaround for this: Don't use
waitForElementToBeRemoved
.The text was updated successfully, but these errors were encountered: