third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html - Issue 2907443002: Upstream service worker "XHR" test to WPT

Side by Side Diff: third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-iframe.html

Issue 2907443002: Upstream service worker "XHR" test to WPT (Closed)

Patch Set: Created 3 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html ('K') | « third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https-expected.txt ('k') | third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/fetch-request-xhr-sync-worker.js » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <title>Service Worker: Synchronous XHR doesn't deadlock iframe</title>
	mike3 2017/05/29 16:14:21 I'll also remove the reference to "blocking" from I'll also remove the reference to "blocking" from this title.
	3 <script>

	4 'use strict';

	5

	6 function performSyncXHR() {

	7 var url = 'fetch-request-xhr-sync-data.txt?bustcache=' + Date.now();

	8 var syncXhr = new XMLHttpRequest();

	9 syncXhr.open('GET', url, false);

	10 syncXhr.send();

	11

	12 if (syncXhr.responseText != 'hello') {

	13 throw 'FAIL';
	falken 2017/05/29 04:22:15 just for expectation hardening, can you include th just for expectation hardening, can you include the output of responseTest in the FAIL? (I'm curious what it is actually) mike3 2017/05/29 16:14:21 Sure. I've refactored this to return the value of Show quoted text On 2017/05/29 04:22:15, falken wrote: > just for expectation hardening, can you include the output of responseTest in > the FAIL? (I'm curious what it is actually) Sure. I've refactored this to return the value of the `responseText` property and use `assert_equals` in the test body. The resulting error in Chromium reads: Show quoted text > assert_equals: value of responseText expected "hello" but got "{\"error\": {\"message\": \"\", \"code\": 404}}" This is the default payload for 404 responses from wptserve [1] [2]. I can see how this extra information may be more informative, but it may also make the test more fragile, for Chromium anyway. That's because we'll need to encode the body in the "expectations" file, and it's possible that some future change to wptserve modifies this message. Because the response body isn't tested within the wptserve project [3], it's tough to say whether it is considered stable. Do you think we should avoid relying on it for that reason? Or should I shore things up by contributing documentation and a few tests to wptserve? [1] https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818... [2] https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818... [3] https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818... falken 2017/05/30 01:08:41 Good point, I think you're right: my suggestion ma Show quoted text On 2017/05/29 16:14:21, mike3 wrote: > On 2017/05/29 04:22:15, falken wrote: > > just for expectation hardening, can you include the output of responseTest in > > the FAIL? (I'm curious what it is actually) > > Sure. I've refactored this to return the value of the `responseText` property > and use `assert_equals` in the test body. The resulting error in Chromium > reads: > > > assert_equals: value of responseText expected "hello" but got "{\"error\": > {\"message\": \"\", \"code\": 404}}" > > This is the default payload for 404 responses from wptserve [1] [2]. I can > see how this extra information may be more informative, but it may also make > the test more fragile, for Chromium anyway. That's because we'll need to > encode the body in the "expectations" file, and it's possible that some > future change to wptserve modifies this message. Because the response body > isn't tested within the wptserve project [3], it's tough to say whether it > is considered stable. Do you think we should avoid relying on it for that > reason? Or should I shore things up by contributing documentation and a few > tests to wptserve? > > [1] > https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818... > [2] > https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818... > [3] > https://github.com/w3c/web-platform-tests/blob/6f1ab5699a30e269a62eec0a573818... Good point, I think you're right: my suggestion made the test more fragile. I can think of some approaches: (1) Go back to the first approach where we just test for the expected string. (Would prefer using 'PASS' instead of 'hello' though.) (2) Add the server-side file with 'FAIL: service worker did not intercept the request' as the response text, instead of relying on 404 from WPT. (3) Use assert_equals on response status code instead of response text. I kind of think (2) makes the test easiest to understand, but may be overkill. I think (3) should be fairly simple and robust. WDYT?
	14 }

	15 }

	16 </script>

OLD	NEW