Repository navigation
test(javascript): assert what load() owes, not what Deno can import - #22252
Conversation
`should install onto terser` asserted that under Deno `phases` is empty, because Deno could not import terser's own sources when the test was written. It sometimes can, and then the test fails — it went red once on #22243, a pull request touching only CSS fixtures, and cost a re-run of a 21-minute job to establish it was not that change's doing. Which sources a runtime reaches is the one thing `load` is written to tolerate: it catches any failure and hands back terser's published entry point. So the runtime is the wrong thing to assert. What `load` owes everywhere is that it installed phases this build declares and nothing else, and Node, which always reaches the sources, still owes the whole set — both of which this now states. The fallback path stays covered by behaviour rather than by a hardcoded outcome: every `should mangle exactly as terser does` case compares webpack's `minify` against terser's own, whichever way `load` resolved.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: webpack/webpack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughChangesSyntax printer test update
Priority: ⬇️ Low Change: Other Merge Risk: ⚪ Minimal · up to This test-only update removes a runtime-specific assumption and aligns expectations with declared phases, without changing product behavior. It is ready to merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
|
|
This PR is packaged and the instant preview is available (95930fd). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@95930fd
yarn add -D webpack@https://pkg.pr.new/webpack@95930fd
pnpm add -D webpack@https://pkg.pr.new/webpack@95930fd |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #22252 +/- ##
=======================================
Coverage 95.03% 95.03%
=======================================
Files 762 762
Lines 105728 105728
Branches 32541 32541
=======================================
+ Hits 100480 100483 +3
+ Misses 5248 5245 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated code sizeComparing base (
Read No asset changed size. No runtime gained or lost a runtime module. Built |
Merging this PR will degrade performance by 27.3%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "many-modules-interop-runtime", scenario '{"name":"mode-development","mode":"development"}', measure 'exec' |
56.5 KB | 141 KB | -59.9% |
| ❌ | Memory | benchmark "asset-modules-bytes", scenario '{"name":"mode-production","mode":"production"}' |
6.6 MB | 8.4 MB | -21.18% |
| ⚡ | Memory | benchmark "hmr", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
967.5 KB | 795.8 KB | +21.58% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing test/deno-terser-phases-assertion (76aafc1) with main (e2023b0)
Footnotes
-
6 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Summary
syntax-printer.unittest.js › should install onto terserasserted thatphasesis empty under Deno, because Deno could not import terser's own sources when it was written. It sometimes can, and the test then fails — it went red once on #22243, a pull request touching only CSS fixtures, and took a re-run of a 21-minute job to establish it wasn't that change's doing. Which sources a runtime reaches is precisely whatload()is written to tolerate, so it asserts the installation contract instead: phases this build declares and nothing else, with Node still owing the whole set.What kind of change does this PR introduce?
test
Did you add tests for your changes?
n/a — this is the test change; the
should mangle exactly as terser doescases already cover the fallback path by comparing webpack'sminifyagainst terser's howeverload()resolved.Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
AI was used: it diagnosed the failure from the
runtimes (deno)log, established it was unrelated to the pull request it appeared on, wrote the replacement assertion, and checked it is load-bearing by makingload()report a phase name the build does not declare and confirming the test fails.Generated by Claude Code
Summary by CodeRabbit