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

Get tests running under Node.js with IndexedDBShim #701

Open
aral opened this issue May 25, 2018 · 20 comments
Open

Get tests running under Node.js with IndexedDBShim #701

aral opened this issue May 25, 2018 · 20 comments

Comments

@aral
Copy link
Contributor

aral commented May 25, 2018

It’s possible to run Dexie under Node using IndexedDBShim (#359 (comment)), however, we do not know for certain that it behaves as expected as the test suite currently only runs in the browser environment.

Given that IndexedDBShim currently has some issues under Node, it would be good to know whether and how it effects expected behaviour in Dexie.

@dfahlander Do you have any thoughts on a strategy to make the test suite isomorphic so that it can be run under both browsers and Node?

@aral
Copy link
Contributor Author

aral commented May 27, 2018

Just a quick update that I’m working on this.

Made some progress using node-qunit but that wasn’t picking up the async tests (it’s not actively maintained and I didn’t deep dive into why it wasn’t working).

Instead, I just got the first batch of test (in tests-table.js) to run under Node using qunit-tap (after seeing this comment by @leobalter – thanks, Leo.)

Currently, it’s a terrible hack (I was just trying to get it working) and I’ve hardcoded some browser-based globals that dexie-unittest-utils.js was setting, etc. I also replaced the imports with requires to get it working.

Next step: going to try and make a test runner that doesn’t require any code changes on the existing tests. Will keep this issue updated on my progress and push a brach to my fork when I have something a bit more presentable.

PS. Currently, all tests in tests_table.js are passing under Node, except for test 5 (not ok 5 - Transaction failed: ReferenceError: IDBKeyRange is not defined, expected: true, got: false, test: where, module: table) and 17 (not ok 17 - ReferenceError: IDBKeyRange is not defined, expected: true, got: false, test: put, module: table). Will take a quick look into those also.

dfahlander added a commit that referenced this issue May 27, 2018
Global IDBKeyRange was used in WhereClause.
@dfahlander
Copy link
Collaborator

Thanks a lot for looking into this. The tests will definitely need to be converted to universal JS in one or other way. I just looked up the issue with IDBKeyRange, and found a few locations in WhereClause. Should be the only ones using it. It's now fixed in master.

@aral
Copy link
Contributor Author

aral commented May 28, 2018

Awesome news on the IDBKeyRange issues. Going to start working on a very minimalist test runner for Node now. Basically, my plan is to:

  1. Update the tests themselves so any browser-specific globals are conditionally applied or replaced with defaults that make sense for Node.
  2. Concatenate all the tests into a single file, removing the imports (and requiring them in one place at the top).
  3. Wrap it with qunit-tap and the IndexedDBShim and run them.

🤞

@dfahlander
Copy link
Collaborator

dfahlander commented May 28, 2018

Thanks! Whatever method you choose to make it run on node, I'm extraordinary happy. Maybe rollup could be of some help with the bundling of all modules into a single CommonJS based one? Currently the test suite is bundled using rollup config in tools/build-configs/rollup.tests.config.js. Maybe just copy that one, remove the 'external' parts (QUnit and dexie) and mock them using using https://github.com/rollup/rollup-plugin-alias?

Edit: the rollup config file currently outputs UMD format, which should detect commonjs and use require() where needed. To make it even more clean, that config file would probably be better off using output.format: "cjs" instead.

@aral
Copy link
Contributor Author

aral commented May 28, 2018

@dfahlander, I just got all the tests running with a custom test runner for Dexie/Node :) Going to issue a pull request in a few minutes – please don’t merge that (I need to update the code styles to fit the project, etc.) Just issuing it so you can take a look. It makes a best effort to get through the whole test suite (and does) but some tests are failing and others are timing out. It feels like a good start though :)

@aral aral mentioned this issue May 28, 2018
@aral
Copy link
Contributor Author

aral commented May 29, 2018

@dfahlander When you’ve had a chance to look at the test results, can you please let me know what your thoughts are on the current state of Dexie on Node with IndexedDBShim. Do you see any major issues? (I will look through the results also but I’m, of course, not as familiar with the project as you are.) Would be great, once you have some time, to triage the fixes.

I hope the tests help and if there’s anything more I can do, please let me know. I’d really like to use this production on Indie Site unless you see major concerns/showstoppers :)

@dfahlander
Copy link
Collaborator

dfahlander commented May 29, 2018

Yes, I will have a look as soon as possible. Please remind me if no feedback from me tomorrow morning.

@aral
Copy link
Contributor Author

aral commented May 29, 2018

@dfahlander Thanks, David. I’m in the process of creating a single issue that summarises all of the issues in a single place to hopefully make your life easier :)

@dfahlander
Copy link
Collaborator

dfahlander commented May 29, 2018

Tried your branch but got Error: Cannot find module 'websql/custom'.

Repro:

npm install
npm run test:node

# Preparing Dexie tests for Node.js.
# Preparing unit test utilities for use in Node.
# ########################################
# tests-table.js
# ########################################
module.js:538
    throw err;
    ^

