vitest icon indicating copy to clipboard operation
vitest copied to clipboard

testing-library/react: cleanup not called automatically with `isolate: false`

Open zrev2220 opened this issue 2 years ago • 10 comments

Describe the bug

Using @testing-library/react with threads set to false in my vite.config.ts, some of my tests are failing with "Found multiple elements" errors.

After some digging, I found I just had to run RTL's cleanup function manually to get things to work. However, I don't have to do this if I set threads to true. Also, on RTL's docs for cleanup, it says:

Please note that this [running cleanup] is done automatically if the testing framework you're using supports the afterEach global and it is injected to your testing environment (like mocha, Jest, and Jasmine). If not, you will need to do manual cleanups after each test.

I have globals enabled, so the conditions for running cleanup automatically should be met. 🤔 So it seems like threads: false is interfering with something, perhaps.

Here's full output from running my tests. 👇 The output shows the JSX from the several cases having run, but the element structure shouldn't all be there at the same time.

 RUN  v0.13.1 C:/[REDACTED]/vitest-rtl-cleanup-bug

 √ src/components/Counter.test.tsx (2) 395ms
 ❯ src/components/DarkMode.test.tsx (3)
   ❯ displays current dark mode (2)
     √ light
     × dark
   × toggles dark mode

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 2 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
 FAIL  src/components/DarkMode.test.tsx > displays current dark mode > dark
AssertionError: expected <div></div> to be null
 ❯ src/components/DarkMode.test.tsx:42:40
     40|     renderWithContext(<DarkMode />);
     41|     expect(screen.getByText(/dark/)).toBeInTheDocument();
     42|     expect(screen.queryByText(/light/)).toBeNull();
       |                                        ^
     43|   });
     44| });

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/2]⎯

 FAIL  src/components/DarkMode.test.tsx > toggles dark mode
TestingLibraryElementError: Found multiple elements with the text: /light/

Here are the matching elements:

Ignored nodes: comments, <script />, <style />
<div>
  🔆 It's light
</div>

Ignored nodes: comments, <script />, <style />
<div>
  🔆 It's light
</div>

(If this is intentional, then use the `*AllBy*` variant of the query (like `queryAllByText`, `getAllByText`, or `findAllByText`)).

Ignored nodes: comments, <script />, <style />
<body>
  <div>
    <div>
      <div>
        🔆 It's light
      </div>
      <button>
        Toggle
      </button>
    </div>
  </div>
  <div>
    <div>
      <div>
        🌙 It's dark
      </div>
      <button>
        Toggle
      </button>
    </div>
  </div>
  <div>
    <div>
      <div>
        🔆 It's light
      </div>
      <button>
        Toggle
      </button>
    </div>
  </div>
</body>
 ❯ Object.getElementError node_modules/@testing-library/dom/dist/config.js:38:19
 ❯ getElementError node_modules/@testing-library/dom/dist/query-helpers.js:25:35
 ❯ getMultipleElementsFoundError node_modules/@testing-library/dom/dist/query-helpers.js:29:10
 ❯ node_modules/@testing-library/dom/dist/query-helpers.js:66:13
 ❯ node_modules/@testing-library/dom/dist/query-helpers.js:111:19
 ❯ src/components/DarkMode.test.tsx:49:16
     47|   const user = userEvent.setup();
     48|   renderWithContext(<DarkMode />);
     49|   expect(screen.getByText(/light/)).toBeInTheDocument();
       |                ^
     50|   expect(screen.queryByText(/dark/)).toBeNull();
     51|

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/2]⎯

Test Files  1 failed | 1 passed (2)
     Tests  2 failed | 3 passed (5)
      Time  3.82s (in thread 419ms, 911.45%)

Reproduction

See https://github.com/zrev2220/vitest-rtl-cleanup-bug.

Tests fail when running npm test (or npm run test). If you run npm run test -- --threads=true to enable threads, the tests will succeed.

In that repo, I added code to call cleanup manually on the branch manual-cleanup. If you checkout this branch and run npm test, the tests will pass now.

System Info

System:
    OS: Windows 10 10.0.19044
    CPU: (16) x64 AMD Ryzen 7 5700U with Radeon Graphics
    Memory: 8.94 GB / 15.35 GB
  Binaries:
    Node: 16.15.0 - C:\Program Files\nodejs\node.EXE
    npm: 8.9.0 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Edge: Spartan (44.19041.1266.0), Chromium (102.0.1245.30)
    Internet Explorer: 11.0.19041.1566
  npmPackages:
    @vitejs/plugin-react: ^1.3.0 => 1.3.2
    vite: ^2.9.9 => 2.9.9
    vitest: ^0.13.1 => 0.13.1

