-
Notifications
You must be signed in to change notification settings - Fork 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
Remove stashed request headers for speculative parser tests #30891
Conversation
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.
The review process for this patch is being conducted in the Chromium project.
c8b804c
to
5f4ca7a
Compare
The old code would append all of the request headers for the request to the stashed value. That causes the failure string for all dependent tests to change when, e.g. the User-Agent string changes due to the test being run on a different bot. This CL removes the headers, as those seem only useful for debugging, and are causing lots of flaky behavior on at least Chromium bots. Prior failure message: FAIL Speculative parsing, document.write(): link-rel-stylesheet-disabled Unhandled rejection: assert_equals: speculative case incorrectly fetched expected "" but got "param-encodingcheck: %C4%9E\r\nHost: web-platform.test:8001\nConnection: keep-alive\nUser-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/999.77.34.5 Safari/537.36\nAccept: text/css,*/*;q=0.1\nReferer: http://web-platform.test:8001/html/syntax/speculative-parsing/generated/document-write/link-rel-stylesheet-disabled.tentative.sub.html\nAccept-Encoding: gzip, deflate\nAccept-Language: en-us,en\n\n" New failure message: FAIL Speculative parsing, document.write(): link-rel-stylesheet-disabled Unhandled rejection: assert_equals: speculative case incorrectly fetched expected "" but got "param-encodingcheck: %C4%9E\r\n" Fixed: 1250457,1249920,1249921,1249954,1250003,1250004 Bug: 1144176 Change-Id: I45dbbb8794c6bf2d5e962cecf531e06a7af09160 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3170927 Commit-Queue: Mason Freed <masonf@chromium.org> Auto-Submit: Mason Freed <masonf@chromium.org> Reviewed-by: Joey Arhar <jarhar@chromium.org> Cr-Commit-Position: refs/heads/main@{#923491}
5f4ca7a
to
299b868
Compare
@zcorpan FYI, let me know if you object to this change or want any tweaks. It was causing all kinds of bot grief before this change. |
@mfreed7 thanks for the ping. The intent is to check that the request headers are equivalent between the speculative fetch and the normal fetch. Is there flake due to the headers being different between speculative fetches and normal fetches? Or flake due to speculative fetches sometimes happening and sometimes not happening? Maybe we don't need to check all request headers, but a few interesting ones at least (like |
If the stashed value is affecting test runs across bots, then that seems like something that shouldn't happen. The UUID is generated for each run of the test ( |
Oh wait, I see. Differences in the failure message is interpreted as flake, right? |
Right - when it fails, it (previously) spit out the entire set of request headers, which includes the User-Agent. On Chromium at least, we store away the failure message as an expectation file, and when that same test runs on multiple bots, the error messages will change and cause flakiness. If you want to compare more headers, maybe opt-in the ones you care about? |
Yes, that makes sense. I can submit a PR in a couple of weeks. |
…e HTML parser tests See #30891 (comment)
…e HTML parser tests See #30891 (comment)
…request headers in speculative HTML parser tests, a=testonly Automatic update from web-platform-tests HTML: check the Origin, Accept, Referer request headers in speculative HTML parser tests See web-platform-tests/wpt#30891 (comment) -- wpt-commits: 048f2519e6217dd715becb5655e6ad70ce75c8f1 wpt-pr: 31730
The old code would append all of the request headers for the request
to the stashed value. That causes the failure string for all dependent
tests to change when, e.g. the User-Agent string changes due to the
test being run on a different bot.
This CL removes the headers, as those seem only useful for debugging,
and are causing lots of flaky behavior on at least Chromium bots.
Prior failure message:
FAIL Speculative parsing, document.write(): link-rel-stylesheet-disabled Unhandled rejection: assert_equals: speculative case incorrectly fetched expected "" but got "param-encodingcheck: %C4%9E\r\nHost: web-platform.test:8001\nConnection: keep-alive\nUser-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/999.77.34.5 Safari/537.36\nAccept: text/css,/;q=0.1\nReferer: http://web-platform.test:8001/html/syntax/speculative-parsing/generated/document-write/link-rel-stylesheet-disabled.tentative.sub.html\nAccept-Encoding: gzip, deflate\nAccept-Language: en-us,en\n\n"
New failure message:
FAIL Speculative parsing, document.write(): link-rel-stylesheet-disabled Unhandled rejection: assert_equals: speculative case incorrectly fetched expected "" but got "param-encodingcheck: %C4%9E\r\n"
Fixed: 1250457,1249920,1249921,1249954,1250003,1250004
Bug: 1144176
Change-Id: I45dbbb8794c6bf2d5e962cecf531e06a7af09160
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3170927
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#923491}