-
Notifications
You must be signed in to change notification settings - Fork 524
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
Avoid structuredClone in Response() #2617
Conversation
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2617 +/- ##
==========================================
- Coverage 85.54% 84.82% -0.73%
==========================================
Files 76 81 +5
Lines 6858 7275 +417
==========================================
+ Hits 5867 6171 +304
- Misses 991 1104 +113 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matteo Collina <hello@matteocollina.com>
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.
This will cause the process to hang on v18 and v20
How? The tests are passing. |
After a bit of digging, This was later fixed in c7eb5f0 by adding an additional However, the given WPT test is marked as fail. It does not seem we are losing anything by removing |
Ah right, I was mistaking it for something else related to this code. When the WPT runner fails a test, it doesn't cause the process to exst with an error (this was changed so it could run with node to report fetch test stats). While those 2 tests are marked as failing, a bunch of others aren't, and those are failing:
|
I think our CI should catch a regression on WPT coverage (cc @panva). |
No, undici's WPTs are ran in undici, not nodejs/node. What runs in node is a daily WPT Report with the bundled undici version's git checkout and its own runner but this does not alert anyone on regressions given it also always pulls the latest WPT from web-platform-tests/wpt. nodejs/citgm#982 would run undici's checkout of WPTs We could also add a github actions job in nodejs/node to run undici's fetch and WPT tests on every PR. |
@KhafraDev I don't recall such a change, necessity. I believe exit codes are irrelevant for the nodejs daily WPT job. https://github.com/nodejs/node/blob/main/.github/workflows/daily-wpt-fyi.yml#L114 What's relevant is that the WPTs run and report, not how they exit. |
Thanks for the clarification, maybe the change was made in node core to ignore the exit code and in undici to always exit with |
FWIW |
Thank you. I have been trying to figure out how to make Downgraded to v5 for now. Hope to be able to switch back to v6 when a new release with this fix is out :) |
We are not landing this, instead we are reverting the problem in Node.js core. |
Fixes #2615
Fixes #2618 (to make the tests pass on Node 20)
Note that this is caused by nodejs/node#51255 by @jasnell.
It can also be reproduced with:
which will fail with: