fix(browser): fail playwright timeouts earlier than a test timeout#7565
fix(browser): fail playwright timeouts earlier than a test timeout#7565sheremet-va merged 14 commits intovitest-dev:mainfrom
Conversation
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
hi-ogawa
left a comment
There was a problem hiding this comment.
Looks good! I was worrying about test.concurrent breaking getWorkerState().current state, but it's probably fine since people wouldn't use concurrent tests with dom interaction/assertions.
| reject( | ||
| copyStackTrace( | ||
| new Error(`Matcher did not succeed in ${timeout}ms`, { | ||
| new Error('Matcher did not succeed in time.', { |
There was a problem hiding this comment.
Is there any reason for removing ${timeout}ms from the error message?
There was a problem hiding this comment.
mostly because the new timeout value is now irrelevant - it's set by Vitest and equal to basically random numbers (to the user) - like 3542ms, 2323ms. I don't think they are worth printing out and just confusing
There was a problem hiding this comment.
Ah, I didn't realize expect.element is doing processTimeoutOptions. It looks good to me 👍
If I remember correctly, playwright doesn't change the error message when they patch timeout, so they are lying 😄
Co-authored-by: Hiroshi Ogawa <[email protected]>
| return options_ | ||
| } | ||
| const currentTest = getWorkerState().current | ||
| const startTime = currentTest?.result?.startTime |
There was a problem hiding this comment.
This might not be a good idea. The current will return the test even if it runs beforeEach and afterEach, and the timeout there must be different!
Description
Closes #7309
Related #7559
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:.