Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent order from ProjectBuilder.apkSorter #779

Merged
merged 1 commit into from
Jul 14, 2019

Conversation

brodycj
Copy link
Contributor

@brodycj brodycj commented Jul 14, 2019

The existing implementation of this function gives a different order depending on the behavior of Array.prototype.sort(), which has led to a test failure on Node.js 12 (see #767).

This update gives a consistent sort order, regardless of the JavaScript engine implementation, now succeeds on Node.js versions 6, 8, 10, and 12.

Resolves #767

I hope this explanation is clear enough. If anyone can think of a better explanation or has any criticism, feedback would be much appreciated.

For reference:

/cc @breautek

This function used to give a different order depending on the behavior
of Array.prototype.sort(), which led to a test failure on Node.js 12
(see apache#767).

This update gives a consistent sort order, regardless of the
JavaScript engine implementation, now succeeds on Node.js
versions 6, 8, 10, and 12.

Resolves apache#767

For reference:
- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This test randomly failing every time I've touched anything in cordova-android has been plaguing me for years

@brodycj brodycj merged commit acad24d into apache:master Jul 14, 2019
@brodycj brodycj deleted the fix-apk-sorter branch July 14, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

npm test fails on Node 12
2 participants