testing-library-docs
testing-library-docs copied to clipboard
docs(react-testing-library): warn about afterEach auto cleanup footgun
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, our afterEach
perhaps take a little bit of time to complete, the react effects settle from the integration test, and an act()
error is emitted because cleanup() has not yet been invoked. Only until after our afterEach
promise settles does RTL cleanup()
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 cleanup
was 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, like babel/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! :)
Sure, I'll send one later.I have an easy repro. The tldr is,
Test a component with >1 asynchronous use effect/setState calls (very common).
Run test, test passes.
Add an afterEach with async behavior.
Observe act errors. afterEach async calls are not protected by RTLs internal act wrapping. RTLs auto cleanup is not invoked until after my afterEach
On Sun, Mar 7, 2021, 1:40 AM Sebastian Silbermann [email protected] wrote:
@eps1lon commented on this pull request.
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?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/testing-library/testing-library-docs/pull/779#pullrequestreview-605826884, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHU57JVWNI6VHKZGT6HAIDTCNCYFANCNFSM4YXML2JA .
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/[email protected][email protected]/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
cleanup
ran 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
afterEach
allowed react to continue to process state changes, specifically outside of any RTL or explicitact()
functions. oursimulateImportantTeardownWork
afterEach 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.