Skip to content

PPLT-6073: raise the PDF regression tolerance and close the PDF coverage gaps - #2432

Merged
rishigupta1599 merged 1 commit into
masterfrom
PPLT-6073_forwardcleanup
Sep 16, 2026
Merged

rishigupta1599 merged 1 commit into
masterfrom
PPLT-6073_forwardcleanup

Conversation

@RaghavsBrowserStack

Copy link
Copy Markdown
Contributor

Fixes the two CI failures seen on the 1.32.10 release PR (#2430). Test-only — no product code changes.

Regression (Track P)

jack sparrow resume.pdf diffed by 648 pixels against a 600 budget, in box [369, 1447, 692, 1448] at a channel delta of 4.

That box is not the circular photo clip the 600 was measured against — it is the hairline rule under the LANGUAGES heading, whose two pixel rows are the only non-white rows across those 324 columns. A second, previously unmeasured source of run-to-run jitter on a fixed Chromium build (the manifest browser and the run browser matched, so the goldens are not stale).

Budget raised to 2000 — roughly 2x the 918-pixel worst case of both sources landing at once, and still 0.1% of the page. A real rendering regression moves glyphs or layout and blows straight past it, and a size change is never tolerated at all. The second source is now documented alongside the first.

@percy/core coverage

All 1322 specs passed; the job failed nyc's 100% threshold on two uncovered spots.

  • pdf-rasterize.js:128 — the asInputError rethrow when a rendered page comes back under Percy's 10px floor. fitScale only clamps the upper bound, so a 4x4pt MediaBox rasterizes to 8x8 at scale 2 and trips it. The new test also pins that this surfaces as a 400, not a 500.
  • pdf-snapshot.js:67-68 — the decoded-size check. The existing oversize test trips the encoded cap first, leaving the decoded one unreachable. The cap rounds up to a whole base64 quantum, so 52428801 bytes encodes to exactly MAX_PDF_BASE64_CHARS and clears it — the two-byte window the second check exists to close.

Verification

Both new specs pass locally. Because nyc returns no data for a spec-filtered run, coverage was confirmed by mutation instead — each target line was broken in turn and the corresponding spec failed:

Mutation Result
pdf-rasterize.js:128throw asInputError(error)throw error Expected 500 to be 400
pdf-snapshot.js:67if (buffer.length > MAX_PDF_BYTES)if (false && …) fell through to the %PDF- magic check

Regression has already passed green with this change.

Merge order

CI runs on the merge result (refs/pull/N/merge), so once this lands on master, re-running #2430 will pick the fix up automatically — no change needed on the release branch.

🤖 Generated with Claude Code

Two CI failures on the 1.32.10 release PR, neither a product bug.

Regression (Track P): `jack sparrow resume.pdf` diffed by 648 pixels against
a 600 budget, in box [369, 1447, 692, 1448] at a channel delta of 4. That box
is not the circular photo clip the 600 was measured against — it is the
hairline rule under the LANGUAGES heading, whose two pixel rows are the only
non-white rows across those 324 columns. A second, previously unmeasured
source of run-to-run jitter on a fixed Chromium build (the manifest browser
and the run browser matched, so the goldens are not stale). Budget raised to
2000, ~2x the 918-pixel worst case of both sources landing at once and still
0.1% of the page, with the second source documented.

@percy/core: all 1322 specs passed; the job failed nyc's 100% threshold on
two uncovered spots.

  - pdf-rasterize.js:128, the asInputError rethrow when a rendered page comes
    back under Percy's 10px floor. fitScale only clamps the upper bound, so a
    4x4pt MediaBox rasterizes to 8x8 at scale 2 and trips it. The new test
    also pins that this surfaces as a 400, not a 500.
  - pdf-snapshot.js:67-68, the decoded-size check. The existing oversize test
    trips the encoded cap first, leaving the decoded one unreachable. The cap
    rounds up to a whole base64 quantum, so 52428801 bytes encodes to exactly
    MAX_PDF_BASE64_CHARS and clears it — the two-byte window the second check
    exists to close.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 42 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Workspace UI

Review profile: CHILL

Plan: Advanced

Run ID: 400bab14-c326-48b6-bde7-a73accbf0094

📥 Commits

Reviewing files that changed from the base of the PR and between b90cd9f and 96d0cc6.

📒 Files selected for processing (2)
  • packages/core/test/pdf-snapshot.test.js
  • test/regression/pdf-render.test.js

Comment @coderabbitai help to get the list of available commands.

@rishigupta1599
rishigupta1599 merged commit 3d66888 into master Sep 16, 2026
98 of 99 checks passed
@rishigupta1599
rishigupta1599 deleted the PPLT-6073_forwardcleanup branch September 16, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants