ci: fail at the end rather than part way through, so the scan still runs (#174)

`sonarqube.yml` already had a documented design for this: run every suite, produce coverage, scan, summarise, then fail at the end from recorded step outcomes. The comments on the gate spell it out and #142 fixed it once already. The integration suite was never wired into it — no `id`, no `continue-on-error`, and absent from the gate, where the unit and end-to-end suites had all three.

So it was the one suite whose failure aborted the job. On every push since #154 started, step 9 failed and steps 10 through 15 were skipped, which means there has been no SonarQube analysis at all for the duration — not a degraded one, none. The end-to-end suite has not run in CI either, which is separately why #116 cannot be verified: the step that would demonstrate its fix is skipped rather than failing.

Fixing only that step would have left the same shape in four other places, so every step from the first suite to the scan is now guarded and named in the gate: starting the backend, installing browsers, merging frontend coverage, and the scan itself, which until now took the summaries down with it. The preconditions before the suites — checkout, installs, build checks, migrations — still fail hard, because when they fail there is genuinely nothing to analyse.

The job still fails. It fails at the end, having produced everything it could.

The integration suite also gains the summary the other two already had. It was the only suite without one, so the failure this workflow has been stuck on presented as 36 assertion errors about categories and price filters rather than as a count — #154 records how expensive that misdirection was to read. `summarize-jest.js` already takes a label, so this is reuse.

`tests/unit/workflowGate.test.ts` asserts the pairing that makes the design work: a step carries `continue-on-error` so it cannot abort the job, and the gate names it so it can still fail the job. Both halves are needed and nothing connected them, which is how this happened — and the other direction is worse, since a step guarded but unnamed cannot fail the job at all. The test checks the invariant rather than a list of names, for the same reason `composeEnvironment.test.ts` reads the real list rather than a copy. Verified by mutation: dropping the integration suite from the gate, un-guarding it, and removing `always()` each fail it.

Confirmed by running the new flag combination rather than assuming it: the integration suite writes `integration-results.json`, the summariser reads it and exits 0, and both that file and `coverage/integration/lcov.info` are still written when the suite fails — which is what makes scanning with a failing suite produce real coverage rather than a fabricated regression.

Closes #174

.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

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([]);
+  });
+});