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

Hooks not working when loaded from an external vitest.setup.js file #3403

Closed
6 tasks done
nicojs opened this issue May 18, 2023 · 7 comments · Fixed by #6689
Closed
6 tasks done

Hooks not working when loaded from an external vitest.setup.js file #3403

nicojs opened this issue May 18, 2023 · 7 comments · Fixed by #6689
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) pr welcome

Comments

@nicojs
Copy link
Contributor

nicojs commented May 18, 2023

Describe the bug

Hooks (i.e. beforeEach and friends) do not work as expected when loaded from an 'external' setup file.

The hooks run for the first test file but not the following. For example, when your setup file has this:

import { beforeEach } from "vitest";

console.log('setup loading...');
beforeEach((a) => {
  console.log(`Detected: ${a.meta.file.name}`);
});

Then you only see "Detected: src/math.spec.js" for the first test; it proceeds to the following test files without executing this beforeEach again.

I'm not entirely sure what "external" means here, but whenever a setup file is loaded from an 'externalize' location from vite-node, the problem occurs:
https://github.com/vitest-dev/vitest/blob/6433224c0a05d29968eead273d1c57c680034566/packages/vite-node/src/client.ts#L278-285

I can reproduce it by putting the vitest.setup.js file inside a sibling directory. Please take a look at the reproduction instructions below.

Reproduction

external-setup.zip

  1. Unzip this file unzip external-setup.zip -d external-setup.
  2. cd external-setup
  3. npm install
  4. cd project
  5. npx vitest run.

You see this output:

 RUN  v0.31.1 /home/nicojs/github/tmp/project

stdout | unknown test
setup external...

stdout | src/math.spec.js > add > should add 40, 2 = 42
From external: src/math.spec.js

 ✓ src/math.spec.js (1)
 ✓ src/min.spec.js (1)

 Test Files  2 passed (2)
      Tests  2 passed (2)
   Start at  23:56:41
   Duration  205ms (transform 21ms, setup 6ms, collect 13ms, tests 2ms, environment 0ms, prepare 55ms)

As you can see, the beforeEach hook is only executed for math.spec.js, not min.spec.js.

System Info

System:
    OS: Linux 5.15 Ubuntu 20.04.5 LTS (Focal Fossa)
    CPU: (16) x64 11th Gen Intel(R) Core(TM) i7-11850H @ 2.50GHz
    Memory: 13.05 GB / 15.49 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 18.12.1 - ~/.nvm/versions/node/v18.12.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.12.1/bin/yarn
    npm: 8.19.2 - ~/.nvm/versions/node/v18.12.1/bin/npm
  Browsers:
    Chrome: 110.0.5481.77

Used Package Manager

npm

Validations

@sheremet-va
Copy link
Member

sheremet-va commented May 19, 2023

Without looking at the reproduction yet, it doesn't sound like it's a bug. Externalized dependencies are imported by Node.js and cached by Node.js.

When second test starts, Vitest imports setup files but since it's already evaluated, Node doesn't run it.

There is no way to clear ESM cache in Node.js.

This can only happen when tests are running in a single context where cache is shared between tests.

You can bypass it by specifying setup file in deps.inline.

@nicojs
Copy link
Contributor Author

nicojs commented May 19, 2023

Thanks for your reaction again @sheremet-va

Without looking at the reproduction yet, it doesn't sound like it's a bug.

The fact that the file is only loaded once shouldn't mean that beforeEach and friends aren't executed for subsequent files, right? That's a strange side effect.

You can bypass it by specifying setup file in deps.inline.

Interesting... this does seem to work, but only when I put it as a regex:

export default defineConfig({
  test: {
    // ...
    deps: { inline: [/vitest.setup.js/] }
  },
})

The full file path or relative file path doesn't seem to work: '/home/nicojs/github/tmp/dist/vitest.setup.js' or 'vitest.setup.js'. Any idea why?

@sheremet-va
Copy link
Member

The fact that the file is only loaded once shouldn't mean that beforeEach and friends aren't executed for subsequent files, right? That's a strange side effect.

All hooks are removed between each test file.

The full file path or relative file path doesn't seem to work: '/home/nicojs/github/tmp/dist/vitest.setup.js' or 'vitest.setup.js'. Any idea why?

The string is the dependency name. Regexp is checked against the full file path.

@nicojs
Copy link
Contributor Author

nicojs commented May 19, 2023

All hooks are removed between each test file.

So you disagree that this is a bug?

The string is the dependency name. Regexp is checked against the full file path.

What would the dependency name be for this file?

I just yesterday realized that the afterAll hook is executed per file. It makes sense after reading to your answers here. Is there a way to detect that it is the last afterAll? Like afterAllInContext or something?

@sheremet-va
Copy link
Member

Like afterAllInContext or something?

No, there is no way to do that when you run tests in a single thread. The fact that you run all test files in the same global context is some kind of workaround and there is no abstraction for it. The highest abstraction we operate upon is the test file. You can also use globalSetup which runs before every test, and its teardown runs when all tests are completed, but it is running in a different global context.

What would the dependency name be for this file?

I don't know 🤷🏻 When a string is received, deps.internal basically checks /node_modules/${string}.

@nicojs
Copy link
Contributor Author

nicojs commented May 19, 2023

I don't know 🤷🏻 When a string is received, deps.internal basically checks /node_modules/${string}.

Right. This workaround works for us: we're using new RegExp(escapeRegExp(vitestSetup)). I will also add this to the e2e test I've discussed at #3017 (reply in thread)

I personally think that "external" vs. not-external is not something users should be aware of when configuring a setup file. So I would like to keep this issue open. If you disagree, feel free to close.

@sheremet-va sheremet-va closed this as not planned Won't fix, can't repro, duplicate, stale Oct 1, 2024
@sheremet-va
Copy link
Member

I think we should always internalize the setup file.

@sheremet-va sheremet-va reopened this Oct 1, 2024
@sheremet-va sheremet-va added pr welcome p3-minor-bug An edge case that only affects very specific usage (priority) labels Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority) pr welcome
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants