Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

BUG=657968
Committed: https://crrev.com/676555942ebaa74b60776e67874efd0b1f9a5779
Cr-Commit-Position: refs/heads/master@{#441614}

[pwnall, 2016-12-13 17:20:16]

Patchset #1 (id:1) has been deleted

[pwnall, 2016-12-13 17:20:22]

Patchset #1 (id:20001) has been deleted

[pwnall, 2016-12-13 17:20:31]

Patchset #1 (id:40001) has been deleted

[pwnall, 2016-12-13 19:04:30]

Patchset #1 (id:60001) has been deleted

[pwnall, 2016-12-16 09:08:16]

Description was changed from

==========
DO NOT LAND: Transaction lifecycle test.

BUG=657968
==========

to

==========
Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

BUG=657968
==========

[pwnall, 2016-12-16 09:09:33]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 09:09:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2016-12-16 09:09:59]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-12-16 10:07:44]

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

[commit-bot: I haz the power, 2016-12-16 10:07:44]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-12-17 02:11:49]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[pwnall, 2016-12-17 02:12:07]

Patchset #2 (id:120001) has been deleted

[pwnall, 2016-12-17 02:12:17]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-12-17 02:12:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 03:26:04]

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

[commit-bot: I haz the power, 2016-12-17 03:26:04]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-12-17 05:39:36]

pwnall@chromium.org changed reviewers:
+ jsbell@chromium.org

[pwnall, 2016-12-17 05:39:37]

PTAL?

[pwnall, 2016-12-18 04:31:21]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-18 04:31:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-18 05:56:07]

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

[commit-bot: I haz the power, 2016-12-18 05:56:08]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-12-19 18:01:30]

Preliminary review notes. I think the Promise logic in several of the tests can
be simplified.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html
(right):

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:25:
// Queue I/O operations to make sure the transaction does not complete
This isn't guaranteed. The setTimeout(0) could fire after the transaction
completes; the IDB implementation could be fast and the timeout could be
extended by the browser.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:67:
}, 'in a setTimeout(0) callback after a failing request is issued, before ' +
I'm not sure you'll be able to reliably test for this. While there should be a
window for tasks to execute between the task where the task that makes the
failing request is issued and the abort event, you can't force the timeout to
occur there.

