Upstream IDBCursor.continuePrimaryKey test to WPT. (issue 2657533002 by pwnall@chromium.org)

[pwn...@chromium.org]

Reviewers: jsbell
CL: https://codereview.chromium.org/2657533002/

Message:
PTAL?

This is an upstream from storage/indexeddb/cursor-continueprimarykey.html

I didn't remove the test because
storage/indexeddb/resources/cursor-continueprimarykey.js made me think that our
test possibly runs in workers as well. I can upstream the test using .any.js
instead of .html, but AFAIK we don't support running those tests.

WDYT?

Description:
Upstream IDBCursor.continuePrimaryKey test to WPT.

BUG=683463

Affected files (+140, -0 lines):
A third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm
M third_party/WebKit/LayoutTests/external/wpt/MANIFEST.json

[jsb...@chromium.org]

On 2017/01/24 02:17:17, pwnall wrote:
> PTAL?
>
> This is an upstream from storage/indexeddb/cursor-continueprimarykey.html
>
> I didn't remove the test because
> storage/indexeddb/resources/cursor-continueprimarykey.js made me think that
our
> test possibly runs in workers as well. I can upstream the test using .any.js
> instead of .html, but AFAIK we don't support running those tests.
>
> WDYT?

Searching doesn't show a reference to that script from a worker test - it
probably just copied the style of an existing test. I'd go ahead and remove.

https://codereview.chromium.org/2657533002/

[pwn...@chromium.org]

Done.
Can you please take a look at the test migration from js-test to testharness,
and see if it is acceptable?

Thank you!

https://codereview.chromium.org/2657533002/

[jsb...@chromium.org]

lgtm with a couple nits


https://codereview.chromium.org/2657533002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm
(right):

https://codereview.chromium.org/2657533002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm#newcode55
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm:55:
{ call: (cursor) => { cursor.continue(); },
nit: no need for () around single argument => functions

https://codereview.chromium.org/2657533002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm#newcode103
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm:103:
txn.oncomplete = verifyContinueCalls;
Needs t.step_func() wrapper

https://codereview.chromium.org/2657533002/

[pwn...@chromium.org]

jsbell: Thank you very much for the quick feedback!

Should I CQ these tests, or wait for the outcome of
https://github.com/w3c/web-platform-tests/issues/4622 ?

https://codereview.chromium.org/2657533002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm
(right):

https://codereview.chromium.org/2657533002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm#newcode55
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm:55:
{ call: (cursor) => { cursor.continue(); },

On 2017/01/24 21:59:49, jsbell wrote:
> nit: no need for () around single argument => functions

Done.

https://codereview.chromium.org/2657533002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm#newcode103
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbcursor-continuePrimaryKey.htm:103:
txn.oncomplete = verifyContinueCalls;

On 2017/01/24 21:59:49, jsbell wrote:
> Needs t.step_func() wrapper

Done.

https://codereview.chromium.org/2657533002/

[jsb...@chromium.org]

On 2017/01/26 00:22:06, pwnall wrote:
> jsbell: Thank you very much for the quick feedback!
>
> Should I CQ these tests, or wait for the outcome of
> https://github.com/w3c/web-platform-tests/issues/4622 ?
>

If you can just make sure the test fails promptly (and doesn't timeout) in
stable w/o the experimental flag, I think it's okay to upstream.

That wpt issue seems to be pointing at some issue with testharness, rather than
an issue with the test.

https://codereview.chromium.org/2657533002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2657533002/40001


https://codereview.chromium.org/2657533002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/cf18468fc7b170fdee94efffd557f17cdcd1086b

https://codereview.chromium.org/2657533002/

[pwn...@chromium.org]

On 2017/01/26 01:20:00, jsbell wrote:
> On 2017/01/26 00:22:06, pwnall wrote:
> > jsbell: Thank you very much for the quick feedback!
> >
> > Should I CQ these tests, or wait for the outcome of
> > https://github.com/w3c/web-platform-tests/issues/4622 ?
> >
>
> If you can just make sure the test fails promptly (and doesn't timeout) in
> stable w/o the experimental flag, I think it's okay to upstream.

Closing the loop -- the test completes immediately without any console output.
The document output includes "Harness: the test ran to completion."

https://codereview.chromium.org/2657533002/

[jeff...@chromium.org]

Upstreamed in https://github.com/w3c/web-platform-tests/pull/4631

(also: would it be useful if the bot posted status notifications like this to
the CL?)

https://codereview.chromium.org/2657533002/

[jsb...@chromium.org]

On 2017/01/26 05:31:57, jeffcarp wrote:

> Upstreamed in https://github.com/w3c/web-platform-tests/pull/4631
>
> (also: would it be useful if the bot posted status notifications like this to
> the CL?)

Yes!


https://codereview.chromium.org/2657533002/

[pwn...@chromium.org]

On 2017/01/26 18:49:03, jsbell wrote:
> On 2017/01/26 05:31:57, jeffcarp wrote:
>
> > Upstreamed in https://github.com/w3c/web-platform-tests/pull/4631
> >
> > (also: would it be useful if the bot posted status notifications like this
to
> > the CL?)
>
> Yes!

An enthusiastic "Yes!" from me too.

https://codereview.chromium.org/2657533002/