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(browser): fix updating snapshot during watch mode #4867

Merged

Conversation

hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Jan 4, 2024

Description

Closes #4865

It might be a little tough to automate a test for this but I'll try something.

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 Jan 4, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

Name Link
🔨 Latest commit d55ac5e
🔍 Latest deploy log https://app.netlify.com/sites/fastidious-cascaron-4ded94/deploys/659d5b59ba0bbc0008aa607a

@hi-ogawa hi-ogawa marked this pull request as ready for review January 4, 2024 10:09

test.after(async () => {
// force exitCode mutated by vitest
process.exitCode = 0
Copy link
Member

Choose a reason for hiding this comment

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

This seems sketchy. What if exitCode was mutated by node:test? Why Vitest mutates exitCode?

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 didn't investigate much but I meant to do something similar as runVitest test util here:

exitCode = process.exitCode
process.exitCode = 0
process.exit = exit

I'll look into more detail what exactly is happening with exit code.

Copy link
Contributor Author

@hi-ogawa hi-ogawa Jan 5, 2024

Choose a reason for hiding this comment

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

I refined some assertions to be more precise about process.exitCode d2366e8

Why Vitest mutates exitCode?

Vitest sets process.exitCode = 1 whenever test failed during runtFiles:

if (hasFailed(files))
process.exitCode = 1

EDIT: I further refactored by introducing wrapWithExitCode utility ccfa85a

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Jan 5, 2024

test/browser/specs/fix-4686.test.mjs (added in #4692) became flaky on playwright/webkit. I don't see other PRs having problems, so most likely addition of new test here is affecting it. I'll debug and try something to make it less flaky.

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Jan 5, 2024

Actually I also need to be careful with automatic process.exit by webdriverio:

// TODO: right now process can only exit with timeout, if we use browser
// needs investigating
process.exit()

Test flaky etc... there are a few things to work out, so I'll put my PRs back to draft.

@hi-ogawa hi-ogawa marked this pull request as draft January 5, 2024 09:33
@hi-ogawa hi-ogawa marked this pull request as ready for review January 9, 2024 00:45
@sheremet-va
Copy link
Member

Thanks!

@sheremet-va sheremet-va merged commit 508fced into vitest-dev:main Jan 9, 2024
18 of 19 checks passed
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.

"u" command (update snapshot) doesn't work during browser pool watch mode
2 participants