GitHub user filmaj opened a pull request:
https://github.com/apache/cordova-lib/pull/562
Removing lazy load, platform command refactor, start of finer-grained unit
tests.
### Platforms affected
all / node ?
### What does this PR do?
A lot!
1. Removed lazy loading of modules.
Per cordova/cordova-discuss#70, it was determined that the obfuscation the
lazy loading introduced was not worth the time saved on load (~250ms in the
worst case). It also opens the door for _much_ easier unit testing - more on
that later.
2. Refactored the platform command
Previously, it was implemented as one 770 lines-of-code module. It is now
split up into an `index.js` main module under a `platform/` subdirectory, with
the high-level commands split out as sub-modules. This kind of split draws
clearer abstraction boundaries, and makes unit testing easier to organize as
there are clearer responsibilities divided amongst the modules.
3. Start of fine-grained unit tests for the platform command
Once point 2 above was implemented, the combination of removing lazy
loading and splitting up the platform command allowed for a simpler layout of
tests: one spec file per module, and mock out everything outside of that one
module. In this PR, that was done for the new `platform/index.js` file as a
first step. The other platform subcommands still need unit tests written for
them.
4. Moved a lot of tests from `spec-cordova/` and `spec-plugman` into
`integration-tests`
At least all of the platform command specs that I could identify as relying
heavily on the filesystem and network, I moved these into the
`integration-tests/` folder. The idea is that as new unit tests are written we
can eliminate integration tests that test the same functionality.
### What testing has been done on this change?
Only unit tests. A lot of the integration tests are failing at this point -
I am unsure if that's a showstopper for this PR or not, looking for feedback on
that.
Worth noting that moving more tests into `integration-tests/` dropped the
code coverage: from 80% to 63%. However, the platform command still needs to
have almost all of its subcommands unit tested, so I think that is bound to go
up. As I mentioned above, moving forward, we should review the tests for
different commands, refactor them as I did here, and try to rewrite a lot of
the integration tests as unit tests instead.
While the code coverage right now has gone down, so has test execution
time! On my machine, running the unit tests went from 700+ seconds to under a
minute. I am confident that if we continue this trend of rewriting integration
tests as unit tests, we can have the same code coverage but have the tests
execute in a few seconds.
Please review / FYI @stevengill, @dpogue, @shazron, @purplecabbage,
@audreyso, @surajpindoria, @kerrishotts.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/filmaj/cordova-lib no-lazy-load
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/562.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 #562
----
commit 74c113df6e46060edef3385a99fe5a383321ccea
Author: filmaj <maj....@gmail.com>
Date: 2017-06-07T19:43:29Z
Revert lazy loading of modules.
commit 37120613749c6695a33cf9ea70d6c9f9e9ffbb2f
Author: filmaj <maj....@gmail.com>
Date: 2017-06-08T23:06:48Z
tweaks to existing tests to get closer to having tests pass after
removing lazy loading, including:
- bump timeout on test that fetches/unpacks tarballs. also ensure failure
cases invoke done so we dont wait in case error conditions get hit.
- remove wrapper specs, leave todo on how to properly unit test.
commit 90b6857f4d6c9d6169f1155d533ce4f7c487174c
Author: filmaj <maj....@gmail.com>
Date: 2017-06-09T19:04:17Z
Keep `raw` object around, alias to module directly, but drop in a
deprecation warning if it is used. Update all code and specs to stop using the
`raw` object.
commit 1a8fa4107c8def09a988f04bfffbc1636200563d
Author: filmaj <maj....@gmail.com>
Date: 2017-06-09T22:31:09Z
moving i/o heavy tests to intgration-tests folder.
move cocoapod plugin test to integration tests, rename for clarity
move platform specs to integration tests directory
commit f16ad7982d6474251edc7ca55a1f6fec7d9e6707
Author: filmaj <maj....@gmail.com>
Date: 2017-06-09T23:25:46Z
move platform module exports to the top, so clear what is exported.
commit 4c0432f0b1ec40fca5c2b403fa2fc4aec1d0d325
Author: filmaj <maj....@gmail.com>
Date: 2017-06-09T23:26:17Z
salvage real unit tests:
- move actual unit tests from the old (mostly integration-test-heavy)
platform.spec to new spec-cordova/platform.spec.js.
- moved the actual unit tests for cordova.platform out of the integration
tests folder and back into the spec folder.
commit ce06491729b82997f275d7c61e799284e95d5c10
Author: filmaj <maj....@gmail.com>
Date: 2017-06-10T01:40:18Z
splitting out cordova.platform into smaller modules. factored a few small
helper functions that will be used in multiple modules into the util module.
commit 7de89184a874a1684497167f219bb1dcfd377bb8
Author: filmaj <maj....@gmail.com>
Date: 2017-06-10T16:28:59Z
cordova save specs moved to integration tests - all of these are end to end.
commit d38e003a47375da2b72d1ac73f9ee6722eac3145
Author: filmaj <maj....@gmail.com>
Date: 2017-06-10T16:34:06Z
minor testing fixes:
- eslint fixes for platform spec.s
- some test fixes.
commit 8b77a2e4098cf7a8b8f3d2c9ff696bd956f4c78e
Author: filmaj <maj....@gmail.com>
Date: 2017-06-12T14:39:10Z
dropped TODOs/observations inside addhelper, dropped todo to merge platform
add error checking tests.
commit 66cefa9b35c8c9c162224d79a42498307d792389
Author: filmaj <maj....@gmail.com>
Date: 2017-06-12T14:51:20Z
lay out unit tests for the top-level platform module and for the addHelper
module.
commit 8784e801759903a88d24b71bfeada708667233df
Author: filmaj <maj....@gmail.com>
Date: 2017-06-12T19:30:36Z
cordova/platform/index unit tests complete.
commit 1a632b751532e7f26c69749d363c100aed23afe1
Author: filmaj <maj....@gmail.com>
Date: 2017-06-12T22:20:50Z
Small reference fixes for getting integration tests at least running.
----
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org
