Bug 92037 – IndexedDB: Refactor layout tests to accomodate new versioning API

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Joshua Bell]

IndexedDB: Refactor layout tests to accomodate new versioning API

[Joshua Bell]

Created attachment 153878 [details]
Patch

[Joshua Bell]

Here's the first batch. I didn't touch anything "tricky", i.e. that had multiple setVersion calls.

Still not sure if this is the best thing to do, since it hides some of the details of the test logic. An alternate approach would be to shim the new open/upgradeneeded API as part of removeVendorPrefixes, so the shim can be removed when the new API lands.

[Alec Flett]

Could we make use of this? http://blog.nparashuram.com/2012/05/indexeddb-setversion-vs-onupgradeneeded.html

I mean we might temporarily be depending on bugs there, but it may save a lot of headache during the transition.

[Alec Flett]

This is really nice. The automatic removal of the old db/etc really cleans stuff up!

[Alec Flett]

Comment on attachment 153878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153878&action=review

> LayoutTests/storage/indexeddb/mozilla/resources/key-requirements.js:14
>  function cleanDatabase()

for future cleanup, seems like these 'cleanDatabase's and similar 'setup' style functions in the tests could be better named, now that indexedDBTest() tends to do most of the 'cleaning'. 

don't worry about it now, I think they're kind of scattered all over.

> LayoutTests/storage/indexeddb/mozilla/resources/odd-result-order.js:17
> +    shouldBeTrue("db instanceof IDBDatabase");

this just seems like redundant checks adding noise to the test

> LayoutTests/storage/indexeddb/resources/key-generator.js:38
> +        indexedDBTest(

Makes me wish for some kind of promises system so we could easily chain indexedDB test stuff together. (which obviously we've talked about before...) 

I'll do some research to see how folks like node.js chain async tests together.

Overall, LGTM - it also makes me think that for some of these tests what we just want is an easy way to specify the full database schema:

indexedDBTest({objectStores: [{name: "foo", "keyPath": "abc", autoIncrement: true, "indexes": [{"name": "bar", "keyPath": bar"}]}]}, nowTestMyStuff);

[Joshua Bell]

(In reply to comment #3)
> Could we make use of this? http://blog.nparashuram.com/2012/05/indexeddb-setversion-vs-onupgradeneeded.html
> 
> I mean we might temporarily be depending on bugs there, but it may save a lot of headache during the transition.

That's basically what this patch does - hides the versioning behind a helper function. 

(In reply to comment #5)
> for future cleanup, seems like these 'cleanDatabase's and similar 'setup' style functions in the tests could be better named, now that indexedDBTest() tends to do most of the 'cleaning'. 

Agreed - I tried to leave the names as-is for now.
 
> > LayoutTests/storage/indexeddb/mozilla/resources/odd-result-order.js:17
> > +    shouldBeTrue("db instanceof IDBDatabase");
> 
> this just seems like redundant checks adding noise to the test

Yeah, not sure where that came from.
 
> Overall, LGTM - it also makes me think that for some of these tests what we just want is an easy way to specify the full database schema:
> 
> indexedDBTest({objectStores: [{name: "foo", "keyPath": "abc", autoIncrement: true, "indexes": [{"name": "bar", "keyPath": bar"}]}]}, nowTestMyStuff);

I actually want to avoid going in that direction - IMHO the layout tests should minimize use of helpers outside the assertion framework, so that someone unfamiliar the helpers can pick up a test and read it without surprises. The helpers (e.g. shouldBe, evalAndExpectException, etc) are self-describing, and the rest is just standard JS and (in our case) IDB.

After further thinking, this leads me towards preferring the approach in:

https://bugs.webkit.org/show_bug.cgi?id=92044

... which could be basically seen as "move tests over to 'upgradeneeded' in preparation for deprecating the old API". The newer API is inherently cleaner, so we get some test cleanup benefit out of it.

[Joshua Bell]

Abandoning this approach in favor of https://bugs.webkit.org/show_bug.cgi?id=92044 (which is also tabled).