Bug 58738 – Port Mozilla's IndexedDB tests: versionchange

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

[Mark Pilgrim (Google)]

[At fishd's suggestion, I'm splitting up the test suite in bug 58560 and getting them reviewed in chunks]

Original test: http://mxr.mozilla.org/mozilla2.0/source/dom/indexedDB/test/test_setVersion_events.html?force=1

This is a port of the Mozilla IndexedDB test for the versionchange event. It tests whether the versionchange event is triggered when multiple connections to the same database are open and one of them calls setVersion. WebKit passes this test already. Passing LayoutTest and expected output file attached.

This is a review-ready patch.

[Mark Pilgrim (Google)]

Created attachment 89937 [details]
LayoutTest + expected output file

[David Grogan]

Comment on attachment 89937 [details]
LayoutTest + expected output file

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

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:42
> +    // db1 will be open when we call db2.setVersion, which will trigger this

Put this in a debug("").

Copy/pasted comment from jorlow that's applicable: "Stuff like this should be in shouldBe/testPassed/debug()'s.  The idea is that you should be able to follow along via the on-screen output."

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:62
> +        shouldBe("event.target.version", "'1'");

Puzzled at the nested quotes - do we need them to pass the shouldBe assert?  Have I just never noticed them elsewhere in our tests?

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:70
> +    request.onsuccess = postSetVersion;

Can you add a listener for request.onblocked?  UnexpectedBlockCallback or function() { testPassed("db2 received blocked event") } would be sufficient.

I think we will fire a blocked event at db2 in spite of the call to db1.close, but I remember the spec being unclear on this point.  Do you know what firefox does?

> third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html:89
> +    // this will block because db2 is still open

Yeah, put this in debug("") inside the onblocked function

[Mark Pilgrim (Google)]

Created attachment 90061 [details]
LayoutTest + expected output file

Put all comments into debug() statements instead and updated expected output file to match.

Added listener for request.onblocked as requested. Yes, we call it one time even though versionchange calls db1.close. Mozilla does not call it at all. Not sure whose bug it is. Can file a separate bug to track it if desired. For now I put in a testPassed() call to document it.

Nested quotes are necessary when the string being compared is a constant. In this case, '1', not 1. If I remove the nested quotes, the assertion fails because it treats 1 as a number and complains of a type mismatch.

[David Grogan]

Unofficial r+.

(In reply to comment #3)
> Created an attachment (id=90061) [details]
> LayoutTest + expected output file
> Added listener for request.onblocked as requested. Yes, we call it one time even though versionchange calls db1.close. Mozilla does not call it at all. Not sure whose bug it is. Can file a separate bug to track it if desired. For now I put in a testPassed() call to document it.

Thanks.  I think it's our bug, the versionchangeevent is probably supposed to be synchronous, not asynchronous as we treat it now.  I'd appreciate it if you filed a bug pointing to https://bugs.webkit.org/attachment.cgi?id=90061 as a test case but don't worry about it if you're swamped.

[Mark Pilgrim (Google)]

Andrei, would you be willing to review this?

[Darin Fisher (:fishd, Google)]

Comment on attachment 90061 [details]
LayoutTest + expected output file

This patch needs a ChangeLog entry.  R-

Please see http://www.webkit.org/coding/contributing.html for guidance.

[Mark Pilgrim (Google)]

Created attachment 90958 [details]
patch with changelog and layouttest

added changelog to patch

[David Grogan]

Unofficial r+

[WebKit Commit Bot]

Comment on attachment 90958 [details]
patch with changelog and layouttest

Rejecting attachment 90958 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-sf-01', 'app..." exit_code: 2

Last 500 characters of output:
tTests/ChangeLog	(revision 84805)
|+++ third_party/WebKit/LayoutTests/ChangeLog	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange-expected.txt
patching file third_party/WebKit/LayoutTests/storage/indexeddb/mozilla/versionchange.html

Failed to run "[u'/Users/abarth/git/webkit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Fisher', u'--f..." exit_code: 1

Full output: http://queues.webkit.org/results/8510791

[Mark Pilgrim (Google)]

Created attachment 91213 [details]
rebase webkit directory

[Darin Fisher (:fishd, Google)]

Comment on attachment 91213 [details]
rebase webkit directory

missing the ChangeLog :(

[Mark Pilgrim (Google)]

Created attachment 91285 [details]
nits addressed once again (regression in previous patch)

[WebKit Commit Bot]

Comment on attachment 91285 [details]
nits addressed once again (regression in previous patch)

Rejecting attachment 91285 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8514456

[James Robinson]

Comment on attachment 91285 [details]
nits addressed once again (regression in previous patch)

commit queue hit a transient timeout, kicking to try again

[WebKit Commit Bot]

Comment on attachment 91285 [details]
nits addressed once again (regression in previous patch)

Clearing flags on attachment: 91285

Committed r85232: <http://trac.webkit.org/changeset/85232>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.