Skip to content
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

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

Closed
6 tasks done
zrev2220 opened this issue Jun 4, 2022 · 11 comments
Closed
6 tasks done
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@zrev2220
Copy link
Contributor

zrev2220 commented Jun 4, 2022

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

@sheremet-va
Copy link
Member

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 🤔

@nukeop
Copy link

nukeop commented Sep 5, 2022

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

@manuganji
Copy link

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.

@astoilkov
Copy link

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.

@gigamesh
Copy link

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

@armaaar
Copy link

armaaar commented Nov 4, 2022

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

@sheremet-va
Copy link
Member

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.

@gianatiempo
Copy link

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.

@chevsunpower
Copy link

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();
  });
});

aterstriep added a commit to CMS-Enterprise/easi-app that referenced this issue Aug 30, 2023
- Disabling threads causes tests to fail
- See vitest-dev/vitest#1430
ClayBenson94 added a commit to CMS-Enterprise/easi-app that referenced this issue Sep 11, 2023
* Add Vite and required packages

* Create vite.config.ts

* Move and update index.html

- Move index.html to root folder
- Remove %PUBLIC_URL% references
- Add Vite entry point

* Update tsconfig.json

* Create vite-env.d.ts

* Remove react-scripts

- Remove react-scripts
- Delete react-app-env.d.ts

* Update scripts in package.json

* Update env variables

* Change build output folder

* Remove craco

* Configure vite.config.ts for docker

* Updated caniuse-lite

Not necessary for vite migration, but removed warning message to update.

* Upgraded react-csv

Previous version was incompatible with Vite because Link component was .js file instead of .jsx

* Set scss load paths

* Hide uswds console notifications

* Upgrade storybook

* Upgrade Apollo

* Fix build script

* Fix Okta bug

* Fix uswds theme paths

* Enable css source maps

* Add autoprefixer

* Add autoprefixer to vite.config.ts

* Add vitest

* Snapshots

* Remove Jest canvas mock

* BusinessCaseReview test error

Error: Uncaught [TypeError: Cannot read properties of null (reading 'offsetWidth')]

* Upgrade Vite

Multiple installed versions of vite was causing type error

* Add vitest globals to tsconfig

* Add vi global to .eslintrc

* Replace jest global with vi

* Set global timezone

- Renamed jest-global-setup.js to global-setup.js
- Set timezone to UTC in global-setup.js
- Added globalSetup file to vite.config.ts

* Mock getTrbRequestDocumentsQuery

- Added mocked query to src/data/mock/trbRequest.ts
- Fixed missing mock query error in Consult.test.tsx

* Snapshots

* Added Vitest UI package

* Added vitest:ui

Opens unit tests in Vitest UI

* Removed jest-launchdarkly-references

* Fix createObjectURL JSDOM error

- Fixes error from react-csv: "TypeError: The "obj" argument must be an instance of Blob. Received an instance of Blob"
- JSDOM does not support window.URL.createObjectURL, so needed to mock in setupTests.ts

* Clay changes for vitest canvas mock, WIP, non functional

* Fix navigationBar test

* Fix setupTests typeError

* Fix Header unit test errors

- Fixed typo
- Wrapped component in Provider
- Updated to async function

* Enable threads

- Disabling threads causes tests to fail
- See vitest-dev/vitest#1430

* Fix flaky TRB Consult test

Test randomly threw error that no more mocked responses were available for getTRBRequestAttendeesQuery. Duplicating the query in the mocks array fixed this.

* Skip Tabs test

* Snapshots

* Update yarn.lock

* Update snapshot

* Remove unit tests that render Okta signin widget

* Remove vitest-canvas-mock

* Add role attribute to csv download link

byRole queries in unit tests were not recognizing csv download links as links because the href prop isn't set. Explicitly setting "role=link" solved this.

* Fix "cannot find offset width of null" error

* Fix unit test classlist argument error

* Remove vitest UI

* Fix references to REACT_APP_ env vars that have not yet been converted to use VITE_

* Enable test coverage when running yarn test:coverage with v8. See https://vitest.dev/guide/coverage.html for info

* Fix cypress plugins env variables bug

Changed import.meta.env to process.env

* Add cypress-vite

- Removed cypress/webpack-preprocessor
- Added cypress-vite

* Cypress-vite config updates

* Empty commit

* Debug file:preprocessor

* Upgrade Cypress

* Comment out tests that import app code

* Comment out missed suite with imports

* Re-introduce webpack for Cypress

* Uncomment tests

* Cleanup

- Removed commented out debug code
- Uncommented e2e test code

* Enable cypress video

* Set long timeout on Okta login

* Try clicking multiple times

* Try clicking multiple times

* Upgrade okta-signin-widget from 6 to 7

* Undo multi click in Cypress

* import css from okta widget using exported path. Add commonjsOptions to transform require to import in imported modules

* Remove extra setting of VITE_OKTA_REDIRECT_URI

* Remove references to craco, add ADR

* Remove unused packages. Add comments, remove TODO comments.

---------

Co-authored-by: ClayBenson94 <clay.benson@oddball.io>
nix6839 added a commit to nix6839/vote-it that referenced this issue Sep 22, 2023
@ezzatron
Copy link

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();
    });

    // ...
  });
});

@sheremet-va sheremet-va changed the title testing-library/react: cleanup not called automatically with threads: false testing-library/react: cleanup not called automatically with isolate: false Jan 19, 2024
@sheremet-va sheremet-va moved this to P2 - 2 in Team Board Feb 12, 2024
@sheremet-va sheremet-va added p2-edge-case Bug, but has workaround or limited in scope (priority) and removed has workaround labels Feb 16, 2024
@sheremet-va sheremet-va moved this from P2 - 2 to Rejected in Team Board Jun 6, 2024
@sheremet-va
Copy link
Member

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.

This is not true. The afterEach hook at the top level will be called for every test.


As a team, we decided that we would not try to find ways to fix this behavior in Vitest. Caching the node_modules is the expected behavior, this is why isolate: false is the fastest way to run tests.

Cached testing-library means that afterEach will be called only once for the first test file. After tests from that file are finished, Vitest resets the file suite clearing all test hooks so they don't conflict with hooks from the next test file (if we didn't have that, all top-level hooks would pile up from different files).

@sheremet-va sheremet-va closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

10 participants