|
|
DescriptionTests for exceptions related to IndexedDB upgrade transaction lifecycle.
BUG=657968
Committed: https://crrev.com/676555942ebaa74b60776e67874efd0b1f9a5779
Cr-Commit-Position: refs/heads/master@{#441614}
Patch Set 1 : Trim blank lines. #
Total comments: 14
Patch Set 2 : Addressed preliminary feedback. #Patch Set 3 : Yanked hacky tests, added meta charset. #
Total comments: 6
Patch Set 4 : Addressed feedback. #
Messages
Total messages: 48 (39 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== DO NOT LAND: Transaction lifecycle test. BUG=657968 ========== to ========== Tests for exceptions related to IndexedDB upgrade transaction lifecycle. BUG=657968 ==========
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #2 (id:120001) has been deleted
Patchset #1 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pwnall@chromium.org changed reviewers: + jsbell@chromium.org
PTAL?
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
Patchset #1 (id:140001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jsbell: Per our discussion, I've pulled the hacky tests into a separate CL: http://crrev.com/2592943002
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pwnall@chromium.org
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.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1483606396944640, "parent_rev": "8f8e0d514a2020e76edde601ab4667f5047a6d12", "commit_rev": "57cb44ea51e5dad9e3344b694d6fa70a54721660"}
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:220001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/676555942ebaa74b60776e67874efd0b1f9a5779 Cr-Commit-Position: refs/heads/master@{#441614} |