Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1280243002:
Add layout tests for Service Worker fallbacked subresource requests. (Closed)
|
Created:
5 years, 4 months ago by horo Modified:
5 years, 4 months ago Reviewers:
falken CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, kenjibaheux+watch_chromium.org, tzik, serviceworker-reviews, nhiroki, falken, kinuko+serviceworker, horo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd layout tests for Service Worker fallbacked subresource requests.
BUG=517364
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200475
Patch Set 1 : #
Total comments: 23
Patch Set 2 : incorporated falken's comment #
Total comments: 4
Patch Set 3 : incorporated falken's comment #
Messages
Total messages: 13 (6 generated)
Patchset #1 (id:1) has been deleted
horo@chromium.org changed reviewers: + falken@chromium.org
falken@ Could you please review this?
Patchset #1 (id:1) has been deleted
i like the exhaustive test coverage, some comments https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-request-fallback.html (right): https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:6: <script src="resources/test-helpers.js?pipe=sub"></script> should we do ?pipe=sub on all our layout tests? I'm never sure. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:11: .then(function(){ nit: space before { https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:17: .catch(function(){ ditto https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:22: }); This pattern seems dangerous, if you have: f(x) { .then(function() { // some random error in your code }) .catch(function() { if (expect) reject(); else resolve(); }) Then you can end up resolving if f(x) succeeded and you expected failure. Is it worth changing the test to the two-arg form style of then()? Even better can we just use helpers like: function assert_resolves(promise, description) { return new Promise(function(resolve, reject) { promise .then( function() { resolve(); }, function() { reject(description); }); }); } function assert_rejects(promise, description) { return new Promise(function(resolve, reject) { promise .then( function() { reject(description); }, function() { resolve(); }); }); } Then xhr_test and img_test don't have to share much code, and the true/false literals become assert_resolves or assert_rejects similar to testharness assert_equals. WDYT? https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:49: }) nit: semicolon https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:57: }) semicolon https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:88: "The SW must catch the request of page load."); s/catch/intercept/ throughout "the request of page load" -> "the request for a main resource" https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:106: "The SW must catch the request of CORS unsupported other origin XHR."); "CORS-unsupported" is easier to read All theses doublequotes should be single quotes https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:115: "The SW must catch the request of CORS supported other origin XHR."); CORS-supported https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:214: "The SW must catch only the first request for image resource which is redirected to othre origin."); other https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-request-fallback-worker.js (right): https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-request-fallback-worker.js:1: var requests = []; Technically this isn't portable since the sw can be killed at some point during the test. I wonder how difficult it would be to just use the cachestorage API to store these requests. At least we need a TODO.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-request-fallback.html (right): https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:6: <script src="resources/test-helpers.js?pipe=sub"></script> On 2015/08/11 09:15:33, falken wrote: > should we do ?pipe=sub on all our layout tests? I'm never sure. It was intended to get the port number with get_host_info() in W3C tests. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... But get_host_info() was moved to get-host-info.js. So ?pipe=sub should be moved to "get-host-info.js". https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:11: .then(function(){ On 2015/08/11 09:15:33, falken wrote: > nit: space before { removed. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:17: .catch(function(){ On 2015/08/11 09:15:33, falken wrote: > ditto removed. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:22: }); On 2015/08/11 09:15:33, falken wrote: > This pattern seems dangerous, if you have: > > f(x) { > .then(function() { > // some random error in your code > }) > .catch(function() { > if (expect) > reject(); > else > resolve(); > }) > > Then you can end up resolving if f(x) succeeded and you expected failure. Is it > worth changing the test to the two-arg form style of then()? > > Even better can we just use helpers like: > > function assert_resolves(promise, description) { > return new Promise(function(resolve, reject) { > promise > .then( > function() { resolve(); }, > function() { reject(description); }); > }); > } > > function assert_rejects(promise, description) { > return new Promise(function(resolve, reject) { > promise > .then( > function() { reject(description); }, > function() { resolve(); }); > }); > } > > Then xhr_test and img_test don't have to share much code, and the true/false > literals become assert_resolves or assert_rejects similar to testharness > assert_equals. WDYT? > > assert_resolves/assert_rejects looks good. I rewrote this test using them. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:49: }) On 2015/08/11 09:15:32, falken wrote: > nit: semicolon Done. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:57: }) On 2015/08/11 09:15:32, falken wrote: > semicolon Done. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:88: "The SW must catch the request of page load."); On 2015/08/11 09:15:32, falken wrote: > s/catch/intercept/ throughout > > "the request of page load" -> "the request for a main resource" Done. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:106: "The SW must catch the request of CORS unsupported other origin XHR."); On 2015/08/11 09:15:33, falken wrote: > "CORS-unsupported" is easier to read > > All theses doublequotes should be single quotes Done. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:115: "The SW must catch the request of CORS supported other origin XHR."); On 2015/08/11 09:15:33, falken wrote: > CORS-supported Done. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:214: "The SW must catch only the first request for image resource which is redirected to othre origin."); On 2015/08/11 09:15:33, falken wrote: > other Done. https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/resources/fetch-request-fallback-worker.js (right): https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/resources/fetch-request-fallback-worker.js:1: var requests = []; On 2015/08/11 09:15:33, falken wrote: > Technically this isn't portable since the sw can be killed at some point during > the test. I wonder how difficult it would be to just use the cachestorage API to > store these requests. At least we need a TODO. Added TODO
lgtm https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-request-fallback.html (right): https://codereview.chromium.org/1280243002/diff/20001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:6: <script src="resources/test-helpers.js?pipe=sub"></script> On 2015/08/12 11:28:10, horo wrote: > On 2015/08/11 09:15:33, falken wrote: > > should we do ?pipe=sub on all our layout tests? I'm never sure. > > It was intended to get the port number with get_host_info() in W3C tests. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > But get_host_info() was moved to get-host-info.js. > So ?pipe=sub should be moved to "get-host-info.js". Oops :) So I guess all get-host-info.js should use ?pipe=sub .. .? Sounds like we need to do a bulk regexp. https://codereview.chromium.org/1280243002/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-request-fallback.html (right): https://codereview.chromium.org/1280243002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:6: <script src='resources/test-helpers.js'></script> these single quotes should be double-quotes https://codereview.chromium.org/1280243002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:169: 'The SW must intercept the request for image.') semicolon
https://codereview.chromium.org/1280243002/diff/60001/LayoutTests/http/tests/... File LayoutTests/http/tests/serviceworker/fetch-request-fallback.html (right): https://codereview.chromium.org/1280243002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:6: <script src='resources/test-helpers.js'></script> On 2015/08/13 06:22:03, falken wrote: > these single quotes should be double-quotes Done. https://codereview.chromium.org/1280243002/diff/60001/LayoutTests/http/tests/... LayoutTests/http/tests/serviceworker/fetch-request-fallback.html:169: 'The SW must intercept the request for image.') On 2015/08/13 06:22:03, falken wrote: > semicolon Done.
The CQ bit was checked by horo@chromium.org
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/1280243002/#ps80001 (title: "incorporated falken's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1280243002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1280243002/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200475 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
