fix(ci): repair the benchmark gate and de-flake the two red platform legs - #356
Merged
Conversation
The perf gate has been red on main since at least 1.0.2 and was NOT reporting a
regression — it was failing before taking a single measurement, so the CI
performance guarantee in CLAUDE.md was effectively vacuous.
Root cause: BenchmarkDotNet writes a per-job project under the benchmark
project's output dir and restores it. That path is inside the repo, so MSBuild
and NuGet walk UP from it and pick up the root Directory.Build.props +
Directory.Packages.props, dragging Central Package Management into the
generated project. On the CI runner restore then tried to resolve the generated
assembly name (a GUID) as a NuGet package:
error NU1101: Unable to find package e0917db2-1744-4b6e-a1dc-bc1266567f06
Only 3 of 33 benchmarks survived; the gate compared that partial result set and
died inside System.Text.Json ("target element has type 'Null'", exit 134) — a
crash that read as an infra flake and hid that nothing was being measured.
Two changes:
1. NetPdf.Benchmarks.csproj writes inert Directory.Build.props/.targets +
a CPM-off Directory.Packages.props into $(TargetDir), so the generated
project's upward search stops below the repo root and it restores
standalone. Sentinels live in bin/ (gitignored) and are rewritten each
build. Two traps are pinned in comments because both bit during
development: $(OutDir) is relative (writes land beside the .csproj, which
turns CPM off for the project itself -> NU1015), and <TargetFrameworks>
runs an outer build with an empty $(TargetDir) (same result).
2. scripts/benchmark-gate.sh no longer lets a partial run reach --compare.
BDN exits 0 even when a generated project fails to build, so the script now
fails with an explicit exit 2 when the run errors, when BDN reports a
boilerplate build failure, or when fewer result files are produced than the
baseline has. A silently incomplete suite can no longer masquerade as a
measured one.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The alpine-musl-x64 and linux-arm64 legs have both been red on main with the same 2 failures (days: 4 and 9) — but 8604 of 8609 tests passed, so this was never the SkiaSharp/native-asset breakage the "non-blocking" labels imply. It was a false positive in the test itself. The document identifies the footer note as "the run with the most glyphs", which silently assumes the note renders as ONE line. CI provisions fonts asymmetrically by design: linux-arm64 installs only fonts-dejavu-core and alpine only ttf-dejavu, while the enforcing linux-x64 runner uses the full Ubuntu font set. Under DejaVu metrics the note wrapped, so the detected "note" was only its first line and its own tail — "departure zulu", exactly the 14 glyphs the failure reported — became a run BELOW it, tripping the "nothing sits below the note" assertion. Wrapping a long line is correct engine behaviour; the test's single-line premise was the bug. The note is now `white-space: nowrap` and the font is pinned like the sibling rendering tests, so the premise holds on every platform. This does not weaken the invariant under test: vertical placement (the note must be last, never overlapped by timeline content) is still fully asserted. Verified by reproducing the failure locally rather than assuming: forcing the note to wrap (no nowrap + wider font) reproduces the exact CI signature — "a run (glyphs=14) ... sits below the footer note" — and the same document with the fix passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR repairs CI/test infrastructure to restore the BenchmarkDotNet performance gate’s integrity (preventing partial benchmark runs from being compared) and de-flakes two platform-specific rendering test failures caused by font-metrics-driven wrapping differences.
Changes:
- Add sentinel MSBuild/NuGet files under the benchmark output directory to prevent repo-level CPM inheritance in BenchmarkDotNet auto-generated job projects.
- Harden
scripts/benchmark-gate.shto fail fast (exit 2) on partial/errored benchmark runs before attempting--compare. - Make
AutoHeightFlexTimelineFooterTestsdeterministic across platforms by pinning font usage and forcing the footer note to remain single-line (white-space: nowrap).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/NetPdf.UnitTests/Rendering/AutoHeightFlexTimelineFooterTests.cs | Prevents platform font differences from causing test false-positives by enforcing a single-line footer note. |
| tests/NetPdf.Benchmarks/NetPdf.Benchmarks.csproj | Writes sentinel files into benchmark output to isolate BenchmarkDotNet generated projects from repo-level MSBuild/NuGet inheritance. |
| scripts/benchmark-gate.sh | Detects benchmark run failures/partial result sets and stops before comparison to keep the gate meaningful. |
Comments suppressed due to low confidence (1)
tests/NetPdf.Benchmarks/NetPdf.Benchmarks.csproj:74
- This paragraph refers to dropping the sentinel files in
$(OutDir), but the target actually writes them to$(TargetDir). Aligning the comment with the implementation will avoid misleading future maintenance/debugging.
Dropping inert sentinels in `$(OutDir)` stops the upward search BELOW the repo root, so the generated
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR #356 review follow-up. All three findings were valid. [P1] The gate could still go green with benchmarks missing from INSIDE an existing report. Comparison printed "missing" for a baseline key with no current measurement and continued, then exited 0 when nothing regressed — and the shell wrapper only counts report FILES, which cannot see this: one file carries many benchmarks (the pinned baselines hold 33 across 7 files), so a truncated export keeps the file count identical. A missing baseline key is now ComparisonOutcome.Incomplete (exit 2), and it OUTRANKS a regression, because an incomplete run cannot vouch for the benchmarks that did run. Verified against the real linux-x64 baseline: truncating one report from 10 benchmarks to 2 leaves the file count at 7 and yields "Compared 25 of 28, Missing: 3" with 0 regressions -> exit 2, where the old code exited 0. To test that without dragging BenchmarkDotNet into the unit-test project, the pure comparison logic moved to BaselineComparison.cs (free of BenchmarkDotNet types) and NetPdf.UnitTests LINKS that single source file. 7 new tests cover match / regression / missing-key / incomplete-outranks-regression / newly-added-benchmark / same-file-count-but-fewer-entries, plus parameterised keying. [P2] capture mode could seed an incomplete baseline, which would then be trusted forever — every later run would "pass" against a baseline missing the benchmarks nobody measures any more. The run + validation (exit code, boilerplate build failure, results produced) is now a shared run_suite_validated helper used by BOTH capture and the gate. [P3] The sentinel comment said $(OutDir) while the code correctly uses $(TargetDir) and the inner comment warns against $(OutDir). Corrected. Verified: build 0 errors/0 warnings; UnitTests 8614 passed / 3 skipped; RenderingCorpus 41; RealDocuments 105; bash -n clean; git diff --check clean. Compare-mode exit codes re-checked end-to-end: truncated -> 2, identical -> 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot review on PR #356: the block still described BDN writing the generated job project under $(OutDir) while the target keys off $(TargetDir), which made it hard to see that the sentinels really do land on the generated project's upward-search path. Both now say $(TargetDir), with a note that it is simply the absolute form of $(OutDir) and the only form safe to use in the target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
raroche
added a commit
that referenced
this pull request
Jul 27, 2026
…deferral The `ci-nonblocking-platform-native-deps` deferral's own removal condition — "both non-blocking legs pass the Test step in CI (green)" — is now met, and its premise is obsolete. It was written when libSkiaSharp genuinely would not load on those images (arm64: `undefined symbol: uuid_generate_random`, then FT_Get_BDF_Property; alpine: musl native-load failure). That is fixed: the fontconfig-hardening step plus the apk prerequisites, together with the SkiaSharp 4.150.1 bump (#355), load the native cleanly. The evidence is that both legs were running 8604 of 8609 tests, and their only failures were two font-dependent ASSERTIONS — AutoHeightFlexTimelineFooterTests, fixed in #356 — never a native-load error. Both have since run the full suite green. - linux-arm64 loses `nonblocking: true`; the alpine job loses `continue-on-error: true`. Both are renamed accordingly (the ", non-blocking" suffix is part of the check name). Neither name is in branch protection's required contexts today, so no existing required check breaks. - macos-x64 stays non-blocking and keeps its own deferral: hosted Intel-mac runner availability, which nothing in this repo can fix. - Deferral removed from docs/deferrals.md + DeferralsParityTests (the documented convention for picking one up), and the macos-x64 entry's cross-reference rewritten so it no longer points at a deleted anchor. NOTE for the maintainer: making these legs enforcing turns the WORKFLOW red on failure, but it does not block a merge until the two renamed contexts are added to branch protection's required checks — worth doing alongside `benchmark gate (linux-x64)`, which is still not required. (PROGRESS.md was also rolled, but it is gitignored — untracked deliberately in #284 as an internal doc — so it stays local and out of this PR.) Verified: build 0 errors; UnitTests 8614 passed / 3 skipped (incl. DeferralsParityTests); git diff --check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
raroche
added a commit
that referenced
this pull request
Jul 27, 2026
…deps deferral (#357) * ci: promote linux-arm64 + alpine to enforcing; close the native-deps deferral The `ci-nonblocking-platform-native-deps` deferral's own removal condition — "both non-blocking legs pass the Test step in CI (green)" — is now met, and its premise is obsolete. It was written when libSkiaSharp genuinely would not load on those images (arm64: `undefined symbol: uuid_generate_random`, then FT_Get_BDF_Property; alpine: musl native-load failure). That is fixed: the fontconfig-hardening step plus the apk prerequisites, together with the SkiaSharp 4.150.1 bump (#355), load the native cleanly. The evidence is that both legs were running 8604 of 8609 tests, and their only failures were two font-dependent ASSERTIONS — AutoHeightFlexTimelineFooterTests, fixed in #356 — never a native-load error. Both have since run the full suite green. - linux-arm64 loses `nonblocking: true`; the alpine job loses `continue-on-error: true`. Both are renamed accordingly (the ", non-blocking" suffix is part of the check name). Neither name is in branch protection's required contexts today, so no existing required check breaks. - macos-x64 stays non-blocking and keeps its own deferral: hosted Intel-mac runner availability, which nothing in this repo can fix. - Deferral removed from docs/deferrals.md + DeferralsParityTests (the documented convention for picking one up), and the macos-x64 entry's cross-reference rewritten so it no longer points at a deleted anchor. NOTE for the maintainer: making these legs enforcing turns the WORKFLOW red on failure, but it does not block a merge until the two renamed contexts are added to branch protection's required checks — worth doing alongside `benchmark gate (linux-x64)`, which is still not required. (PROGRESS.md was also rolled, but it is gitignored — untracked deliberately in #284 as an internal doc — so it stays local and out of this PR.) Verified: build 0 errors; UnitTests 8614 passed / 3 skipped (incl. DeferralsParityTests); git diff --check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(ci): track the un-applied branch-protection contexts as a deferral PR #357 review [P2] — valid, and it names a gap this PR itself created. Promoting linux-arm64 + alpine made them WORKFLOW-enforcing (a failure turns the run red) but NOT merge-enforcing: `main`'s branch protection still requires only build+test (linux-x64) / (windows-x64) / (macos-arm64), security-gate and dependency-scan — verified against the live API. Deleting the old `ci-nonblocking-platform-native-deps` entry removed the only thing tracking the remaining step, so it could quietly be forgotten. The reviewer offered either applying the setting or keeping a tracked follow-up. Applying it is a repository SETTING that needs admin rights and cannot land through a PR, so this takes the tracked-follow-up option and records it properly: - New deferral `ci-branch-protection-required-contexts` (P2) states the current required list, the three checks missing from it, and the exact `gh api` command — including the trap that the API REPLACES the context list, so all eight must be sent, and that the two renamed checks must not be re-added under their old ", non-blocking" names. - Rated P2, not P3, because the third missing context is `benchmark gate (linux-x64)`: until it is required, a genuine perf regression reports red and still merges, which silently weakens the CLAUDE.md performance contract. - ci.yml now carries a CAVEAT next to "Enforcing matrix" pointing at the deferral, so the distinction is visible where the enforcement is declared. - Registered in DeferralsParityTests (IDs + P2 priority), which is what makes the entry impossible to drop silently. Verified: build 0 errors; UnitTests 8614 passed / 3 skipped; branch-protection contexts re-read from the API to confirm the entry's factual claims. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the three CI legs that have been red on
mainsince at least the 1.0.2 release. Neither failure was what its label suggested, and one of them was silently voiding a guarantee we advertise.No
src/changes, so no version bump: this is CI/test infrastructure only, and 1.1.0 is not yet tagged.1.
benchmark gate (linux-x64)— the gate had stopped measuringNot a perf regression. It failed before taking a single measurement, so the CI performance gate promised in
CLAUDE.md(3-page invoice ≤ 200 ms p50, etc.) was effectively vacuous — including the "under 200 ms" figure used in public launch material.BenchmarkDotNet writes a per-job project under the benchmark project's output dir and restores it. That path is inside the repo, so MSBuild/NuGet walk up from it and pick up the root
Directory.Build.props+Directory.Packages.props, dragging Central Package Management into the generated project. On the CI runner, restore then tried to resolve the generated assembly name — a GUID — as a NuGet package:Only 3 of 33 benchmarks survived. The gate compared that partial result set and died inside
System.Text.Json(target element has type 'Null', exit 134) — a crash that read as an infra flake and hid that nothing was being measured.Two changes:
NetPdf.Benchmarks.csprojwrites inertDirectory.Build.props/.targetsplus a CPM-offDirectory.Packages.propsinto$(TargetDir), so the generated project's upward search stops below the repo root and it restores standalone. Sentinels live inbin/(gitignored), rewritten each build. Two traps that bit during development are pinned in comments:$(OutDir)is relative (writes land beside the.csproj, turning CPM off for the project itself → NU1015), and<TargetFrameworks>runs an outer build with an empty$(TargetDir)(same result).scripts/benchmark-gate.shno longer lets a partial run reach--compare. BDN exits 0 even when a generated project fails to build, so the script now exits 2 with a clear message when the run errors, when BDN reports a boilerplate build failure, or when fewer result files are produced than the baseline has. A silently incomplete suite can no longer masquerade as a measured one.2.
alpine-musl-x64+linux-arm64— a false positive, not native breakageBoth legs failed the same 2 tests while 8604 of 8609 passed, so this was never the SkiaSharp/native-asset problem the "non-blocking" labels imply.
AutoHeightFlexTimelineFooterTestsidentifies the footer note as "the run with the most glyphs", which silently assumes the note renders as one line. CI provisions fonts asymmetrically by design — linux-arm64 installs onlyfonts-dejavu-core, alpine onlyttf-dejavu, while the enforcing linux-x64 runner has the full Ubuntu set. Under DejaVu metrics the note wrapped, so the detected "note" was only its first line and its own tail —"departure zulu", exactly the 14 glyphs the failure reported — became a run below it.Wrapping a long line is correct engine behaviour; the test's single-line premise was the bug. The note is now
white-space: nowrapwith the font pinned like the sibling rendering tests. The invariant under test (the note must be last, never overlapped by timeline content) is still fully asserted.Verification
Both fixes were verified by reproducing the failures, not by assuming:
nowrap+ wider font) yielding the exact CI signature —a run (glyphs=14) ... sits below the footer note— and the same document passes with the fix.Local suites: build 0 errors · UnitTests 8607 passed / 3 skipped · RenderingCorpus 41 · RealDocuments 105.
Follow-up worth considering
Once this lands and the gate is green,
benchmark gateshould probably become a required check — as it stands, a genuine perf regression still wouldn't block a merge.