-
Notifications
You must be signed in to change notification settings - Fork 527
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
chore: add automated CI testing with —no-intl node #3015
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3015 +/- ##
=======================================
Coverage 93.51% 93.51%
=======================================
Files 89 89
Lines 24231 24234 +3
=======================================
+ Hits 22659 22662 +3
Misses 1572 1572 ☔ View full report in Codecov by Sentry. |
With most recent commits, w/o intl build tests are green, PR description has been updated, and ready for review. As a test, I reintroduced the commit which caused nodejs CI to fail its w/o intl build, and the check failed as expected -- see mweberxyz#6 |
Co-authored-by: Carlos Fuentes <[email protected]>
.bar is a valid, though unpopular, TLD. should be safer to use a domain in .invalid as specified in proposed RFC6761.
"test:cookies": "borp -p \"test/cookie/*.js\"", | ||
"test:node-fetch": "borp -p \"test/node-fetch/**/*.js\"", | ||
"test:eventsource": "npm run build:node && borp --expose-gc -p \"test/eventsource/*.js\"", | ||
"test:fetch": "npm run build:node && borp --expose-gc -p \"test/fetch/*.js\" && borp -p \"test/webidl/*.js\" && borp -p \"test/busboy/*.js\"", | ||
"test:eventsource": "npm run build:node && npm run test:eventsource:nobuild", |
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.
why is building bad?
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.
Build fails with icu-less node build:
![CleanShot 2024-03-29 at 14 25 05@2x](https://private-user-images.githubusercontent.com/1062734/318114485-e95bb636-82aa-4f2c-95b3-cd7afa1c8bc4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjU2NTgxMjgsIm5iZiI6MTcyNTY1NzgyOCwicGF0aCI6Ii8xMDYyNzM0LzMxODExNDQ4NS1lOTViYjYzNi04MmFhLTRmMmMtOTViMy1jZDdhZmExYzhiYzQucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwNiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDZUMjEyMzQ4WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MDE1MjIyZWYwZjgzZjE2YzRmYWRhNmQzY2JjMTU3NzI1MDc3YWRhYjhkYTExOTRjOTcyMmRhMzQxYzE2NDA5NSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.npKWoPvtMj591YaTzBtDO0zqxCP-1NmzImR4e2Ubew4)
But we shouldn't need to support building undici with icu-less node, only running it.
This is why the workflow installs regular node then builds undici prior to switching to the icu-less build for running tests.
I tried to minimize changes to package.json, so as to make people not need to think about keeping the test:javascript:withoutintl
script up-to-date with test strategy changes.
@fraxken Does anything in here conflict with your auditing re: #3012? I tried to be more mindful of security implications this time around (ie- I reviewed the code for this newly called action and made sure to pin it), but would appreciate an extra set of eyes. |
@mweberxyz seems ok to me 😉 |
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.
lgtm
Could this be landed? |
This relates to...
Rationale
This PR extends the Node CI workflow to run undici tests against Node 20 and 21 built with
--without-intl
flag, which would have prevented the regression fixed by #2999 before the commit even merged to undici/main.The full test suite currently failing against
main
, so problematic tests have been fixed where possible, otherwise excluded from running in the w/o intl build. If possible, these problematic tests/functionality should be fixed to run in w/o intl builds and the tests added totest:javascript:withoutintl
(or even remove it entirely and both the w/o intl and regular builds can usetest:javascript
).Note that the initial run, and occasional runs thereafter, of the workflow will take a long time (around 1 hour for the clean Node 21 build) -- but subsequent runs will be much faster due to ccache caching.
Changes
test-without-intl
job to workflows/nodejs.ymltest:javascript:withoutintl
script to run tests known good with an w/o intl node buildtest:eventsource:nobuild
,test:fetch:nobuild
scripts to pull test running logic out oftest:eventsource
andtest:fetch
because undici is built prior to running w/o intl teststest:wpt:withoutintl
script to run wpt tests except those problematic for w/o intl, namely:test/wpt/start-websockets.mjs
, andtest/wpt/start-FileAPI.mjs
Features
N/A
Bug Fixes
N/A
Breaking Changes and Deprecations
N/A
Status