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

audreyso · 2017-07-27T18:24:21Z

Platforms affected

What does this PR do?

added tests for plugin/save.js

What testing has been done on this change?

Checklist

Reported an issue in the JIRA database
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.
Added automated test coverage as appropriate for this change.

stevengill

Left a bunch of feedback.

Also, need to add tests for the helper methods getPluginVariables & versionString.

stevengill · 2017-09-01T02:08:20Z

spec/cordova/plugin/save.spec.js

-        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) {


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

stevengill · 2017-09-01T02:11:56Z

spec/cordova/plugin/save.spec.js

+
+            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
+            save(projectRoot).then(function () {
+                expect(cfg_parser_mock.prototype.addPlugin).toHaveBeenCalledWith(Object({ name: 'VRPlugin' }), [ ]);


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' }), [ ]);```

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.

stevengill · 2017-09-01T02:36:55Z

spec/cordova/plugin/save.spec.js

+            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');


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')

stevengill · 2017-09-01T02:43:50Z

spec/cordova/plugin/save.spec.js

+                'is_top_level': true }};
+            fs.readFileSync.and.returnValue(JSON.stringify(fake_fetch_json));
+            save(projectRoot).then(function () {
+                expect(save.versionString).toHaveBeenCalledWith('^1.1.0');


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')

stevengill · 2017-09-01T02:52:56Z

spec/cordova/plugin/save.spec.js

+                expect(e).toBeUndefined();
+                fail('did not expect fail handler to be invoked');
+            }).done(done);
+        });
    });
    describe('getSpec helper method', function () {


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')

stevengill · 2017-09-01T02:55:37Z

spec/cordova/plugin/save.spec.js

+            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');


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')

stevengill · 2017-09-01T02:57:27Z

Make sure to run npm run cover to see the coverage report for save.js

stevengill · 2017-09-07T04:04:01Z

spec/cordova/plugin/save.spec.js

+        });
+        it('return version passed in, if it is within the valid range', function () {
+            save.versionString.and.callThrough();
+            spyOn(semver, 'valid').and.returnValue('', false);


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);

stevengill · 2017-09-07T04:06:26Z

spec/cordova/plugin/save.spec.js

+        });
+        it('should check and return a valid version', function () {
+            save.versionString.and.callThrough();
+            spyOn(semver, 'valid').and.returnValue('1.3.2', true);


You can just set returnValue to 1.3.2. No need for second argument of true

stevengill · 2017-09-07T04:11:28Z

spec/cordova/plugin/save.spec.js

+            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');


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)

stevengill · 2017-09-07T04:12:07Z

spec/cordova/plugin/save.spec.js

+        it('if no version, should return null', function () {
+            save.versionString.and.callThrough();
+            spyOn(semver, 'valid').and.returnValue('', false);
+            spyOn(semver, 'validRange').and.returnValue('', false);


set the returnValue for 176 and 177 to null

audreyso force-pushed the CB-12361-16-plugin branch 2 times, most recently from 1d529e5 to 65303a9 Compare July 31, 2017 17:52

audreyso force-pushed the CB-12361-16-plugin branch 2 times, most recently from 6e03a40 to 5c46599 Compare August 8, 2017 21:04

audreyso force-pushed the CB-12361-16-plugin branch from 5c46599 to cf309c7 Compare August 17, 2017 00:18

stevengill suggested changes Sep 1, 2017

View reviewed changes

audreyso force-pushed the CB-12361-16-plugin branch 4 times, most recently from 847c10d to 57eceea Compare September 1, 2017 20:42

stevengill reviewed Sep 7, 2017

View reviewed changes

audreyso force-pushed the CB-12361-16-plugin branch 3 times, most recently from c45a3fd to 681c8dd Compare September 7, 2017 20:13

stevengill approved these changes Sep 22, 2017

View reviewed changes

audreyso force-pushed the CB-12361-16-plugin branch from 681c8dd to 760f47f Compare September 25, 2017 16:31

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

a4d91e4

audreyso force-pushed the CB-12361-16-plugin branch from 760f47f to a4d91e4 Compare September 25, 2017 17:03

asfgit merged commit a4d91e4 into apache:master Sep 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

audreyso commented Jul 27, 2017

stevengill left a comment

stevengill Sep 1, 2017

stevengill Sep 1, 2017

stevengill Sep 1, 2017

stevengill Sep 1, 2017

stevengill Sep 1, 2017

stevengill Sep 1, 2017

stevengill Sep 1, 2017

stevengill commented Sep 1, 2017

stevengill Sep 7, 2017

stevengill Sep 7, 2017

stevengill Sep 7, 2017

stevengill Sep 7, 2017

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

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

Conversation

audreyso commented Jul 27, 2017

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

stevengill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengill commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment