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/21 15:35:06 UTC
[GitHub] cordova-lib pull request #566: Plugin specs
GitHub user filmaj opened a pull request:
https://github.com/apache/cordova-lib/pull/566
Plugin specs
*WARNING*: Work in progress! Please review and leave comments.
### What does this PR do?
A lot! The main purposes of this PR are:
1. Address removing lazy loading of submodules that was removed in a previous PR, and how that affected functionality across certain subcommands like the plugin subcommand.
2. Refactor the plugin command.
3. Do what is necessary to get CI working again for this repo.
4. Comb over the tests for the plugin command, and split the tests into integration tests (ones that rely on fixtures and do heavy file and/or network I/O) and unit tests (fast, mocked, logic-focussed and scoped to individual modules-under-test).
5. Get the unit tests to run in a matter of seconds, not minutes (runs in 2 seconds on my machine now - check by running `npm run jasmine`)
6. Remove any integration tests that exercise code that is already tested via unit tests (ideally making CI and test runs faster - runs in a couple minutes now on my machine).
In detail:
- Refactored the `cordova.plugin` command and split it up into submodules along the lines of its subcommands, i.e. `src/cordova/plugin/index` is the main entry point, but now there also exist `add`, `remove`, `list`, etc. submodules. Makes the submodules smaller and hopefully more easily understandable, as well as makes them more testable.
- Added the initial first steps towards unit test coverage for the plugin command. Wrote _a lot_ of pending unit tests for what I believe are high-level goals of each submodule / helper function.
- Moved a whole bunch of tests that are integration tests into the integration-tests directory.
- Removed a whole bunch of integration tests that exercised code we already had code coverage for, either in other integration tests, or in unit tests.
*Worth Noting*:
- There are still plenty of unit tests to fill out and write for the plugin command - this PR is a work in progress and I figured it would be better to issue the PR and get eyes on it early rather than wait til the tests were written. I believe getting this PR reviewed and proofed by the other committers, to ensure I am on the right track, was more important than writing out all the pending tests. That effort can be done in parallel. It is worth discussing whether we want to have the pending tests written up first before we do a next release (I am of the opinion that yes, we should ensure we write up any pending unit tests before doing the next release).
- The tests in plugin.add for plugin version constraint satisfaction helper methods (`getFetchVersion`, `determinePluginVersionToFetch`, `getFailedRequirements`) _do not have pending unit tests written up for them yet_, as, honestly, that code confused the hell out of me. I believe @stevengill was the last one to work on those tests and that code, so I would like to work more closely with Steve to get a better understanding of what and how these methods do what they do. Luckily, we still have test coverage for these methods (under `integration-tests/plugin_fetch.spec.js`), however, I would like to rewrite or migrate these tests so that they do not rely on fixtures and file I/O.
- These are big changes, so any pending PRs will likely need to be rebased once this lands.
### What testing has been done on this change?
ran unit tests, e2e tests and jshint.
FYI (or, even better, please review!) @stevengill, @audreyso, @purplecabbage, @shazron, @imhotep, @kerrishotts, @dpogue, @matrosov-nikita, @goya
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/filmaj/cordova-lib plugin-specs
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/566.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 #566
----
commit e33630d3b2e0eb3e948f7bda1435b511f8e93334
Author: filmaj <ma...@gmail.com>
Date: 2017-06-19T22:18:17Z
the existing plugin specs are all e2e tests... moving them to relevant directory.
commit c773cd2e9a869846b472464d347738404089720a
Author: filmaj <ma...@gmail.com>
Date: 2017-06-19T22:25:38Z
plugin e2e test changes to account for dir move
commit 8d3d6108d86d5c7258c3613ba9d4f16ec3cf439c
Author: filmaj <ma...@gmail.com>
Date: 2017-06-19T23:45:31Z
first pass at plugin command refactor
commit 3d17ab4eb4ac068bc0e31f7d1a6fecb6a77b711a
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T03:54:26Z
refactor of code should be done.
commit 4218ea522845030c24f64ba6aa14340eb34c36be
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T04:02:29Z
deprecate anything related to plugin_parser class.
commit 10b24897a21ee319cffda976667e755497b63328
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T04:10:13Z
eslint fixes for plugin specs
commit a75986076daf2b0fde84f2d1e9dbb08f55858869
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T04:23:18Z
plugin_fetch.spec.js are integration tests - moving to integration-tests dir
commit 9a0217522473a6fb1a562afb6863112dd3fa7cc2
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T04:29:22Z
hooks runner tests are also e2e - moving to integration-tests dir
commit 0677b0463034467c422e044c53b31604dae84b07
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T04:36:29Z
moved plugman fetch specs to integration tests dir
commit d1b8580a671f6ee818a8d12a30611ac99c3f055c
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T04:50:17Z
last of i/o heavy tests into integration-tests dir
commit 9cc1e14a4211b11888b486762f9265a703bad135
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T04:56:02Z
removing unnecessary else from plugin remove. moved plugin-package-parse to plugin subdir and renamed to plugin_spec_parser (to reflect name of module under test).
commit f496408e6f9c56a1b8f959edc075e07df0ba9812
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T19:20:20Z
start of unit tests for cordova.plugin commands
commit 803c3c88c32b39862ee57f4a2b76cdef1b81de0b
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T19:20:54Z
fixes to integration test refrences so they run
commit 29a47cb7d28b974a131cb8f0ac048724de656b36
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T19:21:09Z
todos for cordova.plugin commands.
commit ed888914d85c35b327c606e523953d3e6e6d3ff3
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T19:42:41Z
bye bye save integration specs.
commit c848f7648a663e9b70fa02627f366828da145ca8
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T19:43:22Z
nest certain plugin-remove-save tests into a describe to better reflect structure
commit 81b7b228742b1380d0d95956b7b7d15138a75b19
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T19:49:46Z
:snail: bad refs to prepare
commit 5d86ebb9b1ee3c4c4801551b454a74f9dbf8f2f0
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T19:56:39Z
todo to move plugin fetch integration tests into plugin/add/fetchVersion unit tests.
commit 5e5109f5e20147d48a1f670c656324e7e7353a91
Author: filmaj <ma...@gmail.com>
Date: 2017-06-20T23:36:47Z
pending tests for plugin specs
commit 5a2e5e96bb38fc1edaf1f7c15758861d4b99c375
Author: filmaj <ma...@gmail.com>
Date: 2017-06-21T14:24:45Z
small refactor tweaks, mostly TODOs being dropped
commit 2dc98da5883557f19cbfb08486ba4e02f97f376c
Author: filmaj <ma...@gmail.com>
Date: 2017-06-21T14:32:21Z
remove old platform-add tests. fixes for jshint.
commit 63c762cb7d8cfbbdacbbfa32cc4321c041c958cd
Author: filmaj <ma...@gmail.com>
Date: 2017-06-21T14:39:51Z
plugin util specs implemented
commit bb524e51e98be77ebdf6576b4385fdf46d629ca7
Author: filmaj <ma...@gmail.com>
Date: 2017-06-21T15:17:18Z
first unit test in each plugin submodule, jshint fixes
----
---
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 #566: Refactor Plugin Command, test improvements, p...
Posted by dpogue <gi...@git.apache.org>.
Github user dpogue commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/566#discussion_r123302742
--- Diff: src/cordova/plugin/search.js ---
@@ -0,0 +1,41 @@
+/**
+ 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 opener = require('opener');
+var Q = require('Q');
--- End diff --
casing
---
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 #566: Refactor Plugin Command, test improvements, p...
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/566#discussion_r123359407
--- Diff: src/cordova/plugin_parser.js ---
@@ -18,12 +18,14 @@
*/
var xml = require('cordova-common').xmlHelpers;
+var events = require('cordova-common').events;
/** Deprecated. Use PluginInfo instead. */
-function plugin_parser(xmlPath) {
+function plugin_parser (xmlPath) {
+ events.emit('warn', 'WARNING: Cordova\'s plugin_parser class is now deprecated and will be removed in a future version. Please transition to using the PluginInfo class instead.');
--- End diff --
Good call on deprecation warning
---
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 #566: Refactor Plugin Command, test improvements, p...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj closed the pull request at:
https://github.com/apache/cordova-lib/pull/566
---
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 #566: Refactor Plugin Command, test improvements, p...
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/566#discussion_r123366796
--- Diff: src/cordova/plugin/util.js ---
@@ -0,0 +1,37 @@
+/**
+ 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 path = require('path');
+var PluginInfoProvider = require('cordova-common').PluginInfoProvider;
+
+module.exports.saveToConfigXmlOn = saveToConfigXmlOn;
+module.exports.getInstalledPlugins = getInstalledPlugins;
+
+function getInstalledPlugins (projectRoot) {
+ var pluginsDir = path.join(projectRoot, 'plugins');
+ // TODO: This should list based off of platform.json, not directories within plugins/
--- End diff --
I don't know, honestly. This comment existed before this PR: https://github.com/apache/cordova-lib/blob/master/src/cordova/plugin.js#L598
---
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 #566: Refactor Plugin Command, test improvements, passing ...
Posted by codecov-io <gi...@git.apache.org>.
Github user codecov-io commented on the issue:
https://github.com/apache/cordova-lib/pull/566
# [Codecov](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@a7f55ec`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `30.52%`.
[](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #566 +/- ##
=========================================
Coverage ? 44.11%
=========================================
Files ? 78
Lines ? 4348
Branches ? 852
=========================================
Hits ? 1918
Misses ? 2430
Partials ? 0
```
| [Impacted Files](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/plugman/registry/registry.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL3BsdWdtYW4vcmVnaXN0cnkvcmVnaXN0cnkuanM=) | `31.42% <ø> (ø)` | |
| [src/plugman/fetch.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL3BsdWdtYW4vZmV0Y2guanM=) | `15.06% <ø> (ø)` | |
| [src/cordova/util.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL2NvcmRvdmEvdXRpbC5qcw==) | `77.73% <ø> (ø)` | |
| [src/plugman/install.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL3BsdWdtYW4vaW5zdGFsbC5qcw==) | `77.37% <ø> (ø)` | |
| [src/cordova/platform/addHelper.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL2NvcmRvdmEvcGxhdGZvcm0vYWRkSGVscGVyLmpz) | `14.35% <0%> (ø)` | |
| [src/cordova/plugin/plugin\_spec\_parser.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL2NvcmRvdmEvcGx1Z2luL3BsdWdpbl9zcGVjX3BhcnNlci5qcw==) | `100% <100%> (ø)` | |
| [src/cordova/plugin\_parser.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL2NvcmRvdmEvcGx1Z2luX3BhcnNlci5qcw==) | `100% <100%> (ø)` | |
| [src/cordova/plugin/util.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL2NvcmRvdmEvcGx1Z2luL3V0aWwuanM=) | `100% <100%> (ø)` | |
| [src/cordova/plugin/add.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL2NvcmRvdmEvcGx1Z2luL2FkZC5qcw==) | `14.13% <14.13%> (ø)` | |
| [src/cordova/plugin/remove.js](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree#diff-c3JjL2NvcmRvdmEvcGx1Z2luL3JlbW92ZS5qcw==) | `23.94% <23.94%> (ø)` | |
| ... and [4 more](https://codecov.io/gh/apache/cordova-lib/pull/566?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/cordova-lib/pull/566?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/566?src=pr&el=footer). Last update [a7f55ec...ceee7a4](https://codecov.io/gh/apache/cordova-lib/pull/566?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
[GitHub] cordova-lib pull request #566: Refactor Plugin Command, test improvements, p...
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/566#discussion_r123357002
--- Diff: src/cordova/plugin/util.js ---
@@ -0,0 +1,37 @@
+/**
+ 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 path = require('path');
+var PluginInfoProvider = require('cordova-common').PluginInfoProvider;
+
+module.exports.saveToConfigXmlOn = saveToConfigXmlOn;
+module.exports.getInstalledPlugins = getInstalledPlugins;
+
+function getInstalledPlugins (projectRoot) {
+ var pluginsDir = path.join(projectRoot, 'plugins');
+ // TODO: This should list based off of platform.json, not directories within plugins/
--- End diff --
I agree that looking in the plugins directory is a bad way of doing this. Having a plugin in the plugins directory
By `platform.json`, do you mean each platform's json file? Example, `platforms/browser/browser.json`. This file contains installed plugins + dependent plugins. But we would have to get this info for each platform installed. This file also lives in `plugins/browser.json`. This file at least represents plugins that are actually installed.
`plugins/fetch.json` also has information on plugins.
---
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