Feature request: Need to habitually test on pre-release forms of supported target platforms #2230
Assignees
Labels
bug
Something isn't working
Comments
1 task
|
See also Agoric/agoric-sdk#9265 |
This was referenced Apr 19, 2024
kriskowal
added a commit
that referenced
this issue
Apr 21, 2024
refs: #2230 ## Description This change adds `latest` to the CI [node setup action](https://github.com/actions/setup-node) 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-canary` out of `actions/setup-node`, and you can add `yarn install --ignore-engines`, but `yarn` will still die with error if any dependency specifies `engines` since the canary versions do not match other version ranges, even broad ones like `>= 16`. This may be worth revisiting with newer versions of `yarn` or 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.~ - ~[ ] Updates `NEWS.md` for user-facing changes.~
|
@kriskowal , @frazarshad , I co-assigned this to the three of us. @kriskowal I see you merged #2231 despite the two CI failures https://github.com/endojs/endo/actions/runs/8760680740/job/24046036596 . What’s failing there is exactly what this PR was adding That’s why I tried #2232, #2233, and #2234 . #2234 brings this back to green CI with 21.x, which it finds, rather than 22.x, which it does not. Also, how did you merge this despite the CI failures? Isn’t that supposed to be what CI protects us from? Could/should you back out #2231 while we make progress towards getting the state of #2234 reviewed and merged? |
erights
added a commit
that referenced
this issue
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
#2198 was erroneously closed by #2200 because #2198 only manifested on browsers and we manually tested #2200 on those browsers to "verify" that it was fixed.
However, we are not set up to run our normal
yarn testCI tests habitually (or under CI) on those browser engines. We current do our regularyarn testCI testing, where everything was coming up green, but only because the most recent Node we CI is Node 20, and Node 20 does not yet have v8's problematic stack accessor behavior. I separately reopened #2198 until those old tests are fixed, in progress at #2229. But currently I can only verify that #2229 fixes it by runningyarn testlocally with Node 21.So, feature request: We should as much as possible run all our normal tests on the pre-release form of all target platforms we support, so we get early warning when we're about to stop working on a release.
Fortunately, in this case, it looks like the
src/fixes from #2200 do still actually fix the #2198 problem. The only remaining problem, being addressed in #2229, is to update the tests. This would indeed have caused CI failures as soon as we would upgrade to support Node 22, but only because the tests are wrong. The code being tested already seems good for Node 21 and thus presumably for Node 22, at least as far as these issues are concerned.The text was updated successfully, but these errors were encountered: