Skip to content

Commit bd25e0e

Browse files
committed
test(integration): no skipped tests, and fail the run if any appear
A skipped test reads exactly like a passing one. Every silent hole this suite has found took that shape, and the skips it was itself carrying hid one more. **The bug the skips were hiding.** `dbVariant()` inferred the database from the presence of `PGRST_URL`. The new Drizzle `db=supabase` cell needs no PostgREST and left it unset, so the variant reported `postgres`: EQL installed WITHOUT `--supabase`, the role grants were never applied, and the grants test quietly did not run. The suite passed because Drizzle connects as `postgres`, never as `anon`. The variant is now explicit (`CS_IT_DB_VARIANT`) in both workflows, and the inference survives only as a documented fallback for a hand-run suite. **The skips are gone**, all ten: - The eight per-family "defers <domain>" notices were `it.skip`. They asserted nothing. `domainsForFamily` already excludes deferred domains, and `test-kit-families.test.ts` asserts — as a PASSING test — that the excluded set is exactly the nine block-ORE domains, each with a reason. - The two harness gates were `it.runIf`. They are now single tests that assert both branches: on Supabase the three roles exist and hold the `eql_v3` and `eql_v3_internal` grants; on plain Postgres those roles do not exist at all, which is a fact worth stating and makes the variant claim falsifiable. PostgREST is asserted on its own axis (`PGRST_URL` set) rather than on the variant — conflating "is this Supabase" with "is PostgREST up" is what made `dbVariant()` lie. **And a guard so they cannot come back.** `no-skips-reporter.ts` fails the integration run if any test is skipped, naming each one. Verified: adding a single `it.skip` exits 1. Scoped to the integration config. The unit suites still carry the `LIVE_*` gates and their 16 skips; working those out is a separate change. Verified on all three CI cells, each 0 skipped, 0 failed: drizzle / postgres 619 passed drizzle / supabase 619 passed supabase / supabase 665 passed
1 parent c78b60f commit bd25e0e

7 files changed

Lines changed: 124 additions & 30 deletions

File tree

.github/workflows/integration-drizzle.yml

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,13 @@ jobs:
5959
include:
6060
- db: postgres
6161
database-url: postgres://cipherstash:password@localhost:55432/cipherstash
62+
pgrest-url: ''
6263
- db: supabase
6364
database-url: postgres://postgres:password@localhost:55433/postgres
65+
# The supabase compose file starts PostgREST anyway. Drizzle does not
66+
# use it, but supplying the URL lets the harness assert the `anon`
67+
# path on the database it is actually running against.
68+
pgrest-url: http://localhost:55430
6469

6570
env:
6671
CS_WORKSPACE_CRN: ${{ secrets.CS_WORKSPACE_CRN }}
@@ -70,8 +75,14 @@ jobs:
7075
# Job-level env, not a `.env` file: `dotenv/config` does not override an
7176
# already-set `process.env`, so these win and no secret is written to disk.
7277
DATABASE_URL: ${{ matrix.database-url }}
73-
# Scoped to the Drizzle suites plus the adapter-agnostic ones. `PGRST_URL`
74-
# is deliberately unset: a Supabase suite scoped here would throw, not skip.
78+
PGRST_URL: ${{ matrix.pgrest-url }}
79+
# EXPLICIT, never inferred. The variant decides whether EQL is installed
80+
# with `--supabase` (and therefore whether the role grants are applied).
81+
# Inferring it from `PGRST_URL` reported `postgres` for this job's Supabase
82+
# cell and silently skipped the grants.
83+
CS_IT_DB_VARIANT: ${{ matrix.db }}
84+
# Scoped to the Drizzle suites plus the adapter-agnostic ones. The Supabase
85+
# adapter suites are not run here; they have their own job.
7586
CS_IT_SUITE: >-
7687
integration/drizzle-v3/**/*.integration.test.ts,
7788
integration/harness.integration.test.ts,

.github/workflows/integration-supabase.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ jobs:
6262
# as they do on a real Supabase project.
6363
DATABASE_URL: postgres://postgres:password@localhost:55433/postgres
6464
PGRST_URL: http://localhost:55430
65+
# EXPLICIT, never inferred from `PGRST_URL` — see `dbVariant()`.
66+
CS_IT_DB_VARIANT: ${{ matrix.db }}
6567
# Scoped to the suites this database serves. The Drizzle suites talk to
6668
# plain Postgres and get their own job; the harness and bloom suites are
6769
# adapter-agnostic and run here because a database is already up.

