mattmilan-dev commented 4 years ago

Platforms affected

iOS

Motivation and Context

Fixes https://github.com/apache/cordova/issues/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

[x] I've run the tests to see all new and existing tests pass
[x] I added automated test coverage as appropriate for this change
[x] Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
[x] 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)
[x] I've updated the documentation if necessary

breautek commented 4 years ago

Looks like there is a couple of tests failing.

This is a unit test that you can find here: https://github.com/apache/cordova-ios/blob/be68c9f6d49aff85a5f7a52ffcbfd31d5b018a90/tests/spec/unit/lib/check_reqs.spec.js#L59-L74

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 commented 4 years ago

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 commented 4 years ago

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

mattmilan-dev commented 4 years ago

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. 👍

codecov-io commented 4 years ago

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%)`	:arrow_down:

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 commented 4 years ago

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 commented 4 years ago

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

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 commented 4 years ago

There's always a breaking exception haha!

raphinesse commented 4 years ago

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

mattmilan-dev commented 4 years ago

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

Looking good to me.

breautek commented 4 years ago

LGTM!

sanyashvets commented 4 years ago

let's go boiii

sanyashvets commented 4 years ago

this PR would be added to 5.0.2 version, right?

raphinesse commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@raphinesse, that'll work. Thanks!

apache / cordova-ios

(iOS) Added Semver to version comparison checks. #695

Platforms affected

Motivation and Context

Description

Testing

Checklist

Codecov Report