chore: add automated CI testing with —no-intl node#3015
chore: add automated CI testing with —no-intl node#3015mcollina merged 7 commits intonodejs:mainfrom
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. |
|
I started looking into the actual test failures, the problems might not be all that arduous. First, there's a failing test which is simply a misaligned console.table assertion. Then, some tests run I've gotten down into the Will update with more findings and if I get the tests to fully pass. |
|
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: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.
Build fails with icu-less node build:
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.
|
One other omission I realize I haven't mentioned: 
|
|
@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 😉 |
|
Could this be landed? |
7 participants
This relates to...
Rationale
This PR extends the Node CI workflow to run undici tests against Node 20 and 21 built with
--without-intlflag, 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-intljob to workflows/nodejs.ymltest:javascript:withoutintlscript to run tests known good with an w/o intl node buildtest:eventsource:nobuild,test:fetch:nobuildscripts to pull test running logic out oftest:eventsourceandtest:fetchbecause undici is built prior to running w/o intl teststest:wpt:withoutintlscript to run wpt tests except those problematic for w/o intl, namely:test/wpt/start-websockets.mjs, andtest/wpt/start-FileAPI.mjsFeatures
N/A
Bug Fixes
N/A
Breaking Changes and Deprecations
N/A
Status