ci coverage: fail PRs on per-package coverage decrease or overall floor breach#8092
Open
hemarina wants to merge 8 commits intoAzure:mainfrom
Open
ci coverage: fail PRs on per-package coverage decrease or overall floor breach#8092hemarina wants to merge 8 commits intoAzure:mainfrom
hemarina wants to merge 8 commits intoAzure:mainfrom
Conversation
hemarina
requested review from
JeffreyCA,
danieljurek,
jongio,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the azd code coverage enforcement model by shifting PR-time enforcement to a two-gate diff script (Get-CoverageDiff.ps1) and wiring it into the Azure Pipelines coverage upload stage, with supporting mage targets, docs updates, and a new Pester test suite.
Changes:
- Reworks
Get-CoverageDiff.ps1to emit a plain-text CI log report and optionally fail PR builds on (1) per-package regression beyond a tolerance and/or (2) overall coverage falling below an absolute floor. - Updates the CodeCoverage_Upload pipeline stage to merge coverage via
mage coverage:reportand run the PR diff gate against a downloaded baseline build. - Adds automated Pester tests for the coverage diff script and runs them in GitHub Actions when the script changes.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| eng/scripts/Test-CodeCoverageThreshold.ps1 | Updates script header docs to reflect that CI enforcement moved elsewhere. |
| eng/scripts/Get-CoverageDiff.Tests.ps1 | Adds Pester coverage-diff tests (new file). |
| eng/scripts/Get-CoverageDiff.ps1 | Rewrites diff logic/reporting and adds two-gate PR enforcement behavior. |
| eng/pipelines/templates/stages/code-coverage-upload.yml | Switches coverage merge plumbing to mage coverage:report and wires PR diff gate into the stage. |
| eng/pipelines/release-cli.yml | Removes the old minimum-coverage threshold parameter wiring. |
| cli/azd/magefile.go | Updates coverage mage targets, adds coverage:report, and adds changed-file resolution logic. |
| cli/azd/docs/code-coverage-guide.md | Documents the new PR gate and related mage targets/env vars. |
| cli/azd/AGENTS.md | Adds contributor guidance for mage coverage:pr. |
| cli/azd/.vscode/cspell.yaml | Adds new terminology to cspell allowlist. |
| .github/workflows/cli-ci.yml | Runs Pester tests for the coverage diff script when it changes. |
wbreza
reviewed
May 8, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
🔍 Code Review — PR #8092
ci coverage: fail PRs when touched files drop below per-file coverage floor
General note: PRs should always include a description of their changes regardless of how trivial they may seem. A good description helps reviewers understand intent, provides context for future readers, and satisfies governance checks.
P0 — Blocks Merge
1. Empty PR description + missing issue link
- PR body is empty. No
Fixes #7799despite the branch name referencing it. pr-governanceCI check FAILS because of this — mechanical merge blocker.- Fix: Add PR description with
Fixes #7799and a summary of the two-gate coverage model.
P1 — Should Fix Before Merge
2. PR title says "per-file" but implementation is "per-package"
- Title: "fail PRs when touched files drop below per-file coverage floor"
- Reality: Gates operate at Go package granularity, not per-file.
- Fix: Update title to reflect per-package granularity (e.g., "ci coverage: add two-gate PR coverage check (per-package regression + absolute floor)").
3. AGENTS.md says 65% floor but code uses 69%
- AGENTS.md: "overall coverage falls below 65%"
- Actual default everywhere else (script, pipeline, guide): 69%
- Fix: Update AGENTS.md to say 69%.
4. GitHub Actions coverage-script-tests job missing permissions: block
- Job inherits
pull-requests: writefrom workflow-level but only needscontents: read. - Violates repo's least-privilege convention.
- Fix: Add
permissions: { contents: read }to the job.
5. -SkipPublisherCheck on Pester install (supply chain)
Install-Module -Name Pester ... -SkipPublisherCheckdisables code signing verification.- Fix: Remove
-SkipPublisherCheckif possible; at minimum pin to-RequiredVersion 5.x.x.
6. go install mage@latest — unpinned dependency (supply chain)
- CI installs
mage@latestwhich is non-deterministic across builds. - Fix: Pin to a specific version (e.g.,
mage@v1.15.0) or use the version from go.mod.
7. os.RemoveAll errors silently ignored in magefile.go
- Lines 536-537:
_ = os.RemoveAll(tmpUnit)— if removal fails (Windows file locking), stale covdata contaminates the merge. - Fix: Check and return the error.
P2 — Consider
8. No Go unit tests for new magefile functions — resolveChangedFilesForDiff(), Coverage.PR(), Coverage.Report() have zero test coverage. Pester suite covers downstream behavior, so lower urgency.
9. Parameter naming mismatch — Docs use PackageCoverageDecreaseTolerance but code uses MaxPackageDecrease. Align terminology for searchability.
10. Artifact validation only checks directory existence — Test-Path returns true for empty dirs. A failed platform leg with an empty dir would pass validation silently.
11. Exit code documentation inconsistent — Some docs say "exit non-zero", code uses exit 2 specifically. Standardize across docs and mage comments.
✅ Positives
- Well-designed two-gate model — per-package regression + absolute floor is a sound approach
- Excellent Pester test suite — 857 lines covering boundary conditions, locale-invariant formatting, gate combinations
- Good fail-closed design — git failures fall back to full package scan, never silently skip gates
mage coverage:pr— great DX for contributors to preview the gate locally- Consolidated coverage reporting — single source of truth via
mage coverage:report
vhvb1989
approved these changes
May 8, 2026
hemarina
changed the title
jongio
reviewed
May 9, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Solid rework - the two-gate model (per-package decrease + overall floor) with a single source of truth in Get-CoverageDiff.ps1 is a clean design. wbreza's review covers the major items (AGENTS.md 65/69 mismatch, -SkipPublisherCheck, silent os.RemoveAll errors, missing Go tests); one additional item below.
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
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.
Closes #7799
Summary
Replaces the legacy single-threshold gate (
Test-CodeCoverageThreshold.ps1) with a fail-loud, two-gate PR coverage check so coverage regressions block merges instead of silently slipping in.What changed
Two-gate enforcement (PR builds only)
A PR fails (
exit 2) if either gate breaches:-MaxPackageDecrease)0.5pp-MinOverallCoverage)69.0%Both are configurable via pipeline parameters; either gate can be disabled independently with the
-1sentinel. Any non-zero exit other than2indicates a script/infra error, not a gate breach.New:
eng/scripts/Get-CoverageDiff.ps1Replaces
Test-CodeCoverageThreshold.ps1for CI use. Features:git diff --diff-filter=AMRD origin/<targetBranch>...HEAD).,decimal leakage on non en-US agents).##vso[task.logissue]annotation so the skip is visible).0clean,2gate breach (with-FailOnGate), other non-zero = script error.New: tests + CI wiring
eng/scripts/Get-CoverageDiff.Tests.ps1— 37 Pester tests covering per-package scoping, changed-file input handling, profile parsing edge cases, both gates and their boundaries, locale invariance,-1disable sentinels, deletion-only PRs,-TopN/-MinDeltafilters, input validation.cli/azd/magefile_test.go— 11 Go tests coveringresolveCoverageFile,resolveBaselineFile, andresolveChangedFilesForDiff(incl. real-git happy path with bare-upstream simulation, on-main, detached-HEAD, strict-mode error, and a concurrency-regression assertion that two resolves on the same repo produce distinct temp paths).cli-ci.ymlas new jobs:coverage-script-tests(Pester, pinned to v5.7.1,permissions: contents: read) andmagefile-tests(Go,permissions: contents: read).Pipeline (
eng/pipelines/templates/stages/code-coverage-upload.yml)cover-unit+cover-intartifacts.magepinned tov1.17.2(must stay in sync withcli/azd/go.mod; comment in the pipeline reminds future maintainers).Mage targets (
cli/azd/magefile.go)New
Coveragenamespace with local-developer ergonomics:mage coverage:diff— preview the gate locally againstorigin/main. Advisory by default; setCOVERAGE_FAIL_ON_DECREASE=1to make it fail-loud (exit 2).mage coverage:pr— fail-loud preview that mirrors the CI gate exactly.os.CreateTempfor the changed-files list so two concurrent local runs can't clobber each other's input and silently produce wrong gate results.os.RemoveAllfailures).Docs (
cli/azd/docs/code-coverage-guide.md)Rewritten contributor guide: pipeline flow, the two-gate model, mage targets, env vars (with precise
exit 2semantics), troubleshooting runbook, branch-protection setup (withgh apiadmin commands), and a worked example showing a 1.0pp drop tripping the per-package gate.Cleanup
Test-CodeCoverageThreshold.ps1doc-comment refreshed (removed stalerelease-cli.ymlreference; clarified it is now local-only viaGet-LocalCoverageReport.ps1).MinimumCoveragePercent: 65parameter removed fromrelease-cli.yml.GH_TOKENenv var from the PR coverage diff step (noghinvocations in that step).-SkipPublisherCheck, pinned to-RequiredVersion 5.7.1.cspell.yamloverrides extended for new domain words (textfmt,logissue,runbook,covcounters,covmeta,AMRD).Validation
coverage-script-testsGHA job: 37/37 Pester tests passingmagefile-testsGHA job: 11/11 Go tests passing (incl. concurrency regression check)cspell lintclean across all touched filesgo vet -tags mage .cleancode-coverage-upload.yml,release-cli.yml,cli-ci.yml)Effective behavior after merge
exit 2) on per-package decrease > 0.5pp or overall < 69%mage coverage:diff)COVERAGE_FAIL_ON_DECREASE=1mage coverage:pr)Follow-ups (out of scope)
coverage-script-tests,magefile-tests, andazure-dev - cli (CodeCoverage_Upload Upload)checks as required in branch protection (admin-only;gh apicommand in the runbook).