-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Update split-cache.html test. #29612
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.
b245c2f
to
125f634
Compare
125f634
to
80f3ea5
Compare
Hello, When this patch land, could you merge it despite the flakes on Firefox? The flakes were already there, independently of my patch. |
This test was requesting the same resource using fetch and using navigations. Navigation are request with credentials. The fetch requests weren't. This patch updates the test to make the fetch request to send credentials. This ensures passing this test is not simply the consequence of discriminating anonymous and credentialled request in the HTTP cache, but really about implementing a split cache. This ensures implementing: https://docs.google.com/document/d/1lvbiy4n-GM5I56Ncw304sgvY5Td32R6KHitjRXvkZ6U/edit#heading=h.i5fo7881c05m won't cause one test case of this test to pass when split cache is disabled. https://chromium-review.googlesource.com/c/chromium/src/+/2964120/7/third_party/blink/web_tests/virtual/not-split-http-cache/external/wpt/fetch/http-cache/split-cache-expected.txt Bug: 1221529 Change-Id: I3e7fdfa1a21570b5128f87238238aea4a92a8a0f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3015115 Commit-Queue: Arthur Sonzogni <[email protected]> Reviewed-by: Maksim Orlovich <[email protected]> Cr-Commit-Position: refs/heads/master@{#906681}
80f3ea5
to
9cf9d2e
Compare
The flakes were already there, independently of my patch. Could you approve this patch? |
+CC next WPT import sheriff: @past I need this patch in follow-up works, I would be happy to be able to land it. |
Add a test. Check whether the HTTP cache discriminate the credentialled requests from the anonymous ones. The expectations used are Firefox's behavior. Chrome fail the test. Later, I would like to make Chrome to converge toward Firefox and run a performance experiment. whatwg/fetch issue: whatwg/fetch#1253 Design doc: https://docs.google.com/document/d/1lvbiy4n-GM5I56Ncw304sgvY5Td32R6KHitjRXvkZ6U/edit# See: https://github.com/web-platform-tests/wpt/pull/29867/checks?check_run_id=3222766321 Once: #29612 (comment) resolved, I am expecting: ┌────────────────────────────────────────────┬───────┬───────┬────────┐ │Test │ Chrome│ Safari│ Firefox│ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.html │ FAIL │ Fail │ PASS │ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.serviceworker.html│ FAIL │ FAIL │ PASS │ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.sharedworker.html │ FAIL │ FAIL │ PASS │ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.worker.html │ FAIL │ FAIL │ PASS │ └────────────────────────────────────────────┴───────┴───────┴────────┘ Bug:1221529 Change-Id: I0537108f473f37f42eef7b4fa1079cd88d987b62
Thanks! |
Add a test. Check whether the HTTP cache discriminate the credentialled requests from the anonymous ones. The expectations used are Firefox's behavior. Chrome fail the test. Later, I would like to make Chrome to converge toward Firefox and run a performance experiment. whatwg/fetch issue: whatwg/fetch#1253 Design doc: https://docs.google.com/document/d/1lvbiy4n-GM5I56Ncw304sgvY5Td32R6KHitjRXvkZ6U/edit# See: https://github.com/web-platform-tests/wpt/pull/29867/checks?check_run_id=3222766321 Once: #29612 (comment) resolved, I am expecting: ┌────────────────────────────────────────────┬───────┬───────┬────────┐ │Test │ Chrome│ Safari│ Firefox│ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.html │ FAIL │ Fail │ PASS │ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.serviceworker.html│ FAIL │ FAIL │ PASS │ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.sharedworker.html │ FAIL │ FAIL │ PASS │ ├────────────────────────────────────────────┼───────┼───────┼────────┤ │credentials.tentative.any.worker.html │ FAIL │ FAIL │ PASS │ └────────────────────────────────────────────┴───────┴───────┴────────┘ Bug:1221529 Change-Id: I0537108f473f37f42eef7b4fa1079cd88d987b62
This test was requesting the same resource using fetch and using
navigations. Navigation are request with credentials. The fetch requests
weren't.
This patch updates the test to make the fetch request to send
credentials. This ensures passing this test is not simply the
consequence of discriminating anonymous and credentialled request in the
HTTP cache, but really about implementing a split cache.
This ensures implementing:
https://docs.google.com/document/d/1lvbiy4n-GM5I56Ncw304sgvY5Td32R6KHitjRXvkZ6U/edit#heading=h.i5fo7881c05m
won't cause one test case of this test to pass when split cache is
disabled.
https://chromium-review.googlesource.com/c/chromium/src/+/2964120/7/third_party/blink/web_tests/virtual/not-split-http-cache/external/wpt/fetch/http-cache/split-cache-expected.txt
Bug: 1221529
Change-Id: I3e7fdfa1a21570b5128f87238238aea4a92a8a0f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3015115
Commit-Queue: Arthur Sonzogni <[email protected]>
Reviewed-by: Maksim Orlovich <[email protected]>
Cr-Commit-Position: refs/heads/master@{#906681}