Merge pull request 'ci: fail at the end rather than part way through, so the scan still runs (#174)' (#176) from feature/174-graceful-sonarqube-failure into main

Reviewed-on: #176

.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
+      # Last, so a failing step still produces coverage, a scan and every
-      # summaries first. Without this step continue-on-error above would turn a
+      # 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
+      - name: Fail if any guarded step failed
-        if: always() && (steps.unit.outcome == 'failure' || steps.e2e.outcome == 'failure')
+        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([]);
+  });
+});