-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(runner): fast sequential task updates missing #8121
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(runner): fast sequential task updates missing #8121
Conversation
a09e008 to
2493350
Compare
2493350 to
cecaa86
Compare
hi-ogawa
left a comment
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.
Good catch! throttle implementation by me (and copilot) was too poor 😅 #7700
I just checked tanstack's throttle implementation https://github.com/TanStack/pacer/blob/0f2cd3cfc45956dc40e6b865210c47d56f5fc129/packages/pacer/src/throttler.ts#L155 and it looks like the concept is called trailing.
Though we can maybe tweak the delay, I think throttling event timing breaking users expectation like #8108 is not completely avoidable. Maybe we can mention this in the doc?
| await new Promise(resolve => setTimeout(resolve, 150)) | ||
| console.log("Test running!") |
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.
If this order is flipped, then can onUserConsoleLog often appear before onTestCaseReady?
console.log("Test running!")
await new Promise(resolve => setTimeout(resolve, 150))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.
Yeah that's mentioned on the PR description, this part:
Note:
- As
onUserConsoleLogis not throttled, it's still possible that reporters see test case's console logs beforeonTestCaseReady, if logs happen 100ms within the start.
So on the test runner thread side, the test case's console.log happens after test-prepare is called. It's reported to the main thread in different order since onUserConsoleLog is not handled by onTaskUpdate.
Description
The
sendTasksUpdateThrottledis called sequentially withsuite-prepareandtest-prepare. Thesuite-prepareis reported to main thread immediately. Thetest-preparehitsthrottle's limiter, and is ignored. It is reported to main thread on the next invokation ofsendTasksUpdateThrottled, which istest-finished. If user's test case is taking 5000ms, the reporting oftest-preparetakes 5000ms too.As solution we'll need to make sure
throttleflushes its queue even when it's not called again.Note:
onUserConsoleLogis not throttled, it's still possible that reporters see test case's console logs beforeonTestCaseReady, if logs happen 100ms within the start.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:.