Skip to content

IndexedDB: Add tests that verify the source of IDBRequests #7680

Merged
inexorabletash merged 2 commits intoweb-platform-tests:masterfrom
inexorabletash:idb-sources
Oct 17, 2017
Merged

IndexedDB: Add tests that verify the source of IDBRequests #7680
inexorabletash merged 2 commits intoweb-platform-tests:masterfrom
inexorabletash:idb-sources

Conversation

@inexorabletash
Copy link
Contributor

@inexorabletash inexorabletash commented Oct 11, 2017 •
edited by wpt-pr-bot
Loading

Missing coverage noted in w3c/IndexedDB#213

Verify the source property of requests returned by operations made against stores, indexes, and cursors.

@inexorabletash inexorabletash requested a review from pwnall October 11, 2017 21:03
@ghost
Copy link

ghost commented Oct 11, 2017 •
edited by ghost
Loading

Build PASSED

Started: 2017-10-17 22:10:02
Finished: 2017-10-17 22:13:55

Failing Jobs

  • MicrosoftEdge:14.14393

Unstable Results

Browser: "Microsoftedge 14.14393" (failures allowed)

View in: WPT PR Status | TravisCI

Test Subtest Results Messages
/IndexedDB/idbindex-request-source.html   OK: 2
TIMEOUT: 8
  The source of the request from index => index.count() is the index itself PASS: 2
  The source of the request from index => index.get(0) is the index itself PASS: 2
  The source of the request from index => index.getAll() is the index itself FAIL: 2
 Object doesn't support property or method 'getAll'
  The source of the request from index => index.getAllKeys() is the index itself FAIL: 2
 Object doesn't support property or method 'getAllKeys'
  The source of the request from index => index.getKey(0) is the index itself PASS: 2
  The source of the request from index => index.openCursor() is the index itself PASS: 2
  The source of the request from index => index.openKeyCursor() is the index itself FAIL: 2
 assert_unreached: open should succeed Reached unreachable code
/IndexedDB/idbobjectstore-request-source.html   OK: 6
TIMEOUT: 4
  The source of the request from store => store.add(0) is the object store itself PASS: 6
  The source of the request from store => store.clear() is the object store itself PASS: 6
  The source of the request from store => store.count() is the object store itself PASS: 6
  The source of the request from store => store.delete(0) is the object store itself PASS: 6
  The source of the request from store => store.get(0) is the object store itself PASS: 6
  The source of the request from store => store.getAll() is the object store itself FAIL: 6
 Object doesn't support property or method 'getAll'
  The source of the request from store => store.getAllKeys() is the object store itself FAIL: 6
 Object doesn't support property or method 'getAllKeys'
  The source of the request from store => store.getKey(0) is the object store itself FAIL: 6
 Object doesn't support property or method 'getKey'
  The source of the request from store => store.openCursor() is the object store itself PASS: 6
  The source of the request from store => store.openKeyCursor() is the object store itself FAIL: 6
 assert_unreached: open should succeed Reached unreachable code
  The source of the request from store => store.put(0) is the object store itself PASS: 6

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Oct 12, 2017 •
edited
Loading

Weird... Edge will throw due to unsupported functions (getAll, getAllKeys, getKey/openKeyCursor on store)... but why is it flaky? And why is it hitting the 'open should succeed' error? Hrm...

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Oct 12, 2017

I've narrowed down the flakiness to what appears to be an Edge bug - cross-talk between operations on unrelated databases:

function t(dbname) {
  indexedDB.deleteDatabase(dbname).onsuccess = e => {
    const open = indexedDB.open(dbname, 1);

    open.onupgradeneeded = e => {
      const db = open.result;
      db.createObjectStore('store');
    };

    open.onerror = e => { 
      console.error('open should succeed');
    };

    open.onsuccess = e => {
      console.log('open succeeded');
      const db = open.result;
      db.close();
      indexedDB.deleteDatabase(dbname);
    };
  };
}

t('db1');
t('db2');

The second open flakily succeeds or fails.

@siusin
Copy link
Contributor

siusin commented Oct 13, 2017

/cc @nolanlawson
any suggestion on this Edge bug?

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Oct 13, 2017 •
edited
Loading

I sent the repro to @aliams as well.

In case it's not clear, the repro has nothing to do with the features under test here. It's an unfortunate coincidence. It likely causes flakiness in other tests on Edge, but it's worst here because these test bodies are effectively synchronous; the test steps don't need to wait for a request's response to come back, so the race is "lost" far more frequently than in the more common tests with an asynchronous request/response cycle.

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Oct 13, 2017

Hah, per @aliams - @nolanlawson discovered this issue 3 years ago: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/175642/

pwnall
pwnall approved these changes Oct 13, 2017
View reviewed changes
Copy link
Contributor

@pwnall pwnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one nit.

IndexedDB/idbcursor-request-source.html Outdated
store.put('value');
store.openCursor().onsuccess = t.step_func(e => {
const cursor = e.target.result;
assert_equals(eval(expr).source, cursor,
Copy link
Contributor

@pwnall pwnall Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You could avoid eval here by having each string be a closure instead, and using toString() on the closure. To be clear, here's some incomplete code below.

[
  cursor => { cursor.update(0) },
].forEach(fn ...

assert_equals(fn(cursor), cursor, `${fn}`.source ... })

Same for the cases below.

Copy link
Contributor Author

@inexorabletash inexorabletash Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Ms2ger
Copy link
Contributor

Ms2ger commented Oct 16, 2017

@inexorabletash you planning to fix the not?

@inexorabletash
Copy link
Contributor Author

inexorabletash commented Oct 16, 2017

Yeah, it's just been the weekend in this TZ and I'm OOO from work today.

If anyone else wants to push a change to this PR and land I'd be okay with that, though.

@inexorabletash inexorabletash merged commit 6834933 into web-platform-tests:master Oct 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pwnall pwnall pwnall approved these changes

@chunywang chunywang Awaiting requested review from chunywang

@dumbmatter dumbmatter Awaiting requested review from dumbmatter

@odinho odinho Awaiting requested review from odinho

@yunxiaoxie yunxiaoxie Awaiting requested review from yunxiaoxie

@zhaozihao zhaozihao Awaiting requested review from zhaozihao

@zqzhang zqzhang Awaiting requested review from zqzhang

Assignees

No one assigned

Labels

IndexedDB wg-webapps

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@inexorabletash @siusin @Ms2ger @pwnall @wpt-pr-bot