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/22 20:44:58 UTC
[GitHub] cordova-lib pull request #568: Reorganized unit test directory structure + u...
GitHub user filmaj opened a pull request:
https://github.com/apache/cordova-lib/pull/568
Reorganized unit test directory structure + updated npm task names
### What does this PR do?
This is a proof-of-concept pull request mainly introduction file organizational changes. Please treat this PR as a discussion point, not as something that needs to be mergeable right away. I wanted to get the feedback of the community and PMC on this change before I went ahead with anything.
Ping @purplecabbage, @stevengill, @shazron, @mwbrooks, @devgeeks, @surajpindoria, @imhotep, @dpogue, @kerrishotts, @matrosov-nikita, @goya, @jcesarmobile, @macdonst, @infil00p. If I missed anyone, please pull them in too ;)
The single biggest change, and takeaway, from this PR is that the unit tests under `spec-cordova/` and `spec-plugman/` are consolidated into a single top-level `spec/` directory. The idea behind this change (and hopefully future to come changes) is that there will exist one unit test spec file per javascript source file as a rough rule-of-thumb. Further, the test directory structure should mirror, as closely as possible, the source directory structure. I feel that that is an implied expectation when it comes to unit test file structures. I think possibly one reason why so many integration tests exist in this repository is that that structure right now is not honoured. So, when contributions come in, the typical expectation of "I made a change in src/foo.js, but I don't see a unit test file under spec/foo.spec.js" is broken, and people feel they have to write integration tests to get the kind of test coverage their contribution requires. Hopefully we can break that pattern here an
d continue to march towards a healthier and faster test suite.
Other than the directory changes, a summary of smaller but also relevant changes:
- consolidated the differing spec-plugman and spec-cordova jasmine configuration files
- consolidated all the helper modules (there are three!?) under the top-level `spec/` directory: `helper.js`, `helpers.js` and `common.js`. Combining those into one test-helper file is left as an exercise for a future pull request :)
- changed `package.json` `npm run` tasks to better reflect the purpose of the task. In particular, changed `npm run jasmine` to `npm run unit-tests`. I also thought of changing the `jshint` task to be `linter`, but then decided against it. I am open to hearing opinions on this.
- related to the last point, now running `npm test` runs the integration tests _as well_. They currently take around 400 seconds to run on my machine, but without them right now we cannot guarantee we will have regressions. I am hoping that as we rewrite more and more integration tests into smaller, faster, more focussed unit tests, the integration test times will continue to drop.
- made some README changes to reflect the change in `npm` task names, and also fixed the link to the issue tracker :)
### What testing has been done on this change?
`npm test`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/filmaj/cordova-lib spec-consolidation
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/568.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 #568
----
commit bc4218e3d09682b0bdcd5a2a119e29c0561a03ea
Author: filmaj <ma...@gmail.com>
Date: 2017-06-22T20:29:57Z
Reorganized unit test directory a little bit, consolidated into a single spec/ dir. Put jasmine config and helper modules in top-level spec dir. Changed package.json npm run scripts to reflect purposes of tasks. Updated readme to reflect package.json npm run script changes.
----
---
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-lib issue #568: Reorganized unit test directory structure + updated ...
Posted by imhotep <gi...@git.apache.org>.
Github user imhotep commented on the issue:
https://github.com/apache/cordova-lib/pull/568
I support this change. I believe it will make writing tests for new features easier if each source file has a matching spec file and all source/spec folders are mirrored.
---
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-lib pull request #568: Reorganized unit test directory structure + u...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj closed the pull request at:
https://github.com/apache/cordova-lib/pull/568
---
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-lib issue #568: Reorganized unit test directory structure + updated ...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-lib/pull/568
Thanks for the feedback @stevengill, good point. I've just removed `npm run ci` and updated travis and appveyor to just run `npm test` - just like a developer would on their local machine.
---
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-lib issue #568: Reorganized unit test directory structure + updated ...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the issue:
https://github.com/apache/cordova-lib/pull/568
looks good to me. Only thing I would add is updating travis and appveyor as well. They run the integration tests as a separate task currently.
---
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-lib issue #568: Reorganized unit test directory structure + updated ...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/cordova-lib/pull/568
# [Codecov](https://codecov.io/gh/apache/cordova-lib/pull/568?src=pr&el=h1) Report
> Merging [#568](https://codecov.io/gh/apache/cordova-lib/pull/568?src=pr&el=desc) into [master](https://codecov.io/gh/apache/cordova-lib/commit/1a28929564d32d8a85dca039ee8f34656c594dfd?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/cordova-lib/pull/568?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #568 +/- ##
=======================================
Coverage 44.11% 44.11%
=======================================
Files 78 78
Lines 4348 4348
Branches 852 852
=======================================
Hits 1918 1918
Misses 2430 2430
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-lib/pull/568?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-lib/pull/568?src=pr&el=footer). Last update [1a28929...bc4218e](https://codecov.io/gh/apache/cordova-lib/pull/568?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