-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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): fail playwright timeouts earlier than a test timeout #7565
fix(browser): fail playwright timeouts earlier than a test timeout #7565
Conversation
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
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.
@@ -91,7 +91,7 @@ export function createExpectPoll(expect: ExpectStatic): ExpectStatic['poll'] { | |||
const rejectWithCause = (cause: any) => { | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for removing ${timeout}ms
from the error message?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <hi.ogawa.zz@gmail.com>
return options_ | ||
} | ||
const currentTest = getWorkerState().current | ||
const startTime = currentTest?.result?.startTime |
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 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.yaml
unless you introduce a new test example.Tests
pnpm test:ci
.Documentation
pnpm run docs
command.Changesets
feat:
,fix:
,perf:
,docs:
, orchore:
.