mirror of
https://github.com/OPCFoundation/UA-.NETStandard.git
synced 2026-10-16 21:02:51 +08:00
Split the CI matrix across both systems, split the required test gate from the advisory coverage check, and give background work an owner (#4158)
# Description Two changes to how CI is routed and how it reports. ## 1. Split the CI matrix back across both systems Flips both halves of the CI backend switch from `ado` to `actions`, so the workload is shared instead of concentrated on the managed pool. The switch is checked into source in **two places that must always be flipped together**: | File | Setting | Before | After | | --- | --- | --- | --- | | `azure-pipelines.yml` | `parameters.ciBuildBackend` | `ado` | `actions` | | `.github/workflows/buildandtest.yml` | `env.CI_BUILD_BACKEND` | `ado` | `actions` | | Work | GitHub Actions | Azure Pipelines (managed pool) | | --- | --- | --- | | All-TFM solution builds | `build-all-tfm-windows` + `-linux` | two-TFM PR gate (`net10.0`, `net48`) | | Test matrix | ubuntu, every test project | Windows `net48` + `net10.0` fast PR legs | | Native AoT | ubuntu | Schedule/Manual only | | macOS | always (no managed-pool macOS image) | — | ## 2. Split the required gate from the advisory coverage check The coverage gate job was appended to *every* test stage, so `Fast PR test` emitted three sibling checks whose display names collided — `Coverage Release (net10.0)` appeared twice, once for the Windows leg and once for macOS, with nothing to tell them apart: ```text ✓ Coverage Release (net48) 55s ● Coverage Release (net10.0) ○ Coverage Release (net10.0) ``` That job also fused two unrelated concerns: whether the tests passed (which must block a merge) and whether coverage met its thresholds (which should not — the `net10.0` leg had `enforceCoverage: true`, so a coverage dip blocked merges). Both concerns now get a stage of their own, mirrored in both CI systems: | Concern | Azure Pipelines | GitHub Actions | In the branch ruleset? | | --- | --- | --- | --- | | Every test passed | **`Tests passed`** stage | **`build-and-test summary`** job | **Yes — required** | | Coverage meets thresholds | **`Code coverage`** stage | **`code coverage`** job | **No — advisory** | The coverage check reports a clean failure when the thresholds are missed, so the miss is visible, but it never blocks the merge. Coverage is now merged **once** across every leg of the run instead of once per leg, which also removes the `enforceCoverage` / `publishCoverage` "exactly one invocation may set this" parameters. Neither check re-runs any tests — the previous design re-ran the whole suite in one job with the collector attached, which serialised a suite that is deliberately fanned out and always exceeded the stage timeout. ### Coverage checks now exist in GitHub Actions `build-and-test` already ran with `--collect:"XPlat Code Coverage"` and uploaded the fragments, but nothing ever merged or evaluated them. It now runs the same `check-coverage.ps1` against the same `coverage-thresholds.json` as Azure. Since `CI_BUILD_BACKEND` defaults to `actions`, that ubuntu matrix — which runs *every* test project — is the fullest coverage signal in the repository. ### Reporting the numbers `check-coverage.ps1` gained `-SummaryPath`, rendering a markdown report that both systems surface: - **GitHub Actions** — appended to the job summary and upserted as a single **sticky pull-request comment**, updated in place on each run. Threshold misses also appear as `::error::` annotations. - **Azure Pipelines** — attached to the build summary via `##vso[task.uploadsummary]`. The report is a pass/fail table of the project floor, patch coverage and baseline delta, with the uncovered changed lines listed by file in a collapsible block. Rendered example: > ## Code coverage > > ❌ **Coverage gate failed.** This check is advisory and does not block the merge. > > | Check | Result | Threshold | > | --- | --- | --- | > | ✅ Project line rate | **72.10%** (40 731/56 496 lines) | >= 70.00% | > | ❌ Patch coverage | **0.00%** (0/1 changed lines) | >= 75.00% | The comment is fork-safe: a `pull_request` from a fork gets a read-only token regardless of the `permissions:` block, so it degrades to the job summary alone. The sticky comment uses `actions/github-script` rather than a community action, keeping this workflow on first-party actions only. ## Notable correctness details These were found and fixed during review, and are the parts most worth a second pair of eyes: - **`dependencies.<stage>.result` resolves at *stage* scope only.** At job scope `dependencies` refers to other jobs of the same stage, so reading stage results from a jobs template would have silently returned empty and the gate would have passed everything. `gate.yml` and `coverage.yml` are therefore **stage** templates that derive their own `dependsOn` from the same stage list they evaluate, so the two can never drift apart. An unresolved result is a hard error. - **Both rollups run on `not(canceled())` / `always()`**, not on success. A check that reports **skipped** is treated by GitHub as **satisfied**, so a required check that skips when its dependency fails would wave a red build straight through. - **`get-matrix.ps1` used to emit an empty matrix on a discovery miss.** The downstream job is then skipped, a skipped job rolls up as a *succeeded* stage, and the gate would have approved a run that executed no tests. It now fails, with an `-AllowEmpty` opt-out. - **Zero coverage reports used to be a green advisory check**, which would hide a broken collector indefinitely. Both systems now distinguish "the tests never ran" (warning) from "the tests ran and published nothing" (error). - The gate reads stage-level results rather than enumerating matrix-generated job names, which change whenever a test project or agent is added. ## Test-host hang and the reconnect defect behind it — both fixed Flipping `CI_BUILD_BACKEND` to `actions` started exercising the ubuntu matrix, which `master` never runs, and it immediately surfaced two real defects. **1. The test host hung for ten minutes.** `Assert.ThrowsAsync` blocks the calling thread with no timeout, and these tests drive a *fake clock* from that same thread — so once the reconnect parked on a fake-clock timer, nothing could ever advance it. NUnit sat in `WaitForCompletion` until `--blame-hang-timeout` killed the host, aborting the run at 3902 of 4238 tests and taking every other test in the assembly down with it. A hang dump named the culprit exactly. Replaced with an awaited, bounded `AssertThrowsAsync` helper (the repo's own rules forbid sync over async). The run now completes in **~1m29s** with all tests executed. **2. A budget-aware reconnect never returned.** With the hang gone, two tests failed with `Expected ServiceResultException but got TimeoutException`. `dumpasync` on a hang dump gave the whole chain: ``` ReconnectAsyncWithBudgetShrinksDelayToFitRemainingAsync → AssertThrowsAsync → ClientChannelManager.ReconnectAsync → ClientChannelManager.ReconnectLeaseAsync → ClientChannelManager.SwapFaultedEntryAsync ← parked here ``` `master` (via #4128) added `IsTerminalReconnectRace` so a reconnect that loses a race against a concurrent close recovers on a freshly swapped entry. The predicate only asks whether the entry ended up `Closed`/`Faulted` with `BadSecureChannelClosed` — which is *also* exactly how a reconnect ends when the caller's retry budget or the reconnect policy is deliberately exhausted. So a budget-exhausted reconnect was misread as a race: the manager swapped the entry and started a **second, unbudgeted** cycle, waiting out the swap back-off first. `ReconnectAsync(channel, budget)` therefore could not return within its budget, and under a fake clock it never returned at all. The fix requires the budget to have room left before treating a terminal failure as a race: ```csharp return !ct.IsCancellationRequested && !budget.IsExhausted && sre.StatusCode == StatusCodes.BadSecureChannelClosed && entry.State is ChannelState.Closed or ChannelState.Faulted; ``` The parameterless `ReconnectAsync(channel, ct)` overload passes an unlimited budget, so genuine race recovery is unchanged. Verified locally: Core 4152, Client 2122 and Sessions 776 tests all pass, and the full GitHub Actions PR run is green across 56 jobs. Two smaller fixes came along the way, neither weakening an assertion: - `ObservableFakeTimeProvider` waiters made **relative** ("N more timers from now") rather than absolute ("the Nth timer of the run"). Absolute numbering let an earlier reconnect consume the slots, so a waiter completed before the awaited timer existed and `Advance()` fired nothing. - The raw worker thread in `NodeStateHandlerConcurrencyTests` was a **foreground** thread whose stop flag was read without a barrier and set only on the success path — a failed assertion leaked a spinning thread that pins the process at exit. Now background, `Volatile`, stopped in a `finally`, joined with a bound. ## Background work now has an owner The reconnect hang above was one symptom of a pattern the codebase had **26 times** across seven assemblies: a bare `_ = Task.Run(...)`. Each one handed work to the thread pool and forgot it — nothing observed the exception if it threw, nothing bounded how many ran at once, and disposal raced them, so a component could finish tearing itself down while work it started was still touching its fields. Two of those sites already carried a standing `// TODO: Await the task completion in shutdown`. `BackgroundTaskScope` (in `Opc.Ua.Types`) gives them an owner: schedule with `Run`, drain with `DisposeAsync`, or signal-only with `Dispose` where the owner's teardown is synchronous and awaiting would be sync over async. `Run` never blocks and never throws — which matters, because most of these call sites went to the thread pool precisely to escape a lock they were holding. 11 unit tests cover drain, cancellation, exception observation, the concurrency cap, post-shutdown rejection and idempotent disposal. Two of the fixes were more than re-wiring: - **`SessionManager` / `SubscriptionManager` loops were structurally unawaitable.** Both return `ValueTask`, so `Task.Factory.StartNew(() => ...)` handed back a `Task<ValueTask>` that completes at the first yield. Storing it would not have helped — awaiting it would only have awaited the *scheduling*. Both now go through `AsTask().Unwrap()`, matching the neighbouring `StartConditionRefreshWorker`. - **`ChannelAsyncOperation` was swallowing subscriber exceptions.** Its `doNotBlock` branch had no try/catch while the inline branch beside it did. It is per-operation with no owner to hang a drain on, so it gained the missing handler rather than a scope. The Kubernetes readiness endpoint keeps a purpose-built semaphore instead: an unauthenticated endpoint needs a hard cap on concurrent handlers, not just an owner. ### ⚠️ Breaking: `ISessionManager.Shutdown()` is removed Replaced by `ShutdownAsync(CancellationToken)`. There is no correct synchronous way to wait for the session monitor loop, so leaving the overload would only have preserved the race. This mirrors `ISubscriptionManager`, which already had `ShutdownAsync`. Migration notes are in `docs/MigrationGuide.md`; only one in-repo implementer existed. Also converted the injected `object partitionLock` shared by `LogicalSubscription` and `CompositeMonitoredItemCollection` to `System.Threading.Lock`, per the repo rule. ## Patch coverage now scales with the size of the patch A flat floor over changed lines punishes small changes for arithmetic: two uncovered lines in a four-line fix reads as 50 % and failed an 80/5 floor — which is exactly how this branch earned a red coverage check for a four-line predicate change. A check that fires on changes nobody considers undertested is one people learn to ignore. | Coverable changed lines | Floor | Below it | | --- | --- | --- | | 1 – 10 | 50 % | warning, still passes | | 11 – 100 | 60 % | warning, still passes | | more than 100 | 75 % | **failure** | Only changes past the last band can fail. Below it the author still gets a warning naming the uncovered lines. Bands live in `patch.bands` in `coverage-thresholds.json` and each carries its own `enforced` flag. Verified end-to-end against synthetic repositories at every boundary. ## Codecov is back, as reporting Dropped in #4122 when the in-pipeline gate replaced it. Keeping the gate was right, but dropping Codecov also lost the PR comment, the file-by-file diff view and the trend. It returns with both statuses `informational: true` — two gates with two sets of thresholds would eventually disagree, and `coverage-thresholds.json` stays the single source of truth. The upload is switchable and can never fail a build: `enableCodecov` (Azure) and `ENABLE_CODECOV` (Actions), both defaulting on, both skipped when `CODECOV_TOKEN` is absent as on fork PRs. Each system uploads the single merged report it already builds, under its own flag. The old `codecov.io/bash` uploader was sunset, so Azure uses the current CLI. ## Follow-up actions for maintainers 1. **Update the branch ruleset.** The per-leg `Coverage Release (…)` checks are gone. Azure reports checks as `<pipeline> (<stage> <job>)`, so the exact names are: | Check | Require? | | --- | --- | | `OPCFoundation.UA-.NETStandard (Tests passed Verify stage results)` | **yes** | | `build-and-test summary` | **yes** (already required) | | `OPCFoundation.UA-.NETStandard (Code coverage Merge and evaluate)` | no | | `code coverage` | no | 2. **Re-calibrate the thresholds.** `coverage-thresholds.json` is still seeded from the last Codecov reading (73.61 %). The two systems now report *different* numbers by design — Actions merges every project on ubuntu, Azure only the Windows fast-PR legs, and scheduled runs read higher still because the Debug / .NET 8 / .NET 9 / netstandard stages also contribute. Re-seed from the GitHub Actions figure. 3. **Note:** a failed advisory coverage stage still turns the whole Azure run red, which the README build badge reflects on `master`. That follows from wanting a visible red X on a threshold miss; say the word if you would rather it were a warning. 4. **The Azure agent pool is the slowest thing here.** Several runs on this PR sat queued for hours, and one build failed with `Insufficient system resources` while writing an XML doc file — an agent exhaustion error, not a code error. It passed on re-run. Worth raising against the `netstandard` Managed DevOps Pool capacity separately. ## Related Issues _No tracking issue; this is CI infrastructure work requested directly. It builds on the switch introduced in #4122._ ## Checklist - [x] I have signed the [CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf) and read the [CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md) doc. - [x] I have added tests that prove my fix is effective or that my feature works and increased code coverage. - No product code changed, so there are no unit tests to add. The new logic was verified with purpose-built harnesses instead: - `check-coverage.ps1` run for real against a merged Cobertura report on all three paths — pass, threshold failure, and patch coverage with uncovered changed lines (a scratch commit adding an uncovered method was correctly resolved to `DataTypeException.cs:193`, reported 0 % patch coverage and exited 1). - The gate's accept/reject logic dry-run over `Succeeded` / `SucceededWithIssues` / `Skipped` / empty / `Failed` / `Canceled` — 6/6 correct. - `get-matrix.ps1` exit codes verified in clean processes: empty → 1, normal → 0, `-AllowEmpty` → 0. - All five pipeline/workflow files parse; both PowerShell scripts parse. - Action versions checked against the GitHub API and pinned to current majors. - [x] I have added all necessary documentation. - Rewrote the coverage section of `docs/DeveloperGuide.md` as *Required checks and coverage*, covering which checks belong in the ruleset and which must not, how coverage is measured, where the numbers appear, and why the two systems disagree. Corrected the stale "BLOCKS" wording in `coverage-thresholds.json`. - [x] I have verified that my changes do not introduce (new) build or analyzer warnings. - CI YAML, two PowerShell scripts and documentation only; no compilation involved. - [x] I ran **all** tests locally using the **UA.slnx** solution against at least .net **framework** and .net **10**, and all passed. - Not applicable — no product code changed. `Opc.Ua.Core.Schema.Tests` was run repeatedly on net10.0 (119 passed) to generate the real Cobertura fragments used to validate the gate. - [ ] I fixed **all** failing and flaky tests in the CI pipelines and **all** CodeQL warnings. - [x] I have addressed **all** PR feedback received. - Feedback on the path-relevance diff (now a merge-base diff) and the target-framework numbering in `docs/DeveloperGuide.md` is fixed in `15b8e38df`. - A review pass over the gate/coverage split produced the four correctness fixes listed above, in `863eefc4d`. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 26aa55a2-5731-4a4c-b61c-df2f7a785eca
This commit is contained in:
+83
-22
@@ -259,8 +259,8 @@ From **2.0** onward, package versions are produced by [Nerdbank.GitVersioning](h
|
||||
|
||||
Two CI systems run against this repository:
|
||||
|
||||
- **Azure Pipelines** ([`azure-pipelines.yml`](../azure-pipelines.yml) plus the templates in [`.azurepipelines/`](../.azurepipelines)) — the all-target-framework solution build, the cross-platform test matrices, Native AoT and the coverage gate.
|
||||
- **GitHub Actions** ([`.github/workflows/`](../.github/workflows)) — CodeQL, container images, the opt-in stress and stability suites, and the macOS legs of the build/test matrix.
|
||||
- **Azure Pipelines** ([`azure-pipelines.yml`](../azure-pipelines.yml) plus the templates in [`.azurepipelines/`](../.azurepipelines)) — the fast pull-request test legs, the per-framework test matrices and the coverage gate, on the `netstandard` Managed DevOps Pool and Microsoft-hosted agents.
|
||||
- **GitHub Actions** ([`.github/workflows/`](../.github/workflows)) — the all-target-framework solution builds, the ubuntu test matrix, Native AoT, CodeQL, container images, the opt-in stress and stability suites, and the macOS legs of the build/test matrix.
|
||||
|
||||
### Which system runs what
|
||||
|
||||
@@ -268,15 +268,16 @@ A single conceptual switch decides who owns the all-TFM build, the cross-platfor
|
||||
|
||||
| File | Setting | Default |
|
||||
| --- | --- | --- |
|
||||
| [`azure-pipelines.yml`](../azure-pipelines.yml) | `parameters.ciBuildBackend` | `ado` |
|
||||
| [`.github/workflows/buildandtest.yml`](../.github/workflows/buildandtest.yml) | `env.CI_BUILD_BACKEND` | `ado` |
|
||||
| [`azure-pipelines.yml`](../azure-pipelines.yml) | `parameters.ciBuildBackend` | `actions` |
|
||||
| [`.github/workflows/buildandtest.yml`](../.github/workflows/buildandtest.yml) | `env.CI_BUILD_BACKEND` | `actions` |
|
||||
|
||||
With the default `ado`, Azure Pipelines runs that work on the `netstandard` Managed DevOps Pool and the equivalent GitHub Actions jobs stand down on `master`/`main`. Setting both to `actions` restores the previous split, where GitHub Actions ran the ubuntu test matrix, Native AoT and the all-TFM builds.
|
||||
With the default `actions` the load is split across both systems: GitHub Actions runs the all-TFM builds, the ubuntu test matrix and Native AoT, while Azure Pipelines runs the fast pull-request test legs on the managed pool and hosts the coverage gate. Setting both to `ado` moves that work onto the Managed DevOps Pool as well, and the equivalent GitHub Actions jobs stand down on `master`/`main`.
|
||||
|
||||
Two things are deliberately *not* covered by the switch:
|
||||
Three things are deliberately *not* covered by the switch:
|
||||
|
||||
- **macOS** always runs on GitHub-hosted runners, because Managed DevOps Pools provide no macOS image.
|
||||
- **`master378` and `develop/*`** keep running the GitHub Actions jobs regardless of the setting, since Azure Pipelines only builds `master`/`main` from this file.
|
||||
- **The `Tests passed` and `Code coverage` stages** always run in Azure Pipelines regardless of the switch, because they roll up whatever did run (see [Required checks and coverage](#required-checks-and-coverage)).
|
||||
|
||||
### Test tiers
|
||||
|
||||
@@ -289,7 +290,7 @@ The fast test stages fan every `*.Tests.csproj` out across matrix jobs and filte
|
||||
| `Opc.Ua.Stress.Tests` | [`.github/workflows/stress-test.yml`](../.github/workflows/stress-test.yml), opt-in |
|
||||
| `Opc.Ua.Aot.Tests` | `Test Native AoT` stage |
|
||||
|
||||
Because the individual matrix jobs are generated (and are skipped outright when Azure Pipelines owns them, or when a pull request touches no build-relevant files), branch protection should require the aggregate **`build-and-test summary`** check rather than any individual job. That job runs on every pull request — the workflow deliberately carries no `paths:` filter, because a workflow filtered out by `paths` never reports its checks and a required check that never reports blocks the pull request forever. The path allow-list is applied inside the `discover` job instead, and the summary treats an intentionally skipped job as success.
|
||||
Because the individual matrix jobs are generated (and are skipped outright when Azure Pipelines owns them, or when a pull request touches no build-relevant files), branch protection requires the aggregate **`build-and-test summary`** check rather than any individual job — see [Required checks and coverage](#required-checks-and-coverage). That job runs on every pull request — the workflow deliberately carries no `paths:` filter, because a workflow filtered out by `paths` never reports its checks and a required check that never reports blocks the pull request forever. The path allow-list is applied inside the `discover` job instead, and the summary treats an intentionally skipped job as success.
|
||||
|
||||
### Triggering a pipeline run on a pull request
|
||||
|
||||
@@ -305,35 +306,95 @@ To start the run, a repository owner or a collaborator with `Write` permission c
|
||||
|
||||
This setting lives in the Azure DevOps portal (pipeline → **More actions** → **Triggers** → **Pull request validation**), not in YAML.
|
||||
|
||||
### Coverage gates
|
||||
### Required checks and coverage
|
||||
|
||||
Every test stage ends in a **`Coverage <configuration> (<framework>)`** job. Each test matrix entry collects coverage while it runs and publishes its raw Cobertura fragment as a pipeline artifact; the coverage job then re-assembles those fragments with ReportGenerator and evaluates them. It never re-runs the tests — doing so would serialise a suite that was deliberately fanned out across matrix jobs and blow the stage timeout.
|
||||
Two concerns are deliberately kept apart, and both CI systems expose the same pair of checks:
|
||||
|
||||
That job is the single per-(framework, configuration) signal, so it — not an individual matrix entry — is what the GitHub branch ruleset should require. It fails when:
|
||||
| Concern | Azure Pipelines | GitHub Actions | In the branch ruleset? |
|
||||
| --- | --- | --- | --- |
|
||||
| Every test passed | **`Tests passed`** stage | **`build-and-test summary`** job | **Yes — required** |
|
||||
| Coverage meets the thresholds | **`Code coverage`** stage | **`code coverage`** job | **No — advisory** |
|
||||
|
||||
- any test job in its stage failed, was cancelled or never ran,
|
||||
- no coverage was produced at all,
|
||||
- or, on the reference leg, the merged report misses the thresholds.
|
||||
Azure Pipelines reports its checks to GitHub as `<pipeline> (<stage> <job>)`, so the two names to look for in the ruleset are `OPCFoundation.UA-.NETStandard (Tests passed Verify stage results)` and `OPCFoundation.UA-.NETStandard (Code coverage Merge and evaluate)`.
|
||||
|
||||
Only the **net10.0 Release** leg (`enforceCoverage: true` in [`azure-pipelines.yml`](../azure-pipelines.yml)) blocks on the thresholds and publishes the merged report to the build summary and Codacy — it is the configuration [`coverage-thresholds.json`](../coverage-thresholds.json) is calibrated against. The other legs report their numbers as a warning and still block on test failures. Every leg publishes its merged report as the `coverage-report-<suffix>` artifact.
|
||||
> **`Tests passed` is fail-closed, not fail-red.** Its verdict lives in the stage `condition`, which is the one place Azure Pipelines reliably exposes stage results. When a test stage fails the condition is false, the stage is skipped, and Azure Pipelines posts **no check at all** for it — so the required check stays unfulfilled and the merge stays blocked. You will see the failing test job in red and `Tests passed` still waiting, rather than two red checks.
|
||||
|
||||
The thresholds are enforced by [`.azurepipelines/check-coverage.ps1`](../.azurepipelines/check-coverage.ps1):
|
||||
The coverage check reports a clean failure when the thresholds are missed, so a miss is visible on the pull request, but it never blocks the merge. Do not add it to the ruleset — that would make a coverage dip unmergeable, which is not the intent.
|
||||
|
||||
Both required checks are single rollup jobs on purpose. The jobs underneath them are matrix-generated, so their names change whenever a test project or an agent is added, and they are skipped wholesale by the CI backend switch or by the path filter. Requiring a generated job name would therefore break as soon as the matrix changed.
|
||||
|
||||
The two rollups reach their verdict differently, and the difference matters:
|
||||
|
||||
| | How the verdict is reached | What a failing dependency looks like |
|
||||
| --- | --- | --- |
|
||||
| `build-and-test summary` (Actions) | Runs on `always()` and inspects `needs.*.result` inside the job, calling `exit 1` itself. | The check reports **failure** — a red X. |
|
||||
| `Tests passed` (Azure) | Encoded in the stage `condition`, the one place Azure Pipelines reliably exposes stage results. | The stage is skipped and Azure posts **no check** — the required check stays unfulfilled. |
|
||||
|
||||
The Actions job must use `always()` (rather than the implicit "all needs succeeded") precisely because a job that is *skipped* because a dependency failed surfaces to GitHub as `skipped`, and a required check reporting `skipped` is treated as **satisfied** — it would wave a red build straight through. The Azure stage is safe from that trap for a different reason: a skipped Azure *stage* posts nothing at all, so there is no `skipped` conclusion for the ruleset to accept. Verified on build 16613, where `Fast PR test` failed, `Tests passed` was skipped, and no `Tests passed` check-run reached the pull request.
|
||||
|
||||
#### How coverage is measured
|
||||
|
||||
Every test matrix entry collects coverage while it runs and publishes its raw Cobertura fragment as an artifact. The coverage check then downloads every fragment the run produced, merges them **once** with ReportGenerator, and evaluates the merged report. It never re-runs the tests — doing so serialises a suite that was deliberately fanned out across matrix jobs and blows the stage timeout.
|
||||
|
||||
The evaluation is [`.azurepipelines/check-coverage.ps1`](../.azurepipelines/check-coverage.ps1), shared by both CI systems and driven by [`coverage-thresholds.json`](../coverage-thresholds.json):
|
||||
|
||||
| Check | Behaviour |
|
||||
| --- | --- |
|
||||
| **Project floor** | *Blocking.* Total line and branch rates must meet the absolute floors in `coverage-thresholds.json`. The `ignore` globs are applied here too, so samples, tests and generated code do not count. |
|
||||
| **Patch coverage** | *Blocking on pull requests.* Lines you added or modified must reach `patch.target` percent, tolerating `patch.threshold` percentage points. |
|
||||
| **Baseline delta** | *Advisory only.* Reports how total coverage compares with the recorded `baselineLineRate` and never fails the build. |
|
||||
| **Project floor** | Total line and branch rates must meet the absolute floors in `coverage-thresholds.json`. The `ignore` globs are applied here too, so samples, tests and generated code do not count. |
|
||||
| **Patch coverage** | On pull requests, lines you added or modified must reach a floor that is **graduated by how much changed** — see below. The uncovered changed lines are listed by file. |
|
||||
| **Baseline delta** | Reports how total coverage compares with the recorded `baselineLineRate`. Warning only, even within this advisory check. |
|
||||
|
||||
Ratchet `minimumLineRate`, `minimumBranchRate` and `baselineLineRate` **upward** as coverage improves; never lower them to turn a red build green.
|
||||
Ratchet `minimumLineRate`, `minimumBranchRate` and `baselineLineRate` **upward** as coverage improves; never lower them to turn a red check green.
|
||||
|
||||
To reproduce a gate failure locally, generate the same report with [`tests/codecoverage.cmd`](../tests/codecoverage.cmd) (or [`tests/codecoverage.sh`](../tests/codecoverage.sh)) and run the script against it:
|
||||
##### Patch coverage is graduated by patch size
|
||||
|
||||
A coverage percentage over a handful of lines carries almost no information. One uncovered line in a two-line fix reads as 50%, and a flat floor would fail it — which teaches authors to ignore the check rather than act on it. So the requirement scales with how much actually changed:
|
||||
|
||||
| Coverable changed lines | Floor | Below the floor |
|
||||
| --- | --- | --- |
|
||||
| 1 – 10 | 50 % | :warning: **warning**, check still passes |
|
||||
| 11 – 100 | 60 % | :warning: **warning**, check still passes |
|
||||
| more than 100 | `patch.target` − `patch.threshold` (75 %) | :x: **failure** |
|
||||
|
||||
Only changes larger than the last band can fail the patch check. At that size the percentage is meaningful, and a large untested change is exactly what the check exists to catch. Below it you still get a warning naming the uncovered lines, so the signal is never silent — it just does not block.
|
||||
|
||||
The bands live in `patch.bands` in [`coverage-thresholds.json`](../coverage-thresholds.json). They are consulted in order and the first band whose `maxChangedLines` covers the patch wins; set `enforced: true` on a band to make it blocking. Anything larger than the last band falls through to `patch.target` − `patch.threshold` and is always enforced.
|
||||
|
||||
Remember that the coverage check as a whole is advisory and stays out of the branch ruleset — an enforced band produces a red `Code coverage` check, not a blocked merge.
|
||||
|
||||
##### Codecov
|
||||
|
||||
The merged report is also uploaded to [codecov.io](https://codecov.io), which is where the pull-request comment, the file-by-file diff view and the coverage trend live. **Codecov does not gate.** Both of its status checks are `informational: true` in [`codecov.yml`](../codecov.yml), because two gates with two sets of thresholds would eventually disagree about the same pull request and the easier one to silence would win. The rules that actually decide are the ones above.
|
||||
|
||||
Each CI system uploads the report it merged, under its own flag (`azure`, `actions`), since the two matrices deliberately cover different legs.
|
||||
|
||||
The upload is optional on both systems and never fails a build:
|
||||
|
||||
| | Turn it off with | Also skipped when |
|
||||
| --- | --- | --- |
|
||||
| Azure Pipelines | the `enableCodecov` pipeline parameter (default `true`) | the `CODECOV_TOKEN` secret variable is unset |
|
||||
| GitHub Actions | the `ENABLE_CODECOV` workflow `env` (default `'true'`) | the `CODECOV_TOKEN` secret is unavailable, as on fork pull requests |
|
||||
|
||||
Keep the `ignore` list in `codecov.yml` in step with the one in `coverage-thresholds.json`, or the two will report on different code.
|
||||
|
||||
#### Where the numbers appear
|
||||
|
||||
The script renders a markdown summary that both systems surface, so you never have to open a raw log to see why coverage moved:
|
||||
|
||||
- **GitHub Actions** — appended to the run's job summary, and posted as a single sticky pull-request comment that is updated in place on each run. Threshold misses additionally appear as run annotations. On a pull request **from a fork** the token is read-only, so the comment is skipped and only the job summary is written.
|
||||
- **Azure Pipelines** — attached to the build summary via `##vso[task.uploadsummary]`, alongside the usual Code Coverage tab and the Codacy upload.
|
||||
|
||||
Both also publish the merged HTML report as a `coverage-report` artifact.
|
||||
|
||||
> The two systems report **different numbers**, and that is expected. With the default `actions` backend, GitHub Actions merges every test project on ubuntu, whereas Azure Pipelines merges only the Windows fast-PR legs. The GitHub figure is the more representative one. Scheduled runs read higher still, because the Debug, .NET 8/9 and netstandard stages also contribute fragments.
|
||||
|
||||
To reproduce a coverage failure locally, generate the same report with [`tests/codecoverage.cmd`](../tests/codecoverage.cmd) (or [`tests/codecoverage.sh`](../tests/codecoverage.sh)) and run the script against it:
|
||||
|
||||
```powershell
|
||||
./.azurepipelines/check-coverage.ps1 -CoberturaPath ./CodeCoverage/Cobertura.xml -BaseRef master
|
||||
./.azurepipelines/check-coverage.ps1 -CoberturaPath ./CodeCoverage/Cobertura.xml -BaseRef master -SummaryPath ./coverage-summary.md
|
||||
```
|
||||
|
||||
Omit `-BaseRef` to check only the project floor.
|
||||
Omit `-BaseRef` to check only the project floor, and `-SummaryPath` to skip the markdown summary.
|
||||
|
||||
## Contributing and pull requests
|
||||
|
||||
|
||||
Reference in New Issue
Block a user