cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From filmaj <...@git.apache.org>
Subject [GitHub] cordova-lib pull request #562: Removing lazy load, platform command refactor...
Date Mon, 12 Jun 2017 22:49:00 GMT
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.fil@gmail.com>
Date:   2017-06-07T19:43:29Z

    Revert lazy loading of modules.

commit 37120613749c6695a33cf9ea70d6c9f9e9ffbb2f
Author: filmaj <maj.fil@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.fil@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.fil@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.fil@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.fil@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.fil@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.fil@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.fil@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.fil@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.fil@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.fil@gmail.com>
Date:   2017-06-12T19:30:36Z

    cordova/platform/index unit tests complete.

commit 1a632b751532e7f26c69749d363c100aed23afe1
Author: filmaj <maj.fil@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 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


Mime
View raw message