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

fix(vitest): invalidate setupFiles with extension-less path #6749

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Oct 20, 2024

Description

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Oct 20, 2024

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 6d64553
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/6714c7775cdbf40008f10e7d
😎 Deploy Preview https://deploy-preview-6749--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hi-ogawa hi-ogawa marked this pull request as ready for review October 20, 2024 09:17
if (source === 'setup') {
getWorkerState().moduleCache.delete(filepath)
const resolved = await this.__vitest_executor.resolveUrl(filepath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be resolved here even without the extension?

resolved.setupFiles = toArray(resolved.setupFiles || []).map(file =>

This worked before without problems

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine it's working for most of the case, but the issue was when it's extension-less relative path. For example, setupFiles: ['./setup.ts'] works but setupFiles: ['./setup'] fails, which was OP's repro.

Alternatively, we can use Vite server resolver during resolveConfig or sometime later if we restructure a bit, but I thought there might be a reason not to bring that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am saying that it shouldn't fail even without the extension

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should fix it for sure, but I don't think this was a regression and extension-less relative path has never been handled with proper invalidation. I'll think about the solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any way, I don't think the current fix in this PR is the correct one. In the very least it seems weird to me that it processes the same setup file in every test (which means it processes the same value multiple times). The setup file ID should be resolved already at the config stage. The resolvePath in resolveConfig is supposed to be able to resolve any JS paths even without an extension as far as I understand - why doesn't it? 🤔

@hi-ogawa hi-ogawa changed the title fix(vitest): resolve setupFiles before invalidate fix(vitest): invalidate setupFiles with extension-less path Oct 21, 2024
@hi-ogawa hi-ogawa marked this pull request as draft October 26, 2024 00:56
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

Successfully merging this pull request may close these issues.

No stdout from beforeEach/afterEach when failing test and --no-isolate --no-file-parallelism
3 participants