Reorganized unit test directory structure + updated npm task names #568
Conversation
Codecov Report
@@ Coverage Diff @@
## master #568 +/- ##
=======================================
Coverage 43.06% 43.06%
=======================================
Files 78 78
Lines 4361 4361
Branches 852 852
=======================================
Hits 1878 1878
Misses 2483 2483Continue to review full report at Codecov.
|
filmaj
force-pushed
the
spec-consolidation
branch
2 times, most recently
from
ffb8300
to
fc65a18
Compare
|
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. |
|
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. |
|
Thanks for the feedback @stevengill, good point. I've just removed |
- consolidate spec-cordova/ and spec-plugman/ 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. remove "npm run ci" (dosnt fit in new tasks layout) - updated readme to reflect package.json npm run script changes. - update appveyor and travis to just run "npm test" - just like a dev would on their local machine.
filmaj
force-pushed
the
spec-consolidation
branch
from
07a4fa3
to
d164a88
Compare
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/andspec-plugman/are consolidated into a single top-levelspec/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 and continue to march towards a healthier and faster test suite.Other than the directory changes, a summary of smaller but also relevant changes:
spec/directory:helper.js,helpers.jsandcommon.js. Combining those into one test-helper file is left as an exercise for a future pull request :)package.jsonnpm runtasks to better reflect the purpose of the task. In particular, changednpm run jasminetonpm run unit-tests. I also thought of changing thejshinttask to belinter, but then decided against it. I am open to hearing opinions on this.npm testruns 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.npmtask names, and also fixed the link to the issue tracker :)What testing has been done on this change?
npm test.