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

Incorrect behavior after set injectGlobals: false in jest config #1240

Closed
krutoo opened this issue Oct 6, 2023 · 15 comments
Closed

Incorrect behavior after set injectGlobals: false in jest config #1240

krutoo opened this issue Oct 6, 2023 · 15 comments

Comments

@krutoo
Copy link

krutoo commented Oct 6, 2023

"@testing-library/react": "^14.0.0",
"jest": "^29.7.0",
"jest-environment-jsdom": "^29.7.0",

Relevant code or config:

I'm trying to switch from injected globals to import needed functions from @jest/globals

i just add insectGlobals: false in jest.config.ts

What you did:

I added needed imports to test file and tests fails with errors like:

expect(received).toHaveLength(expected)

    Expected length: 0
    Received length: 1

What happened:

errors are reproduced only if there are two or more tests

it looks like the render result is not reset and they are rendered into one document

Reproduction:

Here an repo with example of incorrect behavior

https://github.com/krutoo/testing-library-react-jest-no-inject-globals

@krutoo
Copy link
Author

krutoo commented Oct 10, 2023

if injectGlobas == undefined | true there is no problem

@krutoo
Copy link
Author

krutoo commented Oct 10, 2023

it has same behavior in Bun test

@leonadler
Copy link

leonadler commented Oct 11, 2023

I encountered the same problem with vitest (config.test.globals true/false)

This seems to solve it in vitest - please check if something similar works in Jest / Bun test:

// vitest.config.ts
export default defineConfig({
  // ...
  setupFiles: ['src/testing/setupTests.tsx')]
});
// setupTests.tsx
import { cleanup } from '@testing-library/react';
import { afterEach } from 'vitest';
afterEach(() => cleanup());

Jest has setupFiles and setupFilesAfterEnv, I'm not sure which one you would need.

Bun has bun test --preload ./setup.ts (1).


The reason this happens: with injectGlobals: false, there is no global beforeEach (duh), so react-testing-library would not know which beforeEach to call, as it is testing framework agnostic (i.e. you can use it without jest).

@krutoo
Copy link
Author

krutoo commented Oct 12, 2023

@leonadler thank you for workaround

i will try to use this solution but

i think that it is need to be fixed in testing library

@leonadler
Copy link

It can't really be, is what I'm saying - You might be using jest, or Bun (so you yourself are already using 2 test runners!), I am using vitest, someone else might be using mocha, ava, ...
React-Testing-Library would have to probe every test runner that exists, and would have to know which test runner you are using, which it can't - unless there is a beforeEach global, which you are disabling with the injectGlobals: false.

They could add a section about it in the readme, I would agree - but they can't detect your testing framework if you basically configure it to canBeDetected = false.

@MatanBobi
Copy link
Member

Hi @krutoo, thanks for opening this.
As written in the docs for cleanup:

Please note that this 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.

Since there's no afterEach, no cleanup is being called. If you believe this phrasing isn't clear enough, we're open for PR's in our docs repo.

I'm closing this one because as already written here by @leonadler, we can't have dedicated code for each test runner nor we wan't to inject an afterEach if it's not injected because that's too invasive.

@MatanBobi MatanBobi closed this as not planned Won't fix, can't repro, duplicate, stale Oct 12, 2023
@leonadler
Copy link

leonadler commented Oct 12, 2023

@MatanBobi Sorry if this entirely the wrong place to ask, but next to the code that does beforeEach, you have this part:

    // This matches the behavior of React < 18.
    let previousIsReactActEnvironment = getIsReactActEnvironment()
    beforeAll(() => {
      previousIsReactActEnvironment = getIsReactActEnvironment()
      setReactActEnvironment(true)
    })

    afterAll(() => {
      setReactActEnvironment(previousIsReactActEnvironment)
    })

Do I understand correctly that with React >= 18 this is not necessary anymore? Or do consumers with setGlobals: false have to also do this beforeAll()+afterAll() logic?

@MatanBobi
Copy link
Member

@leonadler AFAIR, IS_REACT_ACT_ENVIRONMENT wasn't available before React 18, what React did before was testing if typeof jest === 'undefined' so this code is 100% relevant for React >=18. If consumers use setGlobals: false`, they should do this on their own. I'm hesitating if we should write this in the docs too.

@leonadler
Copy link

leonadler commented Oct 12, 2023

Ok, thanks - just FYI, with TypeScript this gets a bit unwieldly and awkward, so maybe the module could export getIsReactActEnvironment, setIsReactActEnvironment? 🤔 : (I can do the PR)

// my-project/external-modules.d.ts

declare module '@testing-library/react/dist/act-compat' {
  export function getIsReactActEnvironment(): unknown;
  export function setReactActEnvironment(env: unknown): void;
}
// my-project/setup-tests.ts
import { cleanup } from '@testing-library/react';
import { getIsReactActEnvironment, setReactActEnvironment } from '@testing-library/react/dist/act-compat';
// ^ meh 🙁
import { afterAll, afterEach, beforeAll, beforeEach, vitest } from 'vitest';

let previousIsReactActEnvironment = getIsReactActEnvironment();
beforeAll(() => {
  previousIsReactActEnvironment = getIsReactActEnvironment();
  setReactActEnvironment(true);
});
afterEach(() => cleanup());
afterAll(() => {
  setReactActEnvironment(previousIsReactActEnvironment);
});

@MatanBobi
Copy link
Member

I understand.
Can you please open a feature request for this one so we'll be able to discuss it first? This is really an implementation detail that I'm not sure we want to be exported as part of our public API (or maybe not in this specific form).

@krutoo
Copy link
Author

krutoo commented Oct 12, 2023

can package log some warning message about inability to apply automatic cleanup when afterEach is undefined?

@MatanBobi
Copy link
Member

@krutoo I've created a PR to add a warning in that case.

@MatanBobi
Copy link
Member

@krutoo - This PR was reverted because it created too much problems for our users. We'll re-circle it to see if there's a different approach to do it.

@MattyBalaam
Copy link

@MatanBobi Is there a new issue to track this?

@MatanBobi
Copy link
Member

MatanBobi commented Nov 26, 2023

@MattyBalaam - I just opened one: #1257

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants