diff --git a/.editorconfig b/.editorconfig index 09e0273..13018b0 100644 --- a/.editorconfig +++ b/.editorconfig @@ -11,7 +11,7 @@ trim_trailing_whitespace = true indent_style = space indent_size = 4 -[*.{json,yml,yaml,csproj,props,targets,slnx,resx,webmanifest}] +[*.{json,yml,yaml,csproj,props,targets,slnx,resx,runsettings,webmanifest}] indent_size = 2 [*.{js,css,html,razor}] diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fcf1d28..10bf9b9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,7 +1,9 @@ name: CI # The whole pipeline for a change, in one file and in one shape: build it, test it, publish it, -# then drive a browser over the thing that was published. The commit is compiled exactly once — +# then drive a browser over the thing that was published and judge what the tests covered. The +# coverage report comes out of the gate's own test run, so the number is about the code that +# passed rather than a second run of it. The commit is compiled exactly once — # `dotnet publish --no-build` reuses what `dotnet build` produced and what `dotnet test` ran # against, so the app the browsers see is the app the unit tests passed on, not a rebuild of it. @@ -60,7 +62,23 @@ jobs: - run: dotnet restore - run: dotnet build --no-restore --configuration Release - - run: dotnet test --no-build --configuration Release --verbosity normal + + # The collector rides along with the run this gate already does — one test run, not a second + # one for the number. coverage.runsettings decides which files it may count (the same file + # scripts/coverage.sh passes), and the Coverage job below is what judges the report. + - name: Test, with the coverage collector attached + run: >- + dotnet test --no-build --configuration Release --verbosity normal + --collect:"XPlat Code Coverage" --settings coverage.runsettings + --results-directory artifacts/coverage + + # Handed to the Coverage job below, and kept for the week a reviewer might want to open the + # Cobertura report itself rather than the summary that job writes. + - uses: actions/upload-artifact@v7 + with: + name: coverage + path: artifacts/coverage/ + retention-days: 7 # The one compile in this pipeline ends here. `--no-build` publishes what the two steps above # already produced; without it this would be a second compile of the same commit. @@ -79,14 +97,54 @@ jobs: - name: Drop the native libraries for other architectures run: find artifacts/app/runtimes -mindepth 1 -maxdepth 1 ! -name linux-x64 -exec rm -rf {} + - # A day, because nothing reads this after the run that made it — unlike the reports below, - # which are uploaded for a person to open. + # A day, because nothing reads this after the run that made it — unlike the reports, which + # are uploaded for a person to open. - uses: actions/upload-artifact@v7 with: name: app path: artifacts/app/ retention-days: 1 + # Whether the code this branch changed is tested. Not the repository's overall percentage, which + # a review cannot act on — Core sits above 95%, so a solution-wide 80% gate would pass with an + # entirely untested new service in the diff. scripts/coverage.mjs takes the added and rewritten + # lines, keeps the ones the instrumenter counted as coverable, and holds them to an 80% floor; + # docs/testing.md has the reasoning. The result is written to this run's summary page, so the + # number is read without opening a log or downloading the report. + # + # Advisory, like the two browser jobs: it goes red under the floor but `main`'s ruleset requires + # "Build and test" alone, so it reports rather than blocks. It needs no compiler — the report was + # produced by the test run above — so it is node and a git history, and it is over in seconds. + coverage: + name: Coverage + needs: build + if: github.event_name != 'workflow_call' + runs-on: ubuntu-latest + timeout-minutes: 10 + steps: + # The whole history, unlike every other job here: judging the change means diffing against + # the merge base, and the single-commit checkout the others get has nothing to diff against. + - uses: actions/checkout@v7 + with: + fetch-depth: 0 + + - uses: actions/setup-node@v7 + with: + node-version: 22 + + - uses: actions/download-artifact@v7 + with: + name: coverage + path: artifacts/coverage + + # The base branch rather than a hardcoded main, so a pull request onto a release branch is + # judged on what it actually changed. workflow_dispatch has no pull request and falls back to + # the repository's default branch. + - name: Judge the lines this change touched + run: node scripts/coverage.mjs + env: + COVERAGE_BASE: origin/${{ github.event.pull_request.base.ref || github.event.repository.default_branch }} + # What the app does: the public/admin split, the dialogs, the match-day journey, both languages, # the phone layout. See docs/testing.md. # diff --git a/CLAUDE.md b/CLAUDE.md index 9313dd2..ee9c2e0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -186,9 +186,11 @@ admin matrix; it is not part of this repository.) - Work on a feature branch. `main` takes pull requests only, and the merge button stays disabled until **Build and test** is green and every review thread is resolved (`.github/rulesets/main-build-and-test.json`). -- `ci.yml` runs `dotnet build -c Release` + `dotnet test` on every pull request. `fly-deploy.yml` - *calls that same workflow* as the gate its deploy job depends on, then smoke-checks `/health` - until it reports the commit that was just built. +- `ci.yml` runs `dotnet build -c Release` + `dotnet test` on every pull request. That test run + carries the coverage collector, and an advisory **Coverage** job judges the lines the pull + request changed against the 80% floor and writes the numbers to the run's summary page. + `fly-deploy.yml` *calls that same workflow* as the gate its deploy job depends on, then + smoke-checks `/health` until it reports the commit that was just built. - Merging to `main` *proposes* a deploy; it does not perform one. The deploy job runs in the `production` environment, which has a required reviewer, so the run waits at *Waiting* until the maintainer approves it. There is no staging environment — that approval is the last look. diff --git a/coverage.runsettings b/coverage.runsettings new file mode 100644 index 0000000..11f2586 --- /dev/null +++ b/coverage.runsettings @@ -0,0 +1,39 @@ + + + + + + + + cobertura + + + [FootballFormation.UI]*,[FootballFormation.Web]* + + + **/Migrations/*.cs,**/DesignTimeDbContextFactory.cs,**/*.razor,**/*.razor.cs,**/*.g.cs,**/*.designer.cs + + + Obsolete,GeneratedCodeAttribute,ExcludeFromCodeCoverageAttribute + + + + + diff --git a/docs/testing.md b/docs/testing.md index 49710fb..6eb9f08 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -91,8 +91,8 @@ COVERAGE_SKIP_TEST=1 scripts/coverage.sh # re-judge the last run without re-ru `coverlet.collector` writes a Cobertura report into `artifacts/coverage/`, and `coverage.mjs` answers the only question a review can act on: **is the code this branch changed covered?** The -floor is **80% of the changed lines**, and the script exits non-zero under it, so it works as a -pipeline step as it stands. +floor is **80% of the changed lines**, and the script exits non-zero under it, which is exactly how +CI runs it — see [the Coverage job](#one-pipeline-one-compile). **The gate is the change, not the repository, and that is the whole design.** Core is above 96% line coverage, so a solution-wide 80% gate would pass with an entirely untested new service in the @@ -100,14 +100,36 @@ diff — the number would move by tenths. The script takes the added and rewritt `git diff --unified=0` against the merge base (uncommitted work included), keeps the ones the instrumenter counted as coverable, and reports per file with the uncovered line numbers. -Three things are deliberately outside the number: - -- **`UI` and `Web` are not measured at all.** The test project references `Core` alone, and the - other two have no unit tests by design — `tests/ui` and `visual-check.sh` are what cover them. - A change there is reported as unmeasured rather than counted as a miss. -- **Migrations are excluded.** A `Down()` is never executed by the suite and never will be; - counting scaffolded code would make the gate a lottery on how much of it a change touched. -- **`DesignTimeDbContextFactory`** exists for `dotnet ef` and runs in no test. +### What the collector may count + +`coverage.runsettings` at the repository root is what decides, and both `scripts/coverage.sh` and +CI's test step pass it, so a local number and a pipeline number mean the same thing. Everything +below is out of the report entirely — not merely out of the judgement: + +- **`UI` and `Web`**, by module (`[FootballFormation.UI]*`, `[FootballFormation.Web]*`) and by + file (`**/*.razor`, `**/*.razor.cs`). The test project references `Core` alone, so nothing from + either is instrumented today; naming them keeps that true the day somebody adds a reference for + one helper. A `.razor` compiles to a generated class whose lines map back to markup and a + `.razor.cs` is the other half of that same partial class — neither is reachable without rendering + a component, and nothing in `tests/` renders one. Those two are covered by `tests/ui` and + `visual-check.sh` instead, and a change there is reported as unmeasured rather than as a miss. +- **Migrations and the model snapshot.** A `Down()` is never executed by the suite and never will + be, and counting scaffolded code makes the gate a lottery on how much of it a change touched. + Excluding them took `Core` from a comfortable 96.4% over 9,960 lines to an honest 93.3% over + 2,509. +- **`DesignTimeDbContextFactory`**, which exists for `dotnet ef` and runs in no test. +- **Generated and deliberately-marked code**, by attribute — `GeneratedCode`, `ExcludeFromCodeCoverage`, + `Obsolete`. `CompilerGeneratedAttribute` is deliberately *not* one of them: it is not just + lambdas and iterator state machines, it is how the compiler marks every `async` method body and + every auto-property, and excluding it took `ServiceOperation.RunAdminAsync` — the write guard + every service call goes through — and most of `DatabaseSafety` out of the report along with the + scaffolding. A change that silently stopped judging the admin check would be worse than the + scaffolding problem this file exists to fix. + +`coverage.mjs` still recognises migrations and `DesignTimeDbContextFactory` by name, and treats any +other changed `Core` file the report never mentions the same way: named under the table as excluded +rather than dropped from the diff silently, because a reviewer has to know the change contains code +this number says nothing about. Branch coverage sits near 75% and is reported for information, not gated — the line floor is what the `code-reviewer` agent enforces. And a floor is not a target: 100% of a change whose only test @@ -160,18 +182,34 @@ Adding a spec that leans on a new app class means adding it to `SELECTORS` too. ### One pipeline, one compile -Everything lives in `.github/workflows/ci.yml`, in three jobs on one chain: +Everything lives in `.github/workflows/ci.yml`, in four jobs on one chain: ``` -Build and test ──┬── Playwright - (required) └── Visual check - (both advisory) +Build and test ──┬── Coverage + (required) ├── Playwright + └── Visual check + (all three advisory) ``` **`Build and test`** restores, builds Release, runs `dotnet test`, then publishes — and the publish is `--no-build`, so it hands on exactly what the unit tests just ran against rather than compiling the commit a second time. It prunes the published `runtimes/` to `linux-x64` and uploads the result -as the `app` artifact. +as the `app` artifact. The test step carries `--collect:"XPlat Code Coverage" --settings +coverage.runsettings`, so the report comes out of the run that is already the gate rather than out +of a second run of the same tests, and it is uploaded as the `coverage` artifact. + +**`Coverage`** downloads that report and runs `scripts/coverage.mjs` over it — the same script and +the same 80% floor as a local `scripts/coverage.sh`, with `COVERAGE_BASE` pointed at the pull +request's base branch. It is the one job checked out with `fetch-depth: 0`, because judging a +change means diffing against its merge base and a single-commit checkout has nothing to diff +against. It compiles nothing and needs no browser, so it is Node and a git history and is over in +seconds. The verdict, the whole-project line and branch numbers, and a per-file table with the +uncovered line numbers are written to `$GITHUB_STEP_SUMMARY` — the run's own front page, so the +result is read without opening a log or downloading the Cobertura report. + +Advisory, like the browser jobs below: it goes red under the floor without blocking a merge, since +`main`'s ruleset names **Build and test** and nothing else. Promoting it is one line in +`.github/rulesets/main-build-and-test.json` and nothing in the workflow. **`Playwright` and `Visual check`** download that artifact and start it. Neither calls a compiler; they install the .NET SDK only for the runtime to run `dotnet FootballFormation.Web.dll` with. diff --git a/scripts/coverage.mjs b/scripts/coverage.mjs index a49c6ad..8eb7aa1 100644 --- a/scripts/coverage.mjs +++ b/scripts/coverage.mjs @@ -6,7 +6,7 @@ // the added lines instead makes the gate about the change, which is the only thing a review can // still act on. -import { readFileSync, readdirSync, existsSync } from 'node:fs'; +import { readFileSync, readdirSync, existsSync, appendFileSync } from 'node:fs'; import { join, relative } from 'node:path'; import { execFileSync } from 'node:child_process'; @@ -15,10 +15,9 @@ const THRESHOLD = Number(process.env.COVERAGE_THRESHOLD ?? 80); const BASE = process.env.COVERAGE_BASE ?? 'origin/main'; const REPORT_DIR = process.env.COVERAGE_DIR ?? join(REPO, 'artifacts/coverage'); -// Scaffolded or design-time only, and excluded from the judgement rather than from the report. -// A migration's Down() is never executed by the suite and never will be; DesignTimeDbContextFactory -// exists for `dotnet ef` and runs in no test. Counting them would make the gate a lottery on how -// much scaffolding a change happened to touch. +// Scaffolded or design-time only, named ahead of the report rather than discovered from it — see +// the `!files.has(key)` check below for the general case, which catches everything else +// coverage.runsettings excludes. const EXCLUDED = [/^Migrations\//, /^Data\/DesignTimeDbContextFactory\.cs$/]; const git = (...args) => execFileSync('git', args, { cwd: REPO, encoding: 'utf8' }); @@ -36,6 +35,18 @@ function findReports(dir) { function parseCobertura(path) { const xml = readFileSync(path, 'utf8'); const root = (xml.match(/([^<]*)<\/source>/) ?? [, ''])[1]; + // Branches are read off the root element rather than summed: a carries its taken/total + // in a condition-coverage string, and the totals are already there to be read. Failing loudly + // here matters because pct(0, 0) reads as a trivial 100% pass everywhere else in this script — + // exactly the reading a missing attribute must never get. + const header = (xml.match(/]*>/) ?? [])[0]; + if (!header) throw new Error(`${path}: no root element — not a Cobertura report`); + const headerNumber = name => { + const m = header.match(new RegExp(`${name}="([^"]*)"`)); + if (!m) throw new Error(`${path}: element has no ${name} attribute`); + return Number(m[1]); + }; + const branches = { covered: headerNumber('branches-covered'), valid: headerNumber('branches-valid') }; const files = new Map(); let totals = { covered: 0, valid: 0 }; @@ -56,7 +67,7 @@ function parseCobertura(path) { if (hits > 0) totals.covered++; } - return { root, files, totals }; + return { root, files, totals, branches }; } // The lines this branch added or rewrote, from the unified diff's hunk headers. Diffed against the @@ -96,7 +107,7 @@ if (reports.length === 0) { process.exit(2); } -const { root, files, totals } = parseCobertura(reports[0]); +const { root, files, totals, branches } = parseCobertura(reports[0]); const { byFile, mergeBase } = addedLines(BASE); const sourceRoot = relative(REPO, root) + '/'; // e.g. src/FootballFormation.Core/ @@ -115,9 +126,18 @@ for (const [path, added] of byFile) { excluded.push(path); continue; } + // A file coverage.runsettings excludes by attribute or by a pattern EXCLUDED does not know + // about (a new generated-code shape, say) never appears in the report at all. That is + // different from a file that IS in the report but whose particular added lines all landed on + // a brace, a using, a blank: only the first is worth naming, so a changed file the runsettings + // dropped is never mistaken for one this diff simply didn't touch anywhere coverable. + if (!files.has(key)) { + excluded.push(path); + continue; + } // A line absent from the report is not coverable — a brace, a using, a field declaration, a // blank. Only lines the instrumenter counted are judged, so whitespace can't dilute the number. - const lines = files.get(key) ?? new Map(); + const lines = files.get(key); const coverable = [...added].filter(n => lines.has(n)); if (coverable.length === 0) continue; const hit = coverable.filter(n => lines.get(n) > 0); @@ -133,17 +153,20 @@ const pct = (hit, total) => (total === 0 ? 100 : (hit / total) * 100); const changedHit = measured.reduce((n, f) => n + f.hit, 0); const changedTotal = measured.reduce((n, f) => n + f.total, 0); +measured.sort((a, b) => pct(a.hit, a.total) - pct(b.hit, b.total)); +const under = f => pct(f.hit, f.total) < THRESHOLD; + console.log(`Coverage of the change (base ${mergeBase.slice(0, 12)}, threshold ${THRESHOLD}%)\n`); -console.log(` Core overall: ${pct(totals.covered, totals.valid).toFixed(1)}% (${totals.covered}/${totals.valid} lines)`); +console.log(` Core overall: ${pct(totals.covered, totals.valid).toFixed(1)}% (${totals.covered}/${totals.valid} lines)` + + `, branches ${pct(branches.covered, branches.valid).toFixed(1)}% (${branches.covered}/${branches.valid})`); if (measured.length === 0) { console.log(' Changed lines: none measurable in Core.\n'); } else { console.log(` Changed lines: ${pct(changedHit, changedTotal).toFixed(1)}% (${changedHit}/${changedTotal})\n`); - for (const f of measured.sort((a, b) => pct(a.hit, a.total) - pct(b.hit, b.total))) { - const p = pct(f.hit, f.total); - const flag = p < THRESHOLD ? 'FAIL' : ' ok'; - console.log(` ${flag} ${p.toFixed(1).padStart(5)}% ${f.hit}/${f.total} ${f.path}`); + for (const f of measured) { + const flag = under(f) ? 'FAIL' : ' ok'; + console.log(` ${flag} ${pct(f.hit, f.total).toFixed(1).padStart(5)}% ${f.hit}/${f.total} ${f.path}`); if (f.missed.length) console.log(` uncovered lines: ${f.missed.join(', ')}`); } } @@ -156,17 +179,57 @@ if (unmeasured.length) { for (const p of unmeasured) console.log(` ${p}`); } if (excluded.length) { - console.log(`\n Excluded (scaffolded or design-time):`); + console.log(`\n Excluded from the report (coverage.runsettings):`); for (const p of excluded) console.log(` ${p}`); } -const failing = measured.filter(f => pct(f.hit, f.total) < THRESHOLD); -if (changedTotal > 0 && pct(changedHit, changedTotal) < THRESHOLD) { - console.log(`\nFAIL: the change is ${pct(changedHit, changedTotal).toFixed(1)}% covered, under the ${THRESHOLD}% floor.`); - process.exit(1); -} -if (failing.length) { - console.log(`\nFAIL: ${failing.length} changed file(s) under the ${THRESHOLD}% floor.`); - process.exit(1); +const failing = measured.filter(under); +const verdict = + changedTotal > 0 && pct(changedHit, changedTotal) < THRESHOLD + ? `FAIL: the change is ${pct(changedHit, changedTotal).toFixed(1)}% covered, under the ${THRESHOLD}% floor.` + : failing.length + ? `FAIL: ${failing.length} changed file(s) under the ${THRESHOLD}% floor.` + : 'PASS'; + +console.log(`\n${verdict}`); + +// The same answer on the run's own page, because that is where it will actually be read: the log +// is two clicks in and the Cobertura report is a download. Writes nothing outside Actions. +if (process.env.GITHUB_STEP_SUMMARY) { + const shown = (hit, total) => `${pct(hit, total).toFixed(1)}% (${hit}/${total})`; + const md = [ + '## Coverage', + '', + `Judged against \`${mergeBase.slice(0, 12)}\`, floor **${THRESHOLD}%** of the lines this change touched.`, + '', + '| Scope | Lines | Branches |', + '| --- | ---: | ---: |', + `| \`FootballFormation.Core\`, whole project | ${shown(totals.covered, totals.valid)} | ${shown(branches.covered, branches.valid)} |`, + `| Lines this change touched | ${measured.length === 0 ? '—' : `**${shown(changedHit, changedTotal)}**`} | — |`, + '', + ]; + + if (measured.length) { + md.push('| | Changed file | Lines | Uncovered |', '| --- | --- | ---: | --- |'); + for (const f of measured) { + md.push(`| ${under(f) ? '❌' : '✅'} | \`${f.path}\` | ${shown(f.hit, f.total)} | ` + + `${f.missed.length ? f.missed.join(', ') : '—'} |`); + } + md.push(''); + } else { + md.push('No measurable Core lines in this change.', ''); + } + + const details = (summary, paths) => + paths.length ? [`
${summary} (${paths.length})`, '', + ...paths.map(p => `- \`${p}\``), '', '
', ''] : []; + md.push( + ...details('Not measured here — no unit tests by design, see tests/ui and scripts/visual-check.sh', unmeasured), + ...details('Excluded from the report — see coverage.runsettings', excluded), + `**${verdict}**`, + ); + + appendFileSync(process.env.GITHUB_STEP_SUMMARY, md.join('\n') + '\n'); } -console.log(`\nPASS`); + +process.exit(verdict === 'PASS' ? 0 : 1); diff --git a/scripts/coverage.sh b/scripts/coverage.sh index da9829b..5111cd9 100755 --- a/scripts/coverage.sh +++ b/scripts/coverage.sh @@ -21,7 +21,10 @@ if [ -z "${COVERAGE_SKIP_TEST:-}" ]; then rm -rf "$OUT" # Release, like CI — warnings are errors there, so a coverage run that passes in Debug and fails # the build in CI would be the worst of both. - dotnet test "$REPO" -c Release --collect:"XPlat Code Coverage" --results-directory "$OUT" + # coverage.runsettings is what decides which files the collector counts, and CI's test step + # passes the same file — a local number and a pipeline number mean the same thing. + dotnet test "$REPO" -c Release --collect:"XPlat Code Coverage" \ + --settings "$REPO/coverage.runsettings" --results-directory "$OUT" fi COVERAGE_DIR="$OUT" node "$REPO/scripts/coverage.mjs"