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/12 22:49:00 UTC
[GitHub] cordova-lib pull request #562: Removing lazy load, platform command refactor...
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 <ma...@gmail.com>
Date: 2017-06-07T19:43:29Z
Revert lazy loading of modules.
commit 37120613749c6695a33cf9ea70d6c9f9e9ffbb2f
Author: filmaj <ma...@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 <ma...@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 <ma...@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 <ma...@gmail.com>
Date: 2017-06-09T23:25:46Z
move platform module exports to the top, so clear what is exported.
commit 4c0432f0b1ec40fca5c2b403fa2fc4aec1d0d325
Author: filmaj <ma...@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 <ma...@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 <ma...@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 <ma...@gmail.com>
Date: 2017-06-10T16:34:06Z
minor testing fixes:
- eslint fixes for platform spec.s
- some test fixes.
commit 8b77a2e4098cf7a8b8f3d2c9ff696bd956f4c78e
Author: filmaj <ma...@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 <ma...@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 <ma...@gmail.com>
Date: 2017-06-12T19:30:36Z
cordova/platform/index unit tests complete.
commit 1a632b751532e7f26c69749d363c100aed23afe1
Author: filmaj <ma...@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
[GitHub] cordova-lib pull request #562: Removing lazy load, platform command refactor...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122330032
--- Diff: src/util/alias.js ---
@@ -0,0 +1,27 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/
+
+var cordova_events = require('cordova-common').events;
+
+module.exports = function aliasMethodToRawWithDeprecationNotice(property, targetObj, component) {
+ targetObj.raw[property] = function() {
+ cordova_events.emit('warn', 'Use of `' + component + '.raw.*` methods is deprecated and `' + component + '.raw` will be removed in a future release. Please migrate to using the top-level `' + component + '.*` methods instead.');
--- End diff --
*nit: The habit of using backticks in output sort of muddies the water when we eventually move to using proper ES6 backtick javascript string formatting.
---
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 #562: Removing lazy load, platform command refactor...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122334506
--- Diff: src/util/alias.js ---
@@ -0,0 +1,27 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/
+
+var cordova_events = require('cordova-common').events;
+
+module.exports = function aliasMethodToRawWithDeprecationNotice(property, targetObj, component) {
+ targetObj.raw[property] = function() {
+ cordova_events.emit('warn', 'Use of `' + component + '.raw.*` methods is deprecated and `' + component + '.raw` will be removed in a future release. Please migrate to using the top-level `' + component + '.*` methods instead.');
--- End diff --
Werd, will remove.
---
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 #562: Removing lazy load, platform command refactor...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122219931
--- Diff: spec-cordova/platform.spec.ios.js ---
@@ -1,137 +0,0 @@
-/**
--- End diff --
Yes: 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-lib pull request #562: Removing lazy load, platform command refactor...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/562
---
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 #562: Removing lazy load, platform command refactor...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122224237
--- Diff: spec-cordova/platform/addHelper.spec.js ---
@@ -0,0 +1,515 @@
+/**
--- End diff --
Sort of. The old tests I `xdescribe`d them so they don't run. Instead, based on the code and general purpose of the `addHelper` module, I wrote up four `it()` statements describing tests we should implement to get decent coverage and exercise the spirit of the module. I also added a big mulitline comment roughly sketching out what `addHelper` does - we should convert that comment into more `it()` statements to lay out the overview of unit tests for this module.
Finally, we should go over the old tests I `xdescribe`d at the bottom and figure out a) if we already have the coverage for what those tests do elsewhere, or if not b) write up unit tests to replace them (possibly in a different module - depends on what the test is testing).
---
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 #562: Removing lazy load, platform command refactor...
Posted by kerrishotts <gi...@git.apache.org>.
Github user kerrishotts commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122233685
--- Diff: spec-cordova/platform/index.spec.js ---
@@ -0,0 +1,181 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/
+/* eslint-env jasmine */
+
+var rewire = require('rewire');
+var platform = rewire('../../src/cordova/platform');
+var cordova_util = require('../../src/cordova/util');
+
+describe('cordova/platform', function () {
+ var hooksRunnerRevert;
+ var projectRoot = 'somepath';
+ beforeEach(function () {
+ // TODO: if we can change HooksRunner from a prototypal class to a function or object,
+ // we could eliminate the need for rewire here and use just jasmine spies.
+ hooksRunnerRevert = platform.__set__('HooksRunner', function () {});
+ spyOn(cordova_util, 'cdProjectRoot').and.returnValue(projectRoot);
+ });
+
+ afterEach(function () {
+ hooksRunnerRevert();
+ });
+
+ describe('main module function', function () {
+ describe('error/warning conditions', function () {
+ // TODO: what about other commands? update? save?
+ it('should require at least one platform for add and remove commands', function (done) {
+ // targets = empty array
+ platform('add', []).then(function () {
+ fail('should not succeed without targets');
+ }, function (err) {
+ expect(err).toMatch(/You need to qualify.* with one or more platforms/gi);
--- End diff --
Out of scope for this commit, but I'd love to see the ability to use `expect(err).toBeInstanceOf(CordovaErrors.ExpectedOneOrMorePlatforms)`. That way should the error message ever change, the test still passes. Plus, then we don't have to worry about ignoring dynamic portions of the error.
I have no idea how big a job that might be (I expect it'd be pretty big!), so I'm just throwing it out there as a "nice-to-have", but I wouldn't want it to hold up any progress on this commit, which is super important.
Thanks for your hard work on this, BTW!
---
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 #562: Removing lazy load, platform command refactor...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122101116
--- Diff: src/cordova/platform.js ---
@@ -1,769 +0,0 @@
-/**
--- End diff --
to confirm, you ripped out this file into multpile smaller files. I like it. It has been replaced by addHelper.js, check.js, getPlatformDetailsFromDir.js, index.js, list.js, remove.js and save.js
I see tests for addHelper. Do the other modules need new unit tests?
---
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 #562: Removing lazy load, platform command refactor, start...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-lib/pull/562
We'd have to be OK with either removing the e2e tests from standard CI until we finish the chip-away, or be OK with CI failing for a while. Both are tough calls.
---
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 #562: Removing lazy load, platform command refactor, start...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the issue:
https://github.com/apache/cordova-lib/pull/562
I think we should get CI passing before merging. I'm looking into e2e failures now.
---
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 #562: Removing lazy load, platform command refactor...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122328245
--- Diff: spec-cordova/platform/index.spec.js ---
@@ -0,0 +1,181 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/
+/* eslint-env jasmine */
+
+var rewire = require('rewire');
+var platform = rewire('../../src/cordova/platform');
+var cordova_util = require('../../src/cordova/util');
+
+describe('cordova/platform', function () {
+ var hooksRunnerRevert;
+ var projectRoot = 'somepath';
+ beforeEach(function () {
+ // TODO: if we can change HooksRunner from a prototypal class to a function or object,
+ // we could eliminate the need for rewire here and use just jasmine spies.
+ hooksRunnerRevert = platform.__set__('HooksRunner', function () {});
+ spyOn(cordova_util, 'cdProjectRoot').and.returnValue(projectRoot);
+ });
+
+ afterEach(function () {
+ hooksRunnerRevert();
+ });
+
+ describe('main module function', function () {
+ describe('error/warning conditions', function () {
+ // TODO: what about other commands? update? save?
+ it('should require at least one platform for add and remove commands', function (done) {
+ // targets = empty array
+ platform('add', []).then(function () {
+ fail('should not succeed without targets');
+ }, function (err) {
+ expect(err).toMatch(/You need to qualify.* with one or more platforms/gi);
--- End diff --
I would suggest we defined error codes before we define static properties, and start by manually looking things up. But yeah, I'd consider this out of scope.
---
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 #562: Removing lazy load, platform command refactor...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122097578
--- Diff: spec-cordova/platform/addHelper.spec.js ---
@@ -0,0 +1,515 @@
+/**
--- End diff --
This file is just a placeholder for now right?
---
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 #562: Removing lazy load, platform command refactor, start...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-lib/pull/562
Oh so it's merged now? OK, gonna focus more on these tests then.
---
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 #562: Removing lazy load, platform command refactor, start...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-lib/pull/562
EDIT: added a new point number 5 to the above list to point out that I'm moving certain integration tests into unit tests where I believe it makes sense.
---
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 #562: Removing lazy load, platform command refactor, start...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-lib/pull/562
Cheers. The CI is failing due to the end to end tests failing (also JSHint, but that's minor, and I think JSHint in particular is on its way out anyways).
I will keep chipping away at rewriting the integration tests as unit tests wherever I can, however that could take some time, which stands at odds with "merge this in sooner rather than later." 😬
---
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 #562: Removing lazy load, platform command refactor...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122223046
--- Diff: src/cordova/platform.js ---
@@ -1,769 +0,0 @@
-/**
--- End diff --
Sure do! Basically, each module should have unit tests. A good rule of thumb is to have one spec file per module file.
To really get a sense of what coverage exists and what is missing, run `npm run cover` and get the code coverage report. Note that as of right now, the unit tests still have _some_ integration tests - so the code coverage report from a unit test standpoint is somewhat inaccurate (integration or end to end tests are very coarse, so they probably cover a lot of code unintentionally). I would recommend a) looking through the spec-cordova and spec-plugman tests for heavy i/o, very broad tests that we can classify as integration tests and moving them into integration-tests and b) replacing them with finer-grained unit tests.
This cordova-ios PR is an example of that general pattern: 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-lib pull request #562: Removing lazy load, platform command refactor...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122097628
--- Diff: spec-cordova/platform.spec.ios.js ---
@@ -1,137 +0,0 @@
-/**
--- End diff --
These tests got moved out to cordova-ios?
---
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 #562: Removing lazy load, platform command refactor...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/562#discussion_r122284211
--- Diff: spec-cordova/platform/index.spec.js ---
@@ -0,0 +1,181 @@
+/**
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+ http://www.apache.org/licenses/LICENSE-2.0
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+*/
+/* eslint-env jasmine */
+
+var rewire = require('rewire');
+var platform = rewire('../../src/cordova/platform');
+var cordova_util = require('../../src/cordova/util');
+
+describe('cordova/platform', function () {
+ var hooksRunnerRevert;
+ var projectRoot = 'somepath';
+ beforeEach(function () {
+ // TODO: if we can change HooksRunner from a prototypal class to a function or object,
+ // we could eliminate the need for rewire here and use just jasmine spies.
+ hooksRunnerRevert = platform.__set__('HooksRunner', function () {});
+ spyOn(cordova_util, 'cdProjectRoot').and.returnValue(projectRoot);
+ });
+
+ afterEach(function () {
+ hooksRunnerRevert();
+ });
+
+ describe('main module function', function () {
+ describe('error/warning conditions', function () {
+ // TODO: what about other commands? update? save?
+ it('should require at least one platform for add and remove commands', function (done) {
+ // targets = empty array
+ platform('add', []).then(function () {
+ fail('should not succeed without targets');
+ }, function (err) {
+ expect(err).toMatch(/You need to qualify.* with one or more platforms/gi);
--- End diff --
That makes a lot of sense - certainly would be much less brittle and thus make the tests more useful.
Perhaps file a JIRA issue for it and assign to me? I'd be happy to put together a PR proofing that approach - if it passes muster, we can then chip away at changing that.
---
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 #562: Removing lazy load, platform command refactor, start...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the issue:
https://github.com/apache/cordova-lib/pull/562
Lets merge and keep chipping away at tests in separate prs.
On Thu, Jun 15, 2017 at 4:20 PM, Fil Maj <no...@github.com> wrote:
> Cheers. The CI is failing due to the end to end tests failing (also
> JSHint, but that's minor, and I think JSHint in particular is on its way
> out anyways).
>
> I will keep chipping away at rewriting the integration tests as unit tests
> wherever I can, however that could take some time, which stands at odds
> with "merge this in sooner rather than later." 😬
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/cordova-lib/pull/562#issuecomment-308891220>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKWQOCgDDQK55GXhTlCyAXriKDpRZJdks5sEbwugaJpZM4N3yBF>
> .
>
---
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 #562: Removing lazy load, platform command refactor, start...
Posted by purplecabbage <gi...@git.apache.org>.
Github user purplecabbage commented on the issue:
https://github.com/apache/cordova-lib/pull/562
This all looks great to me, good work.
I would like to move forward with this asap as it is big, and affects a lot of different areas.
Let's keep our merge conflict exposure low and get this in soon.
I am looking into failing tests on CI.
---
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