(iOS) Added Semver to version comparison checks.

[mattmilan-dev]

Platforms affected

iOS

Motivation and Context

Fixes apache/cordova#165

Description

Adds semver NPM to package.json and updates the compareVersions method to allow semver based plugins to be read and compared.

Testing

Tested using runkit. https://runkit.com/devleaf-matt/cordova-version-comparison

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[breautek]

semver.coerce drops labels.

When cleanV1 and cleanV2 is equal, do you think it's worth checking the original version variables and determine if one has a prerelease label, then return the appropriate integer?

e.g.

var version1 = "1.0.0";
var version2 = "1.0.0-beta.0";

var cleanV1 = semver.valid(semver.coerce(version1));
var cleanV2 = semver.valid(semver.coerce(version2));

if (cleanV1 === cleanV2) {
    if (version1.indexOf('-') > -1 && version2.indexOf('-') === -1) {
        // version1 is prerelease, favour version2 instead.
    }
    else if (version2.indexOf('-') > -1 && version1.indexOf('-') === -1) {
        //version 2 is prelease, favour version 1
    }
    else {
        return 0;
    }
}

[mattmilan-dev]

Makes a lot of sense to do that actually. I've used .includes as a formality to make the code easier to read, as the performance loss is minimal in comparison to indexOf. However, if we need backwards compatibility with < IE 11 (As it's a CLI tool i'm assuming this isn't necessary), i'll go with the indexOf instead!

[breautek]

You can use .includes, as long as it's supported in Node 6. If not, then this patch needs to wait until we drop support for node 6, which will likely come in next major release.

[raphinesse]

Our JSDocs say "Compares two semver-notated version strings". So in my opinion, this whole function could (and should) be implemented as follows:

exports.compareVersions = semver.compare

[breautek]

I didn't realize this but semver actually does handling comparing prerelease labels, so my suggestion is just doing the work over again.

We also don't need to use coerce.

semver.valid("1.0.0.0"); // null
semver.valid("1.0.0"): // "1.0.0"
semver.valid("1.0.0-beta.0"); // "1.0.0-beta.0"

Exporting semver.compare directly will change what compareVersions will return as an error, if it does receive an invalid semver, which will need to refactored in the unit tests but I think that's fine.

[raphinesse]

Yes, the expectation in the unit test will have to be adapted for the new error

[breautek]

Looks like there is a couple of tests failing.

This is a unit test that you can find here:

cordova-ios/tests/spec/unit/lib/check_reqs.spec.js

Lines 59 to 74 in be68c9f

	it('should throw error because version is not following semver-notated.', (done) => {
	shellWhichSpy.and.returnValue('/bin/node');
	const checkTool = checkReqs.__get__('checkTool');
	checkReqs.__set__('versions', {
	get_tool_version: getToolVersionSpy.and.returnValue(new Promise((resolve) => {
	return resolve('1.0.0');
	}).catch((error) => { console.log(error); })),
	compareVersions: originalVersion.compareVersions
	});
	checkTool('node', 'v1.0.0').catch((error) => {
	expect(error).toEqual('Version should contain only numbers and dots');
	done();
	});
	});

1) check_reqs checkTool method should throw error because version is not following semver-notated.
  Message:
    Error: Timeout - Async function did not complete within 5000ms (set by jasmine.DEFAULT_TIMEOUT_INTERVAL)
  Stack:
    Error: Timeout - Async function did not complete within 5000ms (set by jasmine.DEFAULT_TIMEOUT_INTERVAL)
        at <Jasmine>
        at ontimeout (timers.js:386:11)
        at tryOnTimeout (timers.js:250:5)
        at Timer.listOnTimeout (timers.js:214:5)

This particular error is stating that a function isn't resolving in a meaningful time, perhaps a resolve isn't being called somewhere.

This particular test is to test for completely invalid semver, and it's using v1.0.0 to do that test, which I think under your changes, v1.0.0 can easily be coerced into a valid semver so you'll likely have to update this to use something a bit more outlandish, such as a.b.c.

/Users/travis/build/apache/cordova-ios/bin/templates/scripts/cordova/lib/versions.js
  183:67  error  Trailing spaces not allowed  no-trailing-spaces
  187:52  error  Trailing spaces not allowed  no-trailing-spaces
  192:1   error  Trailing spaces not allowed  no-trailing-spaces

These are eslint errors, at the following line/column nunmbers there are extra whitespaces that need to be removed.

[mattmilan-dev]

Hi @breautek,

Apologies for the lack of understanding, I haven't done much with unit testing before (something I probably need to look further into). Do I need to modify and push up a more appropriate unit test in order for the checks to pass? The ES lint errors will be my VS Code, i'll remove those and re-push the version.js file.

Thanks!

[breautek]

No problem, we all start somewhere. I've only began unit testing my code earlier this year, and it's an important piece of knowledge when working on larger projects. I'll walk you through this one, please keep notes ;)