packages/stack/integration/harness.integration.test.ts

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -66,26 +66,60 @@ it('installs the ORE opclass only when the connecting role is a superuser', asyn
6666
}
6767
})
6868

69-
it.runIf(dbVariant() === 'supabase')(
70-
'grants anon USAGE on both eql_v3 and eql_v3_internal',
71-
async () => {
69+
/**
70+
* Both database variants are asserted, neither is skipped.
71+
*
72+
* A skipped test reads exactly like a passing one, and the two skips that used to
73+
* live here hid a real bug: `dbVariant()` inferred `postgres` for the Drizzle job
74+
* running against `supabase/postgres`, so EQL installed without `--supabase`, the
75+
* grants were never applied, and the grants test quietly did not run.
76+
*
77+
* So assert the whole truth: on a Supabase database the roles exist and hold the
78+
* grants; on a plain one they do not exist at all.
79+
*/
80+
it('applies the anon grants on Supabase, and has no such roles on plain Postgres', async () => {
81+
const [roles] = await sql<{ count: string }[]>`
82+
SELECT count(*)::text AS count FROM pg_roles
83+
WHERE rolname IN ('anon', 'authenticated', 'service_role')
84+
`
85+
86+
if (dbVariant() === 'supabase') {
87+
expect(Number(roles?.count)).toBe(3)
88+
7289
// `eql_v3_internal` is load-bearing: the SECURITY INVOKER extractors resolve
7390
// it with the CALLER's privileges, so without this grant every encrypted
7491
// filter fails for `anon` with "permission denied for schema".
75-
const [row] = await sql<{ eql_v3: boolean; internal: boolean }[]>`
92+
const [privs] = await sql<{ eql_v3: boolean; internal: boolean }[]>`
7693
SELECT has_schema_privilege('anon', 'eql_v3', 'USAGE') AS eql_v3,
7794
has_schema_privilege('anon', 'eql_v3_internal', 'USAGE') AS internal
7895
`
96+
expect(privs).toEqual({ eql_v3: true, internal: true })
97+
return
98+
}
7999

80-
expect(row).toEqual({ eql_v3: true, internal: true })
81-
},
82-
)
100+
// Plain Postgres: the Supabase roles do not exist, so there is nothing to
101+
// grant. Asserting their ABSENCE is what makes the variant claim falsifiable —
102+
// if this database ever grew them, the `--supabase` install path would have to
103+
// run here too.
104+
expect(Number(roles?.count)).toBe(0)
105+
})
83106

84-
it.runIf(dbVariant() === 'supabase')(
85-
'serves PostgREST as anon through the authenticator role',
86-
async () => {
87-
const response = await fetch(`${pgrestUrl()}/`)
107+
/**
108+
* PostgREST is asserted on its own axis, not on the variant. The Drizzle job runs
109+
* against the Supabase database and does not need PostgREST; conflating "is this
110+
* Supabase" with "is PostgREST up" is precisely what made `dbVariant()` lie.
111+
*/
112+
it('serves PostgREST when configured, and is not configured otherwise', async () => {
113+
const url = process.env['PGRST_URL']
88114

89-
expect(response.status).toBe(200)
90-
},
91-
)
115+
if (!url) {
116+
// Only the plain-Postgres compose file omits PostgREST. A Supabase database
117+
// with no `PGRST_URL` means the job forgot to pass it.
118+
expect(dbVariant()).toBe('postgres')
119+
return
120+
}
121+
122+
const response = await fetch(`${pgrestUrl()}/`)
123+
124+
expect(response.status).toBe(200)
125+
})
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
import type { Reporter, TestModule } from 'vitest/node'
2+
3+
/**
4+
* Fail the integration run if any test was skipped.
5+
*
6+
* A skipped test reads exactly like a passing one. Every silent hole this suite
7+
* has found took that shape: `contains` never ran on `text_match` because its
8+
* needle was `''`; the non-ASCII ORE needle test had zero cases after the OPE
9+
* re-pin; the Supabase grants check quietly did not run for the Drizzle job
10+
* because `dbVariant()` mis-inferred the database. In each case a green run
11+
* reported coverage it did not have.
12+
*
13+
* So the integration suites carry no `skip`, no `todo`, no `runIf` and no
14+
* `skipIf`. Environmental differences are asserted rather than skipped: a plain
15+
* Postgres has no `anon` role, and that is a fact worth stating. Where a
16+
* behaviour genuinely cannot be exercised — the block-ORE domains cannot hold
17+
* data on managed Postgres — the domain is excluded from the matrix by the
18+
* catalog's `deferred` field, and a separate, PASSING test asserts that the
19+
* excluded set is exactly what it should be.
20+
*
21+
* The unit suites still skip (the `LIVE_*` gates), so this is scoped to the
22+
* integration config rather than imposed repo-wide.
23+
*/
24+
export default class NoSkipsReporter implements Reporter {
25+
private readonly skipped: string[] = []
26+
27+
onTestModuleEnd(testModule: TestModule): void {
28+
for (const test of Array.from(testModule.children.allTests())) {
29+
const result = test.result()
30+
if (result.state === 'skipped') this.skipped.push(test.fullName)
31+
}
32+
}
33+
34+
onTestRunEnd(): void {
35+
if (this.skipped.length === 0) return
36+
37+
const list = this.skipped.map((name) => ` - ${name}`).join('\n')
38+
console.error(
39+
`\n${this.skipped.length} test(s) were SKIPPED. The integration suites must not skip:\n` +
40+
`${list}\n\n` +
41+
'A skipped test reads exactly like a passing one. Assert the environmental\n' +
42+
"difference instead, or exclude the domain via the catalog's `deferred`\n" +
43+
'field and assert the excluded set separately.\n',
44+
)
45+
process.exitCode = 1
46+
}
47+
}

packages/stack/integration/vitest.config.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,5 +80,9 @@ export default defineConfig({
8080
* only place to do it without changing product logging behaviour.
8181
*/
8282
silent: 'passed-only',
83+
84+
// Fail the run if anything is skipped. A skipped test reads exactly like a
85+
// passing one, and every silent hole this suite has found took that shape.
86+
reporters: ['default', resolve(__dirname, 'no-skips-reporter.ts')],
8387
},
8488
})

packages/test-kit/src/install.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,15 @@ export type DbVariant = 'postgres' | 'supabase'
1515
/**
1616
* Which database the integration suite is pointed at.
1717
*
18-
* `PGRST_URL` is only ever set for the Supabase variant — the Drizzle suite
19-
* talks straight to Postgres — so its presence identifies the variant without a
20-
* second environment variable to keep in sync. `CS_IT_DB_VARIANT` overrides,
21-
* for the case where that stops being true.
18+
* Set `CS_IT_DB_VARIANT` explicitly. The `PGRST_URL` fallback exists only for a
19+
* developer running one suite by hand, and it is a guess: it once inferred
20+
* `postgres` for the Drizzle job running against `supabase/postgres`, because
21+
* that job needs no PostgREST and leaves the variable unset. The consequence was
22+
* silent — EQL installed without `--supabase`, so the role grants were never
23+
* applied and the grants test skipped, while the suite passed because Drizzle
24+
* connects as `postgres` rather than as `anon`.
25+
*
26+
* The variant decides how EQL is installed. It must not be a guess in CI.
2227
*/
2328
export function dbVariant(): DbVariant {
2429
const explicit = process.env['CS_IT_DB_VARIANT']

packages/test-kit/src/run-family-suite.ts

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,7 @@
11
import { afterAll, beforeAll, describe, expect, it } from 'vitest'
22
import type { IntegrationAdapter } from './adapter.ts'
33
import type { DomainSpec } from './catalog.ts'
4-
import {
5-
deferredForFamily,
6-
domainsForFamily,
7-
type FamilyDomain,
8-
type FamilyName,
9-
} from './families.ts'
4+
import { domainsForFamily, type FamilyName } from './families.ts'
105
import { negativeOps, type Plain, positiveOps, type QueryOp } from './ops.ts'
116
import {
127
comparePlain,
@@ -148,7 +143,6 @@ export function runFamilySuite(
148143
): void {
149144
const adapter = makeAdapter()
150145
const domains = domainsForFamily(family)
151-
const deferred = deferredForFamily(family)
152146

153147
// Unique per run so a crashed run never leaves a table that shadows the next.
154148
const runId = Math.random().toString(36).slice(2, 8)
@@ -179,9 +173,6 @@ export function runFamilySuite(
179173
await adapter.teardown()
180174
})
181175

182-
if (deferred.length > 0) {
183-
it.skip(`defers ${deferred.map((d) => d.bare).join(', ')}: ${deferred[0]?.reason}`, () => {})
184-
}
185176

186177
for (const domain of domains) {
187178
describe(domain.bare, () => {

0 commit comments

Comments
 (0)