third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html - Issue 2556373003: Tests for exceptions related to IndexedDB upgrade transaction lifecycle.

Side by Side Diff: third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-backend-aborted.html

Issue 2556373003: Tests for exceptions related to IndexedDB upgrade transaction lifecycle. (Closed)

Patch Set: Trim blank lines. Created 4 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « third_party/WebKit/LayoutTests/storage/indexeddb/resources/support-promises.js ('k') | third_party/WebKit/LayoutTests/storage/indexeddb/upgrade-transaction-lifecycle-committed.html » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!doctype html>

	2 <title>IndexedDB: backend-aborted versionchange transaction lifecycle</title>

	3 <link rel="help"

	4 href="https://w3c.github.io/IndexedDB/#upgrade-transaction-steps">

	5 <link rel="help"

	6 href="https://w3c.github.io/IndexedDB/#dom-idbdatabase-createobjectstore">

	7 <link rel="help"

	8 href="https://w3c.github.io/IndexedDB/#dom-idbdatabase-deleteobjectstore">

	9 <link rel="author" href="pwnall@chromium.org" title="Victor Costan">

	10 <script src="../../resources/testharness.js"></script>

	11 <script src="../../resources/testharnessreport.js"></script>

	12 <script src="resources/support-promises.js"></script>

	13 <script>

	14 'use strict';

	15

	16 promise_test(t => {

	17 let timeoutPromise = null, abortPromise = null;

	18 return createDatabase(t, database => {

	19 createBooksStore(t, database);

	20 }).then(database => {

	21 database.close();

	22 }).then(() => migrateDatabase(t, 2, (database, transaction, request) => {

	23 createNotBooksStore(t, database);

	24

	25 // Queue I/O operations to make sure the transaction does not complete
	jsbell 2016/12/19 18:01:30 This isn't guaranteed. The setTimeout(0) could fir 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. pwnall 2016/12/20 02:46:34 Yes :( I don't know how to do better here, though. Show quoted text 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.
	26 // before setTimeout's callback is called.

	27 const store = transaction.objectStore('books');

	28 for (let i = 0; i < 1000; ++i)

	29 store.put({ title: 'Bedrock Nights ' + i, author: 'Barney' });

	30

	31 let abortFired = false;

	32 abortPromise = new Promise((resolve, reject) => {

	33 transaction.addEventListener('abort', () => {

	34 abortFired = true;

	35 resolve();

	36 }, false);

	37 });

	38

	39 timeoutPromise = new Promise((resolve, reject) => {

	40 setTimeout(() => {

	41 const testStepPromise = new Promise((resolve, reject) => {

	42 assert_false(

	43 abortFired,

	44 'The abort event should fire after setTimeout expires');

	45 assert_equals(

	46 request.transaction, transaction,

	47 "The open request's transaction should be reset after onabort");

	48 assert_throws(

	49 'TransactionInactiveError',

	50 () => { database.createObjectStore('books2'); },

	51 'createObjectStore exception should reflect that the ' +

	52 'transaction is still running');

	53 assert_throws(

	54 'TransactionInactiveError',

	55 () => { database.deleteObjectStore('books'); },

	56 'deleteObjectStore exception should reflect that the ' +

	57 'transaction is still running');

	58 resolve();

	59 });

	60 resolve(testStepPromise);

	61 }, 0);

	62 });

	63

	64 transaction.objectStore('books').add(BOOKS_RECORD_DATA[0]);

	65 transaction._willBeAborted();

	66 })).then(() => Promise.all([timeoutPromise, abortPromise]));

	67 }, 'in a setTimeout(0) callback after a failing request is issued, before ' +
	jsbell 2016/12/19 18:01:30 I'm not sure you'll be able to reliably test for t 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. pwnall 2016/12/20 02:46:34 Ack :( I agree with the statements above. It's tem Show quoted text 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?
	68 'the abort event is fired');

	69

	70 promise_test(t => {

	71 let abortPromise = null;

	72 return createDatabase(t, database => {

	73 createBooksStore(t, database);

	74 }).then(database => {

	75 database.close();

	76 }).then(() => migrateDatabase(t, 2, (database, transaction, request) => {
	jsbell 2016/12/19 18:01:30 I think the test would be more readable if this fu 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. pwnall 2016/12/20 02:46:34 Done. I hadn't done that before because migrateDat Show quoted text 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.
	77 abortPromise = new Promise((resolve, reject) => {

	78 transaction.addEventListener('abort', () => {

	79 const testStepPromise = new Promise((resolve, reject) => {
	jsbell 2016/12/19 18:01:30 I don't think this Promise does anything useful he 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) pwnall 2016/12/20 02:46:34 Unfortunately, some sort of wrapping is required t Show quoted text 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
	80 assert_equals(

	81 request.transaction, transaction,

	82 "The open request's transaction should be reset after onabort");

	83 assert_throws(

	84 'InvalidStateError',

	85 () => { database.createObjectStore('books2'); },

	86 'createObjectStore exception should reflect that the ' +

	87 'transaction is no longer running');

	88 assert_throws(

	89 'InvalidStateError',

	90 () => { database.deleteObjectStore('books'); },

	91 'deleteObjectStore exception should reflect that the ' +

	92 'transaction is no longer running');

	93

	94 resolve();

	95 });

	96 resolve(testStepPromise);

	97 }, false);

	98 transaction.objectStore('books').add(BOOKS_RECORD_DATA[0]);

	99 transaction._willBeAborted();

	100 });

	101 })).then(() => abortPromise);

	102 }, 'while the abort event is fired due to a bad request');
	jsbell 2016/12/19 18:01:30 Can you reword this description? It looks like it' Can you reword this description? It looks like it's missing some words. pwnall 2016/12/20 02:46:34 Done. I tried to use better descriptions. Show quoted text 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.
	103

	104 promise_test(t => {

	105 let abortPromise = null;

	106 return createDatabase(t, database => {

	107 createBooksStore(t, database);

	108 }).then(database => {

	109 database.close();

	110 }).then(() => migrateDatabase(t, 2, (database, transaction, request) => {
	jsbell 2016/12/19 18:01:30 Same as above; this can just return abortPromise Same as above; this can just return abortPromise pwnall 2016/12/20 02:46:34 Done. Show quoted text On 2016/12/19 18:01:30, jsbell wrote: > Same as above; this can just return abortPromise Done.
	111 abortPromise = new Promise((resolve, reject) => {

	112 transaction.addEventListener('abort', () => {

	113 const timeoutPromise = new Promise((resolve, reject) => {

	114 setTimeout(() => {

	115 const testStepPromise = new Promise((resolve, reject) => {
	jsbell 2016/12/19 18:01:30 Same comment here about testStepPromise above - it Same comment here about testStepPromise above - it seems unnecessary. pwnall 2016/12/20 02:46:34 Done. Show quoted text On 2016/12/19 18:01:30, jsbell wrote: > Same comment here about testStepPromise above - it seems unnecessary. Done.
	116 assert_equals(

	117 request.transaction, null,

	118 "The open request's transaction should be reset after " +

	119 'onabort microtasks');

	120 assert_throws(

	121 'InvalidStateError',

	122 () => { database.createObjectStore('books2'); },

	123 'createObjectStore exception should reflect that the ' +

	124 'transaction is no longer running');

	125 assert_throws(

	126 'InvalidStateError',

	127 () => { database.deleteObjectStore('books'); },

	128 'deleteObjectStore exception should reflect that the ' +

	129 'transaction is no longer running');

	130 resolve();

	131 });

	132 resolve(testStepPromise);

	133 }, 0);

	134 });

	135 resolve(timeoutPromise);

	136 }, false);

	137 transaction.objectStore('books').add(BOOKS_RECORD_DATA[0]);

	138 transaction._willBeAborted();

	139 });

	140 })).then(() => abortPromise);

	141 }, 'in a setTimeout(0) callback after the abort event is fired due to a bad ' +

	142 'request');

	143

	144 </script>

OLD	NEW