Skip to content

Comments

fix(browser): fix updating snapshot during watch mode#4867

Merged
sheremet-va merged 17 commits intovitest-dev:mainfrom
hi-ogawa:fix-browser-pool-watch-update-snapshot
Jan 9, 2024
Merged

fix(browser): fix updating snapshot during watch mode#4867
sheremet-va merged 17 commits intovitest-dev:mainfrom
hi-ogawa:fix-browser-pool-watch-update-snapshot

Conversation

@hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Jan 4, 2024 •
edited
Loading

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:.

@netlify
Copy link

netlify bot commented Jan 4, 2024 •
edited
Loading

✅ 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
hi-ogawa
hi-ogawa commented Jan 4, 2024
View reviewed changes
sheremet-va
sheremet-va reviewed Jan 4, 2024
View reviewed changes
test/browser/specs/update-snapshot.test.mjs Outdated

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

@sheremet-va sheremet-va Jan 4, 2024 •
edited
Loading

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

@hi-ogawa hi-ogawa Jan 4, 2024

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:

vitest/test/test-utils/index.ts

Lines 40 to 42 in 044c738

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 •
edited
Loading

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:

vitest/packages/vitest/src/node/core.ts

Lines 505 to 506 in d2366e8

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 hi-ogawa mentioned this pull request Jan 5, 2024
Closed
6 tasks
@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:

vitest/packages/browser/src/node/providers/webdriver.ts

Lines 93 to 95 in 3d18efd

// 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
@motss motss mentioned this pull request Jan 6, 2024
Closed
6 tasks
@hi-ogawa hi-ogawa marked this pull request as ready for review January 9, 2024 00:45
@sheremet-va
Copy link
Member

sheremet-va commented Jan 9, 2024

Thanks!

@sheremet-va sheremet-va merged commit 508fced into vitest-dev:main Jan 9, 2024
renovate bot referenced this pull request in JoshuaKGoldberg/sinon-timers-repeatable Feb 27, 2024
@renovate
chore(deps): update dependency vitest to v1.3.1 (#180