Error: Cannot find module 'websql/custom'
    at Function.Module._resolveFilename (module.js:536:15)
    at Function.Module._load (module.js:466:25)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at o (/Users/daw/repos/dexie2/node_modules/indexeddbshim/dist/indexeddbshim-node.js:1:563)
    at /Users/daw/repos/dexie2/node_modules/indexeddbshim/dist/indexeddbshim-node.js:1:754
    at Object.22../CFG (/Users/daw/repos/dexie2/node_modules/indexeddbshim/dist/indexeddbshim-node.js:6712:15)
    at o (/Users/daw/repos/dexie2/node_modules/indexeddbshim/dist/indexeddbshim-node.js:1:703)
    at /Users/daw/repos/dexie2/node_modules/indexeddbshim/dist/indexeddbshim-node.js:1:754
    at Object.21../CFG (/Users/daw/repos/dexie2/node_modules/indexeddbshim/dist/indexeddbshim-node.js:6691:19)
{ Error: Exited with code 1
    at ChildProcess.child.child_process.fork.on (/Users/daw/repos/dexie2/node_modules/child-process-es6-promise/index.js:106:37)
    at emitTwo (events.js:126:13)
    at ChildProcess.emit (events.js:214:7)
    at maybeClose (internal/child_process.js:925:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:209:5) code: 1, stderr: '', stdout: '', signal: null }
...

@aral
Copy link
Contributor Author

aral commented May 29, 2018

I wonder if that’s because I added IndexedDBShim as a dev dependency? Nuking my node_modules now and doing a fresh npm install to try and reproduce. Will update.

@aral
Copy link
Contributor Author

aral commented May 29, 2018

Hmm, just nuked node_modules and did a fresh npm install. It’s not that…

Update: Also did an npm cache verify, still working on my end… 🤔

@aral
Copy link
Contributor Author

aral commented May 29, 2018

What Node version are you running? (grasping at straws here, but who knows…) I’m on a Mac (latest High Sierra) running node v8.11.1 and npm version 6.0.1.

Update: And I’m using nvm… I wonder if its linked in the SQLite native module somehow on my machine independent of the local npm install.

@dfahlander
Copy link
Collaborator

I'll try remove node_modules and retry...

@dfahlander
Copy link
Collaborator

Still issue but I'll upgrade node and npm... I'm also on mac high sierra.

@dfahlander
Copy link
Collaborator

dfahlander commented May 29, 2018

Got your test suite for node running! Thanks!

Analyse

  • Sorting issues. Seems it does not order results on queried index. (
    table: 23,24,25, 104, 105
    collection: 10, 11, 13, 40,
  • QUnit patching error (notEqual (or deepEqual) not polyfilled) (table 114, 115, upgrading 25
  • Times out instead of throwing when accessing a non-existing table (
    table 123, transaction 10, 12, 14,
  • Strange errors: collection 68, 70, 72, 74: str.replace is not a function.
  • Charset incompatibility: collection 92 - 101.
  • Unknown problems: collection 227. Maybe a lack of multientry support in IndexedDBShim.
  • Creating multiple databases: open 18, 19, 20, 21,
  • Native async await not supported: transaction 12,
  • unhandledrejection event may not work as expected in node:
    exception-handling 1
    misc 12,13, 28, 29, 30
  • General timeout instead of exceptions: exception-handling 6, 7, 8,
  • Issue DB unresponsive after multiple Table.update() or Collection.modify() #360 seems to be present: misc 18
  • Blob compatibility: blob 1, 3. Blobs are DOM-specific and should not be used in node anyway. For supporting blobs in IndexedDBShim, it must be able to structure-clone it. Workaround: Use binary arrays instead (such as UInt8Array).

Summary

Most operations goes fine. All basic normal stuff seems to work, even IndexedDB 2.0 features such as binary keys, are supported, but beware the following:

  • Sorting seems not to work correctly with IndexedDBShim on node. That is, db.table.orderBy() is not useful, nor can you rely on the order of rows returned from a query.
  • Error handling suffers from timing out instead of throwing for some operations. May be related to Dexie src using CustomEvent to trigger unhandled rejections. May be solved in Dexie src. Could possibly also be that IndexedDBShim would not handle errors as expected.
  • Seems not to support certain unicode chars (collection test numbers 92 - 101 where we generate a string out of random characters).
  • Seems to be issues working on multiple databases at the same time.
  • Native async await can result in that a transaction is committed too early (which is also the case in Firefox. This is due to incompatiblity between IndexedDB(Shim) and the native Promise). Transpiled async await still fine.
  • unhandledrejection event not signalled as expected. Reason: We trigger the event via a CustomEvent which is DOM-specific. Could be solved in Dexie src.
  • Use UInt8Array instead of Blob to support both node and browser.

@aral
Copy link
Contributor Author

aral commented May 29, 2018

@dfahlander Thank you so much, I’m still in the middle of creating a separate issue with all the test failures for easy reference (got interrupted). Your analysis and summary are invaluable. Hugely appreciate it :)

@aral
Copy link
Contributor Author

aral commented May 29, 2018

@dfahlander
Copy link
Collaborator

dfahlander commented May 29, 2018

I think indexeddbshim/IndexedDBShim#296 regards more to that native async/await commits too early. As long as the tests stick to using the global.Promise or Dexie.Promise, and transpile async/await instead of using native raw async/await, this should not be an error as Dexie works around this exact same issue (in Firefox, IE + older versions of Edge and Safari).

@aral
Copy link
Contributor Author

aral commented May 29, 2018

I’ve confirmed that the tests are, in fact, using Dexie.Promise (via instanceof) and not native promises so, based on what you wrote above, that’s not the issue. If you have any ideas on how we can go about fixing the sorting/ordering issues, please do let me know. I’m a bit out of my depth here but willing to dive in and learn to help if I can :)

@dfahlander
Copy link
Collaborator

I am suspecting a collation issue in the websql module maybe. IndexedDB assumes ordinal sort while it may be the case that sqlite defaults to another collation. Just a theory though.

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

No branches or pull requests

2 participants