From 6cfe381d59f02ade707cdf5700fa179d3e3c34cc Mon Sep 17 00:00:00 2001 From: Musa Musa Date: Thu, 27 Aug 2026 10:16:08 +0100 Subject: [PATCH 1/2] fix: make the SQL integration suite runnable, and honest when it is not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixing the path in #9 only got the file found. Actually running it surfaced two things. The describe-level gate is `URL_ ? describe : describe.skip`, which tests whether RESILIX_TEST_DATABASE_URL is SET — but `pnpm test:integration` always sets it, defaulting to localhost:5459. So the script can never distinguish "asked for integration tests" from "has a database", and without one the failure was a bare AggregateError with two node:net frames and no indication of what to do. It now names the URL it tried, carries the docker run line, and points at `pnpm test` to skip. Then it was run for real, against PostgreSQL 14.15, which had not happened since the reorg: 13 of 13 pass. That is a second data point for classifySql — the mappings were captured on PostgreSQL 16 and hold on 14 too. One trap found while setting that up, now in CONTRIBUTING: initdb --auth=trust makes the "bad password -> transient" case unfalsifiable, because a bad password does not fail. It reports `NO THROW` until pg_hba.conf uses scram-sha-256 for 127.0.0.1. Twelve of thirteen passing with the thirteenth failing for a harness reason is exactly the shape that gets mistaken for a library bug. --- .changeset/sql-integration-runnable.md | 16 ++++++++++++++++ CONTRIBUTING.md | 19 ++++++++++++++++++- src/scenarios/sql-integration.test.ts | 23 ++++++++++++++++++++++- 3 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 .changeset/sql-integration-runnable.md diff --git a/.changeset/sql-integration-runnable.md b/.changeset/sql-integration-runnable.md new file mode 100644 index 0000000..ba3c06b --- /dev/null +++ b/.changeset/sql-integration-runnable.md @@ -0,0 +1,16 @@ +--- +"resilix": patch +--- + +`pnpm test:integration` is runnable again, and says what to do when it is not. + +The path fix landed separately; this is what running it revealed. The describe-level gate only +checks whether `RESILIX_TEST_DATABASE_URL` is *set*, and the script always sets it with a +localhost default — so without a database the suite failed with a bare `AggregateError` and two +`node:net` stack frames. It now names the URL it tried, gives the `docker run` line, and points at +`pnpm test` for skipping. + +Verified end to end against a real PostgreSQL 14.15: **13 of 13 pass**, so `classifySql`'s +mappings hold on 14 as well as the 16 they were captured against. One caveat now documented: +`initdb --auth=trust` makes the "bad password" case unfalsifiable, and that test reports +`NO THROW` until `pg_hba.conf` uses `scram-sha-256` for `127.0.0.1`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a288f80..52e8785 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -84,7 +84,7 @@ pnpm verify # everything CI runs — do this before pushing pnpm test # unit tests pnpm test:coverage # with thresholds pnpm test:compat # opossum's OWN suite against our shim (362/362) -pnpm test:integration # classifySql against a real Postgres; needs RESILIX_TEST_DATABASE_URL +pnpm test:integration # classifySql against a real Postgres (see below) pnpm lint # biome, including the restricted-globals rule pnpm typecheck pnpm build @@ -101,6 +101,23 @@ New to the codebase? [Reading the code](https://resilix.js.org/guide/reading-the maps every source file to the paper or chapter behind it, and gives a dependency-ordered path through the ~3,500 lines. +### Running the SQL integration tests + +They need a reachable Postgres; `pnpm test:integration` supplies a default URL pointing at +port 5459, so it fails rather than skips if nothing is listening. The error message carries this +command, but for reference: + +```bash +docker run --rm -d -p 5459:5432 \ + -e POSTGRES_PASSWORD=resilix -e POSTGRES_DB=resilix_test postgres:16 +``` + +Without Docker, a local Postgres works — note that `initdb --auth=trust` makes the +"bad password" case unfalsifiable, so set a password and use `scram-sha-256` for +`127.0.0.1` in `pg_hba.conf` or that one test fails with `NO THROW`. + +`pnpm test` skips the whole file, so the default workflow needs no database at all. + Specs live in `docs/specs/` and are written **before** the code they describe — the adaptive limiter and the retry/throttling work both gated on theirs. If you are adding a policy, the spec comes first, and it should carry the provenance of every default it proposes. diff --git a/src/scenarios/sql-integration.test.ts b/src/scenarios/sql-integration.test.ts index 81a49c1..20519f4 100644 --- a/src/scenarios/sql-integration.test.ts +++ b/src/scenarios/sql-integration.test.ts @@ -33,7 +33,28 @@ suite("classifySql against a real PostgreSQL server", () => { beforeAll(async () => { pg = await import("pg"); client = new pg.Client({ connectionString: URL_ }); - await client.connect(); + // `pnpm test:integration` always sets RESILIX_TEST_DATABASE_URL, defaulting to localhost, so + // the describe-level gate cannot tell "asked for integration tests" from "has a database". + // Without this the failure is a bare AggregateError with two node:net frames and no hint of + // what to do about it. + try { + await client.connect(); + } catch (cause) { + throw new Error( + [ + `No reachable Postgres at ${URL_}`, + "", + " Start one: docker run --rm -d -p 5459:5432 \\", + " -e POSTGRES_PASSWORD=resilix -e POSTGRES_DB=resilix_test postgres:16", + " Or: pnpm test (skips these entirely)", + "", + "These verify classifySql against real pg and Prisma errors, so they cannot run", + "against fixtures — a captured fixture keeps passing after a driver changes shape,", + "which is the whole reason this file exists.", + ].join("\n"), + { cause }, + ); + } await client.query("drop table if exists resilix_probe"); await client.query( "create table resilix_probe (id int primary key, n int not null check (n > 0))", From ac79fda9b3ce0304d7df400e02bcaf697beb0052 Mon Sep 17 00:00:00 2001 From: Musa Musa Date: Thu, 27 Aug 2026 10:21:41 +0100 Subject: [PATCH 2/2] fix: pin the opossum suite, and make a stall say why MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adding test:compat to CI in #9 turned an environment-sensitive check into a required one, and it failed on the first PR after: 170 of 362, with test.js reporting STALLED in CI while passing locally in 6 seconds. Two problems behind that, both worth fixing regardless of which one it was. The suite tracked opossum's main branch. .opossum-compat/ is not cached in CI, so every run fetched afresh and the README's "362 of 362" was being measured against whatever opossum had merged that morning. The expected value of a required check must not be something upstream can change without us. It is now pinned to decbedf6, and bumping it is a deliberate act. And a stall reported only the word STALLED, discarding the child's stdout, stderr and exit status. That is why the CI failure could not be diagnosed and had to be guessed at — first as a 60s timeout, which the 16-second total run already ruled out. It now prints the exit code, the signal, and the last twelve lines the child produced, or says explicitly that there was none. 362 of 362 locally at the pin. Whether CI agrees is now something the output will explain rather than something to infer. --- .changeset/pin-opossum-suite.md | 14 ++++++++++++++ scripts/opossum-compat.mjs | 22 +++++++++++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-) create mode 100644 .changeset/pin-opossum-suite.md diff --git a/.changeset/pin-opossum-suite.md b/.changeset/pin-opossum-suite.md new file mode 100644 index 0000000..39faaaf --- /dev/null +++ b/.changeset/pin-opossum-suite.md @@ -0,0 +1,14 @@ +--- +"resilix": patch +--- + +The opossum compatibility suite is now pinned to a commit instead of tracking `main`. + +`.opossum-compat/` is not cached in CI, so every run re-fetched the suite — meaning the README's +"362 of 362" was measured against whatever opossum had merged that morning. A required check whose +expected value can change upstream without us is not a check. Bumping the pin is now a deliberate +act, with the new total. + +A stalled file also reports its exit status and the tail of its output. It previously printed only +the word `STALLED` with the child's output discarded, so when `test.js` produced no TAP summary on +a CI runner while passing locally, there was nothing to diagnose from. diff --git a/scripts/opossum-compat.mjs b/scripts/opossum-compat.mjs index 0c26b80..96818ad 100644 --- a/scripts/opossum-compat.mjs +++ b/scripts/opossum-compat.mjs @@ -22,7 +22,11 @@ import { fileURLToPath } from "node:url"; const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const HARNESS = join(ROOT, ".opossum-compat"); -const REF = process.env.OPOSSUM_REF ?? "main"; +// PINNED, not "main". CI re-fetches on every run because .opossum-compat/ is not cached, so +// tracking a branch meant the README's "362 of 362" was measured against whatever opossum had +// merged that morning — a required check whose expected value upstream can change without us. +// Bump this deliberately, with the new total, and never as a drive-by. +const REF = process.env.OPOSSUM_REF ?? "decbedf63d7815049233e544dcb351590ff0c84e"; /** Their suite, minus the files that unit-test opossum's private modules. */ const TESTS = [ @@ -102,12 +106,12 @@ const run = (file) => child.stderr.on("data", (d) => { out += d; }); - child.on("close", () => { + child.on("close", (code, signal) => { clearTimeout(kill); const pass = Number(out.match(/^# pass\s+(\d+)/m)?.[1] ?? 0); const fail = Number(out.match(/^# fail\s+(\d+)/m)?.[1] ?? 0); const stalled = !/^# pass/m.test(out); - done({ file, pass, fail, stalled, out }); + done({ file, pass, fail, stalled, out, code, signal }); }); }); @@ -131,6 +135,7 @@ const main = async () => { let fail = 0; console.log(`\n${"FILE".padEnd(32)}${"PASS".padStart(6)}${"FAIL".padStart(6)}`); console.log("-".repeat(46)); + const stalls = results.filter((r) => r.stalled); for (const r of results.sort((a, b) => a.file.localeCompare(b.file))) { pass += r.pass; fail += r.fail; @@ -145,6 +150,17 @@ const main = async () => { `${"TOTAL".padEnd(32)}${String(pass).padStart(6)}${String(fail).padStart(6)} ${pct}%\n`, ); + // A STALLED file used to report only the word STALLED, with the child's output discarded — so + // when test.js stalled on a CI runner while passing locally, there was nothing to diagnose from + // and the cause had to be guessed at. Print the exit status and the tail of what it actually + // said. + for (const r of stalls) { + console.error(`\n${r.file} produced no TAP summary (exit=${r.code} signal=${r.signal})`); + const tail = r.out.trimEnd().split("\n").slice(-12); + for (const line of tail) console.error(` ${line}`); + if (r.out.trim() === "") console.error(" (no output at all)"); + } + console.log("Excluded, and why:"); for (const [f, why] of Object.entries(EXCLUDED)) console.log(` ${f.padEnd(20)} ${why}`);