fix(electron): tracing with @playwright/test#31437
Merged
mxschmitt merged 3 commits intomicrosoft:mainfrom Jul 1, 2024
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
b2a2232 to
a49e958
Compare
a49e958 to
a7b0496
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Member
|
I think you are on the right track, but I don't think that is the right place for the fix. Consider a situation where tracing is started, but no trace events are emitted. As the trace file is created lazily, it won't be there when stopping the trace either. So the fix should rather remove the non-existing temporary files from the list before iterating over them in mergeTraceFiles. You can probably test it using the ttest rather than an installation test btw. |
pavelfeldman
approved these changes
Jul 1, 2024
This reverts commit 8f67ee1.
This comment has been minimized.
This comment has been minimized.
Contributor
Test results for "tests 1"8 flaky28408 passed, 655 skipped Merge workflow run. |
Contributor
Test results for "tests others"2 flaky17787 passed, 482 skipped Merge workflow run. |
Contributor
Test results for "tests 2"4 fatal errors, not part of any test 92 flaky206326 passed, 9230 skipped, 1390 did not run Merge workflow run. |
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 #31473
This regressed in e7a11c0#diff-d7c041137e763edae5c4d18bc8ded9a1ac668078e310712341c444ef3aac423b - v1.45.0.
When Electron browser context gets closed, the request object gets disposed. But tracing was never started on the Electron request instance. This throws now:
In normal PWT setup, this is not a problem, since we already start the tracing before. But since we don't check if the Tracing has been started, it throws now, that we didn't start it before.
I see two solutions:
a) We call didCreateContext in Electron, see footnote
b) just ignore the stop call, it wasn't marked with the start symbol.