From 7750a3486b6b2c804b725cd3a9d9713e5ead29ca Mon Sep 17 00:00:00 2001 From: "Frank Pigeon Jr." <4629398+fpigeonjr@users.noreply.github.com> Date: Thu, 27 Aug 2026 15:58:22 -0500 Subject: [PATCH 1/3] Add ratcheting coverage-floor gate to CI Mirrors the mechanism built for ngx-uswds (#237 -> #259): - coverage-floor.json holds the four coverage floors (statements, branches, functions, lines), seeded to coverage measured after the Vitest migration (#626) landed. - scripts/check-coverage.mjs reads floors from that file and fails with a clear per-metric message if measured coverage drops below any floor. --bump rewrites the floor file up to current measured coverage, ratchet-only (never lowers a floor). - test-app/vitest.config.mts now emits the json-summary reporter so coverage-summary.json exists for the gate to read. - Wired into CI (.github/workflows/ci.yml) as a required step after the Vitest test run. - AGENTS.md documents the ratchet policy: feature/spec PRs must not edit coverage-floor.json; raising it is a separate, deliberate coverage:bump commit. Closes #627 --- .github/workflows/ci.yml | 7 ++ AGENTS.md | 14 +++ coverage-floor.json | 6 + package.json | 2 + scripts/check-coverage.mjs | 128 ++++++++++++++++++++ scripts/check-coverage.test.mjs | 208 ++++++++++++++++++++++++++++++++ test-app/vitest.config.mts | 2 +- 7 files changed, 366 insertions(+), 1 deletion(-) create mode 100644 AGENTS.md create mode 100644 coverage-floor.json create mode 100644 scripts/check-coverage.mjs create mode 100644 scripts/check-coverage.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e9071c4f7..e436fb5da 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -43,6 +43,13 @@ jobs: - name: Run test-app harness with coverage run: npm --prefix test-app test + # Ratcheting coverage-floor gate (#627): fails the build if measured + # coverage drops below any floor recorded in coverage-floor.json. + # Floors only ever move up, via `npm run coverage:bump`, landed as its + # own deliberate commit — feature/spec PRs must not edit that file. + - name: Enforce coverage-floor gate + run: npm run coverage:check + # Coverage is reported here (Vitest's istanbul text-summary in the log # above, plus the lcov artifact below). Enforcement of the 80% new-code # floor (target 90%) is tracked by #576 and applied once the green diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..fa6e09b75 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,14 @@ +# AGENTS.md + +Guidance for AI coding agents working in this repository. + +## Testing + +- Library specs (`src/**/*.spec.ts`) run against the `test-app` workspace's Vitest harness (Analog's Angular Vite plugin + `@vitest/coverage-istanbul`). From the repo root: `npm ci && npm ci --prefix test-app && npm --prefix test-app test`. +- Coverage is enforced by a ratcheting floor gate: `coverage-floor.json` at the repo root records the minimum required statements/branches/functions/lines percentages, and `node scripts/check-coverage.mjs` (wired into CI as `npm run coverage:check`) fails the build with a clear per-metric message if measured coverage (from `test-app/coverage/coverage-summary.json`) drops below any floor. +- **`coverage-floor.json` is a ratchet — it only ever moves up, and only via a dedicated bump.** Feature and spec PRs must **not** edit `coverage-floor.json` directly; raising it is a separate, deliberate commit made with `npm run coverage:bump` (rewrites the floor file to the currently measured coverage, never lowering an existing entry). This keeps the shared floor file from becoming a merge-conflict magnet across parallel coverage-improvement PRs. +- The gate script itself is covered by `node --test scripts/check-coverage.test.mjs` (Node's built-in test runner, no extra deps). + +## Lint + +- `npm run lint` / `npm --prefix test-app run lint` run ESLint. A ratcheting warning-baseline gate (`eslint-baseline.json`, `scripts/check-lint-baseline.mjs`) works the same way as the coverage gate above — only ever lower the baseline via `--bump`, never raise it by hand. diff --git a/coverage-floor.json b/coverage-floor.json new file mode 100644 index 000000000..a13030cad --- /dev/null +++ b/coverage-floor.json @@ -0,0 +1,6 @@ +{ + "statements": 53, + "branches": 39, + "functions": 50, + "lines": 52 +} diff --git a/package.json b/package.json index 93a85908d..0867d25e0 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,8 @@ "lint:report": "ng lint --format json --output-file eslint-report.json", "lint:baseline": "npm run lint:report && node scripts/check-lint-baseline.mjs root eslint-report.json", "lint:baseline:bump": "npm run lint:report && node scripts/check-lint-baseline.mjs --bump root eslint-report.json", + "coverage:check": "node scripts/check-coverage.mjs test-app/coverage/coverage-summary.json", + "coverage:bump": "node scripts/check-coverage.mjs --bump test-app/coverage/coverage-summary.json", "validate:publish": "node scripts/validate-publish-package.mjs" }, "peerDependencies": { diff --git a/scripts/check-coverage.mjs b/scripts/check-coverage.mjs new file mode 100644 index 000000000..a3590729f --- /dev/null +++ b/scripts/check-coverage.mjs @@ -0,0 +1,128 @@ +#!/usr/bin/env node +/** + * Ratcheting coverage gate for the test-app Vitest harness. + * + * Floors live in `coverage-floor.json` at the repo root, not hardcoded in + * this script, so that ordinary feature/spec PRs never need to touch the + * gate itself. That file is a *ratchet*: only ever raise it. Feature/spec + * PRs must NOT edit `coverage-floor.json` directly — that is the exact + * shared-file conflict this mechanism exists to avoid on parallel PRs. When + * coverage genuinely improves, lock the gain in with a dedicated bump: + * + * npm run coverage:bump + * + * which rewrites `coverage-floor.json` to the current measured values. + * Commit that on its own so the shared floor file rarely collides. + * + * Usage: + * node scripts/check-coverage.mjs [path/to/coverage-summary.json] + * node scripts/check-coverage.mjs --bump [path/to/coverage-summary.json] + */ +import { readFileSync, writeFileSync } from "node:fs"; +import { resolve, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; + +const METRICS = ["statements", "branches", "functions", "lines"]; + +const scriptDir = dirname(fileURLToPath(import.meta.url)); +const floorPath = resolve(scriptDir, "..", "coverage-floor.json"); + +const args = process.argv.slice(2); +const bump = args.includes("--bump"); +const summaryArg = args.find((arg) => !arg.startsWith("--")); +const summaryPath = resolve( + summaryArg ?? "test-app/coverage/coverage-summary.json" +); + +let total; +try { + total = JSON.parse(readFileSync(summaryPath, "utf8")).total; +} catch (error) { + console.error(`✖ Could not read coverage summary at ${summaryPath}`); + console.error(` ${error.message}`); + console.error(" Run `npm --prefix test-app test` first to generate it."); + process.exit(1); +} + +let floors; +try { + floors = JSON.parse(readFileSync(floorPath, "utf8")); +} catch (error) { + console.error(`✖ Could not read coverage floors at ${floorPath}`); + console.error(` ${error.message}`); + process.exit(1); +} + +if (bump) { + const next = {}; + let raised = false; + for (const metric of METRICS) { + const pct = total?.[metric]?.pct; + if (typeof pct !== "number") { + console.error(`✖ ${metric}: missing from coverage summary; cannot bump.`); + process.exit(1); + } + const floored = Math.floor(pct); + const rawCurrent = floors[metric]; + // Treat a missing or malformed floor as 0 so a corrupt + // coverage-floor.json can never poison the ratchet with NaN/null values. + const current = Number.isFinite(rawCurrent) ? rawCurrent : 0; + // Ratchet only ever moves up. + next[metric] = Math.max(current, floored); + if (next[metric] > current) { + raised = true; + console.log( + ` ↑ ${metric.padEnd(11)} ${current}% → ${next[metric]}% (measured ${pct.toFixed(2)}%)` + ); + } else { + console.log( + ` = ${metric.padEnd(11)} ${current}% (measured ${pct.toFixed(2)}%)` + ); + } + } + if (!raised) { + console.log( + "\n✓ Floors already at or above current coverage; nothing to bump." + ); + process.exit(0); + } + writeFileSync(floorPath, `${JSON.stringify(next, null, 2)}\n`); + console.log( + `\n✓ Wrote raised floors to ${floorPath}. Commit this change on its own.` + ); + process.exit(0); +} + +const failures = []; +for (const metric of METRICS) { + const floor = floors[metric]; + const pct = total?.[metric]?.pct; + if (!Number.isFinite(floor)) { + failures.push(`${metric}: missing or invalid in coverage-floor.json`); + continue; + } + if (typeof pct !== "number") { + failures.push(`${metric}: missing from coverage summary`); + continue; + } + const status = pct >= floor ? "✓" : "✖"; + const line = ` ${status} ${metric.padEnd(11)} ${pct.toFixed(2)}% (floor ${floor}%)`; + if (pct < floor) { + failures.push(line.trim()); + } + console.log(line); +} + +if (failures.length > 0) { + console.error("\n✖ Coverage gate failed:"); + for (const failure of failures) { + console.error(` ${failure}`); + } + console.error( + "\nCoverage dropped below the committed ratchet in coverage-floor.json.\n" + + "Add tests to restore it — do not lower the floors to go green." + ); + process.exit(1); +} + +console.log("\n✓ Coverage gate passed."); diff --git a/scripts/check-coverage.test.mjs b/scripts/check-coverage.test.mjs new file mode 100644 index 000000000..a4737531a --- /dev/null +++ b/scripts/check-coverage.test.mjs @@ -0,0 +1,208 @@ +#!/usr/bin/env node +/** + * Tests for the coverage gate script (scripts/check-coverage.mjs). + * + * These run on the Node built-in test runner (no extra deps) by invoking the + * script as a child process against temp fixture files, so we exercise the + * real CLI surface (exit codes, --bump, malformed floors) rather than + * internals. + * + * Run: node --test scripts/check-coverage.test.mjs + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, writeFileSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; + +const scriptDir = dirname(fileURLToPath(import.meta.url)); +const script = join(scriptDir, "check-coverage.mjs"); +const floorPath = resolve(scriptDir, "..", "coverage-floor.json"); + +/** Run the gate, returning { status, stdout, stderr }. */ +function run(args) { + try { + const stdout = execFileSync("node", [script, ...args], { + encoding: "utf8", + }); + return { status: 0, stdout, stderr: "" }; + } catch (error) { + return { + status: error.status ?? 1, + stdout: error.stdout?.toString() ?? "", + stderr: error.stderr?.toString() ?? "", + }; + } +} + +function writeSummary(dir, pcts) { + const total = Object.fromEntries( + Object.entries(pcts).map(([k, v]) => [k, { pct: v }]) + ); + const path = join(dir, "summary.json"); + writeFileSync(path, JSON.stringify({ total })); + return path; +} + +function withTempDir(fn) { + const dir = mkdtempSync(join(tmpdir(), "covgate-")); + try { + return fn(dir); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +/** + * Runs `fn` with the real coverage-floor.json swapped for `floors`, restoring + * the original afterwards. The script always reads the repo-root floor file, + * so we back it up rather than parameterise the path. + */ +function withFloors(floors, fn) { + const backup = readFileSync(floorPath, "utf8"); + try { + writeFileSync(floorPath, `${JSON.stringify(floors, null, 2)}\n`); + return fn(); + } finally { + writeFileSync(floorPath, backup); + } +} + +test("passes when coverage meets every floor", () => { + withFloors({ statements: 53, branches: 39, functions: 50, lines: 52 }, () => { + withTempDir((dir) => { + const summary = writeSummary(dir, { + statements: 85, + branches: 86, + functions: 55, + lines: 85, + }); + const { status, stdout } = run([summary]); + assert.equal(status, 0); + assert.match(stdout, /Coverage gate passed/); + }); + }); +}); + +test("fails and names the metric below its floor", () => { + withFloors({ statements: 53, branches: 39, functions: 50, lines: 52 }, () => { + withTempDir((dir) => { + const summary = writeSummary(dir, { + statements: 40, + branches: 86, + functions: 55, + lines: 45, + }); + const { status, stderr } = run([summary]); + assert.equal(status, 1); + assert.match(stderr, /Coverage gate failed/); + assert.match(stderr, /statements/); + assert.match(stderr, /lines/); + assert.doesNotMatch(stderr, /branches\s+8/); + }); + }); +}); + +test("--bump raises floors up to measured coverage", () => { + withFloors({ statements: 53, branches: 39, functions: 50, lines: 52 }, () => { + withTempDir((dir) => { + const summary = writeSummary(dir, { + statements: 85, + branches: 86, + functions: 55, + lines: 85, + }); + const { status } = run(["--bump", summary]); + assert.equal(status, 0); + const written = JSON.parse(readFileSync(floorPath, "utf8")); + assert.deepEqual(written, { + statements: 85, + branches: 86, + functions: 55, + lines: 85, + }); + }); + }); +}); + +test("--bump never lowers a floor (ratchet-only)", () => { + withFloors({ statements: 90, branches: 90, functions: 90, lines: 90 }, () => { + withTempDir((dir) => { + const summary = writeSummary(dir, { + statements: 85, + branches: 86, + functions: 55, + lines: 85, + }); + const { status, stdout } = run(["--bump", summary]); + assert.equal(status, 0); + assert.match(stdout, /nothing to bump/); + const written = JSON.parse(readFileSync(floorPath, "utf8")); + assert.deepEqual(written, { + statements: 90, + branches: 90, + functions: 90, + lines: 90, + }); + }); + }); +}); + +test("a malformed floor (string/null) fails the gate without emitting NaN", () => { + withFloors( + { statements: "53", branches: null, functions: 50, lines: 52 }, + () => { + withTempDir((dir) => { + const summary = writeSummary(dir, { + statements: 85, + branches: 86, + functions: 55, + lines: 85, + }); + const { status, stderr, stdout } = run([summary]); + assert.equal(status, 1); + assert.match(stderr, /statements: missing or invalid/); + assert.match(stderr, /branches: missing or invalid/); + assert.doesNotMatch(stdout + stderr, /NaN/); + }); + } + ); +}); + +test("--bump treats a malformed floor as 0 and writes clean numbers", () => { + withFloors( + { statements: "53", branches: null, functions: 50, lines: 52 }, + () => { + withTempDir((dir) => { + const summary = writeSummary(dir, { + statements: 85, + branches: 86, + functions: 55, + lines: 85, + }); + const { status } = run(["--bump", summary]); + assert.equal(status, 0); + const written = JSON.parse(readFileSync(floorPath, "utf8")); + assert.deepEqual(written, { + statements: 85, + branches: 86, + functions: 55, + lines: 85, + }); + for (const value of Object.values(written)) { + assert.ok(Number.isFinite(value)); + } + }); + } + ); +}); + +test("exits non-zero when the coverage summary is missing", () => { + const { status, stderr } = run([ + join(tmpdir(), "does-not-exist-covgate.json"), + ]); + assert.equal(status, 1); + assert.match(stderr, /Could not read coverage summary/); +}); diff --git a/test-app/vitest.config.mts b/test-app/vitest.config.mts index 2cd1a2de7..b3bb5841e 100644 --- a/test-app/vitest.config.mts +++ b/test-app/vitest.config.mts @@ -77,7 +77,7 @@ export default defineConfig({ ], coverage: { provider: "istanbul", - reporter: ["text-summary", "lcovonly"], + reporter: ["text-summary", "lcovonly", "json-summary"], reportsDirectory: "coverage", // Specs run directly against the library's root `../src` tree instead // of a copy inside test-app/src; without this, Vitest's coverage From dde08f2f4f1f8243fc299d78c853dc5de18d08c7 Mon Sep 17 00:00:00 2001 From: "Frank Pigeon Jr." <4629398+fpigeonjr@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:05:45 -0500 Subject: [PATCH 2/3] Expand AGENTS.md with repo-wide agent guidance Beyond the coverage-gate policy, document: - the raw-source dual-workspace repo shape (root library vs. test-app tooling workspace) and why specs are colocated under root src/ - the frozen consumer-deep-imports.json publish contract and its breaking-change trap when restructuring src/ui-kit - single-spec run command, passWithNoTests rationale, and that the gate scripts' own tests (check-coverage.test.mjs, check-lint-baseline.test.mjs, check-baseline-not-increased.test.mjs) are not currently wired into any CI workflow - the check-baseline-not-increased.mjs guard against hand-raising the ESLint baseline - a map of what each CI workflow does - the dual tsconfig split and test-setup.ts jsdom shims --- AGENTS.md | 45 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fa6e09b75..f5f0df40f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,13 +2,48 @@ Guidance for AI coding agents working in this repository. +## Repo shape + +This is a **raw-source, dual-workspace** Angular library, not a normal Angular CLI app: + +- **Root** (`src/ui-kit/`, `src/formly/`) — the `@gsa-sam/sam-ui-elements` library itself. It is published as raw `.ts`/`.scss` source (no build step, no `dist/`) — consumers compile it themselves. `index.ts` re-exports `src/ui-kit`; `src/formly/index.ts` is a second, separately-imported entry point. +- **`test-app/`** — a _separate_ npm workspace (own `package.json`/`package-lock.json`/`node_modules`) that exists solely to host the Angular build tooling and Vitest test runner needed to exercise the root library's specs. Always `npm ci` (root) **and** `npm ci --prefix test-app` before running anything. +- Specs live next to the source they test under root `src/**/*.spec.ts` (not copied into `test-app`). `test-app/vitest.config.mts` globs both `test-app/src/**/*.spec.ts` and `../src/**/*.spec.ts`, with `allowExternal: true` so istanbul still instruments files outside `test-app`'s own root. + +## Publish contract — do not break silently + +`scripts/validate-publish-package.mjs` (`npm run validate:publish`) enforces that `npm pack` still includes every file listed in `scripts/consumer-deep-imports.json` — a frozen list of deep `@gsa-sam/sam-ui-elements/src/...` import paths used by real downstream consumers (`iae-sam-front-end` and others). **Renaming or moving a `src/ui-kit` file that appears in that list is a breaking change for consumers even though nothing inside this repo imports it.** Check that file before restructuring `src/ui-kit`. + ## Testing -- Library specs (`src/**/*.spec.ts`) run against the `test-app` workspace's Vitest harness (Analog's Angular Vite plugin + `@vitest/coverage-istanbul`). From the repo root: `npm ci && npm ci --prefix test-app && npm --prefix test-app test`. -- Coverage is enforced by a ratcheting floor gate: `coverage-floor.json` at the repo root records the minimum required statements/branches/functions/lines percentages, and `node scripts/check-coverage.mjs` (wired into CI as `npm run coverage:check`) fails the build with a clear per-metric message if measured coverage (from `test-app/coverage/coverage-summary.json`) drops below any floor. -- **`coverage-floor.json` is a ratchet — it only ever moves up, and only via a dedicated bump.** Feature and spec PRs must **not** edit `coverage-floor.json` directly; raising it is a separate, deliberate commit made with `npm run coverage:bump` (rewrites the floor file to the currently measured coverage, never lowering an existing entry). This keeps the shared floor file from becoming a merge-conflict magnet across parallel coverage-improvement PRs. -- The gate script itself is covered by `node --test scripts/check-coverage.test.mjs` (Node's built-in test runner, no extra deps). +- Run specs: `npm ci && npm ci --prefix test-app && npm --prefix test-app test` (Vitest, `--coverage`, istanbul provider, jsdom environment). Config lives in `test-app/vitest.config.mts` — **coverage options must stay in this root config**; a shared/extended base config would have them silently ignored (falls back to Vitest defaults: html/clover/json instead of lcov). +- Run one spec or pattern: `cd test-app && npx vitest run --config vitest.config.mts -t ""`, or pass a spec path directly. +- `it.skip`/`describe.skip`-only files (e.g. `label-wrapper`, `fieldset-wrapper`, the fully-commented-out `alert.spec.ts`) are intentional and contain zero runnable tests — `passWithNoTests: true` is set so these don't fail the run; this preserves prior Karma behavior. +- Coverage is enforced by a ratcheting floor gate: `coverage-floor.json` at the repo root records minimum statements/branches/functions/lines percentages. `node scripts/check-coverage.mjs` (wired into CI as `npm run coverage:check`) fails with a clear per-metric message if measured coverage (from `test-app/coverage/coverage-summary.json`) drops below any floor. +- **`coverage-floor.json` is a ratchet — it only ever moves up, and only via a dedicated bump.** Feature and spec PRs must **not** edit `coverage-floor.json` directly; raising it is a separate, deliberate commit made with `npm run coverage:bump` (rewrites the floor file to the currently measured coverage, never lowering an existing entry). This avoids merge conflicts across parallel coverage-improvement PRs. +- The gate script itself is covered by `node --test scripts/check-coverage.test.mjs` (Node's built-in test runner, no extra deps). This is **not currently wired into any CI workflow** — run it manually after touching `scripts/check-coverage.mjs`. +- Playwright E2E smoke test (`test-app/e2e/`) runs via `npm --prefix test-app run test:e2e`, gated separately in `.github/workflows/e2e.yml`. ## Lint -- `npm run lint` / `npm --prefix test-app run lint` run ESLint. A ratcheting warning-baseline gate (`eslint-baseline.json`, `scripts/check-lint-baseline.mjs`) works the same way as the coverage gate above — only ever lower the baseline via `--bump`, never raise it by hand. +- `npm run lint` (root) / `npm --prefix test-app run lint` run `ng lint` (ESLint) per workspace. +- A ratcheting warning-baseline gate works the same way as coverage: `eslint-baseline.json` (keys `root`, `test-app`) + `scripts/check-lint-baseline.mjs` fail the build if warnings exceed the recorded baseline, or if there are _any_ errors (errors always fail regardless of the baseline). `--bump` only ever lowers the baseline. +- `.github/workflows/lint.yml` additionally runs `scripts/check-baseline-not-increased.mjs`, comparing `eslint-baseline.json` on the PR branch against the base branch — this closes the hole where a contributor could raise the ceiling by hand-editing the JSON in the same PR that adds new warnings. The `--bump` scripts are the only sanctioned way to change either baseline file; same rule as `coverage-floor.json` above. +- Both `scripts/check-lint-baseline.test.mjs` and `scripts/check-baseline-not-increased.test.mjs` exist and pass but, like the coverage gate tests, are **not run in CI** — run `node --test scripts/*.test.mjs` manually when touching any gate script. + +## Formatting + +- `npm run format:check` / `npm run format` (Prettier). Applies to the whole repo — remember to run it on new root-level `scripts/*.mjs` files too, not just `src/`. + +## CI workflows + +- `.github/workflows/ci.yml` — installs both workspaces, runs `npm --prefix test-app test`, then `npm run coverage:check`. +- `.github/workflows/lint.yml` — format check, baseline-not-increased guard, `ng lint` + baseline gate for both workspaces. +- `.github/workflows/e2e.yml` — Playwright smoke test. +- `.github/workflows/publish.yml` — npm Trusted Publisher (OIDC) flow, gated on `validate:publish`; only trigger is a GitHub Release (or a dry-run `workflow_dispatch`). + +## Angular / TypeScript quirks + +- Root `tsconfig.json` targets `es2015`/`commonjs` with `strictNullChecks: false` — this is a legacy config for the raw-source library, distinct from `test-app`'s stricter `tsconfig.spec.json` (`es2022`, TestBed/Vitest types). Don't assume one workspace's TS settings apply to the other. +- `test-app/src/test-setup.ts` shims several `jsdom` gaps Karma never needed to care about: `Element.scrollIntoView`, `Element.innerText` (falls back to `textContent`), and `Element.animate` (Web Animations API used by `@angular/animations`). If a spec hangs or throws on one of these, check this file before adding a per-spec workaround. +- `@gsa-sam/icons` is a `test-app`-only dependency (not a root peerDependency) but is imported by root library source (e.g. `header-next`); it's aliased explicitly in `vitest.config.mts` since specs run directly against root `src/` and Node's module resolution wouldn't otherwise reach `test-app/node_modules`. From 31a12eecf5fb3c8210d80040ddfdd5f1c5455b3f Mon Sep 17 00:00:00 2001 From: "Frank Pigeon Jr." <4629398+fpigeonjr@users.noreply.github.com> Date: Thu, 27 Aug 2026 16:20:24 -0500 Subject: [PATCH 3/3] Address Copilot review: guard floor decreases, preserve fractional coverage - scripts/check-coverage-floor-not-decreased.mjs (new) + tests: CI-only guard comparing coverage-floor.json on the PR branch against the base branch, failing if any metric's floor decreased. Mirrors the existing ESLint baseline guard (check-baseline-not-increased.mjs / .github/workflows/lint.yml). Wired into .github/workflows/ci.yml as a PR-only step, requiring fetch-depth: 0 on checkout to resolve the base SHA's blob. - scripts/check-coverage.mjs: --bump now preserves istanbul's fractional pct value (e.g. 53.56) instead of Math.floor-ing it, so sub-1% gains are no longer silently discarded from the ratchet. Re-seeded coverage-floor.json to the fractional measured values. - scripts/check-coverage.test.mjs: added a fractional-coverage bump test. --- .github/workflows/ci.yml | 14 ++ AGENTS.md | 6 +- coverage-floor.json | 8 +- .../check-coverage-floor-not-decreased.mjs | 83 +++++++++ ...heck-coverage-floor-not-decreased.test.mjs | 163 ++++++++++++++++++ scripts/check-coverage.mjs | 8 +- scripts/check-coverage.test.mjs | 22 +++ 7 files changed, 294 insertions(+), 10 deletions(-) create mode 100644 scripts/check-coverage-floor-not-decreased.mjs create mode 100644 scripts/check-coverage-floor-not-decreased.test.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e436fb5da..ef81ae065 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,10 @@ jobs: steps: - name: Check out repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + # Needed so `git show :coverage-floor.json` below can + # resolve the base branch's blob (mirrors lint.yml's guard step). + fetch-depth: 0 - name: Setup Node uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 @@ -37,6 +41,16 @@ jobs: - name: Install test-app dependencies run: npm ci --prefix test-app + # Guards against coverage-floor.json being lowered directly on this PR + # branch (paired with a coverage regression) rather than earned via + # `npm run coverage:bump`. Mirrors the ESLint baseline guard in + # .github/workflows/lint.yml. + - name: Guard against a lowered coverage floor + if: github.event_name == 'pull_request' + run: | + git show "${{ github.event.pull_request.base.sha }}:coverage-floor.json" > /tmp/base-coverage-floor.json 2>/dev/null || echo '{}' > /tmp/base-coverage-floor.json + node scripts/check-coverage-floor-not-decreased.mjs /tmp/base-coverage-floor.json coverage-floor.json + # Runs the full spec harness with coverage via Vitest (Analog's Angular # Vite plugin + istanbul coverage). `vitest run` exits non-zero on any # spec failure, gating the build. diff --git a/AGENTS.md b/AGENTS.md index f5f0df40f..d4b4a45ec 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,9 +19,9 @@ This is a **raw-source, dual-workspace** Angular library, not a normal Angular C - Run specs: `npm ci && npm ci --prefix test-app && npm --prefix test-app test` (Vitest, `--coverage`, istanbul provider, jsdom environment). Config lives in `test-app/vitest.config.mts` — **coverage options must stay in this root config**; a shared/extended base config would have them silently ignored (falls back to Vitest defaults: html/clover/json instead of lcov). - Run one spec or pattern: `cd test-app && npx vitest run --config vitest.config.mts -t ""`, or pass a spec path directly. - `it.skip`/`describe.skip`-only files (e.g. `label-wrapper`, `fieldset-wrapper`, the fully-commented-out `alert.spec.ts`) are intentional and contain zero runnable tests — `passWithNoTests: true` is set so these don't fail the run; this preserves prior Karma behavior. -- Coverage is enforced by a ratcheting floor gate: `coverage-floor.json` at the repo root records minimum statements/branches/functions/lines percentages. `node scripts/check-coverage.mjs` (wired into CI as `npm run coverage:check`) fails with a clear per-metric message if measured coverage (from `test-app/coverage/coverage-summary.json`) drops below any floor. -- **`coverage-floor.json` is a ratchet — it only ever moves up, and only via a dedicated bump.** Feature and spec PRs must **not** edit `coverage-floor.json` directly; raising it is a separate, deliberate commit made with `npm run coverage:bump` (rewrites the floor file to the currently measured coverage, never lowering an existing entry). This avoids merge conflicts across parallel coverage-improvement PRs. -- The gate script itself is covered by `node --test scripts/check-coverage.test.mjs` (Node's built-in test runner, no extra deps). This is **not currently wired into any CI workflow** — run it manually after touching `scripts/check-coverage.mjs`. +- Coverage is enforced by a ratcheting floor gate: `coverage-floor.json` at the repo root records minimum statements/branches/functions/lines percentages (fractional, e.g. `53.56`). `node scripts/check-coverage.mjs` (wired into CI as `npm run coverage:check`) fails with a clear per-metric message if measured coverage (from `test-app/coverage/coverage-summary.json`) drops below any floor. +- **`coverage-floor.json` is a ratchet — it only ever moves up, and only via a dedicated bump.** Feature and spec PRs must **not** edit `coverage-floor.json` directly; raising it is a separate, deliberate commit made with `npm run coverage:bump` (rewrites the floor file to the currently measured coverage — preserving istanbul's fractional `pct`, not floored to a whole percent — and never lowering an existing entry). `.github/workflows/ci.yml` also runs `scripts/check-coverage-floor-not-decreased.mjs` on PRs, comparing `coverage-floor.json` against the base branch and failing if any metric's floor decreased, so a floor can't be hand-lowered in the same PR that regresses coverage. This mirrors the ESLint baseline guard below. +- The gate scripts are covered by `node --test scripts/check-coverage.test.mjs` and `node --test scripts/check-coverage-floor-not-decreased.test.mjs` (Node's built-in test runner, no extra deps). These are **not currently wired into any CI workflow** — run them manually after touching either script. - Playwright E2E smoke test (`test-app/e2e/`) runs via `npm --prefix test-app run test:e2e`, gated separately in `.github/workflows/e2e.yml`. ## Lint diff --git a/coverage-floor.json b/coverage-floor.json index a13030cad..16fefaba0 100644 --- a/coverage-floor.json +++ b/coverage-floor.json @@ -1,6 +1,6 @@ { - "statements": 53, - "branches": 39, - "functions": 50, - "lines": 52 + "statements": 53.56, + "branches": 39.77, + "functions": 50.31, + "lines": 52.84 } diff --git a/scripts/check-coverage-floor-not-decreased.mjs b/scripts/check-coverage-floor-not-decreased.mjs new file mode 100644 index 000000000..197ecdfdf --- /dev/null +++ b/scripts/check-coverage-floor-not-decreased.mjs @@ -0,0 +1,83 @@ +#!/usr/bin/env node +/** + * Guards against a coverage-floor being lowered directly in + * coverage-floor.json rather than earned via `coverage:bump`. + * + * The per-metric gate (check-coverage.mjs) only evaluates the floors + * committed on the branch being checked, so a contributor could lower a + * floor (e.g. `statements` from 53 to 30) in the same PR that regresses + * coverage, and the gate would still pass. This script closes that hole by + * comparing coverage-floor.json on the PR branch against the base branch's + * version and failing if any metric's floor decreased — the same shape as + * check-baseline-not-increased.mjs for eslint-baseline.json. + * + * New metric keys that don't exist on the base branch are allowed (they + * can't be "decreased" if there's nothing to compare against). Any increase + * or unchanged value is allowed, matching the ratchet-only semantics of + * `coverage:bump`. + * + * Usage: + * node scripts/check-coverage-floor-not-decreased.mjs + */ +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +const [baseArg, headArg] = process.argv.slice(2); + +if (!baseArg || !headArg) { + console.error( + "Usage: node scripts/check-coverage-floor-not-decreased.mjs " + ); + process.exit(1); +} + +function loadFloors(label, path) { + try { + return JSON.parse(readFileSync(resolve(path), "utf8")); + } catch (error) { + console.error(`✖ Could not read ${label} coverage floors at ${path}`); + console.error(` ${error.message}`); + process.exit(1); + } +} + +const base = loadFloors("base-branch", baseArg); +const head = loadFloors("pull-request", headArg); + +let hasDecrease = false; + +for (const metric of Object.keys(head)) { + if (!(metric in base)) { + // New metric entry — nothing to compare against, so it can't be a + // decrease from a previously-trusted value. + continue; + } + + const baseValue = base[metric]; + const headValue = head[metric]; + + if (!Number.isFinite(baseValue) || !Number.isFinite(headValue)) { + console.error( + `✖ ${metric}: invalid coverage-floor value (base: ${baseValue}, head: ${headValue})` + ); + hasDecrease = true; + continue; + } + + if (headValue < baseValue) { + console.error( + `✖ ${metric}: coverage floor decreased ${baseValue}% → ${headValue}%. ` + + "The committed floor can only go up (via `npm run coverage:bump`), never down. " + + "Revert this change to coverage-floor.json." + ); + hasDecrease = true; + } +} + +if (hasDecrease) { + process.exit(1); +} + +console.log( + "✓ coverage-floor.json: no metric's floor decreased vs. the base branch." +); diff --git a/scripts/check-coverage-floor-not-decreased.test.mjs b/scripts/check-coverage-floor-not-decreased.test.mjs new file mode 100644 index 000000000..c5f891192 --- /dev/null +++ b/scripts/check-coverage-floor-not-decreased.test.mjs @@ -0,0 +1,163 @@ +#!/usr/bin/env node +/** + * Tests for scripts/check-coverage-floor-not-decreased.mjs — the CI-only + * guard that compares coverage-floor.json on a PR branch against the base + * branch's version and fails if any metric's floor decreased. + * + * Run: node --test scripts/check-coverage-floor-not-decreased.test.mjs + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, writeFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; + +const scriptDir = dirname(fileURLToPath(import.meta.url)); +const script = join(scriptDir, "check-coverage-floor-not-decreased.mjs"); + +function run(args) { + try { + const stdout = execFileSync("node", [script, ...args], { + encoding: "utf8", + }); + return { status: 0, stdout, stderr: "" }; + } catch (error) { + return { + status: error.status ?? 1, + stdout: error.stdout?.toString() ?? "", + stderr: error.stderr?.toString() ?? "", + }; + } +} + +function withTempDir(fn) { + const dir = mkdtempSync(join(tmpdir(), "floor-guard-")); + try { + return fn(dir); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +} + +function writeFloors(dir, name, data) { + const path = join(dir, name); + writeFileSync(path, JSON.stringify(data)); + return path; +} + +test("passes when the floor is unchanged", () => { + withTempDir((dir) => { + const base = writeFloors(dir, "base.json", { + statements: 53, + branches: 39, + functions: 50, + lines: 52, + }); + const head = writeFloors(dir, "head.json", { + statements: 53, + branches: 39, + functions: 50, + lines: 52, + }); + const { status } = run([base, head]); + assert.equal(status, 0); + }); +}); + +test("passes when a floor is raised", () => { + withTempDir((dir) => { + const base = writeFloors(dir, "base.json", { + statements: 53, + branches: 39, + functions: 50, + lines: 52, + }); + const head = writeFloors(dir, "head.json", { + statements: 60, + branches: 39, + functions: 50, + lines: 52, + }); + const { status } = run([base, head]); + assert.equal(status, 0); + }); +}); + +test("fails when a floor is lowered", () => { + withTempDir((dir) => { + const base = writeFloors(dir, "base.json", { + statements: 53, + branches: 39, + functions: 50, + lines: 52, + }); + const head = writeFloors(dir, "head.json", { + statements: 30, + branches: 39, + functions: 50, + lines: 52, + }); + const { status, stderr } = run([base, head]); + assert.equal(status, 1); + assert.match(stderr, /statements: coverage floor decreased 53% → 30%/); + }); +}); + +test("fails when any of multiple metrics is lowered", () => { + withTempDir((dir) => { + const base = writeFloors(dir, "base.json", { + statements: 53, + branches: 39, + functions: 50, + lines: 52, + }); + const head = writeFloors(dir, "head.json", { + statements: 53, + branches: 39, + functions: 50, + lines: 10, + }); + const { status, stderr } = run([base, head]); + assert.equal(status, 1); + assert.match(stderr, /lines: coverage floor decreased 52% → 10%/); + }); +}); + +test("allows a brand-new metric entry not present on the base branch", () => { + withTempDir((dir) => { + const base = writeFloors(dir, "base.json", { statements: 53 }); + const head = writeFloors(dir, "head.json", { + statements: 53, + "new-metric": 1, + }); + const { status } = run([base, head]); + assert.equal(status, 0); + }); +}); + +test("treats a missing base-branch floor file as an empty floor set", () => { + withTempDir((dir) => { + const base = writeFloors(dir, "base.json", {}); + const head = writeFloors(dir, "head.json", { statements: 53 }); + const { status } = run([base, head]); + assert.equal(status, 0); + }); +}); + +test("fails on a non-numeric floor value", () => { + withTempDir((dir) => { + const base = writeFloors(dir, "base.json", { statements: 53 }); + const head = writeFloors(dir, "head.json", { statements: "fifty-three" }); + const { status, stderr } = run([base, head]); + assert.equal(status, 1); + assert.match(stderr, /invalid coverage-floor value/); + }); +}); + +test("exits non-zero with usage when arguments are missing", () => { + const { status, stderr } = run([]); + assert.equal(status, 1); + assert.match(stderr, /Usage:/); +}); diff --git a/scripts/check-coverage.mjs b/scripts/check-coverage.mjs index a3590729f..96ec4332f 100644 --- a/scripts/check-coverage.mjs +++ b/scripts/check-coverage.mjs @@ -62,13 +62,15 @@ if (bump) { console.error(`✖ ${metric}: missing from coverage summary; cannot bump.`); process.exit(1); } - const floored = Math.floor(pct); const rawCurrent = floors[metric]; // Treat a missing or malformed floor as 0 so a corrupt // coverage-floor.json can never poison the ratchet with NaN/null values. const current = Number.isFinite(rawCurrent) ? rawCurrent : 0; - // Ratchet only ever moves up. - next[metric] = Math.max(current, floored); + // Ratchet only ever moves up. Preserve the fractional measured value + // (istanbul reports two decimal places) rather than flooring it, so a + // sub-1% improvement (e.g. 53.12% -> 53.87%) is still locked in instead + // of silently discarded. + next[metric] = Math.max(current, pct); if (next[metric] > current) { raised = true; console.log( diff --git a/scripts/check-coverage.test.mjs b/scripts/check-coverage.test.mjs index a4737531a..e5cd48e10 100644 --- a/scripts/check-coverage.test.mjs +++ b/scripts/check-coverage.test.mjs @@ -127,6 +127,28 @@ test("--bump raises floors up to measured coverage", () => { }); }); +test("--bump preserves fractional measured coverage instead of flooring it", () => { + withFloors({ statements: 53, branches: 39, functions: 50, lines: 52 }, () => { + withTempDir((dir) => { + const summary = writeSummary(dir, { + statements: 53.87, + branches: 39.12, + functions: 50.99, + lines: 52.5, + }); + const { status } = run(["--bump", summary]); + assert.equal(status, 0); + const written = JSON.parse(readFileSync(floorPath, "utf8")); + assert.deepEqual(written, { + statements: 53.87, + branches: 39.12, + functions: 50.99, + lines: 52.5, + }); + }); + }); +}); + test("--bump never lowers a floor (ratchet-only)", () => { withFloors({ statements: 90, branches: 90, functions: 90, lines: 90 }, () => { withTempDir((dir) => {