Bug 101676 – IndexedDB: convert tests from setVersion to upgradeneeded

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: convert tests from setVersion to upgradeneeded

[David Grogan]

Created attachment 173162 [details]
Patch

[David Grogan]

Josh, could you take a look at this?  It's about 1/4 of the layout tests that need converting.  It'd be nice to get some feedback before I finish them up.  When you did a first pass a few months ago did you have to load each of them into an editor?  I've got some vim macros that do most of the work for me but I've found it pretty time consuming to edit, run layout test, iterate if necessary, manually for each test.

[Joshua Bell]

(In reply to comment #2)
> Josh, could you take a look at this? 

Will do, gardening permitting.

> It's about 1/4 of the layout tests that need converting.  It'd be nice to get some feedback before I finish them up.  When you did a first pass a few months ago did you have to load each of them into an editor?  I've got some vim macros that do most of the work for me but I've found it pretty time consuming to edit, run layout test, iterate if necessary, manually for each test.

If I recall, about 90% of the tests followed a small set of patterns. I can't recall if I got very fast at editing them by hand or if I used some (complex) Perl one-liners to edit them. I know I tackled them in batches by identifying those patterns so it was pretty mechanical/safe to do the updates and ensure that e.g. the output only changed in a predictable way.

Not very helpful, sorry.

[Joshua Bell]

Comment on attachment 173162 [details]
Patch

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

Overall lgtm

> LayoutTests/storage/indexeddb/resources/cursor-delete.js:12
>      debug("setVersionSuccess():");

Remove, or possibly do another pass with the preamble(evt) idiom.

> LayoutTests/storage/indexeddb/resources/cursor-skip-deleted.js:17
>      debug("setVersionSuccess():");

Ditto.

> LayoutTests/storage/indexeddb/resources/delete-closed-database-object.js:41
>      gc();

I think this can be removed if db.close() is called explicitly.

That said, we should probably add a test that ensures connections are closed on gc() - I think that may be the intent of this test, actually. The other tests that call gc() are a bit sketchier (from a quick glance).

> LayoutTests/storage/indexeddb/resources/delete-closed-database-object.js:43
> +    debug("calling open with high version - shouldn't be blocked because both connections should be dead");

FWIW, I'd propose doing all of the tests that exercise connections like this in a separate patch from the straightforward conversions, so we can review them more carefully to make sure the test semantics are the same.

> LayoutTests/storage/indexeddb/resources/deleteIndex.js:12
> +    transaction = evalAndLog("transaction = event.target.transaction;");

This isn't needed any more now that postTwiddling is called on open.

> LayoutTests/storage/indexeddb/resources/shared.js:184
> +          var openRequest = evalAndLog("indexedDB.open(dbname, " + optionalVersion + ")");

Some IDEs (like the one I use) will complain that "var VARNAME" appears twice. Can you move it out of the if/else?

> LayoutTests/storage/indexeddb/resources/shared.js:188
> +        openRequest.onupgradeneeded = upgradeCallback;

openRequest.onblocked = unexpectedBlockedCallback; here too?

[David Grogan]

Comment on attachment 173162 [details]
Patch

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

Thanks for taking a look.

>> LayoutTests/storage/indexeddb/resources/cursor-delete.js:12
>>      debug("setVersionSuccess():");
> 
> Remove, or possibly do another pass with the preamble(evt) idiom.

I'll get them in a later patch.

>> LayoutTests/storage/indexeddb/resources/delete-closed-database-object.js:43
>> +    debug("calling open with high version - shouldn't be blocked because both connections should be dead");
> 
> FWIW, I'd propose doing all of the tests that exercise connections like this in a separate patch from the straightforward conversions, so we can review them more carefully to make sure the test semantics are the same.

That's a good idea, I'll remove this test from this patch.

>> LayoutTests/storage/indexeddb/resources/deleteIndex.js:12
>> +    transaction = evalAndLog("transaction = event.target.transaction;");
> 
> This isn't needed any more now that postTwiddling is called on open.

Done.

>> LayoutTests/storage/indexeddb/resources/shared.js:184
>> +          var openRequest = evalAndLog("indexedDB.open(dbname, " + optionalVersion + ")");
> 
> Some IDEs (like the one I use) will complain that "var VARNAME" appears twice. Can you move it out of the if/else?

Done.

>> LayoutTests/storage/indexeddb/resources/shared.js:188
>> +        openRequest.onupgradeneeded = upgradeCallback;
> 
> openRequest.onblocked = unexpectedBlockedCallback; here too?

Done.  And thanks, this could save us from timeouts instead of explicit failures.

[David Grogan]

Created attachment 173340 [details]
Patch

[David Grogan]

Created attachment 173367 [details]
Patch

[David Grogan]

Josh/Alec, could you take a look when you get a chance?  These are all the layout tests that were trivial to convert from setVersion to upgradeneeded.

Of our 206 layout tests, there are 99 here that were trivial to convert.  In keeping with the idea that I'm working on removing setVersion and not refactoring the tests, I did approximately the minimum necessary to get them to work without setVersion.

There are 52 more tests that still use setVersion that range from slightly non-trivial to complex.

That leaves us with ~50 tests that don't use setVersion at all.

[David Grogan]

Somehow while pending opens are being processed another open gets added to the pending queue.

ASSERTION FAILED: m_pendingOpenCalls.isEmpty()
../../third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp(423) : void WebCore::IDBDatabaseBackendImpl::processPendingCalls()

[David Grogan]

(In reply to comment #9)
> Somehow while pending opens are being processed another open gets added to the pending queue.
> 
> ASSERTION FAILED: m_pendingOpenCalls.isEmpty()
> ../../third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp(423) : void WebCore::IDBDatabaseBackendImpl::processPendingCalls()

And this is on the wrong bug, sorry.

[Joshua Bell]

Comment on attachment 173367 [details]
Patch

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

lgtm with one note that doesn't require any changes:

> LayoutTests/storage/indexeddb/mozilla/resources/bad-keypath.js:18
> +    event.target.transaction.onabort = unexpectedAbortCallback;

I'm wondering if we want this addition, especially while we're trying to make all the tests have a consistent initial setup. I'm of two minds:

(1) a dedicated test that if the transaction aborts, an "error" event is fired *should* be sufficient, and makes doing this in every test redundant.

(2) if we want them in each test, we could have indexedDBTest() do:

openRequest.addEventListener("upgradeneeded", function(evt) { evt.target.transaction.addEventListener("abort", unexpectedAbortCallback); }

I believe the W3C tests do something similar to #2 where they trap all events by default. I think I personally lean towards #1.

All that said, it looks like it's only been added in a few of these tests, so I say land this as-is.

[David Grogan]

(In reply to comment #11)
> (From update of attachment 173367 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173367&action=review
> 
> lgtm with one note that doesn't require any changes:
> 
> > LayoutTests/storage/indexeddb/mozilla/resources/bad-keypath.js:18
> > +    event.target.transaction.onabort = unexpectedAbortCallback;
> 
> I'm wondering if we want this addition, especially while we're trying to make all the tests have a consistent initial setup. I'm of two minds:
> 
> (1) a dedicated test that if the transaction aborts, an "error" event is fired *should* be sufficient, and makes doing this in every test redundant.

Good point.  I didn't do (2) because I wanted a test to have the option of allowing its versionchange transaction to abort.  I now see that that doesn't make sense because the unexpectedErrorCallback on the open request will be triggered on an abort.

> (2) if we want them in each test, we could have indexedDBTest() do:
> 
> openRequest.addEventListener("upgradeneeded", function(evt) { evt.target.transaction.addEventListener("abort", unexpectedAbortCallback); }
> 
> I believe the W3C tests do something similar to #2 where they trap all events by default. I think I personally lean towards #1.
> 
> All that said, it looks like it's only been added in a few of these tests, so I say land this as-is.

Yeah, I only started putting them in half-way.  I'll leave them out in the future.

[David Grogan]

Tony could you review this?

(For whatever definition of "review" is appropriate for a 300K test refactor.)

[Tony Chang]

Comment on attachment 173367 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 173367 [details]
Patch

Clearing flags on attachment: 173367

Committed r134252: <http://trac.webkit.org/changeset/134252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.