fix(test-runner): report Worker Cleanup step for passing tests#39746
Closed
vazidmansuri005 wants to merge 1 commit intomicrosoft:mainfrom
Closed
fix(test-runner): report Worker Cleanup step for passing tests#39746vazidmansuri005 wants to merge 1 commit intomicrosoft:mainfrom
vazidmansuri005 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
Previously, the Worker Cleanup step (which includes worker-scoped fixture teardown timing) was only reported when a test failed. This made it impossible for custom reporters to identify slow worker fixture teardowns in successful runs. The fix extends the condition that triggers the Worker Cleanup step to also run when the current test is the last in its group (!nextTest), not just when the worker is stopping due to failure. This ensures cleanup timing data is always available in result.steps via the onTestEnd reporter callback. Fixes microsoft#38350
838b57f to
7180702
Compare
Member
|
Looks like AI generated blob |
Contributor
Author
Hi @pavelfeldman, fair point — the test file changes looked like a bulk edit and I understand why that raised a flag. The actual code change in workerMain.ts is small (extending if (this._isStopped) to if (this._isStopped || !nextTest) to trigger Worker Cleanup on the last test in a group). The Would you be open to guidance on the preferred approach here — should I resubmit with only targeted new tests and accept that existing tests need updating, or is there a better way |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #38350
Problem
The
Worker Cleanupstep (which includes fixture teardown timing) was only reported inresult.stepswhen a test failed. For passing tests, this data was omitted from theonTestEndreporter callback, making it impossible for custom reporters to identify slow fixture teardowns in successful runs.Root cause: In
workerMain.ts, the Worker Cleanup step was gated behindif (this._isStopped), which is only set totrueon test failure. For passing tests, fixture teardown happened later ingracefullyClose()using a fakeTestInfowith no-op step callbacks — so no step data was ever reported.Fix
Extended the condition from:
to:
When
nextTestisundefined(last test in the group), the Worker Cleanup step now runs and reports timing data as part of that test's result.Performance guard: Worker fixture teardown (
teardownScope('worker')) is only performed whenthis._isStoppedis true (worker is actually shutting down). When!nextTesttriggers the cleanup on a passing test, only test-scoped fixtures and afterAll hooks are processed (both no-ops at that point). This preserves worker fixture reuse across test groups — the browser is not restarted between files.Test plan
should report worker cleanup step for passing testtest-step.spec.tsto include the now-reported Worker Cleanup steptest-step.spec.tspassreporter.spec.ts(32 pass),playwright.trace.spec.ts(45 pass),playwright.connect.spec.ts(5 pass)