From 298515d156a786a8781a8a98f2c1cf6cb1a5b133 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 22 Oct 2026 19:04:20 +0200 Subject: [PATCH] ci: fail the mutation floors step, and scale nightly property runs by a factor The floors step piped scripts/mutation-floors.mjs into tee without pipefail, so it took tee's exit status and a module under its floor could never fail the run. It now runs under bash with pipefail, as do the other steps that pipe into tee, and a guard reads every workflow and fails on a tee pipe without it. The other piped steps were checked: each either validates what the pipe produced or pipes a command that cannot fail. The nightly runs set FC_NUM_RUNS, an absolute count that replaced every property's own: the database backup round trip, which states twelve cases for its cost, ran a hundred times against a timeout sized for twelve. FC_RUNS_FACTOR now multiplies each property's count (five for the unit suite, four for integration), propertyTimeout grows a test's timeout by the same factor, and the backup round trip uses it. --- .github/workflows/mutation.yml | 7 +- .github/workflows/test-nightly.yml | 13 ++- docs/contributing/testing.md | 8 +- src/__tests__/fast-check-runs-factor.test.ts | 39 +++++++ src/__tests__/workflow-pipefail-guard.test.ts | 96 ++++++++++++++++ .../backup-random-account.property.test.ts | 106 +++++++++--------- tests/property/fast-check.ts | 34 +++++- 7 files changed, 242 insertions(+), 61 deletions(-) create mode 100644 src/__tests__/fast-check-runs-factor.test.ts create mode 100644 src/__tests__/workflow-pipefail-guard.test.ts diff --git a/.github/workflows/mutation.yml b/.github/workflows/mutation.yml index cc92b01ad..a49998145 100644 --- a/.github/workflows/mutation.yml +++ b/.github/workflows/mutation.yml @@ -35,8 +35,13 @@ jobs: - name: Run Stryker run: pnpm exec stryker run --concurrency 4 + # Without pipefail the step takes the exit status of `tee`, which is + # always 0, so a module under its floor could never fail the run. - name: Check the per-module floors - run: node scripts/mutation-floors.mjs | tee -a "$GITHUB_STEP_SUMMARY" + shell: bash + run: | + set -o pipefail + node scripts/mutation-floors.mjs | tee -a "$GITHUB_STEP_SUMMARY" - name: Upload the report if: always() diff --git a/.github/workflows/test-nightly.yml b/.github/workflows/test-nightly.yml index b33b9b5c8..88c1e0a27 100644 --- a/.github/workflows/test-nightly.yml +++ b/.github/workflows/test-nightly.yml @@ -58,9 +58,12 @@ jobs: echo "FC_SEED=$(( $(date +%s%N) % 2147483647 ))" >> "$GITHUB_ENV" - name: Unit tests, shuffled + shell: bash env: - FC_NUM_RUNS: "500" + # Five times each property's own case count (tests/property/fast-check.ts). + FC_RUNS_FACTOR: "5" run: | + set -o pipefail echo "Order seed ${TEST_SEED}, property seed ${FC_SEED}" | tee -a "$GITHUB_STEP_SUMMARY" pnpm exec vitest run --sequence.shuffle --sequence.seed="${TEST_SEED}" \ --reporter=default --reporter=json --outputFile.json=unit-report.json @@ -104,14 +107,18 @@ jobs: echo "FC_SEED=$(( ${{ github.run_id }} % 2147483647 ))" >> "$GITHUB_ENV" - name: Integration tests, shuffled + shell: bash env: HEALTHLOG_INTEGRATION_WORKERS: "3" - FC_NUM_RUNS: "100" + # Four times each property's own case count; a property's timeout + # grows by the same factor (`propertyTimeout`). + FC_RUNS_FACTOR: "4" # Quoted for the reason given in integration.yml. API_TOKEN_HMAC_KEY: "dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd" ENCRYPTION_KEY: "0000000000000000000000000000000000000000000000000000000000000000" SESSION_SECRET: "integration-tests" run: | + set -o pipefail echo "Order seed ${TEST_SEED}, property seed ${FC_SEED}" | tee -a "$GITHUB_STEP_SUMMARY" pnpm exec vitest run --config vitest.integration.config.mts \ --shard=${{ matrix.shard }}/4 \ @@ -150,7 +157,9 @@ jobs: path: reports - name: Regenerate + shell: bash run: | + set -o pipefail shopt -s nullglob integration=(reports/nightly-integration-report-*/*.json) unit=(reports/nightly-unit-report/*.json) diff --git a/docs/contributing/testing.md b/docs/contributing/testing.md index 100e9a043..fd0a7d434 100644 --- a/docs/contributing/testing.md +++ b/docs/contributing/testing.md @@ -164,9 +164,11 @@ file needs isolation for a reason the classifier cannot see, put a `fast-check` properties live in `*.property.test.ts` files and take their parameters from `tests/property/fast-check.ts`. Pull requests run every property with a fixed seed, so a run is reproducible on any machine. The -nightly run uses a fresh `FC_SEED` and more cases (`FC_NUM_RUNS`); a failure -prints the seed and the shrunk counterexample, and -`FC_SEED= pnpm test ` replays it. +nightly run uses a fresh `FC_SEED` and multiplies every property's own case +count by `FC_RUNS_FACTOR`; a failure prints the seed and the shrunk +counterexample, and `FC_SEED= pnpm test ` replays it. A property +whose cases are expensive states a small count and passes +`propertyTimeout(baseMs)` as its test timeout, which grows by the same factor. Write a property when the code has a law: an inverse (encrypt and decrypt, a unit and its inverse, a wall clock and its instant), an invariant (a rate diff --git a/src/__tests__/fast-check-runs-factor.test.ts b/src/__tests__/fast-check-runs-factor.test.ts new file mode 100644 index 000000000..a9fa55ed2 --- /dev/null +++ b/src/__tests__/fast-check-runs-factor.test.ts @@ -0,0 +1,39 @@ +/** + * The nightly property runs multiply each property's own case count instead + * of replacing it. A nightly count of 100 for every property ran the backup + * round trip, which states twelve cases for its cost, eight times over + * against a timeout sized for twelve. + */ +import { afterEach, describe, expect, it, vi } from "vitest"; + +async function loadWith(factor: string | undefined) { + vi.resetModules(); + if (factor === undefined) vi.stubEnv("FC_RUNS_FACTOR", ""); + else vi.stubEnv("FC_RUNS_FACTOR", factor); + return import("../../tests/property/fast-check"); +} + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +describe("FC_RUNS_FACTOR", () => { + it("keeps each property's own count on a pull request", async () => { + const { propertyParameters, propertyTimeout } = await loadWith(undefined); + expect(propertyParameters(12).numRuns).toBe(12); + expect(propertyParameters().numRuns).toBe(100); + expect(propertyTimeout(60_000)).toBe(60_000); + }); + + it("multiplies each property's count and the timeout it was given", async () => { + const { propertyParameters, propertyTimeout } = await loadWith("4"); + expect(propertyParameters(12).numRuns).toBe(48); + expect(propertyParameters(300).numRuns).toBe(1200); + expect(propertyTimeout(60_000)).toBe(240_000); + }); + + it("ignores a value that is not a positive number", async () => { + const { propertyParameters } = await loadWith("-3"); + expect(propertyParameters(12).numRuns).toBe(12); + }); +}); diff --git a/src/__tests__/workflow-pipefail-guard.test.ts b/src/__tests__/workflow-pipefail-guard.test.ts new file mode 100644 index 000000000..a7fa32b27 --- /dev/null +++ b/src/__tests__/workflow-pipefail-guard.test.ts @@ -0,0 +1,96 @@ +/** + * A workflow step that pipes a command into `tee` runs with pipefail. + * + * GitHub runs a `run:` script with `bash -e {0}` unless the step names a + * shell, and without pipefail a pipeline's exit status is its last command's. + * `tee` always succeeds, so `node scripts/mutation-floors.mjs | tee -a + * "$GITHUB_STEP_SUMMARY"` passed whatever the floors said. A step that names + * `shell: bash` runs as `bash --noprofile --norc -eo pipefail {0}`; this guard + * asks for that and for `set -o pipefail` in the script as well, so the + * intent is visible where the pipe is. + */ +import { readdirSync, readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { describe, expect, it } from "vitest"; +import YAML from "yaml"; + +const WORKFLOWS = join(process.cwd(), ".github", "workflows"); + +interface Step { + name?: string; + run?: string; + shell?: string; +} + +/** Whether a script pipes something into `tee` on a line that is not a comment. */ +function pipesIntoTee(script: string): boolean { + return script + .split("\n") + .some((line) => !/^\s*#/.test(line) && /[^|]\|\s*tee\b/.test(line)); +} + +/** The steps of a workflow that pipe into `tee` without pipefail. */ +function offenders(file: string, text: string): string[] { + const doc = YAML.parse(text) as { + defaults?: { run?: { shell?: string } }; + jobs?: Record< + string, + { defaults?: { run?: { shell?: string } }; steps?: Step[] } + >; + }; + const out: string[] = []; + for (const [jobName, job] of Object.entries(doc.jobs ?? {})) { + (job.steps ?? []).forEach((step, index) => { + if (!step.run || !pipesIntoTee(step.run)) return; + const shell = + step.shell ?? job.defaults?.run?.shell ?? doc.defaults?.run?.shell; + const ok = shell === "bash" && /\bset -o pipefail\b/.test(step.run); + if (!ok) out.push(`${file} ${jobName} #${index} ${step.name ?? ""}`); + }); + } + return out; +} + +function workflowFiles(): string[] { + return readdirSync(WORKFLOWS).filter((f) => /\.ya?ml$/.test(f)); +} + +describe("workflow steps that pipe into tee", () => { + it("reads the workflows it claims to check", () => { + expect(workflowFiles().length).toBeGreaterThan(10); + const teeSteps = workflowFiles().filter((f) => + pipesIntoTee(readFileSync(join(WORKFLOWS, f), "utf8")), + ); + expect(teeSteps.length).toBeGreaterThan(0); + }); + + it("run with pipefail", () => { + const found = workflowFiles().flatMap((f) => + offenders(f, readFileSync(join(WORKFLOWS, f), "utf8")), + ); + expect(found).toEqual([]); + }); + + it("catches the shape it guards against", () => { + const bad = [ + "jobs:", + " check:", + " steps:", + " - name: floors", + ' run: node scripts/mutation-floors.mjs | tee -a "$GITHUB_STEP_SUMMARY"', + ].join("\n"); + expect(offenders("bad.yml", bad)).toEqual(["bad.yml check #0 floors"]); + const good = [ + "jobs:", + " check:", + " steps:", + " - name: floors", + " shell: bash", + " run: |", + " set -o pipefail", + ' node scripts/mutation-floors.mjs | tee -a "$GITHUB_STEP_SUMMARY"', + ].join("\n"); + expect(offenders("good.yml", good)).toEqual([]); + }); +}); diff --git a/tests/integration/backup-random-account.property.test.ts b/tests/integration/backup-random-account.property.test.ts index f88553000..33f7c3040 100644 --- a/tests/integration/backup-random-account.property.test.ts +++ b/tests/integration/backup-random-account.property.test.ts @@ -21,7 +21,7 @@ import { legacyStreamedBlobFrom } from "@/__tests__/helpers/legacy-backup-blob"; import type { Prisma } from "@/generated/prisma/client"; import { streamFullBackupJson } from "@/lib/export/full-backup-stream"; -import { assertAsyncProperty } from "../property/fast-check"; +import { assertAsyncProperty, propertyTimeout } from "../property/fast-check"; import { callRoute } from "../kit/call-route"; import { makeAdmin, makeSession, makeUser } from "../kit/factories"; import { POST as restoreBackup } from "./restore-job-driver"; @@ -264,56 +264,62 @@ beforeEach(async () => { }); describe("a generated account", () => { - it("comes back from a disaster-recovery backup exactly as it left", async () => { - await assertAsyncProperty( - fc.asyncProperty(accountArb, async (account) => { - await truncateAllTables(prisma); - await seed(account); - const before = await dump(); + it( + "comes back from a disaster-recovery backup exactly as it left", + async () => { + await assertAsyncProperty( + fc.asyncProperty(accountArb, async (account) => { + await truncateAllTables(prisma); + await seed(account); + const before = await dump(); - let json = ""; - await streamFullBackupJson( - prisma, - OWNER_ID, - (chunk) => { - json += chunk; - }, - { - purpose: "disaster-recovery", - exportedAt: new Date("2026-10-01T00:00:00.000Z"), - }, - ); + let json = ""; + await streamFullBackupJson( + prisma, + OWNER_ID, + (chunk) => { + json += chunk; + }, + { + purpose: "disaster-recovery", + exportedAt: new Date("2026-10-01T00:00:00.000Z"), + }, + ); - await prisma.user.delete({ where: { id: OWNER_ID } }); - await makeUser({ id: OWNER_ID }); - const backup = await prisma.dataBackup.create({ - data: { - userId: OWNER_ID, - type: "RANDOM_ACCOUNT_ROUND_TRIP", - data: await legacyStreamedBlobFrom(async (write) => { - await write(json); - }), - }, - }); - const operator = await makeSession(await makeAdmin()); - const restored = await callRoute(restoreBackup, { - method: "POST", - route: "/api/admin/backups/{id}/restore", - params: { id: backup.id }, - body: { confirm: "RESTORE" }, - auth: operator, - contract: { - skip: "the restore driver answers 200 with the job's report in place of the route's queued 202", - }, - }); - expect( - restored.status, - JSON.stringify(restored.body).slice(0, 300), - ).toBe(200); + await prisma.user.delete({ where: { id: OWNER_ID } }); + await makeUser({ id: OWNER_ID }); + const backup = await prisma.dataBackup.create({ + data: { + userId: OWNER_ID, + type: "RANDOM_ACCOUNT_ROUND_TRIP", + data: await legacyStreamedBlobFrom(async (write) => { + await write(json); + }), + }, + }); + const operator = await makeSession(await makeAdmin()); + const restored = await callRoute(restoreBackup, { + method: "POST", + route: "/api/admin/backups/{id}/restore", + params: { id: backup.id }, + body: { confirm: "RESTORE" }, + auth: operator, + contract: { + skip: "the restore driver answers 200 with the job's report in place of the route's queued 202", + }, + }); + expect( + restored.status, + JSON.stringify(restored.body).slice(0, 300), + ).toBe(200); - expect(await dump()).toEqual(before); - }), - 12, - ); - }); + expect(await dump()).toEqual(before); + }), + 12, + ); + // Twelve round trips take a few seconds; the nightly factor multiplies + // both the cases and this budget. + }, + propertyTimeout(60_000), + ); }); diff --git a/tests/property/fast-check.ts b/tests/property/fast-check.ts index f5dd632da..283198c96 100644 --- a/tests/property/fast-check.ts +++ b/tests/property/fast-check.ts @@ -8,8 +8,13 @@ * prints the seed and the shrunk counterexample, and * `FC_SEED= pnpm test ` replays it exactly. * - * `FC_NUM_RUNS` scales the number of generated cases (the nightly run uses - * more). The default keeps every property file well under a second or two. + * `FC_RUNS_FACTOR` multiplies every property's own case count (the nightly + * run sets it above 1). A factor rather than a count: each property states + * the number of cases its cost allows, a cheap law a few hundred and a + * database round trip a dozen, and a nightly count applied to all of them + * alike ran the round trip a hundred times against a timeout sized for + * twelve. `propertyTimeout` grows a test's own timeout by the same factor. + * The defaults keep every property file well under a second or two. */ import fc from "fast-check"; @@ -21,22 +26,41 @@ function positiveInt(raw: string | undefined): number | null { return Number.isInteger(value) && value > 0 ? value : null; } +function positiveNumber(raw: string | undefined): number | null { + if (!raw) return null; + const value = Number(raw); + return Number.isFinite(value) && value > 0 ? value : null; +} + export const PROPERTY_SEED = positiveInt(process.env.FC_SEED) ?? DEFAULT_SEED; -const SCALE = positiveInt(process.env.FC_NUM_RUNS); +/** The multiplier on every property's case count; 1 on a pull request. */ +export const RUNS_FACTOR = positiveNumber(process.env.FC_RUNS_FACTOR) ?? 1; -/** Parameters for `fc.assert`; `numRuns` is the PR-run default per property. */ +/** + * Parameters for `fc.assert`; `numRuns` is the property's own case count on a + * pull request, multiplied by `FC_RUNS_FACTOR` when it is set. + */ export function propertyParameters( numRuns = 100, overrides: fc.Parameters = {}, ): fc.Parameters { return { seed: PROPERTY_SEED, - numRuns: SCALE ?? numRuns, + numRuns: Math.max(1, Math.round(numRuns * RUNS_FACTOR)), ...overrides, }; } +/** + * A test timeout for a property whose cases are expensive, grown by the same + * factor as its case count, so a nightly run with more cases does not run + * into a timeout sized for the pull-request count. + */ +export function propertyTimeout(baseMs: number): number { + return Math.ceil(baseMs * Math.max(1, RUNS_FACTOR)); +} + /** Run a property with the shared parameters. */ export function assertProperty( property: fc.IProperty,