diff --git a/.gitea/workflows/sonarqube.yml b/.gitea/workflows/sonarqube.yml index 01143d4..11f4141 100755 --- a/.gitea/workflows/sonarqube.yml +++ b/.gitea/workflows/sonarqube.yml @@ -106,11 +106,24 @@ jobs: # Covers everything in src/routes, which the unit suite does not touch — # without this the backend reports around 11% rather than the ~73% it # actually has. + # + # Guarded like the other two suites. It was the only one that was not, so + # it was the only one whose failure aborted the job — taking the scan, the + # end-to-end run and the coverage merge with it. That is what #154 has + # cost on every push since it started: not a degraded analysis, none at + # all. See #174. - name: Backend integration tests with coverage - run: npm run test:integration:cov + id: integration + continue-on-error: true + run: npm run test:integration:cov -- --json --outputFile=integration-results.json working-directory: backend + # Guarded because the scan does not depend on it. A backend that will not + # start fails the end-to-end run below on its own, and the gate records + # both — there is no reason for it to also cost the analysis. - name: Start backend for the end-to-end run + id: backend + continue-on-error: true run: | mkdir -p /tmp/redefined-uploads node dist/server.js > /tmp/backend.log 2>&1 & @@ -126,7 +139,10 @@ jobs: exit 1 working-directory: backend + # A network fetch, and so the least interesting way to lose an analysis. - name: Install Playwright browsers + id: browsers + continue-on-error: true run: npx playwright install --with-deps chromium working-directory: frontend @@ -147,17 +163,32 @@ jobs: # never fire and the log that explains an end-to-end failure would go # unprinted precisely when it is wanted. - name: Backend log - if: steps.e2e.outcome == 'failure' + if: steps.e2e.outcome == 'failure' || steps.backend.outcome == 'failure' run: cat /tmp/backend.log # Fails when nothing was collected rather than writing an empty report. An # uninstrumented dev server lets every test pass while gathering nothing, # and the resulting 0% reads as "the tests stopped covering things". - name: Merge frontend coverage + id: coverage + continue-on-error: true run: npm run coverage:report working-directory: frontend + # Guarded so that a scanner error does not take the summaries below with + # it. The gate still records it, so a failed scan fails the job. + # + # If a coverage report is missing — the end-to-end run collecting nothing, + # say — this scans anyway and SonarQube reports those files as uncovered, + # which reads as a regression rather than as a missing input. Accepted: + # jest still writes coverage for a suite whose tests fail, so the failure + # actually occurring produces all three reports; there is no + # sonar.qualitygate.wait, so a degraded run marks the dashboard and is + # overwritten by the next good one rather than blocking anything; and the + # job fails regardless, so no run in this state reads as clean. - name: SonarQube Scan + id: scan + continue-on-error: true uses: sonarsource/sonarqube-scan-action@v4 env: SONAR_HOST_URL: ${{ secrets.SONAR_HOST_URL }} @@ -170,15 +201,28 @@ jobs: if: always() run: node scripts/summarize-jest.js backend/unit-results.json "Backend Unit Test" + # The suite this workflow has been failing on for weeks, and the only one + # that had no summary — so it presented as 36 assertion errors about + # categories and price filters rather than as a count. #154 records how + # expensive that misdirection was to read. + - name: Summarize integration tests + if: always() + run: node scripts/summarize-jest.js backend/integration-results.json "Backend Integration Test" + - name: Summarize end-to-end tests if: always() run: node scripts/summarize-playwright.js frontend/playwright-results.json - # Last, so a failing suite still produces coverage, a scan and both - # summaries first. Without this step continue-on-error above would turn a + # Last, so a failing step still produces coverage, a scan and every + # summary first. Without this step continue-on-error above would turn a # failing suite into a passing job, which is the one way this change could # do real damage. # + # Every guarded step is listed. That is the invariant — a step carrying + # continue-on-error and missing from here cannot fail the job at all — and + # tests/unit/workflowGate.test.ts asserts it, because the integration + # suite going unlisted is exactly how #174 happened. + # # always() is load-bearing. A step whose `if:` omits it still implicitly # requires every previous step to have succeeded, so this was skipped in # exactly the case it exists for: the summarise step above used to exit 1 @@ -186,6 +230,15 @@ jobs: # code. The job then reported its failure under a name that describes # summarising rather than testing. The summarisers exit 0 now; this is what # fails the run. See #142. - - name: Fail if either suite failed - if: always() && (steps.unit.outcome == 'failure' || steps.e2e.outcome == 'failure') + - name: Fail if any guarded step failed + if: >- + always() && ( + steps.unit.outcome == 'failure' || + steps.integration.outcome == 'failure' || + steps.backend.outcome == 'failure' || + steps.browsers.outcome == 'failure' || + steps.e2e.outcome == 'failure' || + steps.coverage.outcome == 'failure' || + steps.scan.outcome == 'failure' + ) run: exit 1 diff --git a/backend/tests/unit/workflowGate.test.ts b/backend/tests/unit/workflowGate.test.ts new file mode 100644 index 0000000..f8062d5 --- /dev/null +++ b/backend/tests/unit/workflowGate.test.ts @@ -0,0 +1,183 @@ +import { readFileSync } from 'fs'; +import path from 'path'; + +/** + * The guard for #174. + * + * `sonarqube.yml` runs every suite to completion and then fails at the end from + * recorded step outcomes, so that a failing suite still produces coverage, a + * scan and its summaries. Two halves make that work: a step carries + * `continue-on-error: true` so it cannot abort the job, and the final gate names + * it so that it can still fail the job. + * + * Both halves are needed and nothing connected them. The integration suite had + * neither, so it aborted the job where the other two suites did not — and for + * the whole of #154 that meant no SonarQube analysis at all, on any commit. + * Worse is the other direction: a step guarded but left out of the gate cannot + * fail the job *ever*, which turns a broken suite into a green run. + * + * So this asserts the pairing rather than a list of step names. A list would + * pass forever while the next step added went unguarded in exactly the same way, + * which is the mistake `composeEnvironment.test.ts` exists to prevent for + * docker-compose and the one repeated here. + */ + +const REPO_ROOT = path.resolve(__dirname, '..', '..', '..'); +const WORKFLOW = path.join(REPO_ROOT, '.gitea', 'workflows', 'sonarqube.yml'); + +interface Step { + name: string; + id: string | null; + guarded: boolean; +} + +/** + * Parsed by hand rather than with a YAML library, because there is no YAML + * dependency in this workspace and adding one to read four fields would be the + * larger change. The shape being read is fixed and shallow. + * + * Indentation is what separates a step's own keys from anything nested inside + * its `run:` block — a `continue-on-error:` written inside a shell script would + * be more deeply indented and is not matched. + */ +function parseSteps(source: string): Step[] { + const steps: Step[] = []; + let current: Step | null = null; + + // Tolerates carriage returns: a checkout with CRLF endings would otherwise + // leave a stray return that the end anchors cannot match, and every + // assertion below would silently find nothing. That is what the "parsed + // some steps at all" case exists to catch. + for (const line of source.split(/\r?\n/)) { + const named = /^ {6}- name: (.+?)\s*$/.exec(line); + if (named?.[1] !== undefined) { + current = { name: named[1], id: null, guarded: false }; + steps.push(current); + continue; + } + if (!current) continue; + + const id = /^ {8}id: (\S+)\s*$/.exec(line); + if (id?.[1] !== undefined) current.id = id[1]; + + if (/^ {8}continue-on-error: true\s*$/.test(line)) current.guarded = true; + } + + return steps; +} + +/** + * The gate's `if:` expression, as one string. + * + * Folded across lines in the file for readability, so this collects everything + * indented under the `if:` rather than reading a single line. + */ +function gateCondition(source: string): string { + const lines = source.split(/\r?\n/); + const start = lines.findIndex((line) => /^ {6}- name: Fail if any guarded step failed\s*$/.test(line)); + if (start === -1) return ''; + + const collected: string[] = []; + let inside = false; + for (const line of lines.slice(start + 1)) { + if (/^ {8}if:/.test(line)) { + inside = true; + collected.push(line); + continue; + } + if (!inside) continue; + // The next key at step level ends the folded block. + if (/^ {8}\S/.test(line)) break; + collected.push(line); + } + return collected.join(' '); +} + +const source = readFileSync(WORKFLOW, 'utf8'); +const steps = parseSteps(source); +const condition = gateCondition(source); + +describe('sonarqube.yml fails at the end rather than part way through', () => { + // Guards the guard: a parser that matched nothing would make every assertion + // below vacuously true. + it('parsed the workflow at all', () => { + expect(steps.length).toBeGreaterThan(10); + expect(condition).toContain('always()'); + }); + + it('has a gate step', () => { + expect(condition).not.toBe(''); + }); + + /** + * `always()` is load-bearing, and its absence has bitten once already. A step + * whose `if:` omits it still implicitly requires every previous step to have + * succeeded, which would leave this gate as dead code in exactly the case it + * exists for. See #142. + */ + it('evaluates the gate even after a failure', () => { + expect(condition).toContain('always()'); + }); + + it('gives every guarded step an id, or the gate cannot name it', () => { + const anonymous = steps.filter((step) => step.guarded && step.id === null); + expect(anonymous.map((step) => step.name)).toEqual([]); + }); + + /** + * The half that turns a broken suite into a green run. A guarded step missing + * from the gate cannot fail the job at all. + */ + it('names every guarded step in the gate', () => { + const guarded = steps.filter((step) => step.guarded && step.id !== null); + expect(guarded.length).toBeGreaterThan(0); + + const unnamed = guarded.filter((step) => !condition.includes(`steps.${step.id}.outcome`)); + expect(unnamed.map((step) => step.name)).toEqual([]); + }); + + /** + * The other direction: a gate naming a step that no longer exists reads as + * coverage it does not have, and the expression silently evaluates that clause + * to nothing. + */ + it('names nothing in the gate that is not a step', () => { + const ids = new Set(steps.map((step) => step.id).filter((id): id is string => id !== null)); + const named = [...condition.matchAll(/steps\.([A-Za-z0-9_-]+)\.outcome/g)].map((match) => match[1]); + expect(named.length).toBeGreaterThan(0); + + const unknown = named.filter((id) => id !== undefined && !ids.has(id)); + expect(unknown).toEqual([]); + }); + + /** + * Named individually, unlike the structural rules above, because these three + * are the reason the design exists. Un-guarding any of them would restore the + * behaviour #174 fixed — and it was the integration suite, absent from this + * list for as long as it existed, that actually did it. + */ + it.each(['unit', 'integration', 'e2e'])('runs the %s suite to completion', (id) => { + const step = steps.find((candidate) => candidate.id === id); + expect(step).toBeDefined(); + expect(step?.guarded).toBe(true); + }); + + /** + * The scan is the output this whole arrangement protects. Anything between the + * first suite and it that can abort the job takes it down, which is what #174 + * was. + */ + it('guards every step between the first suite and the scan', () => { + const first = steps.findIndex((step) => step.id === 'unit'); + const scan = steps.findIndex((step) => step.id === 'scan'); + expect(first).toBeGreaterThan(-1); + expect(scan).toBeGreaterThan(first); + + // `if:`-only steps run conditionally and cannot abort the job on their own, + // so they need no guard. Backend log is the one such step in this range. + const unguarded = steps + .slice(first, scan + 1) + .filter((step) => !step.guarded && step.name !== 'Backend log'); + expect(unguarded.map((step) => step.name)).toEqual([]); + }); +});