Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2878003003:
Upstream srvc wrkr "redirected resp" test to WPT (Closed)
|
Created:
3 years, 7 months ago by mike3 Modified:
3 years, 7 months ago Reviewers:
falken CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, shimazu+serviceworker_chromium.org, serviceworker-reviews, blink-reviews-w3ctests_chromium.org, nhiroki, kinuko+serviceworker, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, tzik Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpstream srvc wrkr "redirected resp" test to WPT
Reformat the test in terms of a series of distinct sub-tests with
expressive titles. This provides more expressive feedback in the event of
test failure. It also makes test results deterministic in the event of
multiple test failures.
Create a new version of this test that omits references to internal APIs
and place that in the "external" directory for eventual contribution to the
Web Platform Tests project.
Re-name the original test file to explicitly document its role as a
Chromium-specific test and limit its assertions to only those internal
details.
BUG=688116, 658249
R=falken@chromium.org
Review-Url: https://codereview.chromium.org/2878003003
Cr-Commit-Position: refs/heads/master@{#473221}
Committed: https://chromium.googlesource.com/chromium/src/+/732f505038a242a0353375cc78126b3956650b16
Patch Set 1 #Patch Set 2 : Upstream service worker "request" test to WPT #
Total comments: 9
Patch Set 3 : Incorporate review feedback #
Total comments: 22
Patch Set 4 : Incorporate review feedback #
Total comments: 8
Patch Set 5 : Improve in-line comments #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== refactor Initial migration BUG= ========== to ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures can be reported and BUG=688116 R=falken@chromium.org ==========
Mind taking a look, Matt?
Almost forgot!
Due to a subtle difference between two project's implementation of the shared
`fetch-rewrite-worker.js` resource file, some sub-tests required modification.
Specifically: the Web Platform Test's implementation creates new requests from
intercepted requests with the following logic:
init['redirect'] = params['redirect-mode'] || base['redirect'];
Exposing the semantics, "default to the original request's redirect mode." The
Chromium version of the same file performs the operation differently:
if (params['redirect-mode']) {
init['redirect'] = params['redirect-mode'];
}
This exposes the semantics, "default to the redirect mode 'follow.'"
In order to preserve the behavior of the original test, the redirect mode of
"follow" must be explicitly specified in the request URL in cases where it was
previously omitted.
falken@chromium.org changed reviewers: + falken@chromium.org
The CL description looks cut off. This looks good but I think we need to preserve the Chromium-specific tests in http/tests/serviceworker. https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html (right): https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:7: <script> I find this function pretty dense. Could we add some docu? // Tests redirect behavior. It calls fetch_method(url, fetch_option) and tests the resulting response against the expected values. It also adds the response to |cache| and checks the cached response matches the expected values. // |url|: the URL to fetch // |fetch_option|: the options passed to |fetch_method| // |fetch_method|: the method used to fetch. Useful for testing an iframe's fetch() vs. this page's fetch(). // |cache|: A Cache to add the response to // |expected_redirected|: The value of response.redirected // |expected_url_list|: (this should be removed from the WPT) https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:13: expected_url_list) { nit: Could we make this a dictionary of params instead? https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:22: if (self.internals) { We shouldn't put this part in the WPT. self.internals is a Chromium specific API. https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:39: if (self.internals) { ditto
Description was changed from ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures can be reported and BUG=688116 R=falken@chromium.org ========== to ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test. BUG=688116 R=falken@chromium.org ==========
Thanks for the review, Matt! I think this is ready for another look. https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html (right): https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:7: <script> On 2017/05/17 05:09:12, falken wrote: > I find this function pretty dense. Could we add some docu? > > // Tests redirect behavior. It calls fetch_method(url, fetch_option) and tests > the resulting response against the expected values. It also adds the response to > |cache| and checks the cached response matches the expected values. > > // |url|: the URL to fetch > // |fetch_option|: the options passed to |fetch_method| > // |fetch_method|: the method used to fetch. Useful for testing an iframe's > fetch() vs. this page's fetch(). > // |cache|: A Cache to add the response to > // |expected_redirected|: The value of response.redirected > // |expected_url_list|: (this should be removed from the WPT) Acknowledged. https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:13: expected_url_list) { On 2017/05/17 05:09:12, falken wrote: > nit: Could we make this a dictionary of params instead? Done. https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:22: if (self.internals) { On 2017/05/17 05:09:12, falken wrote: > We shouldn't put this part in the WPT. self.internals is a Chromium specific > API. Acknowledged. https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:39: if (self.internals) { On 2017/05/17 05:09:12, falken wrote: > ditto Acknowledged. https://codereview.chromium.org/2878003003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:84: t.add_cleanup(function() { This change was a mistake: the `t` here is the "setup" test, so using its `remove_cleanup` causes the frame to be removed prematurely. I will revert this to `add_completion_callback`.
thanks, this looks good https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html (right): https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:7: of this test in the Web Platform Tests project. --> To make more clear the purpose of this test, and to eliminate duplicated code, I think we should just remove all the non-internals stuff from this file, and have the test fail if self.internals is not present. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:8: <title>Service Worker: Redirected response</title> maybe modify the title to say "Redirected response URL list (uses Chromium-internal API)" https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:25: // |options.options.expected_redirected|: The value of response.redirected can remove this one https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:38: if (self.internals) { can remove lines 32-38 https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:55: if (self.internals) { can remove lines 52-55 https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:78: var CACHE_NAME = 'serviceworker/redirected-response'; if (!self.internals) return Promise.reject('this test requires self.internals because it tests internal data only exposed through self.internals'); https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:142: 'mode: "error", non-intercepted request'); we can remove these ones that don't call redirected_test https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:175: 'mode: "error", mode change: "follow"'); remove https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:187: 'mode: "manual", mode change: "follow"'); remove https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:200: 'mode: "follow", mode change: "redirect"'); remove https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:212: 'mode: "error", mode change: "manual"'); remove
Description was changed from ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test. BUG=688116 R=falken@chromium.org ========== to ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test and limit its assertions to only those internal details. BUG=688116 R=falken@chromium.org ==========
Thanks, Matt! https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html (right): https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:7: of this test in the Web Platform Tests project. --> On 2017/05/18 01:48:06, falken wrote: > To make more clear the purpose of this test, and to eliminate duplicated code, I > think we should just remove all the non-internals stuff from this file, and have > the test fail if self.internals is not present. Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:8: <title>Service Worker: Redirected response</title> On 2017/05/18 01:48:07, falken wrote: > maybe modify the title to say "Redirected response URL list (uses > Chromium-internal API)" Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:25: // |options.options.expected_redirected|: The value of response.redirected On 2017/05/18 01:48:06, falken wrote: > can remove this one Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:38: if (self.internals) { On 2017/05/18 01:48:07, falken wrote: > can remove lines 32-38 Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:55: if (self.internals) { On 2017/05/18 01:48:07, falken wrote: > can remove lines 52-55 Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:78: var CACHE_NAME = 'serviceworker/redirected-response'; On 2017/05/18 01:48:07, falken wrote: > if (!self.internals) return Promise.reject('this test requires self.internals > because it tests internal data only exposed through self.internals'); I took a slightly different approach here: rejecting the `setup` promise. This causes all the subsequent tests to fail with the message about "internals" (as opposed to a reference error about reading a property from an undefined value). https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:142: 'mode: "error", non-intercepted request'); On 2017/05/18 01:48:06, falken wrote: > we can remove these ones that don't call redirected_test Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:175: 'mode: "error", mode change: "follow"'); On 2017/05/18 01:48:07, falken wrote: > remove Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:187: 'mode: "manual", mode change: "follow"'); On 2017/05/18 01:48:07, falken wrote: > remove Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:200: 'mode: "follow", mode change: "redirect"'); On 2017/05/18 01:48:06, falken wrote: > remove Done. https://codereview.chromium.org/2878003003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:212: 'mode: "error", mode change: "manual"'); On 2017/05/18 01:48:06, falken wrote: > remove Done.
lgtm with some comment tweaks also add BUG=658249 to aid archaeology https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html (right): https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:81: // Tests without service workers. I suggest the same comment tweaks as the other file. https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:158: // The service worker returns an opaqueredirect response. also try to format this so it's clear it refers to the following 3 tests. https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html (right): https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:97: // Tests without service workers. I missed it in the first review, but it's hard to tell what this comment refers to. Maybe something like: // =============================================================== // Tests for requests that are out-of-scope of the service worker. // =============================================================== https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:131: // The service worker returns a redirected response. indentation, and // ======================================================= // Tests for requests that are in-scope of the service worker. The service worker returns a redirected response. // =======================================================
CL description should say Upstream "redirected-response" test or something like that?
Description was changed from ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test and limit its assertions to only those internal details. BUG=688116 R=falken@chromium.org ========== to ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test and limit its assertions to only those internal details. BUG=688116, 658249 R=falken@chromium.org ==========
Description was changed from ========== Upstream service worker "request" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test and limit its assertions to only those internal details. BUG=688116, 658249 R=falken@chromium.org ========== to ========== Upstream srvc wrkr "redirected resp" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test and limit its assertions to only those internal details. BUG=688116, 658249 R=falken@chromium.org ==========
> CL description should say Upstream "redirected-response" test > or something like that? For sure > also add BUG=658249 to aid archaeology Got it https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html (right): https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:81: // Tests without service workers. On 2017/05/19 01:58:43, falken wrote: > I suggest the same comment tweaks as the other file. Done. https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/redirected-response.https.html:158: // The service worker returns an opaqueredirect response. On 2017/05/19 01:58:43, falken wrote: > also try to format this so it's clear it refers to the following 3 tests. Done. https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html (right): https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:97: // Tests without service workers. On 2017/05/19 01:58:43, falken wrote: > I missed it in the first review, but it's hard to tell what this comment refers > to. > > Maybe something like: > > // =============================================================== > // Tests for requests that are out-of-scope of the service worker. > // =============================================================== Done. https://codereview.chromium.org/2878003003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/serviceworker/chromium.redirected-response.html:131: // The service worker returns a redirected response. On 2017/05/19 01:58:44, falken wrote: > indentation, and > > // ======================================================= > // Tests for requests that are in-scope of the service worker. The service > worker returns a redirected response. > // ======================================================= Done.
The CQ bit was checked by mike@mikepennisi.com
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org Link to the patchset: https://codereview.chromium.org/2878003003/#ps80001 (title: "Improve in-line comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495208861107750,
"parent_rev": "9fe4c439346a542a207dc9122fe68b8a40f68dcd", "commit_rev":
"732f505038a242a0353375cc78126b3956650b16"}
Message was sent while issue was closed.
Description was changed from ========== Upstream srvc wrkr "redirected resp" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test and limit its assertions to only those internal details. BUG=688116, 658249 R=falken@chromium.org ========== to ========== Upstream srvc wrkr "redirected resp" test to WPT Reformat the test in terms of a series of distinct sub-tests with expressive titles. This provides more expressive feedback in the event of test failure. It also makes test results deterministic in the event of multiple test failures. Create a new version of this test that omits references to internal APIs and place that in the "external" directory for eventual contribution to the Web Platform Tests project. Re-name the original test file to explicitly document its role as a Chromium-specific test and limit its assertions to only those internal details. BUG=688116, 658249 R=falken@chromium.org Review-Url: https://codereview.chromium.org/2878003003 Cr-Commit-Position: refs/heads/master@{#473221} Committed: https://chromium.googlesource.com/chromium/src/+/732f505038a242a0353375cc7812... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/732f505038a242a0353375cc7812... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