you can run npm test to launch both eslint and the unit tests. This is how you can ensure that everything is good before you push.

Unit test primarily focus is to ensure that given a certain input, you expect a certain output. We use jasmine for unit testing, so you can find the documentation for jasmine at https://jasmine.github.io/pages/docs_home.html Use the appropriate API version, not all platforms/plugins use a consistent jasmine version sometimes.

Generally speaking, a unit test tests a function, or something, and when given a certain set of inputs, it tests the output and expect it to be a certain value.

e.g.:

var add = function(a,b) { 
    return a + b;
}

it("should add two numbers", function() {
    var result = add(2, 5);
    expect(result).toBe(7);
});

This is a very simple example, the unit tests in cordova are obviously a bit more complex, but take your time, they'll have a pattern and should be pretty understandable.

If a unit test fails, it generally means that function output or behaviour has changed. Perhaps it is throwing an error, or it's output is returning something completely different than expected (which could signal a breaking change). So ideally, the code should pass the tests without modifying the existing tests. However sometimes, modifying the tests is necessary, depending on the situation.

In your case, the test is failing because it is using asynchronous function, and the done() method is never being called. It is testing the version v1.0.0 and is expecting checkTool to throw an error.

checkTool('node', 'v1.0.0').catch((error) => { 
    expect(error).toEqual('Version should contain only numbers and dots'); 
    done(); 
}); 

You're PR makes it so it can handle version string v1.0.0, so now checkTool is not throwing an error when testing v1.0.0, thus the .catch statement is never being called. So in this case, it should be safe to modify the existing unit test because v1.0.0 is something cordova can now successfully parse and handle. So we can probably change this test to test for something more outlandish, say a.b.c, or maybe 1.2.a

[breautek]

Good job 👍

I don't have easy access to give things a quick local test so, I'll like to add a couple more reviewers.

[mattmilan-dev]

Thank you for an elaborate explanation on unit testing and how it works, I gave the Jasmine docs enough of a read to get this unit test working, but I will read more into them later and hopefully even implement them in my Cordova project that I'm working on at the moment, as they seem exceptionally useful.

I'm out for the rest of the evening now but will keep monitoring if any additional reviewer has any feedback. 👍

[raphinesse]

Thanks for taking the time to work on this @devleaf-matt, and thanks for providing guidance @breautek!

I think we should do this as simple as possible. See my comment about that.

[raphinesse]

Our JSDocs say "Compares two semver-notated version strings". So in my opinion, this whole function could (and should) be implemented as follows:

exports.compareVersions = semver.compare

[codecov-io]

Codecov Report

Merging #695 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #695      +/-   ##
==========================================
- Coverage   74.24%   74.19%   -0.06%     
==========================================
  Files          11       11              
  Lines        1833     1829       -4     
==========================================
- Hits         1361     1357       -4     
  Misses        472      472

Impacted Files	Coverage Δ
bin/templates/scripts/cordova/lib/versions.js	90.24% <100%> (-0.46%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be68c9f...5742b74. Read the comment docs.

[mattmilan-dev]

I didn't know the comparison method would do everything, I guess I overlooked the simple step haha! I've rewritten the unit test and method to work as per your feedback and got a clean npm test on my end.

[raphinesse]

OK, so apparently we haven't only been passing valid semver versions into that function. Surprise! 😒

We use it to check requirements of native dependencies like Xcode too. But the Xcode versions we pass into that function are not valid semver (e.g. 10.1). So we need to be a bit more forgiving.

So what I did is keep the original version if it is a valid semver and only coerce it if it isn't. That way, valid semver versions keep their pre-release identifers and we can still just use semver.compare later on. Of course, pre-release identifers on non-semver versions will still be discarded and thus not considered during the comparison. But I find that behavior acceptable.

[breautek]

There's always a breaking exception haha!

[raphinesse]

Everything passing now. Are you guys OK with my changes?

[mattmilan-dev]

Everything passing now. Are you guys OK with my changes?

Looking good to me.

[breautek]

LGTM!

[sanyashvets]

let's go boiii

[sanyashvets]

this PR would be added to 5.0.2 version, right?

[raphinesse]

@sanyashvets The fact that I added this to the 5.0.2 milestone means that this can go into the next patch, minor or major release. E.g. it could be that there will be no 5.0.2, then it would go into 5.1.0 or even 6.0.0.

[ambroselittle]

I needed this fix for using a beta version if ios-deploy@beta on Catalina. So I just installed from master. Is there a good way to get notified when the fix is published on a release so I can update accordingly then? (Will you update this PR?)

[raphinesse]

@ambroselittle We usually don't update PRs if they get released. You can watch our blog where we announce all releases (has an RSS feed too).

Alternatively, it seems you can setup an E-Mail notification on new versions of cordova-ios by using https://npmnotifier.party/ (I have never used this service myself).

In any case, this change will be in the next release.

[ambroselittle]

@raphinesse, that'll work. Thanks!