-
Notifications
You must be signed in to change notification settings - Fork 313
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
Upgrade Node.js to version 20 #7782
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7782 +/- ##
=========================================
Coverage 67.84% 67.84%
Complexity 2045 2045
=========================================
Files 357 357
Lines 16770 16770
Branches 2378 2378
=========================================
Hits 11378 11378
Misses 4402 4402
Partials 990 990
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
} | ||
result.issues.filter { | ||
it.source == "NPM" && it.severity == Severity.ERROR && it.message.contains("Unexpected token") | ||
} shouldHaveSize 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using the shouldHaveSingleElement { condition }
matcher? Also, prefer the in
operator over contains()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done it slightly differently now, but following your underlying idea to use assertions instead of comparisons.
@@ -9,8 +9,8 @@ GO_DEP_VERSION=0.5.4 | |||
GO_VERSION=1.21.1 | |||
HASKELL_STACK_VERSION=2.7.5 | |||
JAVA_VERSION=17 | |||
NODEJS_VERSION=18.17.1 | |||
NPM_VERSION=8.15.1 | |||
NODEJS_VERSION=20.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message nit:
Use the LTS version
Maybe say "current LTS version" as the LTS version changes over time.
ORT's Npm integration maps Npm's error output to a set of issues. In case of a malformed `package.json`, this yields a single issue with the currently used Npm version. A more recent Npm versions, 10.1.0, yields two issues. Also the wording of the issue to be matched slightly differs. So, relax the test assertion such that it does not break when upgrading Npm to 10.1.0 which is needed in order to upgrade Node to version 20. Signed-off-by: Frank Viernau <[email protected]>
Use the current LTS version for `Node.js` in `.versions` and in `Dockerfile`. When that version is installed via `nvm`, in Dockerfile, Npm version 10.1.0 is installed. So, also upgrade the Npm to that version. Signed-off-by: Frank Viernau <[email protected]>
Merged regardless of unrelated detect failure. |
See individual commits.