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 all unit tests passing #178

Closed
JamesMessinger opened this issue Mar 23, 2015 · 25 comments
Closed

Get all unit tests passing #178

JamesMessinger opened this issue Mar 23, 2015 · 25 comments

Comments

@JamesMessinger
Copy link
Collaborator

@axemclion @DickvdBrink - As we discussed a couple weeks ago, I've added a bunch of new unit tests (221 of them!) to the project. These tests are in a separate directory from the QUnit tests and are not run as part of the CI process, since almost all of them fail when run with IndexedDBShim. The goal is to always have a working CI process with passing tests, so the QUnit tests are the official test suite. As we fix bugs in IndexedDBShim and get the Mocha tests passing, we can convert them to QUnit and add them to the official test suite.

I've tested on every browser, platform, and device that I have available, and here are the results:

Browsers with Native IndexedDB

Browser Platform Passing tests
Internet Explorer 11 Windows All 221 tests pass
Firefox Windows and Mac All 221 tests pass
Chrome Windows and Mac All 221 tests pass
Opera Windows and Mac All 221 tests pass
Safari Mac All 221 tests pass
Mobile Safari iOS 8 All 221 tests pass
Chrome Mobile Android 4.4 and 5.0 All 221 tests pass
IE Mobile Windows Phone 8.1 All 221 tests pass
Cordova App Android 4.4 and 5.0 All 221 tests pass

Browsers without Native IndexedDB (using IndexedDBShim)

Browser Platform Passing tests
Safari 5 Windows 17 tests pass
Mobile Safari iOS 7 40 tests pass
Android Browser Android 4.2 24 tests pass
Cordova App iOS 8 11 tests pass
Cordova App Android 4.2 24 tests pass
Cordova App Windows Phone 8.1 108 tests pass

So, now to get all those tests passing! :)

@JamesMessinger
Copy link
Collaborator Author

Updated stats as of today's commits:

Browser Platform Passing tests
Safari 5 Windows 76 tests pass
Mobile Safari iOS 7 53 tests pass
Android Browser Android 4.2 78 tests pass
Cordova App iOS 8 78 tests pass
Cordova App Android 4.2 78 tests pass
Cordova App Windows Phone 8.1 76 tests pass

@JamesMessinger
Copy link
Collaborator Author

Updated stats as of today's commits:

Browser Platform Passing tests
Safari 5 Windows 95 tests pass
Mobile Safari iOS 7 95 tests pass
Android Browser Android 4.2 95 tests pass
Cordova App iOS 8 133 tests pass
Cordova App Android 4.2 95 tests pass
Cordova App Windows Phone 8.1 93 tests pass

@axemclion
Copy link
Collaborator

@BigstickCarpet Thanks for the awesome work !!

@DickvdBrink
Copy link
Collaborator

Wow, I really like this! Thanks for the awesome work!

@DibranMulder
Copy link
Contributor

@BigstickCarpet Correct me if i'm wrong but Windows Phone 8.1 doesn't have IndexedDb or WebSQL support for files that are loaded from the local file system, am I right? So how can it be that 93 tests are passing? Are you using a SQLite database? And if so, is it on github?

Thanks for the great work, keep it up :)

@JamesMessinger
Copy link
Collaborator Author

@nimrodxx - IE Mobile on Windows Phone 8.1 natively supports IndexedDB. But in a Cordova app, it requires the IndexedDB plugin, which uses IndexedDBShim (this project) and the WebSQL plugin internally

@DibranMulder
Copy link
Contributor

@BigstickCarpet Yeah, I already thought you would came up with that solution. However as pointed out in the issues of the WebSQL plugin. It performs very bad and it is completely sync where the IndexedDb and WebSQL interfaces are async. So that leaves a difference in the interface and most important the user experience.

But wait, @DickvdBrink and I managed to get an async variant of the WebSQL plugin to work. Hopefully this fixes some unit tests. We are persuading our company to opensource it. Ill keep you in touch.

@DickvdBrink
Copy link
Collaborator

@BigstickCarpet, if I recall correctly the WebSQL plug-in returns true\false (or 1\0 cant really remember) while a string was expected for storeProps.

The code I'm talking about is:
https://github.com/axemclion/IndexedDBShim/blob/be681b1d61101bcf08a4c6ac4bf0d466d94f0e1c/src/IDBObjectStore.js#L109

Causing this code to fail: https://github.com/axemclion/IndexedDBShim/blob/be681b1d61101bcf08a4c6ac4bf0d466d94f0e1c/src/IDBObjectStore.js#L157

Hopefully I can check this soon (or maybe @nimrodxx can do it because we work on the same project :))

@JamesMessinger
Copy link
Collaborator Author

