You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by filmaj <gi...@git.apache.org> on 2017/06/13 21:07:19 UTC
[GitHub] cordova-ios pull request #319: Updating unit tests + unit test runner and ad...
GitHub user filmaj opened a pull request:
https://github.com/apache/cordova-ios/pull/319
Updating unit tests + unit test runner and adding platformAPI addPlugin unit tests
# Summary of Changes
- moved `jasmine.json` to `tests/spec`, next to all the unit tests. eliminates the need to have an extra `spec/` dir at the top level.
- updated `package.json` scripts to use the new jasmine config file located at `tests/spec/jasmine.json`. also updated code coverage command to run the right thing.
- moved `bin/lib/check_reqs.js` and `bin/lib/versions.js` to the project template, under `bin/templates/scripts/cordova/lib`, and had the commands that are runnable globally (i.e. not from within a generated project) be able to reference them. as such, had to update `create.js` to change the require paths on the fly for these scripts. this, however, allows us to reference the build js modules directly, opening them up to unit testing. note that [android uses the same technique](https://github.com/apache/cordova-android/blob/master/bin/lib/create.js#L153-L159), and I think it is worth the tradeoff for eliminating try/catch around requires, calling `require` only at runtime, and allowing these modules to be unit tested.
- finally, added unit tests for platform API addPlugin method.
# Results
Worth noting that global code coverage dropped slightly, as reported by instanbul, but I think that is mostly due to now `check_reqs.js` and `versions.js` falling under the coverage 'umbrella' (and those scripts having little-to-no coverage at all). However, `Api.js`' code coverage went from 35% to 50%.
Please review @shazron.
FYI @stevengill @dpogue @kerrishotts @purplecabbage. Feel free to mention anyone else who you think should review or be aware of these changes.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/filmaj/cordova-ios platform-api-unit-tests
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-ios/pull/319.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #319
----
commit d7949036979e051a9a1ab040ead6e51b64d3fdb1
Author: filmaj <ma...@gmail.com>
Date: 2017-06-13T20:56:12Z
Updating unit tests + unit test runner:\
- moved jasmine.json to tests/spec, next to all the unit tests. elimiantes the need to have an extra spec/ dir at the top level.\
- updated package.json scripts to use the new jasmine config file. also updated code coverage command to run the right thing.\
- moved bin/lib/check_reqs.js and bin/lib/versions.js to the project template, and had the commands that are runnable globally (i.e. not from within a generated project) be able to reference them. as such, had to update create.js to change the require paths on the fly for these scripts. this, however, allows us to reference the build js modules directly, opening them up to unit testing.\
- finally, added unit tests for platform API addPlugin method.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Hmm, the diff just shows that those files were moved?
If you `git checkout -- .` from the repo source, and rerun, do you get the same behaviour?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Made some more changes, rebased, confirmed the unit tests now run on my Windows 10 VM.
Sucks to be running tests conditionally but I'm not sure what other option we have.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
I'm trying `npm test` now instead of `npm run unit-tests`... maybe the e2e tests are making changes to the git tree?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Ah, no, wait, I can repro if I run the full `npm test` - although those files are not missing in my git tree (git status is empty). 🤔
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Uh oh 😟 interesting, I don't see that in my local setup.
I must have messed the history up, thanks for checking. I'll try with a fresh clone myself and see if I can repro.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the issue:
https://github.com/apache/cordova-ios/pull/319
I cloned your fork and checked out the feature branch with all your commits.
Then:
```
$ npm install --dev
$ npm test
...
> cordova-ios@4.5.0-dev unit-tests /Users/shazron/Desktop/cordova-ios
> jasmine --config=tests/spec/jasmine.json
module.js:457
throw err;
^
Error: Cannot find module './lib/check_reqs'
at Function.Module._resolveFilename (module.js:455:15)
at Function.Module._load (module.js:403:25)
at Module.require (module.js:483:17)
at require (internal/module.js:20:19)
at Object.<anonymous> (/Users/shazron/Desktop/cordova-ios/bin/templates/scripts/cordova/Api.js:26:18)
at Module._compile (module.js:556:32)
at Object.Module._extensions..js (module.js:565:10)
at Module.load (module.js:473:32)
at tryModuleLoad (module.js:432:12)
at Function.Module._load (module.js:424:3)
npm ERR! Darwin 16.7.0
npm ERR! argv "/Users/shazron/.nvm/versions/node/v6.7.0/bin/node" "/Users/shazron/.nvm/versions/node/v6.7.0/bin/npm" "run" "unit-tests"
npm ERR! node v6.7.0
npm ERR! npm v3.10.9
npm ERR! code ELIFECYCLE
npm ERR! cordova-ios@4.5.0-dev unit-tests: `jasmine --config=tests/spec/jasmine.json`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the cordova-ios@4.5.0-dev unit-tests script 'jasmine --config=tests/spec/jasmine.json'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the cordova-ios package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR! jasmine --config=tests/spec/jasmine.json
npm ERR! You can get information on how to open an issue for this project with:
npm ERR! npm bugs cordova-ios
npm ERR! Or if that isn't available, you can get their info via:
npm ERR! npm owner ls cordova-ios
npm ERR! There is likely additional logging output above.
npm ERR! Please include the following file with any support request:
npm ERR! /Users/shazron/Desktop/cordova-ios/npm-debug.log
npm ERR! Test failed. See above for more details.
```
if I do a `git status` I see:
```
$ git status
On branch platform-api-unit-tests
Your branch is up-to-date with 'origin/platform-api-unit-tests'.
Changes not staged for commit:
(use "git add/rm <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
deleted: bin/templates/scripts/cordova/lib/check_reqs.js
deleted: bin/templates/scripts/cordova/lib/versions.js
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
I tried a fresh clone of my branch, `npm install --dev`, and `npm run unit-tests`, and can't repro - runs fine on my end. Also worth noting that those two files were not missing from my fresh clone.
Worth noting that after cloning and `npm install`ing, a bunch of files were changed under node_modules, according to `git`.
At what point in the process does your repo miss the files? After cloning? Or after `npm install`ing? OR maybe during the test run somehow?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios pull request #319: Updating unit tests + unit test runner and ad...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-ios/pull/319
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/cordova-ios/pull/319
# [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@8c77a0b`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `63.63%`.
[](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #319 +/- ##
=========================================
Coverage ? 63.77%
=========================================
Files ? 14
Lines ? 1623
Branches ? 277
=========================================
Hits ? 1035
Misses ? 588
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [bin/templates/scripts/cordova/lib/versions.js](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3ZlcnNpb25zLmpz) | `11.9% <ø> (ø)` | |
| [bin/templates/scripts/cordova/lib/check\_reqs.js](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2NoZWNrX3JlcXMuanM=) | `40% <ø> (ø)` | |
| [bin/templates/scripts/cordova/lib/build.js](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL2J1aWxkLmpz) | `36.84% <100%> (ø)` | |
| [bin/templates/scripts/cordova/Api.js](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvQXBpLmpz) | `53.89% <40%> (ø)` | |
| [bin/templates/scripts/cordova/lib/run.js](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=tree#diff-YmluL3RlbXBsYXRlcy9zY3JpcHRzL2NvcmRvdmEvbGliL3J1bi5qcw==) | `22.41% <68.75%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=footer). Last update [8c77a0b...82aaf01](https://codecov.io/gh/apache/cordova-ios/pull/319?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Do you run a code-syncing background service like `realsync` or something? Could that be doing it?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Yay, green! And looks like the codecov reporting is fixed too ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by shazron <gi...@git.apache.org>.
Github user shazron commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Same error.
Did:
```
$ git checkout -- .
$ npm test
```
and it shows the same two deleted files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-ios issue #319: Updating unit tests + unit test runner and adding pl...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-ios/pull/319
Props to @shazron for finding out there was an [integration test munging the cordova-ios repo](https://github.com/apache/cordova-ios/blob/8c77a0b991ace06b470dba9496a24e6ef75c2be3/tests/spec/create.spec.js#L115-L128).
Will remove and re-write as a unit test (in this case, for the `run --list` command).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org