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

Unable to mock React.useReducer because of module already loaded. (in i18next react) #4970

Closed
6 tasks done
hadson172 opened this issue Jan 15, 2024 · 14 comments · Fixed by #5050
Closed
6 tasks done

Comments

@hadson172
Copy link

hadson172 commented Jan 15, 2024

Describe the bug

Hi.
Our setup for vitest looks like this:

  • vitest.setup.ts <- here we are loadining translations using i18next react

We have code which tests for components which are using React.useReducer.

We want to mock "react" module and provide implementation for React.useReducer(...) (for rest importOriginal)
Then we are getting this error - https://vitest.dev/guide/common-errors#cannot-mock-mocked-file-js-because-it-is-already-loaded

Using 'react' inside setup make it impossible to mock react modules inside any of the tests.

We can vi.resetModules() in config file or vi.hoisted(() => vi.resetModules()) inside test file, but then config loaded by i18next (all translations) are reset as well. I tried vi.domock('react') as well but does not work.

Is there any solution for this?

Reproduction

From some unknown reason this issue is not reproducible with clean repo all the time (sometimes it work sometimes this exception is thrown). I would be happy to provide more details or try to reproduce this with someone's help.

Edit: https://stackblitz.com/~/github.com/hadson172/vitest-err?startScript=test

System Info

System:
    OS: Windows 11 10.0.22621
    CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12700KF
    Memory: 2.50 GB / 15.85 GB
  Binaries:
    Node: 21.5.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.2.4 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.14.1 - ~\AppData\Local\pnpm\pnpm.EXE
  Browsers:
    Edge: Chromium (120.0.2210.133)
    Internet Explorer: 11.0.22621.1
  npmPackages:
    @vitejs/plugin-react: ^4.2.1 => 4.2.1
    @vitejs/plugin-react-swc: ^3.5.0 => 3.5.0
    vite: ^5.0.0 => 5.0.2
    vitest: ^1.2.0 => 1.2.0

Used Package Manager

npm

Validations

@hadson172
Copy link
Author

Finally I managed reproduce problem - https://stackblitz.com/~/github.com/hadson172/vitest-err?startScript=test

@sheremet-va
Copy link
Member

Finally I managed reproduce problem - stackblitz.com/~/github.com/hadson172/vitest-err?startScript=test

Your link doesn't seem to work for me. Is it a private repository?

@hadson172
Copy link
Author

@sheremet-va Its public. You can get it here - https://github.com/hadson172/vitest-err
Just run vitest to see an error

@Tohid001
Copy link

@hadson172 Hello mate. Any luck with this issue?

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Jan 21, 2024

I think the reproduction showing an error is an intended behavior of Vitest with the rational described in the error message, so that doesn't necessary serve as a bug report reproduction.

My understanding is that such use case of vi.mock('react', ...) inside a test file was no-op and silently ignored previously, so Vitest decided to throw an error explicitly #4862 to help users such as #4759

We expect such usage of vi.mock('react', ...) can be simply removed and tests behavior shouldn't change, but I suppose this bug report is claiming that this assumption was wrong. However, given reproduction doesn't demonstrate this.

It would help if someone can provide a reproduction with something like having vi.mock('lib1', ...) in a test file and vi.mock('lib2-which-uses-lib1', ...) in a setup file then vi.mock('lib1', ...) was still functional in some way.

@hadson172
Copy link
Author

hadson172 commented Jan 21, 2024

@hi-ogawa
We are in the middle of migration to vitest from jest. Not sure if the value was ignored or no, lets asume that in previous versions this mock was silently ignored. (for sure in jest for example this was working fine.)

Its not hard to imagine situation where user has to mock one of the function from module loaded in setup.js (like in my case mock useReducer to unit testing some components).

Currently we found workaround for this. In each of our test files (and there is 10+ of them) we have this few lines on the top of file:

vi.resetModules();
const useReducer = vi.hoisted(() => vi.fn())
vi.mock('react', ...
...
...
...
)
loadI18NextReactTranslations()

It works fine, it adds some overhead for these few tests.
It would be great to have vi.resetModule() function which accept single module instead of all of them, like vi.resetModule('react')

So instead of above sniped we would have

vi.resetModule('react');
const useReducer = vi.hoisted(() => vi.fn())
vi.mock('react', ...
...
...
...
)
//loadI18NextReactTranslations() <- not needed since reset only react, not loaded translations as well

When it comes to current solution. We tried to make some reusable function to not to copy-pase 10 lines of code in each to the tests, something like this:

//tests.utils.ts
const mockUseReducer = () => {
   vi.resetModules();
   const useReducer = vi.fn()
   vi.doMock('react', {.../*use reducer used here*/})
  loadI18NextReactTranslations() <- reload translations
   return useReducer
}

