docs(react-testing-library): warn about afterEach auto cleanup footgun

[cdaringe]

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! :)

[eps1lon]

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?

[cdaringe]

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 ***@***.***> wrote: ***@***.**** 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 <#779 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHU57JVWNI6VHKZGT6HAIDTCNCYFANCNFSM4YXML2JA> .

[cdaringe]

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 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 explicit act() functions. our simulateImportantTeardownWork afterEach occurs before RTLs auto-registering afterEach cleanup

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.

[eps1lon]

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.

[cdaringe]

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

[cdaringe]

Anyway, integration testing is prone to regular act() warnings if this isn't considered.