color: unmark the lab round-trip test as failing - #33351
Conversation
The lab -> sRGB conversion it was pinned to was fixed in #33333, so the test now passes and the runner reports the stale `.failing` marker. Assert the round-trip over the same inputs the hsl twin uses, plus a cube sweep, instead of the single color and the weaker not-null check the marker left behind.
|
Warning Review limit reached
Next review available in: 2 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 |
There was a problem hiding this comment.
LGTM — test-only change removing a stale .failing marker and strengthening coverage using the same pattern as the neighboring hsl tests.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/css/color.test.ts. It removes the .failing marker from the lab round-trips test (which now passes since #33333 fixed the underlying gamut-mapping bug), folds in the adjacent weaker lab output is CSS that Bun can parse back test (upgrading its .not.toBeNull() assertion to an exact .toBe(color(input, "hex")) round-trip check), and adds a lab round-trips across the color cube sweep that is a byte-for-byte mirror of the existing hsl round-trips across the color cube test with only the format string changed.
Security risks
None. This is a test-only change with no modifications to runtime, native, or build code.
Level of scrutiny
Low. The change is mechanical: a stale test.failing marker was breaking CI on main (the runner flags failing-marked tests that pass), and the fix follows the exact structure of the sibling hsl tests already in the file. The new cube sweep uses the same step sizes (37/53/61) and withoutAggressiveGC wrapper as its twin. The #0000ff case is explicitly retained in the input list with a comment explaining why the cube sweep doesn't cover it (steps never reach 255).
Other factors
The bug hunter found no issues. The PR description includes verification that the tests pass on the current build and fail when #33333's fix is reverted, confirming they're load-bearing. The stronger .toBe() assertion strictly subsumes the removed .not.toBeNull() check. No outstanding reviewer comments and no prior reviews from me on this PR.
test/js/bun/css/color.test.tsfails onmain:Cause
test.failing("lab round-trips")was added in #33328 pinned to #33331 (lab()colors on the sRGB gamut boundary were desaturated:#0000ffround-tripped to#002cea). #33333 fixed that conversion by adding the CSS Color 4 initial clip tomap_gamut, but left the marker behind, so the test now passes and the runner flags it.Fix
Drop the marker. The test asserted one color and sat next to
lab output is CSS that Bun can parse back, which looped over eight inputs with the weakernot.toBeNull()check, so this folds them into one: the round-trip over the same input list thehsl round-tripstwin uses, plus#0000ff, and a cube sweep mirroringhsl round-trips across the color cube. The not-null assertion is subsumed (toBe(color(input, "hex"))cannot pass onnull).#0000ffstays an explicit case because the cube sweep steps by 61 and never reaches 255.Verification
Both tests are load-bearing. Reverting #33333's
map_gamutclip turns them red along with the five cases that PR added:The round-trip is exact now, not just for the colors asserted here. #33331 reported 15 of 4096 colors wrong with a worst channel error of 44/255; sweeping ~930k colors on the current build finds zero mismatches:
sweep
The step-17 sweep costs ~1s on a debug build, so the committed cube test uses the coarser steps of its
hsltwin (~29ms). Those steps still catch the regression, as the fail-before run above shows.Closes #33331. The conversion fix landed in #33333; this is the marker it left behind.