then in each of 10 tests files where we have to resetAllmodules and setup them again to mock react

//mytests.spec.ts
const mockedUseReducer = vi.hoisted(() => mockUseReducer())

But when calling mockUseReducer we are getting exception with some "import_v9" in the name or something like this.

It would be really appreciated if there would be some simpler/easier solusion for this case which does not put overhead of execution of setup.js again.

@hi-ogawa
Copy link
Contributor

@hadson172 Thanks for providing the context. I wasn't aware that this issue is motivated by the migration from jest. I cannot give quick suggestions to help your scenario right now but I see that there are some pain points and I appreciate your feedback on current limitation of mocking.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Jan 22, 2024

@hadson172 To follow up what Vitest can improve (or to find simpler workaround), it would be still helpful if you can provide a "reproduction" to illustrate your current workaround. From the pseudo code with loadI18NextReactTranslations alone, it's hard to guess what exactly is bothersome and what exactly could help your case.

@newlukai
Copy link

I would like to add a similar context.

In our case we want to mock the push method of react-router-dom's useHistory hook. So we have this in place:

const mockHistoryPush = vi.fn();

vi.mock('react-router-dom', async () => {
    const actual = await vi.importActual<typeof import("react-router-dom")>("react-router-dom");

    return {
        ...actual,
        useHistory: () => ({
            push: mockHistoryPush,
        })
    };
});

This worked fine so far.

Unfortunately, we now also get this "Cannot mock "react-router-dom" because it is already loaded by" message since we have some utility files also importing "react-router-dom".

Like a Router wrapping utility:

export function wrapRouter(node: React.ReactElement) {
    return <Router>{node}</Router>;
}

or the ScrollToTop hook recommended by the React Router team.

In my opinion, these utility methods/hooks are not comparable to setup files so I'm now quite clueless how to approach this scenario.

@pequeurb
Copy link

We had a similar problem after updating vitest.
I had to go over every test file we had (several hundred across different projects) to attempt to fix this problem.
While I understand the intent behind the new error, everything seemed to work fine before the update, but afterwards I had to spend days trying to figure out how to deal with this.
I really wish this error was introduced in a less "destructive" way (for lack of a better term), or at least with a way to turn it into a warning so that we wouldn't be blocked by it.

The only way that worked for me to work around this was to move many of the vi.mock we had from the test files themselves into the setupTest file.
But since I didn't want to mock them for every test, I "mocked" them to their original implementation, so that nothing else would be impacted.
Using @newlukai 's case as an example, it would look something like this:

// setupTest file
vi.mock("react-router-dom", async () => {
  const actual = await vi.importActual<typeof import("react-router-dom")>(
    "react-router-dom"
  );
  return {
    ...actual,
    useHistory: vi.fn().mockImplementation(actual.useHistory),
  };
});

And then in the test files I can import useHistory and re-mock the implementation to whatever I need it to be.
Something like this:

// unit test file
import { useHistory } from "react-router-dom";
import { Mock } from "vitest";

const mockHistoryPush = vi.fn();

(useHistory as Mock).mockReturnValue({
     push: mockHistoryPush,
 });

But this also meant that I was forced to change the vitest config settings, because if either "mockReset" or "restoreMocks" are set to true, the tests were broken again.

@sheremet-va
Copy link
Member

While I understand the intent behind the new error, everything seemed to work fine before the update, but afterwards I had to spend days trying to figure out how to deal with this.

If it worked before and doesn't work anymore, then it's a bug.

@newlukai
Copy link

Since we just stumbled upon this by accidentally updating vitest I just decided to roll back the vitest version and keep the older one running. But of course, that's not a solution.

I'm confused by the motivation to introduce this new error. From the corresponding issue report I get that mocking was silently just not applied if that module to mock was already loaded. But wouldn't that mean that this setup here:

const mockHistoryPush = vi.fn();

vi.mock('react-router-dom', async () => {
    const actual = await vi.importActual<typeof import("react-router-dom")>("react-router-dom");

    return {
        ...actual,
        useHistory: () => ({
            push: mockHistoryPush,
        })
    };
});

would result in a failure of a test which expect(mockHistoryPush).toHaveBeenCalled()? But these actually succeeded running with an earlier vitest version.

@sheremet-va
Copy link
Member

sheremet-va commented Jan 24, 2024

But wouldn't that mean that this setup here:

It's not enough to say yes or no, this is not a sufficient reproduction. The only way this would've worked when the module was already loaded is if it was imported again by modules in the test file that were not evaluated yet, but it is impossible to know when mocking because it is executed before all imports.

@newlukai
Copy link

I just gave another approach a try. That worked out for me.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants