[ServiceWorker] CacheStorage tests.

[ServiceWorker] CacheStorage tests.

These tests exercise the following methods of CacheStorage:
  * get()
  * has()
  * create()
  * delete()
  * keys()

The editors draft is at:
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#cache-storage

BUG=374802
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182633

[jsbell, 2014-08-01 00:09:11]

Another quick skim

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
File
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:26:
'CacheStroage.keys should only return ' +
Typo: 'Stroage'

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:17:
.then(test.unreached_func('CacheStorage.create should reject ' +
You may be able to avoid splitting the string in some cases by wrapping after
the (

[asanka, 2014-08-01 06:03:21]

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
File
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:26:
'CacheStroage.keys should only return ' +
On 2014/08/01 00:09:11, jsbell wrote:
> Typo: 'Stroage'

Done.

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:17:
.then(test.unreached_func('CacheStorage.create should reject ' +
On 2014/08/01 00:09:11, jsbell wrote:
> You may be able to avoid splitting the string in some cases by wrapping after
> the (

Done.

[dominicc (has gone to gerrit), 2014-08-11 01:57:37]

Comments inline.

In general these look very well crafted tests.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:3:
var test_cache_list = ['enum-1', 'enum-2', 'enum-3'];
Is it worth including interesting names like empty string or ones that differ by
whitespace?

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:27:
'existing caches');
There's inconsistency about ending with a period. From memory one is right,
because the test harness cooks up a failure string. At least end with a period
for consistency with the above two.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:6:
assert_true(cache,
Maybe omit this assertion? You'll get a reasonable failure with the next
assertion.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:15:
// FIXME: Define the behavior for .create() with an empty or
I do not see any bias in the spec against '' as a cache name; I would expect
that to work?

Treating non-Strings will be specified by the Web IDL spec. The IDL declaration
in the Service Worker spec has the parameter as DOMString and not DOMString? so
that tells you null should be "null".

This is a good place to start:

http://www.w3.org/TR/WebIDL/#es-DOMString

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:33:
return caches.has('cheezburger')
Huge +1 to the modest sense of humor.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:78:
// FIXME: Define and test behavior for invalid cache name with delete().
You're free to file GitHub issues against the spec; make a proposal, refer to
the GitHub issue in a comment, and write the test along the lines you proposed.

Later we'll review the issues and update the test if the issue resolution went
another way.

[jsbell, 2014-08-11 22:48:34]

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:78:
// FIXME: Define and test behavior for invalid cache name with delete().
On 2014/08/11 01:57:36, dominicc wrote:
> You're free to file GitHub issues against the spec; make a proposal, refer to
> the GitHub issue in a comment, and write the test along the lines you
proposed.
> 
> Later we'll review the issues and update the test if the issue resolution went
> another way.

Yeah, I'm not clear what 'invalid' might mean here.

For Indexed DB we have tests for various edge cases to ensure that no extra
behavior was introduced:

* passing null (should stringify to "null", as dominicc@ mentions above)
* passing undefined (should stringify to "undefined")
* passing empty string ""
* passing strings with embedded NUL e.g. "abc\x00def"
* passing strings with non-ASCII characters e.g. "\xFF", "\uFFFD"
* passing strings with UTF-16 surrogate pairs e.g. "\uD800\uDC00" 
* passing strings with invalid UTF-16 sequences e.g. "\uDC00\uD800"

These funky strings should "just work", i.e. we're not validating the SW spec
itself since this is all covered by Web IDL as dominicc@ describes. Rather,
we're ensuring that we don't mis-handle those somewhere. These sorts of test
cases are worthwhile if the strings ever get used as something other than an
opaque in-memory string16 on the Chromium-side (e.g. used to create filenames,
serialized to files, etc)

Someone familiar with the Cache implementation (gavinp@?) might advise on
whether it's worth adding these sorts of tests for any aspect of the UI.

[asanka, 2014-08-13 01:22:52]

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:3:
var test_cache_list = ['enum-1', 'enum-2', 'enum-3'];
On 2014/08/11 01:57:36, dominicc wrote:
> Is it worth including interesting names like empty string or ones that differ
by
> whitespace?

Added some variants. It shouldn't hurt, but the resulting errors may not be very
specific. If there are important special cases we'd want to add those to
cache-storage-test-worker.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:27:
'existing caches');
On 2014/08/11 01:57:36, dominicc wrote:
> There's inconsistency about ending with a period. From memory one is right,
> because the test harness cooks up a failure string. At least end with a period
> for consistency with the above two.

I added a '.'. The string put together by the test harness look like
"<description> <error message>" where <error message> is often a verb phrase but
not consistently so. Even if it were I'm not sure how well we'd be able to
phrase every test description as a subject while still leaving the code
comprehensible.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:6:
assert_true(cache,
On 2014/08/11 01:57:36, dominicc wrote:
> Maybe omit this assertion? You'll get a reasonable failure with the next
> assertion.

Done.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:15:
// FIXME: Define the behavior for .create() with an empty or
On 2014/08/11 01:57:36, dominicc wrote:
> I do not see any bias in the spec against '' as a cache name; I would expect
> that to work?
> 
> Treating non-Strings will be specified by the Web IDL spec. The IDL
declaration
> in the Service Worker spec has the parameter as DOMString and not DOMString?
so
> that tells you null should be "null".
> 
> This is a good place to start:
> 
> http://www.w3.org/TR/WebIDL/#es-DOMString

Thanks for the reference! I've updated the tests.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:33:
return caches.has('cheezburger')
On 2014/08/11 01:57:36, dominicc wrote:
> Huge +1 to the modest sense of humor.

Acknowledged.

https://codereview.chromium.org/430993002/diff/20001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:78:
// FIXME: Define and test behavior for invalid cache name with delete().
On 2014/08/11 22:48:34, jsbell wrote:
> On 2014/08/11 01:57:36, dominicc wrote:
> > You're free to file GitHub issues against the spec; make a proposal, refer
to
> > the GitHub issue in a comment, and write the test along the lines you
> proposed.
> > 
> > Later we'll review the issues and update the test if the issue resolution
went
> > another way.
> 
> Yeah, I'm not clear what 'invalid' might mean here.
> 
> For Indexed DB we have tests for various edge cases to ensure that no extra
> behavior was introduced:
> 
> * passing null (should stringify to "null", as dominicc@ mentions above)
> * passing undefined (should stringify to "undefined")
> * passing empty string ""
> * passing strings with embedded NUL e.g. "abc\x00def"
> * passing strings with non-ASCII characters e.g. "\xFF", "\uFFFD"
> * passing strings with UTF-16 surrogate pairs e.g. "\uD800\uDC00" 
> * passing strings with invalid UTF-16 sequences e.g. "\uDC00\uD800"
> 
> These funky strings should "just work", i.e. we're not validating the SW spec
> itself since this is all covered by Web IDL as dominicc@ describes. Rather,
> we're ensuring that we don't mis-handle those somewhere. These sorts of test
> cases are worthwhile if the strings ever get used as something other than an
> opaque in-memory string16 on the Chromium-side (e.g. used to create filenames,
> serialized to files, etc)
> 
> Someone familiar with the Cache implementation (gavinp@?) might advise on
> whether it's worth adding these sorts of tests for any aspect of the UI.

Thanks! I'll wait for gavinp to comment.

[dominicc (has gone to gerrit), 2014-08-15 06:53:01]

> > Someone familiar with the Cache implementation (gavinp@?) might advise on
> > whether it's worth adding these sorts of tests for any aspect of the UI.
> 
> Thanks! I'll wait for gavinp to comment.

What are we waiting for? There's nothing to do with UI in these tests.

[jsbell, 2014-08-15 17:07:24]

On 2014/08/15 06:53:01, dominicc wrote:
> > > Someone familiar with the Cache implementation (gavinp@?) might advise on
> > > whether it's worth adding these sorts of tests for any aspect of the UI.
> > 
> > Thanks! I'll wait for gavinp to comment.
> 
> What are we waiting for? There's nothing to do with UI in these tests.

Oops, typo: s/UI/API/

Since no-one working on Cache specifically has reviewed these, I'll try and page
the Cache API into my brain today and  provide a real review.

[asanka, 2014-08-15 18:03:16]

On 2014/08/15 at 17:07:24, jsbell wrote:
> On 2014/08/15 06:53:01, dominicc wrote:
> > > > Someone familiar with the Cache implementation (gavinp@?) might advise
on
> > > > whether it's worth adding these sorts of tests for any aspect of the UI.
> > > 
> > > Thanks! I'll wait for gavinp to comment.
> > 
> > What are we waiting for? There's nothing to do with UI in these tests.
> 
> Oops, typo: s/UI/API/
> 
> Since no-one working on Cache specifically has reviewed these, I'll try and
page the Cache API into my brain today and  provide a real review.

Thanks! I've also been adding tests to
https://codereview.chromium.org/425413002.

I'm guessing we won't be able to land this until all the tests are passing?

[jsbell, 2014-08-15 18:35:02]

On 2014/08/15 18:03:16, asanka wrote:
> I'm guessing we won't be able to land this until all the tests are passing?

If parts of the tests are passing we could land with failures noted in
-expected.txt files. That way we can track progress as things get fixed and the
number of failures dwindles to 0.

Not sure it's worth it yet, but I'll give the tests a try as I review them and
offer an opinion. :)

[jsbell, 2014-08-15 21:36:13]

Also looking pretty good. Worried about lifetime of the caches across tests
(noted below)

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:7:
return caches.keys()
Can you write 'self.caches' so it's clear to readers unfamiliar with the spec
that this is referencing a property on the global scope?

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:4:
return caches.create('foo')
IIUC, Caches are scoped to an origin, not to a SW registration... so do we need
follow a "delete before creating" pattern in these to ensure we're in a known
state, like we do for SW registrations? Otherwise we may get flaky failures in
the tests. It also helps running the test in a browser (e.g. comparing behavior
w/ FF)

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:5:
.then(t.step_func(function(cache) {
Should not need t.step_func in a promise_test (here and below)

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:59:
'CacheStorage.has should not ignore trailing ' +
Any particular reason to worry about leading/trailing spaces in particular?

I'd be more paranoid about things like embedded NUL (\x00) characters, since at
some point we serialize the names.

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:83:
'CacheStorage.get should return the named Cache object ' +
nit: line length (80 cols)

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:88: //
Returns promise that fulfills after the provided |promise| is fulfilled. The
nit: Returns a promise...

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:89: //
|test| succeeds only if |promise| throws a DOM exception matching |code|. The
I don't believe this needs to be a DOMException, e.g. this works with
'TypeError' which is just a JS type. (testharness's assert_throws is doing the
right thing, but the docs are a little obsolete?)

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:100: throw
"expect_promise_throws: " + description + " Promise did now throw."
nit: line length (80 cols)

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:100: throw
"expect_promise_throws: " + description + " Promise did now throw."
Use assert_unreached or unreached_func?

[jsbell, 2014-08-15 23:07:41]

On 2014/08/15 18:35:02, jsbell wrote:
> On 2014/08/15 18:03:16, asanka wrote:
> > I'm guessing we won't be able to land this until all the tests are passing?
> 
> If parts of the tests are passing we could land with failures noted in
> -expected.txt files. That way we can track progress as things get fixed and
the
> number of failures dwindles to 0.
> 
> Not sure it's worth it yet, but I'll give the tests a try as I review them and
> offer an opinion. :)

Having run the tests: while they mostly fail, there are enough passing cases
that I'd vote for landing (once we've addressed the review feedback). That will
make it easier for the cache implementers to verify the impl and/or tweak the
tests if the the spec changes.

The one thing we may want to change is interfaces-worker.js so that there's one
async_test case per interface and we can track PASS/FAIL for the individual
interfaces exposed in ServiceWorkerGlobalScope

[dominicc (has gone to gerrit), 2014-08-18 01:08:20]

My $0.02: Let's get these in with Pass Fail expectations. This will support
the cache as it lands.

Dominic

On Sat, Aug 16, 2014 at 8:07 AM, <jsbell@chromium.org> wrote:

> On 2014/08/15 18:35:02, jsbell wrote:
>
>> On 2014/08/15 18:03:16, asanka wrote:
>> > I'm guessing we won't be able to land this until all the tests are
>> passing?
>>
>
>  If parts of the tests are passing we could land with failures noted in
>> -expected.txt files. That way we can track progress as things get fixed
>> and
>>
> the
>
>> number of failures dwindles to 0.
>>
>
>  Not sure it's worth it yet, but I'll give the tests a try as I review
>> them and
>> offer an opinion. :)
>>
>
> Having run the tests: while they mostly fail, there are enough passing
> cases
> that I'd vote for landing (once we've addressed the review feedback). That
> will
> make it easier for the cache implementers to verify the impl and/or tweak
> the
> tests if the the spec changes.
>
> The one thing we may want to change is interfaces-worker.js so that
> there's one
> async_test case per interface and we can track PASS/FAIL for the individual
> interfaces exposed in ServiceWorkerGlobalScope
>
> https://codereview.chromium.org/430993002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[asanka, 2014-08-20 03:10:29]

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-test-worker.js:7:
return caches.keys()
On 2014/08/15 21:36:13, jsbell wrote:
> Can you write 'self.caches' so it's clear to readers unfamiliar with the spec
> that this is referencing a property on the global scope?

Done.

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js
(right):

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:4:
return caches.create('foo')
On 2014/08/15 21:36:13, jsbell wrote:
> IIUC, Caches are scoped to an origin, not to a SW registration... so do we
need
> follow a "delete before creating" pattern in these to ensure we're in a known
> state, like we do for SW registrations? Otherwise we may get flaky failures in
> the tests. It also helps running the test in a browser (e.g. comparing
behavior
> w/ FF)

I see. This was a bit of a head scratcher for me since it wasn't obvious from my
reading of the current spec. It also means that:
- A newly minted CacheStorage could have a pre-populated [[NameToCacheMap]].
- tests which touch self.caches will interact with each other since they are
from the same origin.

I'll try to document this and adjust the tests accordingly.

Once the spec is updated, I'll add tests for the scope of caches.

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:5:
.then(t.step_func(function(cache) {
On 2014/08/15 21:36:13, jsbell wrote:
> Should not need t.step_func in a promise_test (here and below)

Ah. Good point! Changed.

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:59:
'CacheStorage.has should not ignore trailing ' +
On 2014/08/15 at 21:36:13, jsbell wrote:
> Any particular reason to worry about leading/trailing spaces in particular?
> 
> I'd be more paranoid about things like embedded NUL (\x00) characters, since
at some point we serialize the names.

I added a test case for an embedded NUL. I didn't have a solid reason for
checking for whitespace. The new list of tests doesn't explicitly call out
whitespace etc..

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-test-worker.js:83:
'CacheStorage.get should return the named Cache object ' +
On 2014/08/15 at 21:36:13, jsbell wrote:
> nit: line length (80 cols)

Done.

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:88: //
Returns promise that fulfills after the provided |promise| is fulfilled. The
On 2014/08/15 at 21:36:13, jsbell wrote:
> nit: Returns a promise...

Done.

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:89: //
|test| succeeds only if |promise| throws a DOM exception matching |code|. The
On 2014/08/15 at 21:36:13, jsbell wrote:
> I don't believe this needs to be a DOMException, e.g. this works with
'TypeError' which is just a JS type. (testharness's assert_throws is doing the
right thing, but the docs are a little obsolete?)

Changed.

https://codereview.chromium.org/430993002/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:100: throw
"expect_promise_throws: " + description + " Promise did now throw."
On 2014/08/15 at 21:36:13, jsbell wrote:
> nit: line length (80 cols)

Looks like it's exactly 80?

Also I chose to throw a string here because the resulting error message is
clearer without the additional 'assert_unreached: .... reached unreachable
code.' I can switch to an assert_unreached or unreached_func, but I'm assuming
that longer term we'd want to have something like this in testharness.js.

I also renamed this to assert_promise_throws() and removed the test parameter
for consistency with assert_*().

[jsbell, 2014-08-20 17:54:13]

lgtm!

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:16: //
Note that this test may collide with other tests running in the same
For Blink layout tests we won't run tests with the same storage directory in
parallel. It would only be an issue if this this didn't delete first, or if
*this* layout test was racy.

So... I think you're doing the right thing here and this is not an issue.

(if a user was running this in a browser loaded a bunch of tests into separate
tabs at the same time it could flake, but we can't do much about that)

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:30: var
cache_name = 'cache-storage/there can be only one';
No Highlander joke? I am disappoint!

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:46: {
name: 'cache-storage/lowercase',
nit: Per style guide, either wrap after { or no space.

[jsbell, 2014-08-20 18:24:22]

Note: this will need the -expected.txt files added, since most of the tests will
fail at the moment.

[asanka, 2014-08-20 21:20:50]

Thanks! I'll add expectations and land once I've verified that the failures make
sense.

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:16: //
Note that this test may collide with other tests running in the same
On 2014/08/20 17:54:13, jsbell wrote:
> For Blink layout tests we won't run tests with the same storage directory in
> parallel. It would only be an issue if this this didn't delete first, or if
> *this* layout test was racy.
> 
> So... I think you're doing the right thing here and this is not an issue.
> 
> (if a user was running this in a browser loaded a bunch of tests into separate
> tabs at the same time it could flake, but we can't do much about that)

Acknowledged.

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:30: var
cache_name = 'cache-storage/there can be only one';
On 2014/08/20 17:54:13, jsbell wrote:
> No Highlander joke? I am disappoint!

:)

https://codereview.chromium.org/430993002/diff/100001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:46: {
name: 'cache-storage/lowercase',
On 2014/08/20 17:54:13, jsbell wrote:
> nit: Per style guide, either wrap after { or no space.

Done.

[jsbell, 2014-08-20 21:44:52]

On 2014/08/20 21:20:50, asanka wrote:
> Thanks! I'll add expectations and land once I've verified that the failures
make
> sense.

We may want to hold off on landing due to http://crbug.com/405701 - i.e. the
tests all just crash right now. :P

It may be possible to work around that by introducing a slight delay in the test
startup - I'll give it a whirl.

[jsbell, 2014-08-21 19:07:48]

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:21:
return self.caches.create();
I missed that this is calling create() with no arguments. This should throw
TypeError due to insufficient # or arguments.

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:25:
'CacheStorage.create should accept an empty name.');
I interpreted this as "empty string" rather than "no argument provided"

[jsbell, 2014-08-21 20:35:01]

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js
(right):

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js:7:
return self.caches.keys()
This need to do a cleanup pass, otherwise it may return keys from previous tests
(e.g. in cache-storage.html).

[asanka, 2014-08-21 21:50:59]

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js
(right):

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js:7:
return self.caches.keys()
On 2014/08/21 20:35:01, jsbell wrote:
> This need to do a cleanup pass, otherwise it may return keys from previous
tests
> (e.g. in cache-storage.html).

Done.

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:21:
return self.caches.create();
On 2014/08/21 19:07:47, jsbell wrote:
> I missed that this is calling create() with no arguments. This should throw
> TypeError due to insufficient # or arguments.
> 

Done.

https://codereview.chromium.org/430993002/diff/160001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:25:
'CacheStorage.create should accept an empty name.');
On 2014/08/21 19:07:48, jsbell wrote:
> I interpreted this as "empty string" rather than "no argument provided"

Acknowledged.

[jsbell, 2014-08-27 18:42:16]

I was going to suggest landing this set of tests but I'm hitting flaky v8
crashes (during GC?) with just this set of tests plus tip-of-tree. It might be
fixed w/ gavinp's https://codereview.chromium.org/433793002 since I wasn't
seeing crashes with that, but it's been a while. I'll see if I can help move
that along...

[jsbell, 2014-08-27 19:30:20]

Yep, no flaky crashes with 433blahblahblah applied...

[asanka, 2014-08-27 19:39:14]

On 2014/08/27 19:30:20, jsbell wrote:
> Yep, no flaky crashes with 433blahblahblah applied...

Awesome! Will commence landing sequence once 433793002 lands.

[asanka, 2014-08-27 20:15:08]

Patchset #12 (id:220001) has been deleted

[dominicc (has gone to gerrit), 2014-08-27 21:12:07]

dominicc@chromium.org changed reviewers:
- dominicc@chromium.org

[jsbell, 2014-08-27 21:56:47]

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:105:
assert_class_string(e, code, description);
This doesn't work - the Error subclasses like TypeError don't define a unique
[[Class]] (ES5) / @@toStringTag (ES6):

Object.prototype.toString.call(new TypeError); // "[object Error]"

[jsbell, 2014-08-27 22:28:24]

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:99: throw
'assert_promise_throws: ' + description + ' Promise did now throw.';
Typo: now -> not

[jsbell, 2014-08-27 23:20:08]

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:121:
return self.caches.create(cache_name)
This needs a 'delete' so it can be re-run.

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:151:
return self.caches.create(cache_name)
Ditto.

[asanka, 2014-08-28 00:02:56]

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:121:
return self.caches.create(cache_name)
On 2014/08/27 23:20:07, jsbell wrote:
> This needs a 'delete' so it can be re-run.

Done.

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:151:
return self.caches.create(cache_name)
On 2014/08/27 23:20:07, jsbell wrote:
> Ditto.

Done.

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:99: throw
'assert_promise_throws: ' + description + ' Promise did now throw.';
On 2014/08/27 22:28:23, jsbell wrote:
> Typo: now -> not

Done.

https://codereview.chromium.org/430993002/diff/200001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:105:
assert_class_string(e, code, description);
On 2014/08/27 21:56:47, jsbell wrote:
> This doesn't work - the Error subclasses like TypeError don't define a unique
> [[Class]] (ES5) / @@toStringTag (ES6):
> 
> Object.prototype.toString.call(new TypeError); // "[object Error]"

Ah. Hack removed. We are now calling assert_promise_throws() with a 'new
TypeError()' instead.

[jsbell, 2014-08-28 21:07:26]

Thanks for sticking with this. With the code CLs applied, everything passes
except for the one issue noted below plus this failure:

FAIL Interfaces and attributes in ServiceWorkerGlobalScope assert_true: match
should be an attribute of CacheStorage expected true got false

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js
(right):

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js:16:
return Promise.resolve(true);
No need for this special case if keys.length === 0 - Promise.all([]) resolves,
so you can drop the if() and the else block.

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js:29:
assert_array_equals(keys,
Order is apparently not guaranteed. Needs an "equivalent" helper, or to .sort()
the expected/actual arrays.

[jsbell, 2014-08-28 21:49:01]

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js
(right):

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js:29:
assert_array_equals(keys,
On 2014/08/28 21:07:26, jsbell wrote:
> Order is apparently not guaranteed. Needs an "equivalent" helper, or to
.sort()
> the expected/actual arrays.

Apparently we're supposed to preserve the order, so this is a bug in the
implementation, not the test. Sorry!

https://github.com/slightlyoff/ServiceWorker/issues/431

[asanka, 2014-08-28 21:59:46]

Looks like the spec still requires .match() to be present on CacheStorage.

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js
(right):

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js:16:
return Promise.resolve(true);
On 2014/08/28 21:07:26, jsbell wrote:
> No need for this special case if keys.length === 0 - Promise.all([]) resolves,
> so you can drop the if() and the else block.

Done.

https://codereview.chromium.org/430993002/diff/240001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/cache-storage-keys-worker.js:29:
assert_array_equals(keys,
On 2014/08/28 21:49:01, jsbell wrote:
> On 2014/08/28 21:07:26, jsbell wrote:
> > Order is apparently not guaranteed. Needs an "equivalent" helper, or to
> .sort()
> > the expected/actual arrays.
> 
> Apparently we're supposed to preserve the order, so this is a bug in the
> implementation, not the test. Sorry!
> 
> https://github.com/slightlyoff/ServiceWorker/issues/431

Acknowledged.

[jsbell, 2014-08-28 22:31:46]

On 2014/08/28 21:59:46, asanka wrote:
> Looks like the spec still requires .match() to be present on CacheStorage.

Agreed, it's an implementation issue, not a test issue.

[jsbell, 2014-09-11 22:27:30]

https://codereview.chromium.org/430993002/diff/260001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/430993002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:96:
function assert_promise_throws(promise, code, description) {
Can we rename this to assert_promise_rejects since Promises reject but
statements throw ?

[asanka, 2014-09-24 03:28:55]

https://codereview.chromium.org/430993002/diff/260001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js
(right):

https://codereview.chromium.org/430993002/diff/260001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/resources/worker-test-harness.js:96:
function assert_promise_throws(promise, code, description) {
On 2014/09/11 22:27:30, jsbell wrote:
> Can we rename this to assert_promise_rejects since Promises reject but
> statements throw ?

Done.

[asanka, 2014-09-24 03:29:05]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-09-24 03:29:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/430993002/280001

[commit-bot: I haz the power, 2014-09-24 03:29:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 03:29:59]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1314.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
15865c98db02cbc5dfceeb9c3909eb5365227518..a72628243fb3c2b50e8837a4c868a6b4419048da
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1314,3 +1314,6 @@ crbug.com/416637 [ Debug ]
virtual/threaded/animations/webkit-perspective.html [
 crbug.com/416775 compositing/scaling/tiled-layer-recursion.html [
NeedsRebaseline ]
 crbug.com/416775 media/video-layer-crash.html [ NeedsRebaseline ]
 crbug.com/416775 media/video-transformed.html [ NeedsRebaseline ]
+
+crbug.com/392621 http/tests/serviceworker/cache-storage.html [ Failure ]
+crbug.com/392621 http/tests/serviceworker/cache-storage-keys.html [ Failure ]

[asanka, 2014-09-24 15:21:03]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-09-24 15:21:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/430993002/300001

[commit-bot: I haz the power, 2014-09-24 16:03:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 16:03:25]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[jsbell, 2014-09-24 17:29:31]

On 2014/09/24 16:03:25, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

Oops - forgot interfaces.html

BTW, instead of adding failure expectations to TestExpectations, how about just
generating/committing the -expected.txt files?

i.e. run locally with --reset-results and then git add/upload them. That way we
get coverage for the tests cases that still PASS.

That also keeps the updates local to the directory when the working code
actually lands.

[asanka, 2014-09-24 20:58:07]

The CQ bit was checked by asanka@chromium.org

[asanka, 2014-09-24 20:58:33]

The CQ bit was unchecked by asanka@chromium.org

[asanka, 2014-09-24 20:59:26]

On 2014/09/24 17:29:31, jsbell wrote:
> On 2014/09/24 16:03:25, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_blink_rel on tryserver.blink
> >
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
> 
> Oops - forgot interfaces.html
> 
> BTW, instead of adding failure expectations to TestExpectations, how about
just
> generating/committing the -expected.txt files?
> 
> i.e. run locally with --reset-results and then git add/upload them. That way
we
> get coverage for the tests cases that still PASS.
> 
> That also keeps the updates local to the directory when the working code
> actually lands.

Good idea. Done. Looks like tests are passing again. interfaces.html was failing
due to missing CacheStorage.match. I also added an -expected file for
interfaces.html.

[asanka, 2014-09-24 20:59:51]

The CQ bit was checked by asanka@chromium.org

[commit-bot: I haz the power, 2014-09-24 21:01:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/430993002/320001

[commit-bot: I haz the power, 2014-09-24 21:27:33]

Committed patchset #16 (id:320001) as 182633