ci: use MTP GitHub Actions report and upload coverage to GitHub - #800
ci: use MTP GitHub Actions report and upload coverage to GitHub#800askpt wants to merge 6 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Coverage OverviewLanguages: C# C# / code-coverage/coverletThe overall coverage in commit 69c22e1 in the Show a code coverage summary of the most covered files.
Updated |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #800 +/- ##
=======================================
Coverage 93.55% 93.55%
=======================================
Files 66 66
Lines 3010 3010
Branches 378 378
=======================================
Hits 2816 2816
Misses 130 130
Partials 64 64 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Replaces ineffective VSTest reporting with native MTP GitHub reporting and adds GitHub code coverage alongside Codecov.
Changes:
- Adds and configures the MTP GitHub Actions reporter.
- Generates both OpenCover and Cobertura coverage.
- Merges and uploads repository-relative coverage to GitHub.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
Directory.Packages.props |
Replaces the obsolete logger package. |
CONTRIBUTING.md |
Documents dual coverage formats. |
build/Common.tests.props |
Adds the reporter to test projects only. |
.github/workflows/e2e.yml |
Enables MTP reporting for E2E tests. |
.github/workflows/code-coverage.yml |
Merges and uploads Cobertura coverage. |
.github/workflows/ci.yml |
Enables MTP reporting in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`needs:` cannot reference jobs in another workflow file, so a single required status check means the gated jobs have to live in one workflow. Fold code-coverage.yml, dotnet-format.yml, aot-compatibility.yml and e2e.yml into ci.yml and add a `CI Gate` job that depends on all of them. Once branch protection requires only `DCO` + `CI Gate`, build, test, coverage, format, E2E and the AOT matrix all become effectively required, and matrix legs can be added or removed without editing branch protection. Along the way this fixes several things that were silently broken: - ci.yml, code-coverage.yml and dotnet-format.yml had no `merge_group` trigger, so the merge queue never validated build, test, coverage or format against the merged result. They now run there. - ci.yml and code-coverage.yml used workflow-level `paths-ignore: "**.md"`. A workflow skipped by path filtering reports no check runs at all, so a required check inside it would leave docs-only pull requests blocked forever. The filter is gone. - The gate uses `if: always()`. Without it the job inherits the implicit `success()` condition and is *skipped* when a dependency fails - and branch protection treats a skipped check as satisfied, which would let exactly the broken pull requests through. The jq predicate also fails on `cancelled` and on any skip that is not explicitly allow-listed. - `github.event.pull_request.head.repo.fork == false` is true on `push` and `merge_group` because GitHub coerces the missing value to 0 before comparing. Combined with `startsWith(github.ref, 'refs/heads/')`, which matches `refs/heads/gh-readonly-queue/*`, adding a merge_group trigger would have published a NuGet package for every queue entry. The packaging job now checks `github.event_name` explicitly, always packs as a validation step, and publishes only on push or a non-fork pull request. - Merge queue runs are never cancelled by the concurrency group; a cancelled run reports a non-success conclusion and ejects the pull request from the queue. Coverage is skipped on `merge_group` - the `gh-readonly-queue/*` ref is thrown away, so uploading it to Codecov is meaningless and `build` already runs the same tests there. It is the only entry in the gate's allow-list. The E2E job deliberately keeps the bare `e2e-tests` id so its check run name is unchanged; it is a required context today and a required check that stops reporting blocks every open pull request. Duplicated setup-dotnet and NuGet cache blocks move into a local composite action. Branch protection still has to be updated after this merges. Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Follow-up to the CI Gate consolidation. Rather than one long ci.yml, each unit
of work becomes a `workflow_call` building block and ci.yml is reduced to an
orchestrator that wires them together and evaluates the gate.
ci.yml
build -> reusable-build.yml
coverage -> reusable-coverage.yml
format -> reusable-format.yml
e2e -> reusable-e2e.yml
aot -> reusable-aot.yml
packaging -> reusable-packaging.yml
ci-gate -- the single required status check
The gate still has to live in ci.yml because `needs:` cannot reference a job in
another workflow file. Everything else moves out.
Beyond readability, actionlint cross-validates the calls, so a typo in an input
or secret name becomes a lint error rather than a runtime failure.
The blocks take no inputs. They have exactly one caller in this repository, so
parameters would only ever be passed their own defaults. The one thing that does
have to cross the boundary is the Codecov token: secrets are not visible to a
called workflow unless they are passed explicitly.
One consequence worth calling out: check runs produced through a reusable
workflow are named `<caller job> / <called job>`, so the checks are now
`Build / ubuntu-latest`, `AOT / linux-arm64`, `Format / dotnet format` and so
on. That means the `e2e-tests` context cannot be preserved the way it was in
the previous commit, and branch protection has to drop it before this merges
rather than after. `CI Gate` is unaffected - it is a plain job in ci.yml, so its
name has no prefix and stays stable as jobs move around underneath it.
Reusable workflows are `workflow_call`-only so they never self-trigger, and
concurrency stays in the caller alone - sharing a group between caller and
callee makes a run cancel itself.
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
The `--report-github` switch used in CI is not a real option. Neither Microsoft.Testing.Platform, xUnit v3, coverlet.MTP nor the .NET SDK register it, so it was silently ignored and no annotations or job summary were ever produced. GitHubActionsTestLogger was dead weight too: it is a VSTest logger, and this repo runs tests through Microsoft Testing Platform, which never loads it. Replace both with the first-party Microsoft.Testing.Extensions.GitHubActionsReport extension and its documented `--report-gh` switch (MTP 2.3.0+). This gives per-assembly log groups, failure/skip annotations and a Markdown job summary. The extension is scoped to test projects so it does not turn the benchmark and AOT sample projects into test applications. It is only published as a prerelease today, so the version is pinned and should be moved to the stable release once one ships. Also add GitHub's built-in code coverage alongside Codecov. Coverlet now emits Cobertura next to OpenCover, the per-project/per-TFM reports are merged into one file with ReportGenerator, and paths are rewritten to be repository-relative so coverage maps onto the diff. Codecov keeps using the unchanged OpenCover reports. The upload is best effort for now, since it needs Code Quality enabled on the repository. Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
cc0ac5b to
79d25c6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
.github/workflows/reusable-coverage.yml:94
- This newly added reporting path can still block the required
CI Gate: a tool-install, glob, ReportGenerator, orsedfailure stops this step before the uploader'sfail-on-errorsetting applies, andci.ymlrejects a failed coverage job. Make both GitHub-only steps non-blocking so Codecov remains the coverage gate as intended.
- name: Merge Cobertura reports (Linux)
if: runner.os == 'Linux'
run: |
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
74bd0f1 to
69c22e1
Compare
Why
While evaluating whether Microsoft Testing Platform's native GitHub Actions reports could replace
GitHubActionsTestLogger, it turned out that neither reporting path was actually working:--report-github, which the build and e2e jobs have been passing for months, is not a real MTP option. It was silently ignored.GitHubActionsTestLoggeris a VSTest logger. Sinceglobal.jsonsets"test": { "runner": "Microsoft.Testing.Platform" }, VSTest loggers are never loaded, so the package was dead weight.Net effect: the repo has had zero GitHub Actions test reporting. This PR makes it real, and adds GitHub's built-in code coverage alongside the existing Codecov upload.
What changed
Test reporting
GitHubActionsTestLoggerwithMicrosoft.Testing.Extensions.GitHubActionsReport, and switched--report-githubto the real--report-ghinreusable-build.ymlandreusable-e2e.yml. The extension is auto-registered byMicrosoft.Testing.Platform.MSBuild, so no code changes are needed, and it only activates whenGITHUB_ACTIONS=true(local runs are unaffected).--report-ghwas introduced in 2.3.0.Coverage (
reusable-coverage.yml)opencover(for Codecov, unchanged) andcoberturafrom a single test run.actions/upload-code-coverage. The Codecov steps are untouched and still run on both Windows and Linux.--coverlet-exclude "[GitHubActionsTestLogger*]*"filter.code-quality: writeis granted on thecoveragejob inci.ymland declared on the called workflow, since a reusable workflow cannot escalate beyond what the caller grants.Things worth a careful look
Microsoft.Testing.Extensions.GitHubActionsReportis only published as1.0.0-alpha.*(pinned to1.0.0-alpha.26377.5). There is no stable release yet. Renovate will not bump it unless prereleases are explicitly allowed for that package, so it may need a manual refresh later.$(MSBuildProjectName.EndsWith('Tests'))inbuild/Common.tests.props. Referencing any MTP extension turns a project into an MTP test application, which madedotnet teststart discovering and runningOpenFeature.BenchmarksandOpenFeature.AotCompatibility. The condition prevents that.<source>. It also filters**/obj/**to drop source-generated files.processing status: succeeded, so Code Quality is already enabled on this repo. The step still runs withfail-on-error: falseso a coverage outage can never block CI, and Codecov remains untouched.OpenFeature.Teststhe summary shows 391 of 437 tests. The dropped tests are the ones that finish last: the summary is written from a snapshot taken roughly 150ms before the run actually ends. Smaller suites (Hosting.Tests106,MultiProvider.Tests255,IntegrationTests10) all report exactly right. This only affects the rendered summary. The exit code and the console output are still accurate, so a failing test can never be hidden from CI gating by this bug.ref: ${{ github.event.pull_request.head.sha || github.sha }}becauseupload-code-coverageneeds the real head commit rather than the merge commit.Validation
Confirmed on a full CI run of this change (all 25 checks green, before restacking):
--report-ghis accepted and the suite runs normally: 2424 tests, 0 failures, 0 build warnings. Under MTP 2.3.3 an unrecognized option is a hard failure, so this also proves the extension is genuinely loaded.actions/upload-code-coverageuploaded it, with GitHub confirmingprocessing status: succeeded.codecov/patchandcodecov/projectboth passed on Linux and Windows.Note on CI while stacked
ci.ymlfilters onpull_request: branches: [main], so once this PR targetsaskpt/ci-required-job-merge-queueno further pushes here will trigger CI. The run against the rebased head was triggered while the base was stillmain. CI resumes automatically once #801 merges and this PR is retargeted tomain. Worth considering whether #801 should drop that base filter so stacked PRs stay covered.Still a draft pending a decision on whether the summary undercount is acceptable while the reporter is prerelease.