@DickvdBrink & @nimrodxx - Thank you guys for helping knock out a few bugs, especially with the WebSQL plug-in, which I am not very familiar with. I just discovered a new issue with the WebSQL plug-in today. It's not throwing errors when primary key constraints are violated. Sounds like this bug, except that bug was supposedly fixed back in 2012. So I'm not sure what's up.

Anyway... here are the new numbers, as of today's commits:

Browser Platform Passing tests
Safari 5 Windows 120 tests pass
Mobile Safari iOS 7 120 tests pass
Android Browser Android 4.2 175 tests pass
Cordova App iOS 8 120 tests pass
Cordova App Android 4.2 175 tests pass
Cordova App Windows Phone 8.1 159 tests pass
Click here to run the tests

@DibranMulder
Copy link
Contributor

@BigstickCarpet how do you run your test on Windows Phone 8.1? Because your test app (https://github.com/BigstickCarpet/cordova-test-app) opens the default browser on our device and all tests pass in that browser.

@JamesMessinger
Copy link
Collaborator Author

@nimrodxx - Yeah, unfortunately Windows Phone 8.1 doesn't allow remote code execution inside a Cordova app, so my Cordova Test App only really works for iOS and Android. To test on Windows Phone 8.1, I just created a Cordova app, added the WebSQL plugin, and then copied the tests directory into the www folder. That allows me to run the unit tests inside of a Cordova webview.

@JamesMessinger
Copy link
Collaborator Author

Here are the new numbers, as of today's commits:

Browser Platform Passing tests
Safari 5 Windows 146 tests pass
Mobile Safari iOS 7 121 tests pass
Android Browser Android 4.2 213 tests pass
Cordova App iOS 8 214 tests pass
Cordova App Android 4.2 213 tests pass
Cordova App Windows Phone 8.1 188 tests pass
Click here to run the tests

@JamesMessinger
Copy link
Collaborator Author

DONE!!!

All 269 Mocha unit tests and 30 QUnit tests are now passing on every desktop and mobile platform that I have available to test:

optimized-screenshots

There are some caveats though:

  1. Safari on Mac
    Safari chokes when running all 269 tests. After about ~130 tests, they all start failing. But if you break the tests up and only run ~100 at a time, they all pass.
  2. Windows Phone 8.1
    There are 13 tests that fail due to bugs in the WebSql plug-in. Namely, these 11 tests fail due to this bug, and these two fail because the WebSql plug-in is synchronous instead of asynchronous. Once those bugs are fixed in the WebSql plug-in, all tests should pass successfully.

@DickvdBrink
Copy link
Collaborator

Wow this is really nice!!
I created a async WebSql plugin at my work for Windows Phone. Need to speak with my employer about this :)
Not sure if the exception bug is there because I'm using the native SQLite library instead of the managed C# one!

Again a big thank you! :)

@JamesMessinger
Copy link
Collaborator Author

Hopefully they'll let you publish it. Open-source only works when it goes both ways. I wish more companies understood that.

@DibranMulder
Copy link
Contributor

@BigstickCarpet @DickvdBrink Just received the green light to opensource our async WebSQL plugin. We are planning to release it asap. I'll keep you posted.

@JamesMessinger
Copy link
Collaborator Author

👍 That's great news! We will almost certainly end up using it at my company, since we target Windows Phone as our primary platform.

Will you be publishing it as a separate WebSql plugin? Or are you planning to submit a PR and get your fixes merged into the MSOpenTech plugin?

@DibranMulder
Copy link
Contributor

Well I think that a separate WebSql plugin is the way to go, since we have changed almost every line of code. But I will have to discuss that with @DickvdBrink

@DibranMulder
Copy link
Contributor

@BigstickCarpet Hey we just released the WebSQL plugin. We didn't have time to test it with the new version of the shim but we will pick it up soon.

@JamesMessinger
Copy link
Collaborator Author

Badass! Looking at it right now. Might have it integrated into my company's mobile apps by the end of the day. Thanks for contributing it!

@kir
Copy link
Contributor

kir commented Apr 17, 2015

This is totally awesome to see this issue fixed. Really great job, thank you very much.

@DickvdBrink
Copy link
Collaborator

I'm not sure if the plugin works right now because I remember making some changes in the shim to make it work. We might need to incorporate them in this shim. But it is a start :)

@JamesMessinger
Copy link
Collaborator Author

Yep. Working on that now. It'll require some changes to the shim and to your plug-in. I'll submit a PR

@axemclion
Copy link
Collaborator

@nimrodxx Have you guys published the plugin on npm ? Note that Cordova plugins are moving to NPM.

@DickvdBrink
Copy link
Collaborator

@axemclion no we haven't because we need to do some more testing and check what additional changes are required on the shim (we have a slightly modified version). After more testing/documentation it will be published on npm with the required keywords :) I'm on the cordova-list so I know about the changes for npm.

edit noticed the fork with the package.json! Nice :)

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

5 participants