test(util-inspect): sample surrogate boundaries instead of all 2047 code units - #36147
test(util-inspect): sample surrogate boundaries instead of all 2047 code units#36147robobun wants to merge 1 commit into
Conversation
…ode units The unpaired-surrogate escape block in "no assertion failures 2" iterated every code unit in 0xd800..0xdffe (~9k util.inspect calls). Under a debug ASAN build that loop alone runs for ~4.3s and pushes the whole test body past the 5s default, so `bun bd test` fails locally while CI (which overrides per-test timeouts via runner.node.mjs) stays green. inspect.js detects lone surrogates with the regex classes [\ud800-\udbff] and [\udc00-\udfff] and a single range compare; there is no per-codepoint table. Covering the boundaries of both halves plus a couple of interior samples exercises the same paths. Before: (fail) no assertion failures 2 [7810.78ms] (timed out after 5000ms) After: (pass) no assertion failures 2 [2092.55ms]
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only workload reduction that keeps the load-bearing assertions.
What was reviewed:
- Confirmed the loop body is per-iteration independent; sampled set exercises both the
charCode < 0xdc00branch (5 high surrogates, incl.+1024pairing → valid low surrogates 0xdc00–0xdfff) and the else branch (5 low surrogates). - Boundary values 0xd800/0xdbff/0xdc00/0xdffe cover the exact edges of the range-regex implementation the PR cites; no per-codepoint table exists to require exhaustive iteration.
- Comment documents the upstream divergence, satisfying the "don't silently weaken" rule.
Extended reasoning...
Overview
Single-line change to test/js/node/util/node-inspect-tests/parallel/util-inspect.test.js: replaces a 2047-iteration for (charCode = 0xd800; charCode < 0xdfff; charCode++) loop with a 10-element array of boundary and interior surrogate code units. A one-line comment records that upstream Node walks the full range and why sampling is equivalent here.
Security risks
None. Test-only, no src/ change, no I/O, no network, no subprocess.
Level of scrutiny
Low. This is a mechanical test-performance fix for a debug+ASAN timeout, following REVIEW.md's "shrink the workload, don't raise the timeout" guidance. The one review concern for this class of change — silently weakening a safety net — is addressed: the util.inspect lone-surrogate escape is implemented as a range regex plus a single >= 0xd800 && <= 0xdfff comparison, so boundary + interior samples are equivalent coverage to the full sweep. The chosen samples hit both sides of the < 0xdc00 branch inside the loop (5 each), and for the high-surrogate branch charCode + 1024 still lands in [0xdc00, 0xdfff] for every sampled value, so the paired-surrogate assertion remains valid.
Other factors
The PR description notes overlap with #36138's stride-17 approach and correctly points out that a stride of 17 skips the 0xdbff/0xdc00 transition — this explicit-list approach is cleaner. No prior reviewer comments to address. Local verification output in the description shows the test now completes in ~2s under the 5s default.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
CI on build 83608: The remaining reds are unrelated to this 2-line test-file diff:
Ready for review. |
1 participant
Problem
bun bd test test/js/node/util/node-inspect-tests/parallel/util-inspect.test.jsfails locally on main:CI doesn't see it because
scripts/runner.node.mjsoverrides the per-test timeout.Cause
The unpaired-surrogate escape block walks every code unit from
0xd800to0xdffe(2047 iterations, ~9kutil.inspectcalls with 200-char strings). Under a debug+ASAN build that loop alone runs for ~4.3s; the rest of the 1500-line test body pushes it over 5s.Fix
inspect.js detects lone surrogates with a range regex (
[\\ud800-\\udbff]/[\\udc00-\\udfff]) and a single>= 0xd800 && <= 0xdfffcompare; there is no per-codepoint table. The 10 boundary/interior samples below cover both high-surrogate and low-surrogate branches, including the exact transition points0xdbff/0xdc00:REVIEW.md: "Don't raise per-test timeouts to make a slow test pass; shrink the workload."
Verification
Release build still runs the same assertions in a few ms.
Note
#36138 carries a ride-along for this as part of its
builtInObjectschange (moving the block to its own test withstep = isDebug ? 17 : 1). A stride of 17 from0xd800lands on0xdbfcthen0xdc0d, skipping the0xdbff/0xdc00boundary. This PR is the standalone minimal version; happy to close if #36138 lands first.This change is test-only; there is no
src/diff to prove fail-before against.no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.