-
Notifications
You must be signed in to change notification settings - Fork 728
docs(react-testing-library): warn about afterEach auto cleanup footgun #779
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not following how this translates to actual code.
Could you give a minimal example code of the problem and how you fixed it?
Consider this basic <App /> you are integration testing. This is a very simple, MVP example app:
// fake api call const getData = (numRecords: number) => sleep(numRecords).then(() => numRecords); const A = () => { const [content, setContent] = React.useState("A is loading"); React.useEffect(() => { getData(10).then(() => setContent("A is ready")); }, []); return <p>{content}</p>; }; const B = () => { const [content, setContent] = React.useState("B is loading"); React.useEffect(() => { getData(50).then(() => setContent("B is ready")); }, []); return <p>{content}</p>; }; export function App() { return ( <div> <A /> <B /> </div> ); }
Here's a basic test asserting that <A /> eventually renders per expectation.
import * as React from "react"; import { render, screen } from "@testing-library/react"; import { App } from "../App"; test("cleanup & act error demo", async () => { render(<App />); expect(await screen.findByText("A is ready")).toBeInTheDocument(); });
PASS src/tests/app.spec.tsx
Everything passes. Everything is fine and serene. 🟢 👼
What if that test needs async teardown? Here's a real looking patch you might apply:
diff --git a/src/__tests__/app.spec.tsx b/src/__tests__/app.spec.tsx index 4c61540..81bd140 100644 --- a/src/__tests__/app.spec.tsx +++ b/src/__tests__/app.spec.tsx @@ -1,6 +1,13 @@ import * as React from "react"; import { render, screen } from "@testing-library/react"; import { App } from "../App"; +import { sleep } from "../timing"; + +const simulateImportantTeardownWork = () => sleep(100); + +afterEach(async () => { + await simulateImportantTeardownWork(); +}); test("cleanup & act error demo", async () => { render(<App />);
What happens in the test now?
PASS src/__tests__/app.spec.tsx ●くろまる Console console.error Warning: An update to B inside a test was not wrapped in act(...). When testing, code that causes React state updates should be wrapped into act(...): act(() => { /* fire events that update state */ }); /* assert on the output */ This ensures that you're testing the behavior the user would see in the browser. Learn more at https://reactjs.org/link/wrap-tests-with-act at B (/Users/c0d01a5/src/react-rtl-integration-testing/src/App.tsx:17:39) at div at App 17 | const [content, setContent] = React.useState("B is loading"); 18 | React.useEffect(() => { > 19 | getData(50).then(() => setContent("B is ready")); | ^ 20 | }, []); 21 | return <p>{content}</p>; 22 | }; at printWarning (node_modules/.pnpm/react-dom@17.0.1_react@17.0.1/node_modules/react-dom/cjs/react-dom.development.js:67:30)
What's happened? Why did we get this error now? All we did was add an afterEach! And that in fact is problematic.
- in the first demo, RTL
cleanupran immediately. Jest froze the console and tore down the worker before additional work could be processed for react errors to surface - in the second demo, the async nature of our
afterEachallowed react to continue to process state changes, specifically outside of any RTL or explicitact()functions. oursimulateImportantTeardownWorkafterEach occurs before RTLs auto-registering afterEachcleanup
Thus, what seemed like a harmless afterEach addition, ended up being quite problematic. Even worse--this example was designed to surface the error, by ensuring that afterEach was sufficiently slow to surface the act() error. In many cases, async behaviors are mocked and fast. That means that this race condition can be subtle to surface for many peoples' code. <A /> and <B /> effects could have certainly settled in the inner waitFor(...), and no one would have been the wiser.
Order matters. What happens if we register our handler, then import RTL and implicitly setup the auto cleanup?
diff --git a/src/__tests__/app.spec.tsx b/src/__tests__/app.spec.tsx index 0864a78..033a93f 100644 --- a/src/__tests__/app.spec.tsx +++ b/src/__tests__/app.spec.tsx @@ -1,14 +1,14 @@ -import * as React from "react"; -import { render, screen } from "@testing-library/react"; -import { App } from "../App"; -import { sleep } from "../timing"; - const simulateImportantTeardownWork = () => sleep(100); afterEach(async () => { await simulateImportantTeardownWork(); }); +import * as React from "react"; +import { render, screen } from "@testing-library/react"; +import { App } from "../App"; +import { sleep } from "../timing"; + test("cleanup & act error demo", async () => { render(<App />); expect(await screen.findByText("A is ready")).toBeInTheDocument();
On test, we get:
PASS src/__tests__/app.spec.tsx
●くろまる Console
console.error
Warning: An update to B inside a test was not wrapped in act(...).
...SNIP SNIP...
console.error
Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.
...SNIP SNIP...
Warning: Can't perform a React state update on an unmounted component.
👀
This is probably the best possible outcome. RTL's magic cleanup happened immediately, just like in the very first example we ran. Unlike in the first example, though, just by means of adding some timing delay, jest did not teardown our test quickly, and we learned that we did not appropriately clean up our effects! This could be desirable or undesirable, based on your design goals. Importantly, in this final example, at least we learned about the possibility of an error in our code.
The objective of this PR it to help the community become aware of this subtle, likely-more-common-than-reported, source of bugs because of
- implicit side-effects and
- async interaction between react, jest, and RTL
We should advise to ensure that cleanup is always called immediately after the test block exits, in almost all cases. We should advise that cleanup certainly be called before any async behaviors, and ideally synchronously after test block exits. To not advise this is to allow for delayed cleanup, and implicitly promote act() errors to be emitted in code blocks well after where we even care about the react runtime.
I personally don't like the import in the middle of the file. I would recommend importing from /pure instead, importing cleanup and calling that manually where you think is appropriate.
Definitely. I didn't even see that pure was an option. I certainly wouldn't shift the imports--it was done only to demonstrate the significance of order and async afterEach impact
Anyway, integration testing is prone to regular act() warnings if this isn't considered.
Uh oh!
There was an error while loading. Please reload this page.
Problem
Our tests implement an
afterEach(async () => { /* ... */ })handler. We registered it, thinking all was well! However, jest processes afterEach handlers in reverse order of registration. That means that our integration test execute, effects could be outstanding due to interactions in the test, the test block exits, ourafterEachperhaps take a little bit of time to complete, the react effects settle from the integration test, and anact()error is emitted because cleanup() has not yet been invoked. Only until after ourafterEachpromise settles does RTLcleanup()get invoked, prompting effect teardown/cancellation, etc.We wrote code that makes it s.t.
act()errors fail our tests. With the above scenario also in play, we found our tests to have some degree of flakiness in noisy/inconsistent environments (like a busy CI agent).The implicit behavior of RTL, I found, was actually undesirable. If I had known
cleanupwas a RTL provision in play just by importing the library, perhaps I would have more rapidly identified it as a potential culprit in our failures. Generally, side-effects as imports can be risky, with general exception when you explicitly import a verb, likebabel/register, etc. I think this library should consider making this behavior explicit.I suspect other community members have periodic act() errors that they consider ghosts in the system, when perhaps they really need to look at their own
afterEach()handlers!Let's warn those users! :)