LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html - Issue 695643002: Add terminateServiceWorker method for layout tests - blink side

Side by Side Diff: LayoutTests/http/tests/serviceworker/chromium/postmessage-after-terminate.html

Issue 695643002: Add terminateServiceWorker method for layout tests - blink side (Closed) Base URL: https://chromium.googlesource.com/chromium/blink.git@master

Patch Set: Created 6 years, 1 month ago

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

Jump to:

View unified diff | Download patch

« no previous file with comments | « no previous file | LayoutTests/http/tests/serviceworker/chromium/resources/ping-worker.js » ('j') | LayoutTests/http/tests/serviceworker/chromium/resources/ping-worker.js » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <title>Tests that postMessage works during and after terminating a service worke r</title>

	3 <script src="../../resources/testharness.js"></script>

	4 <script src="../../resources/testharnessreport.js"></script>

	5 <script src="../../resources/testharness-helpers.js"></script>

	6 <script src="../resources/test-helpers.js"></script>

	7 <script>

	8 var properties = {timeout: 2500};

	9

	10 function sendPing(worker) {

	11 return new Promise(function(resolve) {

	12 var channel = new MessageChannel();
	dominicc (has gone to gerrit) 2014/11/17 02:25:26 Could you indent this slightly differently, follow Could you indent this slightly differently, following the examples here: http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style Below too. Marijn Kruisselbrink 2014/11/17 23:18:44 Done (at least I think so). Show quoted text On 2014/11/17 02:25:26, dominicc wrote: > Could you indent this slightly differently, following the examples here: > > http://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style > > Below too. Done (at least I think so).
	13 channel.port1.onmessage = function(message) {

	14 resolve(message.data);

	15 };

	16 worker.postMessage({port: channel.port2}, [channel.port2]);

	17 });

	18 }

	19

	20 function delay(ms) {

	21 return new Promise(function(resolve) {

	22 window.setTimeout(resolve, ms);

	23 });

	24 }

	25

	26 promise_test(function(test) {

	27 var worker = 'resources/ping-worker.js';

	28 var scope = 'resources/blank.html';

	29 var sw;

	30 return service_worker_unregister_and_register(test, worker, scope)

	31 .then(function(registration) {

	32 return wait_for_update(test, registration);

	33 }).then(function(worker) {

	34 sw = worker;

	35 return sendPing(sw);

	36 }).then(function(reply) {

	37 assert_equals(reply, 1);

	38 return internals.terminateServiceWorker(sw);
	dominicc (has gone to gerrit) 2014/11/17 02:25:25 What if the Service Worker terminated itself, perh What if the Service Worker terminated itself, perhaps in response to specific messages? Depending on whether shutdown is enacted immediately that might make this test more reliable. Marijn Kruisselbrink 2014/11/17 23:18:44 I suppose I could change the test to have the work Show quoted text On 2014/11/17 02:25:25, dominicc wrote: > What if the Service Worker terminated itself, perhaps in response to specific > messages? Depending on whether shutdown is enacted immediately that might make > this test more reliable. I suppose I could change the test to have the worker call internals.terminateServiceWorker itself (does a service worker have access to a ServiceWorker instance? or would this require changing the internals API to be callable from a service worker?). I'm not sure how that would help with making the test more reliable? And instead of this test working around the bug in message/event delivery for terminating workers, message/event delivery should probably just be fixed before landing this CL/enabling this test.
	39 }).then(function() {

	40 //return delay(500);
	Marijn Kruisselbrink 2014/11/04 23:11:54 This test currently only passes when this delay is This test currently only passes when this delay is enabled. dominicc (has gone to gerrit) 2014/11/17 02:25:25 Let's not add a test that is known to be broken. O Show quoted text On 2014/11/04 23:11:54, Marijn Kruisselbrink wrote: > This test currently only passes when this delay is enabled. Let's not add a test that is known to be broken. Obviously timing-based stuff like this has a higher propensity to flake and/or time out, what happened on Win XP bots? Marijn Kruisselbrink 2014/11/17 23:18:44 Well, the test is not broken, it just doesn't curr Show quoted text On 2014/11/17 02:25:25, dominicc wrote: > On 2014/11/04 23:11:54, Marijn Kruisselbrink wrote: > > This test currently only passes when this delay is enabled. > > Let's not add a test that is known to be broken. Obviously timing-based stuff > like this has a higher propensity to flake and/or time out, what happened on Win > XP bots? Well, the test is not broken, it just doesn't currently pass since http://crbug.com/433439 isn't fixed yet. And it doesn't rely on anything timing based. The only potential flakyness is that this test isn't guaranteed to fail even with the current implementation, but if that bug is fixed the test is guarenteed to always pass, with no chance for timing problems. The only potential flakyness is that the service worker manages to terminate completely before the next sendPing is called, in which case the test could incorrectly pass, but that chance is pretty low, and flaky test success is much much less of a problem than flaky test failure anyway. And I can't try this on any bots yet, since I'll first have to land the chromium side changes.
	41 }).then(function() {

	42 return sendPing(sw);

	43 }).then(function(reply) {

	44 assert_equals(reply, 1);

	45 return sendPing(sw);
	dominicc (has gone to gerrit) 2014/11/17 02:25:25 Are two subsequent pings necessary? Are two subsequent pings necessary? Marijn Kruisselbrink 2014/11/17 23:18:44 Not really, just a simple sanity check to make sur Show quoted text On 2014/11/17 02:25:25, dominicc wrote: > Are two subsequent pings necessary? Not really, just a simple sanity check to make sure my worker actually does what I expect it to do, but it doesn't really add anything for this test in particular.
	46 }).then(function(reply) {

	47 assert_equals(reply, 2);

	48 return service_worker_unregister_and_done(test, scope);

	49 });

	50 }, 'Terminate terminates the worker.', properties);
	dominicc (has gone to gerrit) 2014/11/17 02:25:25 Is the test really about that? It's about postMess Is the test really about that? It's about postMessage getting to a terminated/terminating worker, right? Marijn Kruisselbrink 2014/11/17 23:18:44 Well, it's more about both. I started writing this Show quoted text On 2014/11/17 02:25:25, dominicc wrote: > Is the test really about that? It's about postMessage getting to a > terminated/terminating worker, right? Well, it's more about both. I started writing this test to make sure that the internals.terminateServiceWorker method I wrote was working correctly, so that is really what it is testing. That it happens to expose a completely different bug (about postMessage, and any other kind of event delivery to a terminating worker) doesn't really change what the test is testing.
	51

	52 </script>

OLD	NEW