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

test: simplify browser mode tests #4879

Closed
wants to merge 33 commits into from

Conversation

hi-ogawa
Copy link
Contributor

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

Description

Extracted a change from #4878

I split this test case into a separate vitest root directory and rewrote an assertion based on stdout/stderr with startVitest API, so it might be easier to debug.

I'm still not sure why the previous approach is failing in some cases in my new PRs, but I thought this simplification would make sense either way.

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 5, 2024

Deploy Preview for fastidious-cascaron-4ded94 canceled.

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

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Jan 5, 2024

Hmm, now main test runner.test.mjs is failing and the log is indicating no tests are found:

https://github.com/vitest-dev/vitest/actions/runs/7418249822/job/20185934235?pr=4879#step:11:72

@hi-ogawa hi-ogawa changed the title test: simplify browser mode custom base test test: simplify browser mode tests Jan 5, 2024
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Jan 8, 2024

After doing random attempt to simplify the minimal repro, it looks like combining diff and optimizeDeps is causing some flakiness on webkit.

Still doing a dirty debugging, but one thing I noticed is that when switching browser test directory by --root option, Vite's cache directory doesn't change, so it looks like it's causing pre-bundling whenever changing --root since old pre-bundling in test/browser/node_modules/.vite/deps is always invalidated.

This can be observed by:

# first run without --root
$ DEBUG=vite:deps pnpm test-fixtures 

# 2nd run with --root
$ DEBUG=vite:deps pnpm test-fixtures --root fixtures/custom-base

vite:deps removing old cache dir /home/hiroshi/code/others/vitest/test/browser/node_modules/.vite/deps
...

Also I finally got Playwright/Webkit setup on my PC and I confirmed the same issue happens locally as well (though it's not consistent). The error log is not consistent either, but sometimes I get this log:

---- Unhandled Errors

Vitest caught 1 unhandled error during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

--- Unhandled Rejection
TypeError: Importing a module script failed.
----

 Test Files  no tests
      Tests  no tests
     Errors  1 error
   Start at  17:38:46
   Duration  6.43s (transform 0ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 0ms)


 FAIL  Tests failed. Watching for file changes...
       press h to show help, press q to quit
 ✓ test/basic.test.ts (4)
   ✓ basic
   ✓ basic 2
   ✓ x is x
   ✓ y is x

@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Jan 8, 2024

I don't think this simplification particularly matters, so I'll close this.

This PR ended up with me investigating Webkit flakiness and I made a simpler repro in a separate PR #4908.

@hi-ogawa hi-ogawa closed this Jan 8, 2024
@hi-ogawa hi-ogawa deleted the test-browser-flaky-2 branch January 8, 2024 23:49
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.

1 participant