I'll keep pondering.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:76:
}).then(() => migrateDatabase(t, 2, (database, transaction, request) => {
I think the test would be more readable if this function just returned
abortPromise; the follow-on then() would not be needed, and the `let
abortPromise` could be removed; it can just be `return new Promise ...` after
the transaction calls.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:79:
const testStepPromise = new Promise((resolve, reject) => {
I don't think this Promise does anything useful here. The asserts inside run
synchronously when `new Promise` is called, so they could just as easily just be
inside the setTimeout callback function, i.e. this is equivalent to:

setTimeout(() => {
  assert_...;
  assert_...;
  assert_...;

  const testStepPromise = new Promise(resolve => resolve());
  resolve(testStepPromise);
}, 0);

So I'd move the asserts up and remove the testStepPromise entirely, just
resolve() (for timeoutPromise)

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:102:
}, 'while the abort event is fired due to a bad request');
Can you reword this description? It looks like it's missing some words.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:110:
}).then(() => migrateDatabase(t, 2, (database, transaction, request) => {
Same as above; this can just return abortPromise

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:115:
const testStepPromise = new Promise((resolve, reject) => {
Same comment here about testStepPromise above - it seems unnecessary.

[pwnall, 2016-12-20 02:27:55]

Patchset #1 (id:140001) has been deleted

[pwnall, 2016-12-20 02:40:13]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 02:40:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2016-12-20 02:46:34]

jsbell: Thank you very much for getting me some preliminary feedback quickly. I
tried to address it. Can you PTAL?

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html
(right):

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:25:
// Queue I/O operations to make sure the transaction does not complete
On 2016/12/19 18:01:30, jsbell wrote:
> This isn't guaranteed. The setTimeout(0) could fire after the transaction
> completes; the IDB implementation could be fast and the timeout could be
> extended by the browser.

Yes :(
I don't know how to do better here, though. Given that the transaction is
intended to be aborted, I can't use your keep-alive trick of chaining requests
from request completion handlers for as long as necessary.

FWIW, I measured the 4 new tests in this CL using run-webkit-tests, and each of
the new tests flakes 0.1-0.2% of the time (1-2 failures per 1,000 runs). All the
failures were due to timeout, not due to assertion failures. Assuming my code is
correct, I should get an assertion failure if the timeout handler fires too
late.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:67:
}, 'in a setTimeout(0) callback after a failing request is issued, before ' +
On 2016/12/19 18:01:30, jsbell wrote:
> I'm not sure you'll be able to reliably test for this. While there should be a
> window for tasks to execute between the task where the task that makes the
> failing request is issued and the abort event, you can't force the timeout to
> occur there.
> 
> I'll keep pondering.

Ack :(
I agree with the statements above. It's tempting to just drop this set of tests,
but I think there's value to getting test coverage for the window, as the tests
seem to highlight differences between us and Firefox.

We could wrap the whole test in a loop where we start with 100 operations, and
increase the number of operations by a factor of 10 until we get a good test run
(abortFired is false). If we'd go down that route, I'd want to show the number
of operations in any error output, to make it easier to debug the test.

WDYT?

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:76:
}).then(() => migrateDatabase(t, 2, (database, transaction, request) => {
On 2016/12/19 18:01:30, jsbell wrote:
> I think the test would be more readable if this function just returned
> abortPromise; the follow-on then() would not be needed, and the `let
> abortPromise` could be removed; it can just be `return new Promise ...` after
> the transaction calls.

Done.
I hadn't done that before because migrateDatabase ignored its callback return
value. It turns out that the extra complexity in migrateDatabase is mitigated by
all the savings in the tests that use it.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:79:
const testStepPromise = new Promise((resolve, reject) => {
On 2016/12/19 18:01:30, jsbell wrote:
> I don't think this Promise does anything useful here. The asserts inside run
> synchronously when `new Promise` is called, so they could just as easily just
be
> inside the setTimeout callback function, i.e. this is equivalent to:
> 
> setTimeout(() => {
>   assert_...;
>   assert_...;
>   assert_...;
> 
>   const testStepPromise = new Promise(resolve => resolve());
>   resolve(testStepPromise);
> }, 0);
> 
> So I'd move the asserts up and remove the testStepPromise entirely, just
> resolve() (for timeoutPromise)

Unfortunately, some sort of wrapping is required to handle the case when the
asserts fail. t.step_func() didn't work for me, because of a testharness.js bug
[1], so I figured that the sanest way to go about this is to make sure that
every assert is wrapped in a Promise.

[1] https://github.com/w3c/testharness.js/pull/225

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:102:
}, 'while the abort event is fired due to a bad request');
On 2016/12/19 18:01:30, jsbell wrote:
> Can you reword this description? It looks like it's missing some words.

Done.
I tried to use better descriptions.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:110:
}).then(() => migrateDatabase(t, 2, (database, transaction, request) => {
On 2016/12/19 18:01:30, jsbell wrote:
> Same as above; this can just return abortPromise

Done.

https://codereview.chromium.org/2556373003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html:115:
const testStepPromise = new Promise((resolve, reject) => {
On 2016/12/19 18:01:30, jsbell wrote:
> Same comment here about testStepPromise above - it seems unnecessary.

Done.

[commit-bot: I haz the power, 2016-12-20 03:41:58]

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

[commit-bot: I haz the power, 2016-12-20 03:41:59]

Dry run: This issue passed the CQ dry run.

[pwnall, 2016-12-21 01:35:53]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 01:36:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2016-12-21 01:42:26]

jsbell: Per our discussion, I've pulled the hacky tests into a separate CL:
http://crrev.com/2592943002

[commit-bot: I haz the power, 2016-12-21 02:44:20]

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

[commit-bot: I haz the power, 2016-12-21 02:44:21]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-01-04 23:38:49]

lgtm with a few nits

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js
(right):

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js:3:
// Returns an IndexedDB database name likely to be unique to the test case.
I'd drop 'likely to be'

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js:4:
const databaseName = testCase => {
Is there an advantage to `const f = a => { ... };` over `function(a) { ... }` ?

IMHO it makes it less readable since you need to parse further to understand
it's a function.

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js:9:
// IndexedDB requests and transactions.
no longer true - transactions have 'complete' and 'abort' events.

[pwnall, 2017-01-05 06:49:20]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 06:49:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-01-05 06:52:13]

Thank you very much for the feedback!

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js
(right):

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js:3:
// Returns an IndexedDB database name likely to be unique to the test case.
On 2017/01/04 23:38:49, jsbell wrote:
> I'd drop 'likely to be'

Done.

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js:4:
const databaseName = testCase => {
On 2017/01/04 23:38:49, jsbell wrote:
> Is there an advantage to `const f = a => { ... };` over `function(a) { ... }`
?
> 
> IMHO it makes it less readable since you need to parse further to understand
> it's a function.

Done.
Replaced everywhere in the file.

https://codereview.chromium.org/2556373003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js:9:
// IndexedDB requests and transactions.
On 2017/01/04 23:38:49, jsbell wrote:
> no longer true - transactions have 'complete' and 'abort' events.

Done.
I added the other events. Thank you for noticing!

[commit-bot: I haz the power, 2017-01-05 07:54:12]

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

[commit-bot: I haz the power, 2017-01-05 07:54:13]

Dry run: This issue passed the CQ dry run.

[pwnall, 2017-01-05 08:53:16]

The CQ bit was checked by pwnall@chromium.org

[pwnall, 2017-01-05 08:53:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2556373003/#ps220001
(title: "Addressed feedback.")

[commit-bot: I haz the power, 2017-01-05 08:53:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 08:57:43]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1483606396944640,
"parent_rev": "8f8e0d514a2020e76edde601ab4667f5047a6d12", "commit_rev":
"57cb44ea51e5dad9e3344b694d6fa70a54721660"}

[commit-bot: I haz the power, 2017-01-05 08:58:12]

Description was changed from

==========
Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

BUG=657968
==========

to

==========
Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

BUG=657968

Review-Url: https://codereview.chromium.org/2556373003
==========

[commit-bot: I haz the power, 2017-01-05 08:58:13]

Committed patchset #4 (id:220001)

[commit-bot: I haz the power, 2017-01-05 09:00:49]

Description was changed from

==========
Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

BUG=657968

Review-Url: https://codereview.chromium.org/2556373003
==========

to

==========
Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

BUG=657968

Committed: https://crrev.com/676555942ebaa74b60776e67874efd0b1f9a5779
Cr-Commit-Position: refs/heads/master@{#441614}
==========

[commit-bot: I haz the power, 2017-01-05 09:00:50]

Patchset 4 (id:??) landed as
https://crrev.com/676555942ebaa74b60776e67874efd0b1f9a5779
Cr-Commit-Position: refs/heads/master@{#441614}