test: remove expected-durations.test.ts - #36781
Conversation
It asserted no js/*/test/parallel/ entry in test/expected-durations.json exceeds 500 ms, assuming the generator clamps every concurrent-phase span. The generator only clamps inter-dispatch gaps; explicit `(N.Ns)` timed headers are recorded as-is, so a genuinely slow parallel test (e.g. test-fs-read-stream-pos.js at 74 s on musl) fails the test after any regeneration. The remaining assertions only re-validated the shape of a file that is produced by scripts/update-test-durations.mjs. No-Verification-Needed: deletes a test file only
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe pull request deletes the expected duration validation test and removes its checks for lane metadata, test-path keys, parallel-phase coverage, span clamping, lane durations, and minimum table size. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — clean removal of an internal test whose core assertion no longer matches the generator's contract.
What was reviewed:
- Confirmed the 500 ms clamp assertion conflicts with
update-test-durations.mjsrecording explicit(N.Ns)headers at real wall-clock (by design, so the bin-packer sees true cost). - Verified nothing else references this test file; the only grep hit is its own stale entry in
expected-durations.json. - Remaining deleted assertions were shape checks on a machine-generated file — no coverage lost for hand-authored code.
Extended reasoning...
Overview
This PR deletes a single file, test/internal/expected-durations.test.ts (65 lines). The file was a guard test over test/expected-durations.json, a checked-in table produced entirely by scripts/update-test-durations.mjs and consumed by the CI shard bin-packer. It began failing on main after commit a00c4db regenerated the table, because it asserted no js/{node,bun}/test/parallel/* entry exceeds 500 ms — an assumption that only held for clamped inter-dispatch gaps, not for entries derived from explicit (N.Ns) timed headers, which the generator intentionally records at real wall-clock so slow tests weigh correctly in shard packing.
Security risks
None. This touches only an internal test file over CI tooling metadata. No runtime, native, or user-facing code is affected.
Level of scrutiny
Low. This is a test-only deletion with a clearly stated and verifiable rationale. The repo's own review guidance says every deletion needs a stated reason — the PR description provides a precise one (the assertion encoded a wrong assumption about the generator, and the rest of the file re-validated shape invariants the generator itself enforces). Per the same guidance, tests that trip on regens without catching real defects are noise; "do not add tests to check dead code stays dead" applies in spirit here.
Other factors
I confirmed via grep that no other file imports or references expected-durations.test.ts; the sole hit is the durations table listing this test's own runtime, which will drop out on the next regeneration. The commit message and PR description independently explain the mechanism (clamp vs. timed headers), and the failing case (test-fs-read-stream-pos.js at 74 s musl / 90 s windows) is exactly the kind of real signal the bin-packer needs — so "fixing" the test to accommodate it would defeat its only non-trivial assertion. Deletion is the right call.
1 participant
What does this PR do?
Deletes
test/internal/expected-durations.test.ts, which started failing on main after a00c4db regeneratedtest/expected-durations.json.js/{node,bun}/test/parallel/*entry > 500 msscripts/update-test-durations.mjsclamps concurrent-phase inter-dispatch gaps to 500 ms(N.Ns)timed headers bypass the clamp by design, so a genuinely slow parallel test is recorded at real wall-clock —test-fs-read-stream-pos.jsis 74 s on musl / 90 s on windows in the new table, which is what the shard bin-packer should seeHow did you verify your code works?
bun test test/internal/expected-durations.test.tson main reproduces the failure (+ "js/node/test/parallel/test-fs-read-stream-pos.js"); nothing else references the file.