Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Order results correctly when using getAll() and getAllKeys() #326

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dfahlander
Copy link

This will hopefully resolve the ordering issue listed in
dexie/Dexie.js#709

dexie/Dexie.js#701

as referred from #325.

@dfahlander
Copy link
Author

Seems the travis tests failed. Was not able to test locally either. What I did for testing was building the shim and run the dexie tests on it. Still, there are failing tests, but many went from failed to OK. Am I supposed to commit the files in dist together with the source? Would appreciate any help in contributing to IndexedDBShim.

@brettz9
Copy link
Collaborator

brettz9 commented May 31, 2018

This is great, thanks! Can you point me to the particular Dexie tests failing for this case if it is something that could, without too much trouble, map to a vanilla IDB test?

@brettz9
Copy link
Collaborator

brettz9 commented May 31, 2018

And I believe the Travis tests may always fail on the forks...

@brettz9 brettz9 added the Bug label May 31, 2018
@dfahlander
Copy link
Author

@brettz9 I think both Dexie and IndexedDBShim would benefit from integrating the dexie test suite with IndexedDBShim applied - both in browser and on node. This is what @aral is up to. I plan to make a test config in dexie that runs all tests with the shim applied shim applied (one config for using the shim in browser and one for using it on node).

Since there seems to be several tests that fails with the shim, I believe it's not worth the effort to rewrite them to use vanilla indexeddb testing. This could also become a more sustainable road forward, if it will be easy to the the shim in dexie tests and get regression tests in all future versions.

I think me and @aral is on this right now, and we'll try finding and fixing issues in best-effort. Let's see what comes out from that, as a first step. I'll let you know if we need more help.

This fix made 2 other assertions in dexie unit tests turn from fail to ok.
@dfahlander
Copy link
Author

After the last commit, another 2 assertions make it through in the dexie tests. The last commit was that IDBIndex.getAll() should not just ORDER BY the queried key, but also on primary key.

@brettz9
Copy link
Collaborator

brettz9 commented Jun 1, 2018

With your PR I'm now getting failures on 'idbindex_getAll' and 'idbindex_getAllKeys', two W3C web-platform-tests tests that we were not getting failures on previously.

Btw, I just updated master with a fix to Grunt for testing and to the testing Docs.

The tests you want to run are npm run w3c, but you must first follow the installation instructions at https://github.com/w3c/web-platform-tests/ (wasn't too difficult).

@dfahlander
Copy link
Author

@brettz9 thanks for guiding in how to run the tests. I'm trying to setup the test environment to debug it.

@dfahlander
Copy link
Author

dfahlander commented Jun 1, 2018

I've got no luck running the w3c tests. Installed the WPT according to their instructions. Then, the instructions in docs/TESTING did not guide me in how to proceed from there exactly, but as I had installed web-platform-tests in its own location, i symlinked IndexedDBShim/web-platform-tests into that location. Then, I tried both npm run w3c-add-wrap followed by npm run w3c (according to your instructions), but no luck so ran npm run w3c-wrap but still no luck:

missing?:http://localhost:9999/node_modules/babel-polyfill/dist/polyfill.js
missing?:http://localhost:9999/dist/indexeddbshim-noninvasive.js
evalmachine.<anonymous>:3526
    setGlobalVars(null, {
    ^

ReferenceError: setGlobalVars is not defined

@brettz9
Copy link
Collaborator

brettz9 commented Jun 5, 2018

Sorry, I can't figure out why I sometimes don't seem to get notifications from Github issues...

The relevant section of the testing page is no. 4 of https://github.com/axemclion/IndexedDBShim/blob/master/docs/TESTING.md#node-testing

After web-platform-tests are running (npm run web-platform-tests), in a separate process run npm run w3c.

The wrap tests are so you can test IndexedDBShim in the browser using their tests.

@brettz9
Copy link
Collaborator

brettz9 commented Dec 10, 2018

The page at https://github.com/axemclion/IndexedDBShim/blob/master/docs/TESTING.md now has additional info on the now headless testing one can do through the web-platform-tests library (their new recommended way of doing browser testing): ./wpt run --headless chrome IndexedDB.

However, if you'd rather avoid setting up the testing environment, you could just extract the JavaScript within the relevant tests at https://github.com/web-platform-tests/wpt/blob/master/IndexedDB/idbindex_getAll.html and https://github.com/web-platform-tests/wpt/blob/master/IndexedDB/idbindex_getAllKeys.html

@brettz9
Copy link
Collaborator

brettz9 commented Dec 10, 2018

Though I forgot those had some dependencies you'd have to grab too...

@brettz9
Copy link
Collaborator

brettz9 commented Mar 17, 2020

In master, I have now installed npm-run-all to allow parallel processes, so it should be easier now if you might be able to revisit this (running npm run w3c).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants