test(ci): Node.js 22 in CI #2231
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kriskowal
force-pushed
the
kriskowal-ci-canary-latest
branch
2 times, most recently
from
0d1f5a1
to
6239d56
Compare
|
The CI failures for ubuntu-latest look like the ones fixed by #2229, which is what's supposed to happen! Great! |
kriskowal
force-pushed
the
kriskowal-ci-canary-latest
branch
4 times, most recently
from
76b10d5
to
0f9aee7
Compare
kriskowal
force-pushed
the
kriskowal-ci-canary-latest
branch
from
0f9aee7
to
cfa5aef
Compare
kriskowal
changed the title
erights
added a commit
that referenced
this pull request
Apr 21, 2024
erights
added a commit
that referenced
this pull request
Apr 21, 2024
2 tasks
erights
added a commit
that referenced
this pull request
Apr 22, 2024
closes: #2198 refs: #2230 #2200 https://chromium-review.googlesource.com/c/v8/v8/+/4459251 #2229 #2231 ## Description Alternative to #2229 that just hacks `harden` to directly repair a problematic error own stack accessor property, replacing it with a data property. ### Security Considerations Both before and after this PR, `passStyleOf` will reject errors with the v8 problematic error own stack accessor property, preventing the unsafety at stake here. However, this would mean that much existing code that used to be correct will break when run on a v8 with this problem. ### Scaling Considerations Avoids any extra overhead on platforms without this problem, including all platforms other than v8. ### Documentation Considerations probably none. This PR essentially avoids the need to document the v8 problem that it masks. ### Testing Considerations Only needed to repair one test to use `harden` rather than `freeze`, in a case where `harden` was more natural anyway. ### Compatibility Considerations This PR enables more errors to pass that check without further changes to user code. #2229 had similar goals, but would still require more changes to user code than this PR. This is demonstrated by all the test code in #2229 that needed to be fixed that does not need to be fixed in this PR. ### Upgrade Considerations none - ~[ ] Includes `*BREAKING*:` in the commit message with migration instructions for any breaking change.~ - ~[ ] Updates `NEWS.md` for user-facing changes.~
Merged
erights
added a commit
that referenced
this pull request
Jun 17, 2024
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! ✰ ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> > Most PRs should close a specific Issue. All PRs should at least reference one or more Issues. Edit and/or delete the following lines as appropriate (note: you don't need both `refs` and `closes` for the same one): Closes: #2314 Refs: #2231 b11beba ## Description Now that nvm recognizes Node 22, we should probably redo the change from #2231 undone by b11beba . ### Security Considerations Whether our code works on Node 22 is much more relevant, including for security, than whether it works on Node 21. Indeed, until 22 could be used we were using Node 21 only for early warning about Node 22. ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations the point. Now that Node 22 is out, we should test on it. ### Compatibility Considerations Hopefully none ### Upgrade Considerations None
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #2230
Description
This change adds
latestto the CI node setup action so we can see upcoming breaking changes to our platform. The corresponding test results will not be required for merge.Testing canary did not pan out. You can get
22-v8-canaryout ofactions/setup-node, and you can addyarn install --ignore-engines, butyarnwill still die with error if any dependency specifiesenginessince the canary versions do not match other version ranges, even broad ones like>= 16. This may be worth revisiting with newer versions ofyarnor whatever succeeds it.Security Considerations
This increases our visibility into breaking changes on the Node.js platform.
Scaling Considerations
This may prolong CI runs by a multiplicative factor if CI workers are contended.
Documentation Considerations
None.
Testing Considerations
Increases visibility into our future.
Compatibility Considerations
None.
Upgrade Considerations
None.
[ ] Includes*BREAKING*:in the commit message with migration instructions for any breaking change.[ ] UpdatesNEWS.mdfor user-facing changes.