Used Package Manager

npm

Validations

zrev2220 avatar Jun 04 '22 19:06 zrev2220

I think this is a bug with global afterEach. It was defined in the scope of Counter.test.tsx, and was called only inside. Which is strange, because defining it in setupFiles works for every suite 🤔

sheremet-va avatar Jun 05 '22 07:06 sheremet-va

Just found this same exact bug myself. If I render <div> test </div>, every test renders it another time inside a top-level <body>.

nukeop avatar Sep 05 '22 23:09 nukeop

Just found this same exact bug myself. If I render <div> test </div>, every test renders it another time inside a top-level <body>.

I think this behaviour exists in the testing-library/svelte too. I have been trying to debug this issue for my project. To me, calling cleanup inside afterEach isn't helping either.

manuganji avatar Sep 07 '22 06:09 manuganji

I'm experiencing the same thing. However, for me, it doesn't work regardless of what the value of threads is. I'm just moving from Jest → Vitest. Doing test.only() makes each failing test pass.

astoilkov avatar Oct 09 '22 13:10 astoilkov

Not working for me even if I call afterEach(cleanup) manually

gigamesh avatar Oct 13 '22 22:10 gigamesh

I faced the same issue. I can confirm that adding afterEach(cleanup) in my setupFile solved the issue

armaaar avatar Nov 04 '22 15:11 armaaar

When you disable --threads, Vitest runs all code in the same global scope. Vitest doesn't clear require and ESM cache between different tests. Clearing require cache is possible, and would fix the issue here, but the error will come up again, if testing library decides to ship ESM, so I don't like this solution, because it creates inconsistencies between CJS and ESM modules.

Vitest doesn't recommend disabling --threads, because it disables isolation with workers, so at this point this is more of a feature than a bug, and I would recommend adding a workaround to @testing-library docs.

sheremet-va avatar Jan 01 '23 14:01 sheremet-va

In my case, i was wrapping my React component with <BrowserRouter> and was failing until i changed it to <MemoryRouter> and all is working as expected.

gianatiempo avatar Apr 28 '23 23:04 gianatiempo

Just found this same exact bug myself. If I render <div> test </div>, every test renders it another time inside a top-level <body>.

I think this behaviour exists in the testing-library/svelte too. I have been trying to debug this issue for my project. To me, calling cleanup inside afterEach isn't helping either.

I am encountering this issue with Svelte as well. However, calling cleanup in afterEach does actually resolve the issue for me.

import { describe, it, expect, beforeEach, afterEach } from "vitest";
import { fireEvent, render, screen, cleanup } from "@testing-library/svelte";
import ExamplePage from "./+page.svelte";

const { getByText } = screen;

describe("Example Page", () => {
  beforeEach(() => {
    render(ExamplePage);
  });
  afterEach(() => cleanup());

  it("should say 'Hello!'", () => {
    expect(getByText("Hello!")).not.toBeNull();
  });

  it("should count up", async () => {
    expect(getByText("Count: 0")).not.toBeNull();
    await fireEvent.click(getByText("Click Me"));
    expect(getByText("Count: 1")).not.toBeNull();
    await fireEvent.click(getByText("Click Me"));
    await fireEvent.click(getByText("Click Me"));
    await fireEvent.click(getByText("Click Me"));
    await fireEvent.click(getByText("Click Me"));
    expect(getByText("Count: 5")).not.toBeNull();
  });
});

chevsunpower avatar Jul 25 '23 17:07 chevsunpower

I ran into this same issue, but adding cleanup() in afterEach() wasn't working in some cases, until I figured out that you need an adjacent afterEach() block with a call to cleanup() next to every beforeEach() block.

In other words, if you only have afterEach() with cleanup() at the top level, and you have a beforeEach()in a nested describe() block somewhere, you'll still hit this issue, until you add another afterEach() block with a cleanup() in the same scope as the nested beforeEach().

I think this points to something funky with how Vitest is handling beforeEach() / afterEach(), and probably explains why the cleanup() call made by @testing-library/react is having no effect, since it's probably done in a beforeEach() call at the highest possible scope level.

Visual example of what i mean:

describe("level 1", () => {
  beforeEach(async () => {
    render(
      // ...
    );
  });

  afterEach(() => {
    // this is necessary
    cleanup();
  });

  describe("level 2", () => {
    beforeEach(() => {
      // ...
    });

    afterEach(() => {
      // this is also necessary
      cleanup();
    });

    // ...
  });
});

ezzatron avatar Dec 12 '23 01:12 ezzatron