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

Prototype PhantomJS as a test runner #113

Closed
aaronpowell opened this issue Mar 11, 2015 · 17 comments · Fixed by #167
Closed

Prototype PhantomJS as a test runner #113

aaronpowell opened this issue Mar 11, 2015 · 17 comments · Fixed by #167

Comments

@aaronpowell
Copy link
Owner

PhantomJS 2.0 supports indexedDB (from my initial reading). I'd like to see how the tests run in PhantomJS and whether that can be used for CI instead of SauceLabs.

@aaronpowell
Copy link
Owner Author

Hopefully this can fix #108

@lygstate
Copy link
Contributor

Sounds good, may have a try, saucelabs is too slow.

@lygstate
Copy link
Contributor

Now the CI works, except the Pull request.

@aaronpowell
Copy link
Owner Author

SauceLabs isn't slow, I just have to fix my account on there. And the PR didn't fix the build nor did it add PhantomJS support, that is blocked by travis-ci/travis-ci#3225

@lygstate
Copy link
Contributor

The PR fixed the build! It's just because only your pull request can running the SauceLabs unittest,
I've already test locally!

@lygstate
Copy link
Contributor

It' implemented by #108

@aaronpowell
Copy link
Owner Author

Sadly PhantomJS still doesn't support IndexedDB. I could rely on one of the IDB shims but that seems a little dodgy...

@MartijnR
Copy link
Contributor

It does support it now (without shims). Oddly, version 0.10.2 works fine for me in PhantomJS 2.0.0. However, version 0.12.0 fails with an error:

NotFoundError: DOM IDBData/Users/martijnvanderijdt/Enketo/Code/enketo-express Exception 8: An operation failed because the requested data/Users/martijnvanderijdt/Enketo/Code/enketo-express object could not be found. (/var/folders/ng/dlcx_vv962g_kfzmttldhltr0000gn/T/82c49e6d590d0b39fabae986f211f0dd.browserify:2 <- node_modules/db.js/dist/db.min.js:1:0)

I'm not referring to the tests inside this repo, but a whole slew of my own tests of a layer built on top of db.js (so presumably it's the same).

Any thoughts what change in 0.12.0 might have caused this?

@aaronpowell
Copy link
Owner Author

Not that i can think of. Got a repo where I can see the tests?

@MartijnR
Copy link
Contributor

These are the tests: https://github.com/kobotoolbox/enketo-express/blob/master/test/client/store.spec.js of this store.js file.

However, I can try to create a failing phantomjs test in a port of db.js for easier demonstration, when I have time.

@aaronpowell
Copy link
Owner Author

I've just created a karma branch for my testing here, and yep I can repo the same problem.

Damn 😢

@MartijnR
Copy link
Contributor

ah! Well, the good news is that they'll probably pass in v0.10. I tried for a while to identify changes by looking at the code, but didn't see anything suspicious. It's probably a very subtle change.

@aaronpowell
Copy link
Owner Author

Been digging more into this and I'm actually getting closer.

The error that we're seeing is happening on this line: https://github.com/aaronpowell/db.js/blob/master/src/db.js#511

What's happening is that the for-in loop is going over a DOMStringList and then the hasOwnProperty method is called. Because the DOMStringList is empty it only has a single property name, length, but there is a bug in the implementation of hasOwnProperty that says "oh hey, that's part of the object", when really, it shouldn't be.

It eventually gets through to line 511 which it tries to remove an object store named length which doesn't exist and it falls over.

I'm currently trying to introduce a workaround when running in Phantom for that. I'm down to being able to run a 32 of the 83 tests. Woo!

@MartijnR
Copy link
Contributor

MartijnR commented Mar 1, 2016

That's great news! 👍

@aaronpowell
Copy link
Owner Author

I've just pushed a branch where the tests run under PhantomJS 😀

@MartijnR
Copy link
Contributor

MartijnR commented Mar 2, 2016

Woohooo!! :shipit:

@aaronpowell
Copy link
Owner Author

🎉

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

Successfully merging a pull request may close this issue.

3 participants