Files
redefined-designs/backend/tests/unit/composeEnvironment.test.ts
T
bermudalamb f32913ef51
Linting / lint (pull_request) Successful in 1m57s
SonarQube Analysis / sonarqube (pull_request) Failing after 5m3s
refactor: turn on noUncheckedIndexedAccess in both workspaces (#101)
Indexing an array now yields `T | undefined`, which is what it always did — the compiler simply says so. Enabled in backend/tsconfig.json and frontend/tsconfig.json, and in tsconfig.sonar.json alongside it so the drift guard stays satisfied.

The sequencing this issue insisted on was right, and the numbers show why. Before #159 typed the query results, `rows[0]` was `any` and `any` indexes to `any`: the flag would have found close to nothing and the count would have changed completely afterwards. With the rows typed it finds 57 — 50 in the backend, 4 in the frontend, 3 in the Playwright suite — and they fall into three kinds.

Thirty are `rows[0]` after a `rows.length` guard. TypeScript cannot connect the two, and rewriting them as `const [row] = rows; if (!row) …` makes the guard and the use the same check, which is better code independently of the flag.

Ten are rows a statement guarantees — `INSERT … RETURNING`, or a lookup for an id the session middleware has already matched. These get `requireRow(rows, what)`, a new helper in db.ts that throws naming the query. A thrown error rather than a non-null assertion: if the assumption is ever wrong, an assertion hands `undefined` to the next line and fails somewhere unrelated, whereas this fails at the query and says which. asyncRoute turns it into a 500, which is the right answer for "the database did not do what the statement says it does". It also states the assumption once instead of ten times.

The rest is ordinary indexing the compiler cannot prove: a regex capture group that the pattern guarantees, `split('+')[0]`, a modulo kept in range, `hasOwnProperty` failing to narrow an index signature, and Express typing route params as an index signature so `req.params.itemId` is `string | undefined` on a route that cannot match without it.

One correction to this issue's premise, which matters for what it was expected to find. The body says "in a handful it does not guard at all", and the unguarded-500 risk it describes was not found. Every `rows[0]` either sits behind a length check or behind a statement that guarantees a row. What the flag actually bought was the ten places where that guarantee was real but unstated, and those now say so.

Two changes worth calling out because they are not mechanical. TAG_COLORS is typed `[string, ...string[]]` in both copies rather than `as const` — the first attempt used `as const`, which narrowed the elements to literals and broke adminTags, so the annotation keeps `string` while telling the compiler index 0 exists. And the filter drawer's slider falls back to the bounds it was given rather than to null, because null there reads as "no filter" and would widen the results rather than leave them unchanged.

Test files needed changes too, since ts-jest compiles them against the same config: a regex destructure in the compose guard, and ten `mock.calls[0][0]` reads where the surrounding assertions already establish the call happened.

Verified: tsc clean over backend, frontend src and the Playwright suite; unit 254/254; integration 238/238; frontend build clean; lint unchanged in both workspaces.

Closes #101
2026-08-24 15:25:09 -05:00

165 lines
7.4 KiB
TypeScript

import { readFileSync, readdirSync } from 'fs';
import path from 'path';
import { ALWAYS_REQUIRED, validateEnv } from '../../src/envValidation';
/**
* The guard for #107 and #118.
*
* #107 was not the missing variable — it was that nothing connected two files.
* envValidation.ts gained a required variable and docker-compose.qa.yml did
* not set it, and nothing noticed until the container refused to boot on a
* deploy. CI passed throughout, because CI sets its own environment and never
* reads the compose file.
*
* So this reads the real list from the validator rather than a copy. A copy
* would pass forever while the next added variable went unguarded in precisely
* the same way. For the same reason the per-file check below runs `validateEnv`
* itself rather than restating its rules: the point is that each deploying file
* satisfies the validator, and any restatement here is one more thing to drift.
*
* #118 was the other half. This used to read QA's file alone, while production
* ran from a Portainer stack outside the repository that nothing could check.
* That was worse than an even gap, because a green test implied a coverage it
* did not have: UPLOADS_DIR is in ALWAYS_REQUIRED, QA set it, production did
* not, and on 2026-08-23 production refused to boot for exactly that reason —
* while the variable was set in Portainer, where stack variables are
* substituted into the compose file rather than handed to the container, so a
* variable with no line in the file never reaches the app at all.
*/
const REPO_ROOT = path.resolve(__dirname, '..', '..', '..');
/**
* Every deployment this repository describes, and what is true of each.
*
* Registered rather than discovered blindly, because the environments differ on
* purpose — QA runs in demo mode with no PayPal credentials and a mail
* allowlist, production is the reverse of all three — so "the files agree with
* each other" would be the wrong assertion. What each file has to satisfy is
* the validator, plus the handful of things below that are properties of that
* environment rather than of the code.
*
* A compose file at the repository root that is not registered here fails the
* last test in this file. That is deliberate: adding an environment should
* force the decision about what is true of it, rather than silently inheriting
* whatever the loop happened to assert.
*/
const DEPLOYMENTS = [
{
file: 'docker-compose.qa.yml',
demoMode: 'true',
// The entire safety property of QA: delivery is restricted to named
// recipients, so a run against a database full of test fixtures cannot
// email a real customer. Asserted because the compose file claims it must
// not depend on somebody remembering to set something.
requiresMailAllowlist: true
},
{
file: 'docker-compose.prod.yml',
demoMode: 'false',
// Deliberately unrestricted. Production has to be able to reach real
// customers, and it is the one environment where that is correct.
requiresMailAllowlist: false
}
] as const;
// Only real environment entries — `- NAME=value` at an indented list position.
// A mention inside a comment cannot match, because a comment line starts with #.
function environmentEntries(source: string): Map<string, string> {
const entries = new Map<string, string>();
// The split pattern tolerates carriage returns. A checkout with CRLF line
// endings, which is every fresh clone on Windows, otherwise leaves a stray
// carriage return that the end-of-line anchor below cannot match, and every
// assertion in this file then silently finds nothing at all. That is exactly
// the failure the "parsed some entries" case exists to catch, and it is how
// this guard was found to be broken.
for (const line of source.split(/\r?\n/)) {
const match = /^\s+- ([A-Z_0-9]+)=(.*)$/.exec(line);
// Both groups are non-optional in the pattern, so a match always has them —
// but RegExpExecArray cannot say so, and #101 made the compiler insist.
const [, name, value] = match ?? [];
if (name !== undefined && value !== undefined) {
entries.set(name, value.trim());
}
}
return entries;
}
describe.each(DEPLOYMENTS)('$file provides everything the app requires to boot', (deployment) => {
const compose = readFileSync(path.join(REPO_ROOT, deployment.file), 'utf8');
const entries = environmentEntries(compose);
it('parsed some environment entries at all', () => {
// Guards the guard: a regex that matched nothing would make every
// assertion below vacuously true.
expect(entries.size).toBeGreaterThan(5);
});
it.each([...ALWAYS_REQUIRED])('sets %s', (name) => {
expect(entries.has(name)).toBe(true);
});
/**
* The strongest form of this check, and the one that cannot drift: hand the
* file's own entries to the real validator and require it to be satisfied.
*
* An interpolated `${SECRET}` counts as present, which is correct — what is
* being checked is that the *line exists*, since that is what decides whether
* the value reaches the container. Whether the stack variable behind it is
* set is a different failure, and one the app already reports clearly at boot.
*
* Errors only. Production warns about MAIL_ALLOWLIST by design, and silencing
* that warning would remove the notice that this environment can email real
* customers.
*/
it('satisfies validateEnv, the same check the container runs at boot', () => {
const { errors } = validateEnv(Object.fromEntries(entries));
expect(errors).toEqual([]);
});
it(`sets DEMO_MODE to ${deployment.demoMode}`, () => {
expect(entries.get('DEMO_MODE')).toBe(deployment.demoMode);
});
// The reason UPLOADS_DIR is hardcoded rather than taken from a stack
// variable is that it has to agree with the volume mapping. That claim is
// only worth making if something checks it — and its absence from production
// is what stopped that stack booting.
it('points UPLOADS_DIR at the directory the uploads volume is mounted on', () => {
const uploadsDir = entries.get('UPLOADS_DIR');
expect(uploadsDir).toBeTruthy();
expect(compose).toContain(`:${uploadsDir}`);
});
// Interpolated rather than hardcoded, so the secret itself never enters the
// repository. Present as a reference is what matters; an unset stack variable
// resolves to empty, which the validator and the gate both read as "off".
it('references ADMIN_GATE_SECRET from the stack rather than holding a value', () => {
expect(entries.get('ADMIN_GATE_SECRET')).toBe('${ADMIN_GATE_SECRET}');
});
if (deployment.requiresMailAllowlist) {
it('restricts delivery with MAIL_ALLOWLIST', () => {
expect(entries.get('MAIL_ALLOWLIST')).toBeTruthy();
});
}
});
describe('every deployment in the repository is under this guard', () => {
// The failure #118 was about was a file nothing read. A new environment added
// as a compose file and not registered above would reproduce it exactly, so
// the omission has to fail rather than pass quietly.
//
// Scoped to the repository root: backend/docker-compose.test.yml is a bare
// Postgres for the integration suite, with no app service and nothing here to
// say about it.
it('registers every root-level compose file in DEPLOYMENTS', () => {
const found = readdirSync(REPO_ROOT).filter(
(name) => /^docker-compose\..+\.ya?ml$/.test(name)
);
const registered = DEPLOYMENTS.map((d) => d.file);
expect(found.slice().sort()).toEqual(registered.slice().sort());
});
});