Issue 231513003: Convert Service Worker layout tests to W3C testharness-style tests

Issue 231513003: Convert Service Worker layout tests to W3C testharness-style tests (Closed)

Created:
6 years, 8 months ago by jsbell

Modified:
6 years, 8 months ago

Reviewers:
serviceworker-reviews, dominicc (has gone to gerrit)

CC:
blink-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, nhiroki, kinuko, falken, horo+watch_chromium.org, alecflett+watch_chromium.org

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

Convert Service Worker layout tests to W3C testharness-style tests R=dominicc@chromium.org Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171358

Patch Set 2 : Review feedback #

Created: 6 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+155 lines, -287 lines)			Patch
M	LayoutTests/http/tests/serviceworker/indexeddb.html	View	1	1 chunk	+28 lines, -50 lines	0 comments	Download
M	LayoutTests/http/tests/serviceworker/indexeddb-expected.txt	View	1	1 chunk	+1 line, -12 lines	0 comments	Download
D	LayoutTests/http/tests/serviceworker/indexeddb-worker.js	View		1 chunk	+0 lines, -46 lines	0 comments	Download
M	LayoutTests/http/tests/serviceworker/postmessage.html	View	1	1 chunk	+32 lines, -26 lines	0 comments	Download
M	LayoutTests/http/tests/serviceworker/postmessage-expected.txt	View	1	1 chunk	+1 line, -12 lines	0 comments	Download
D	LayoutTests/http/tests/serviceworker/postmessage-worker.js	View		1 chunk	+0 lines, -19 lines	0 comments	Download
M	LayoutTests/http/tests/serviceworker/registration.html	View	1	1 chunk	+58 lines, -45 lines	0 comments	Download
M	LayoutTests/http/tests/serviceworker/registration-expected.txt	View	1	1 chunk	+3 lines, -13 lines	0 comments	Download
A	LayoutTests/http/tests/serviceworker/resources/indexeddb-worker.js	View	1	1 chunk	+26 lines, -0 lines	0 comments	Download
A +	LayoutTests/http/tests/serviceworker/resources/postmessage-worker.js	View		1 chunk	+2 lines, -2 lines	0 comments	Download
A +	LayoutTests/http/tests/serviceworker/resources/registration-worker.js	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
D	LayoutTests/http/tests/serviceworker/serviceworker.js	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	LayoutTests/virtual/serviceworker/http/tests/serviceworker/indexeddb-expected.txt	View		1 chunk	+1 line, -28 lines	0 comments	Download
M	LayoutTests/virtual/serviceworker/http/tests/serviceworker/postmessage-expected.txt	View		1 chunk	+1 line, -21 lines	0 comments	Download
M	LayoutTests/virtual/serviceworker/http/tests/serviceworker/registration-expected.txt	View	1	1 chunk	+3 lines, -13 lines	0 comments	Download

Messages

Total messages: 13 (0 generated)

Expand Messages | Collapse Messages

dominicc (has gone to gerrit)

Very nice. Much more readable than evalAndLog etc. https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/serviceworker/indexeddb.html File LayoutTests/http/tests/serviceworker/indexeddb.html (right): https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/serviceworker/indexeddb.html#newcode16 LayoutTests/http/tests/serviceworker/indexeddb.html:16: test.step_func(function(reason) ...

6 years, 8 months ago (2014-04-09 20:09:08 UTC) #2

jsbell

Thanks for the extensive feedback. I like how the style is converging. https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/serviceworker/indexeddb.html File LayoutTests/http/tests/serviceworker/indexeddb.html ...

6 years, 8 months ago (2014-04-09 23:04:39 UTC) #3

Thanks for the extensive feedback. I like how the style is converging.

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/indexeddb.html (right):

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/indexeddb.html:16:
test.step_func(function(reason) {
On 2014/04/09 20:09:08, dominicc wrote:
> In general it seems surprising to have the test include a different set of
> exceptions depending on execution flow like this.

Agreed. As you can see, the old testRunner versions FAILed in these cases for
indexeddb.html and postmessage.html. I went with the registration.html
convention of passing, but I prefer to FAIL. Changed.

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/indexeddb.html:17:
assert_equals(reason.name, 'DisabledError');
On 2014/04/09 20:09:08, dominicc wrote:
> These asserts should have a description in the third parameter. I find working
> to include the word "should" leads to readable descriptions.

Done. And good advice for consistent phrasing. It's a bit tortured in the
assert_unreached case, so further suggestions welcome.

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/indexeddb.html:22: var open_request =
indexedDB.open('db');
On 2014/04/09 20:09:08, dominicc wrote:
> Use camelCase. Also for get_request.

Done.

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/indexeddb.html:28: get_request.onsuccess =
test.step_func(function(e) {
On 2014/04/09 20:09:08, dominicc wrote:
> Delete unused parameter e? Or rename to _.

Done.

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/registration.html (right):

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/registration.html:1: <!DOCTYPE html>
On 2014/04/09 20:09:08, dominicc wrote:
> My favorite file, this is a nice set of sweet little tests.

Originally written by kinuko, obviously. :)

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/registration.html:10:
test_registerBasics.step_func(function(worker) {
On 2014/04/09 20:09:08, dominicc wrote:
> (function() {
>   var t = async_test(...);
>   ... t.step( ...
> })();

Having tried both, I think I like your `var t` style rather than `this`, since
the names of the tests aren't important and I use IIFEs like lisp uses parens,
when I'm writing for fluent JS readers.

I prefer to terminate them with `... }());` though.

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/registration.html:11: assert_true(!!worker,
'Successfully registered');
On 2014/04/09 20:09:08, dominicc wrote:
> Can we make this more useful, eg !!worker -> worker instanceof ServiceWorker?

I had the 'instanceof' test, actually, but the IDL is currently marked as
NoInterfaceObject so it fails! crbug.com/361828

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/resources/indexeddb-worker.js (right):

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/indexeddb-worker.js:9: function
send(message) {
On 2014/04/09 20:09:08, dominicc wrote:
> I feel like this would be simpler if port, send are just local to the event
> handler and passed to doIndexedDBTest, but up to you, I realize that might be
> excessive tinkering.

Good suggestion. Originally the worker was much chattier.

https://codereview.chromium.org/231513003/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/indexeddb-worker.js:10: if (port)
port.postMessage(message);
On 2014/04/09 20:09:08, dominicc wrote:
> Line break?

Done.

dominicc (has gone to gerrit)

The CQ bit was checked by dominicc@chromium.org

6 years, 8 months ago (2014-04-11 06:28:50 UTC) #5