You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by audreyso <gi...@git.apache.org> on 2017/07/27 18:24:25 UTC

[GitHub] cordova-lib pull request #584: CB-12361 : added tests for plugin/save.js

GitHub user audreyso opened a pull request:

    https://github.com/apache/cordova-lib/pull/584

    CB-12361 : added tests for plugin/save.js

    <!--
    Please make sure the checklist boxes are all checked before submitting the PR. The checklist
    is intended as a quick reference, for complete details please see our Contributor Guidelines:
    
    http://cordova.apache.org/contribute/contribute_guidelines.html
    
    Thanks!
    -->
    
    ### Platforms affected
    
    
    ### What does this PR do?
    
     added tests for plugin/save.js
    
    ### What testing has been done on this change?
    
    
    ### Checklist
    - [X] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database
    - [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
    - [X] Added automated test coverage as appropriate for this change.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/audreyso/cordova-lib CB-12361-16-plugin

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cordova-lib/pull/584.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 #584
    
----

----


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r136486774
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +61,145 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json', function (done) {
    --- End diff --
    
    This test checks that the plugins get removed but not if the new ones get re-added (based on description). I think the description should be updated to just check that existing plugins are getting removed


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r136490386
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +61,145 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should only add top-level plugins to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    -        it('should return a plugin source\'s url or path property immediately');
    -        it('should return a version if a version was provided to plugin id');
    -        it('should return a version that includes scope if scope was part of plugin id');
    -        it('should fall back to using PluginInfoProvider to retrieve a version as last resort');
    +        it('should return a plugin source\'s url or path property immediately', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'url': 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec');
    +            save(projectRoot).then(function (result) {
    +                expect(save.getSpec).toHaveBeenCalledWith(Object({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }), '/some/path', 'VRPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should return a version if a version was provided to plugin id', function (done) {
    +            var fake_fetch_json =
    +                {'cordova-plugin-camera': {'source': {
    +                    'type': 'registry',
    +                    'id': 'cordova-plugin-camera@^1.1.0'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(save.versionString).toHaveBeenCalledWith('^1.1.0');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should return a version that includes scope if scope was part of plugin id', function (done) {
    +            var fake_fetch_json =
    +                {'cordova-plugin-camera': {'source': {
    +                    'type': 'registry',
    +                    'id': '@scoped/package@^1.0.0'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.callThrough();
    +            save(projectRoot).then(function () {
    +                expect(save.getSpec).toHaveBeenCalledWith(Object({ type: 'registry', id: '@scoped/package@^1.0.0' }), '/some/path', 'cordova-plugin-camera');
    --- End diff --
    
    Same, need to check what `save.getSpec` returns. First you need to change line 181 from `save(projectRoot)` to `save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin')`


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r137442083
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +62,131 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('check that existing plugins are getting removed', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('plugins are being removed first and then only top level plugins are being restored', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.addPlugin).not.toHaveBeenCalledWith(Object({ name: 'MastodonSocialPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    -        it('should return a plugin source\'s url or path property immediately');
    -        it('should return a version if a version was provided to plugin id');
    -        it('should return a version that includes scope if scope was part of plugin id');
    -        it('should fall back to using PluginInfoProvider to retrieve a version as last resort');
    +        it('should return a plugin source\'s url or path property immediately', function () {
    +            spyOn(save, 'getSpec').and.callThrough();
    +            save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin');
    +            expect(save.getSpec).toHaveBeenCalledWith(Object({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }), '/some/path', 'VRPlugin');
    +            expect(save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin')).toEqual('https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git');
    +        });
    +
    +        it('getSpec should return a version if a version was provided to plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({id: 'cordova-plugin-camera@^1.1.0'}, '/some/path', 'cordova-plugin-camera')).toEqual('^1.1.0');
    +        });
    +
    +        it('should return a version that includes scope if scope was part of plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({ type: 'registry', id: '@scoped/package@^1.0.0' }, '/some/path', 'cordova-plugin-camera')).toEqual('@scoped/package@^1.0.0');
    +        });
    +
    +        it('should fall back to using PluginInfoProvider to retrieve a version as last resort', function () {
    +            expect(save.getSpec({ id: 'cordova-plugin-camera' }, '/some/path', 'cordova-plugin-camera')).toEqual(null);
    +            expect(plugin_info_provider_mock.prototype.get).toHaveBeenCalled();
    +        });
    +    });
    +
    +    describe('getPluginVariables helper method', function () {
    +        it('if no variables are passed in, should return empty', function () {
    +            expect(save.getPluginVariables()).toEqual([]);
    +        });
    +        it('if variables are passed in, should return result & get added to name and value', function () {
    +            expect(save.getPluginVariables({ variable: 'var 1' })).toEqual([ { name: 'variable', value: 'var 1' } ]);
    +        });
    +    });
    +
    +    describe('versionString helper method', function () {
    +        it('if no version, should return null', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('', false);
    +            spyOn(semver, 'validRange').and.returnValue('', false);
    +            expect(save.versionString()).toBe(null);
    +        });
    +        it('return version passed in, if it is within the valid range', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('', false);
    --- End diff --
    
    `semver.valid` returns null when the version isn't valid (just looked it up at https://github.com/npm/node-semver.
    ```
    spyOn(semver, 'valid').and.returnValue(null);
    ```



---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org


[GitHub] cordova-lib pull request #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r137442686
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +62,131 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('check that existing plugins are getting removed', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('plugins are being removed first and then only top level plugins are being restored', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.addPlugin).not.toHaveBeenCalledWith(Object({ name: 'MastodonSocialPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    -        it('should return a plugin source\'s url or path property immediately');
    -        it('should return a version if a version was provided to plugin id');
    -        it('should return a version that includes scope if scope was part of plugin id');
    -        it('should fall back to using PluginInfoProvider to retrieve a version as last resort');
    +        it('should return a plugin source\'s url or path property immediately', function () {
    +            spyOn(save, 'getSpec').and.callThrough();
    +            save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin');
    +            expect(save.getSpec).toHaveBeenCalledWith(Object({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }), '/some/path', 'VRPlugin');
    +            expect(save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin')).toEqual('https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git');
    +        });
    +
    +        it('getSpec should return a version if a version was provided to plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({id: 'cordova-plugin-camera@^1.1.0'}, '/some/path', 'cordova-plugin-camera')).toEqual('^1.1.0');
    +        });
    +
    +        it('should return a version that includes scope if scope was part of plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({ type: 'registry', id: '@scoped/package@^1.0.0' }, '/some/path', 'cordova-plugin-camera')).toEqual('@scoped/package@^1.0.0');
    +        });
    +
    +        it('should fall back to using PluginInfoProvider to retrieve a version as last resort', function () {
    +            expect(save.getSpec({ id: 'cordova-plugin-camera' }, '/some/path', 'cordova-plugin-camera')).toEqual(null);
    +            expect(plugin_info_provider_mock.prototype.get).toHaveBeenCalled();
    +        });
    +    });
    +
    +    describe('getPluginVariables helper method', function () {
    +        it('if no variables are passed in, should return empty', function () {
    +            expect(save.getPluginVariables()).toEqual([]);
    +        });
    +        it('if variables are passed in, should return result & get added to name and value', function () {
    +            expect(save.getPluginVariables({ variable: 'var 1' })).toEqual([ { name: 'variable', value: 'var 1' } ]);
    +        });
    +    });
    +
    +    describe('versionString helper method', function () {
    +        it('if no version, should return null', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('', false);
    +            spyOn(semver, 'validRange').and.returnValue('', false);
    +            expect(save.versionString()).toBe(null);
    +        });
    +        it('return version passed in, if it is within the valid range', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('', false);
    +            spyOn(semver, 'validRange').and.returnValue('1.3.0', true);
    +            expect(save.versionString('1.3.2')).toBe('1.3.2');
    --- End diff --
    
    I think this should be 
    
    ```
                spyOn(semver, 'validRange').and.returnValue('^1.3.2');
                expect(save.versionString('^1.3.2')).toBe('^1.3.2');
    ```
    
    `semver.validRange` takes in a range (`^1.3.2`) and returns it if it is valid. Returns `null` if it is invalid. So for testing, we need to pass in a range and expect the range to be returned (like my example)


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org


[GitHub] cordova-lib issue #584: CB-12361 : added tests for plugin/save.js

Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the issue:

    https://github.com/apache/cordova-lib/pull/584
  
    Make sure to run `npm run cover` to see the coverage report for save.js


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r136488898
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +61,145 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should only add top-level plugins to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    -        it('should return a plugin source\'s url or path property immediately');
    -        it('should return a version if a version was provided to plugin id');
    -        it('should return a version that includes scope if scope was part of plugin id');
    -        it('should fall back to using PluginInfoProvider to retrieve a version as last resort');
    +        it('should return a plugin source\'s url or path property immediately', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'url': 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec');
    +            save(projectRoot).then(function (result) {
    +                expect(save.getSpec).toHaveBeenCalledWith(Object({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }), '/some/path', 'VRPlugin');
    --- End diff --
    
    I think we need one more expect here. We need to confirm `save.getSpec` returned the url. I think you can use `returnValue` or `toEqual`
    
    ```
     expect(save.getSpec).and.returnValue('https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git')
    ```


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r137442765
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +62,131 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('check that existing plugins are getting removed', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('plugins are being removed first and then only top level plugins are being restored', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.addPlugin).not.toHaveBeenCalledWith(Object({ name: 'MastodonSocialPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    -        it('should return a plugin source\'s url or path property immediately');
    -        it('should return a version if a version was provided to plugin id');
    -        it('should return a version that includes scope if scope was part of plugin id');
    -        it('should fall back to using PluginInfoProvider to retrieve a version as last resort');
    +        it('should return a plugin source\'s url or path property immediately', function () {
    +            spyOn(save, 'getSpec').and.callThrough();
    +            save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin');
    +            expect(save.getSpec).toHaveBeenCalledWith(Object({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }), '/some/path', 'VRPlugin');
    +            expect(save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin')).toEqual('https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git');
    +        });
    +
    +        it('getSpec should return a version if a version was provided to plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({id: 'cordova-plugin-camera@^1.1.0'}, '/some/path', 'cordova-plugin-camera')).toEqual('^1.1.0');
    +        });
    +
    +        it('should return a version that includes scope if scope was part of plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({ type: 'registry', id: '@scoped/package@^1.0.0' }, '/some/path', 'cordova-plugin-camera')).toEqual('@scoped/package@^1.0.0');
    +        });
    +
    +        it('should fall back to using PluginInfoProvider to retrieve a version as last resort', function () {
    +            expect(save.getSpec({ id: 'cordova-plugin-camera' }, '/some/path', 'cordova-plugin-camera')).toEqual(null);
    +            expect(plugin_info_provider_mock.prototype.get).toHaveBeenCalled();
    +        });
    +    });
    +
    +    describe('getPluginVariables helper method', function () {
    +        it('if no variables are passed in, should return empty', function () {
    +            expect(save.getPluginVariables()).toEqual([]);
    +        });
    +        it('if variables are passed in, should return result & get added to name and value', function () {
    +            expect(save.getPluginVariables({ variable: 'var 1' })).toEqual([ { name: 'variable', value: 'var 1' } ]);
    +        });
    +    });
    +
    +    describe('versionString helper method', function () {
    +        it('if no version, should return null', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('', false);
    +            spyOn(semver, 'validRange').and.returnValue('', false);
    --- End diff --
    
    set the returnValue for 176 and 177 to `null`


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org


[GitHub] cordova-lib pull request #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r136489451
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +61,145 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should only add top-level plugins to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    -        it('should return a plugin source\'s url or path property immediately');
    -        it('should return a version if a version was provided to plugin id');
    -        it('should return a version that includes scope if scope was part of plugin id');
    -        it('should fall back to using PluginInfoProvider to retrieve a version as last resort');
    +        it('should return a plugin source\'s url or path property immediately', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'url': 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec');
    +            save(projectRoot).then(function (result) {
    +                expect(save.getSpec).toHaveBeenCalledWith(Object({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }), '/some/path', 'VRPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should return a version if a version was provided to plugin id', function (done) {
    +            var fake_fetch_json =
    +                {'cordova-plugin-camera': {'source': {
    +                    'type': 'registry',
    +                    'id': 'cordova-plugin-camera@^1.1.0'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(save.versionString).toHaveBeenCalledWith('^1.1.0');
    --- End diff --
    
    same as above, this test needs to check that getSpec is returning the version. Need something like:
    ```
     expect(save.getSpec).and.returnValue('^1.1.0')
    ```


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r137442253
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +62,131 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('check that existing plugins are getting removed', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('plugins are being removed first and then only top level plugins are being restored', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.addPlugin).not.toHaveBeenCalledWith(Object({ name: 'MastodonSocialPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    -        it('should return a plugin source\'s url or path property immediately');
    -        it('should return a version if a version was provided to plugin id');
    -        it('should return a version that includes scope if scope was part of plugin id');
    -        it('should fall back to using PluginInfoProvider to retrieve a version as last resort');
    +        it('should return a plugin source\'s url or path property immediately', function () {
    +            spyOn(save, 'getSpec').and.callThrough();
    +            save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin');
    +            expect(save.getSpec).toHaveBeenCalledWith(Object({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }), '/some/path', 'VRPlugin');
    +            expect(save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin')).toEqual('https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git');
    +        });
    +
    +        it('getSpec should return a version if a version was provided to plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({id: 'cordova-plugin-camera@^1.1.0'}, '/some/path', 'cordova-plugin-camera')).toEqual('^1.1.0');
    +        });
    +
    +        it('should return a version that includes scope if scope was part of plugin id', function () {
    +            save.versionString.and.callThrough();
    +            expect(save.getSpec({ type: 'registry', id: '@scoped/package@^1.0.0' }, '/some/path', 'cordova-plugin-camera')).toEqual('@scoped/package@^1.0.0');
    +        });
    +
    +        it('should fall back to using PluginInfoProvider to retrieve a version as last resort', function () {
    +            expect(save.getSpec({ id: 'cordova-plugin-camera' }, '/some/path', 'cordova-plugin-camera')).toEqual(null);
    +            expect(plugin_info_provider_mock.prototype.get).toHaveBeenCalled();
    +        });
    +    });
    +
    +    describe('getPluginVariables helper method', function () {
    +        it('if no variables are passed in, should return empty', function () {
    +            expect(save.getPluginVariables()).toEqual([]);
    +        });
    +        it('if variables are passed in, should return result & get added to name and value', function () {
    +            expect(save.getPluginVariables({ variable: 'var 1' })).toEqual([ { name: 'variable', value: 'var 1' } ]);
    +        });
    +    });
    +
    +    describe('versionString helper method', function () {
    +        it('if no version, should return null', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('', false);
    +            spyOn(semver, 'validRange').and.returnValue('', false);
    +            expect(save.versionString()).toBe(null);
    +        });
    +        it('return version passed in, if it is within the valid range', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('', false);
    +            spyOn(semver, 'validRange').and.returnValue('1.3.0', true);
    +            expect(save.versionString('1.3.2')).toBe('1.3.2');
    +        });
    +        it('should check and return a valid version', function () {
    +            save.versionString.and.callThrough();
    +            spyOn(semver, 'valid').and.returnValue('1.3.2', true);
    --- End diff --
    
    You can just set returnValue to `1.3.2`. No need for second argument of `true`


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org


[GitHub] cordova-lib pull request #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r136490187
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +61,145 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should only add top-level plugins to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin specs to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true }};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            spyOn(save, 'getSpec').and.returnValue('1.0.0');
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin', spec: '1.0.0' }), jasmine.any(Object));
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should write individual plugin variables to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true,
    +                'variables': {
    +                    'var 1': ' '
    +                }}};
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(jasmine.any(Object), [ Object({ name: 'var 1', value: ' ' }) ]);
    +                expect(cfg_parser_mock.prototype.write).toHaveBeenCalled();
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
         });
         describe('getSpec helper method', function () {
    --- End diff --
    
    So with all of these `getSpec` tests, you want to test getSpec directly. Right now, you are doing `save(projectRoot)` but instead you want to do `save.getSpec({ url: 'https://git-wip-us.apache.org/repos/asf/cordova-plugin-camera.git' }, '/some/path', 'VRPlugin')`


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r136487749
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +61,145 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should only add top-level plugins to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    --- End diff --
    
    Maybe for this test, you can redo the checks from the first test. That is confirm that these two expects are run before the addPlugin one
    
    ```
    expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    ```
    
    I think they should pass. If they do, you can also update the description of this test to reflect that the plugins are being removed first and then only top level plugins are being restored. 


---
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 #584: CB-12361 : added tests for plugin/save.js

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/584#discussion_r136487019
  
    --- Diff: spec/cordova/plugin/save.spec.js ---
    @@ -54,15 +61,145 @@ describe('cordova/plugin/save', function () {
             });
         });
         describe('happy path', function () {
    -        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json');
    -        it('should only add top-level plugins to config.xml');
    -        it('should write individual plugin specs to config.xml');
    -        it('should write individual plugin variables to config.xml');
    +        it('should remove all plugins from config.xml and re-add new ones based on those retrieved from fetch.json', function (done) {
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('VRPlugin');
    +                expect(cfg_parser_mock.prototype.removePlugin).toHaveBeenCalledWith('MastodonSocialPlugin');
    +            }).fail(function (e) {
    +                expect(e).toBeUndefined();
    +                fail('did not expect fail handler to be invoked');
    +            }).done(done);
    +        });
    +
    +        it('should only add top-level plugins to config.xml', function (done) {
    +            var fake_fetch_json =
    +                {'VRPlugin': {'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': true
    +                },
    +                'MastodonSocialPlugin': { 'source': {
    +                    'type': 'registry',
    +                    'id': 'id'
    +                },
    +                'is_top_level': false }};
    +
    +            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
    +            save(projectRoot).then(function () {
    +                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);
    --- End diff --
    
    Maybe add another expect to show that the non top level plugin didn't get installed. Something like 
    ```
    expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'MastodonSocialPlugin' }), [ ]);```


---
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 #584: CB-12361 : added tests for plugin/save.js

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/cordova-lib/pull/584


---

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org