Add an RFC for add_top_level_completion_handler#168
Open
gsnedders wants to merge 2 commits intoweb-platform-tests:mainfrom
Open
Add an RFC for add_top_level_completion_handler#168gsnedders wants to merge 2 commits intoweb-platform-tests:mainfrom
add_top_level_completion_handler#168gsnedders wants to merge 2 commits intoweb-platform-tests:mainfrom
Conversation
webkit-commit-queue
pushed a commit
to gsnedders/WebKit
that referenced
this pull request
Oct 18, 2023
… handler https://bugs.webkit.org/show_bug.cgi?id=259409 rdar://problem/112683033 Reviewed by Jonathan Bedard. Previously, we were considering the test as having to run to completion when any testharness.js completion handler first ran (which was sometimes that of the frame within the opened window; the nondeterminism here just adds flakiness to the already bad behaviour). Instead, only output anything for the top-level completion handler, as all results should be passed up to it. Note wptrunner with the WebDriver or Marionette executors only ever pays attention to the top-level completion handler (as they only pay attention to the top-level frame & window), thus they don't have any flakiness like this. Ideally testharness.js would have an API we could use for this; I've filed an RFC at web-platform-tests/rfcs#168 for this, but there's no reason not to do the simple fix ourselves, as at least for now this is a strict progression. (It would stop being a strict progression if at some point testharness.js started allowing fetch_tests_from_window() with a window with noopener.) It seems likely we have other tests that are marked as flaky because of it, but alas there's no easy way to find what tests will have been fixed by this. * LayoutTests/TestExpectations: * LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https-expected.txt: * LayoutTests/http/tests/security/contentSecurityPolicy/upgrade-insecure-requests/basic-upgrade-cors.https.html: * LayoutTests/imported/w3c/web-platform-tests/cookies/partitioned-cookies/partitioned-cookies.tentative.https-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/html/browsers/browsing-the-web/navigating-across-documents/source/navigate-child-function-parent-then-fragment-expected.txt: * LayoutTests/platform/wk2/TestExpectations: * LayoutTests/resources/testharnessreport.js: Canonical link: https://commits.webkit.org/269483@main
Member
Author
…and we've had no substantive disagreement in two weeks, so I guess that means this is accepted despite nobody having reviewed it? 🤨 |
Ms2ger
approved these changes
Oct 30, 2023
Contributor
Ms2ger
left a comment
Ms2ger
left a comment
There was a problem hiding this comment.
No obvious objections from my side
Co-authored-by: Ms2ger <Ms2ger@gmail.com>
Member
Author
Contributor
|
This was discussed during the November 2023 meeting: This RFC may need some updates |
Contributor
|
Per the discussion in https://github.com/web-platform-tests/wpt-notes/blob/47c3ea1a941c0b0f78869ff2d00a2e2f93594636/minutes/2023-11-07.md?plain=1#L24 we might want to try a different implementation approach. We likely need to try with existing tests to see which approach is most robust. |
Member
Author
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
4 participants
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.
Rendered