-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(browser): ensure setup files are re-evaluated on each test run (fixes #8883) #8884
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(browser): ensure setup files are re-evaluated on each test run (fixes #8883) #8884
Conversation
✅ Deploy Preview for vitest-dev ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // if the mode is setup, we need to re-evaluate the setup file on each test run | ||
| if (mode === 'setup' || !hash) { | ||
| hash = Date.now().toString() | ||
| this.hashMap.set(filepath, hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we want to cache bust, is it useful to populate the hashMap when mode is setup?
| if (!hash) { | ||
|
|
||
| // if the mode is setup, we need to re-evaluate the setup file on each test run | ||
| if (mode === 'setup' || !hash) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is the right way but I tried to keep the change minimalistic.
test/browser/specs/runner.test.ts
Outdated
| const { exitCode } = await runBrowserTests({ | ||
| root: './fixtures/isolate-and-setup-file', | ||
| }) | ||
| expect(exitCode).toBe(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a check similar to line 284 that confirms the file actually did run
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ done
Oh! I didn't notice the who instances settings. Good to know 😊
Btw, expect.soft sounds like a better fit for failure analysis here. (I mean in these kind of exitCode + outputs checks)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. We usually keep the error check first instead of the exitCode for that reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
Description
Resolves #8883
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.