diff --git a/AGENTS.md b/AGENTS.md index 48359d3..b264581 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,6 +15,9 @@ `bun run test:db:up` first. - If you added a batch of tests, hand them to a second reviewer before calling the work done. See "A batch of new tests gets an adversarial review, by somebody else" below. +- If you added an end-to-end spec, run it under CI conditions too, not only locally. Hosts + differ between the two, and a `page.route()` glob that matches nothing passes locally and + tests nothing anywhere. ## Testing @@ -47,9 +50,9 @@ For an API route, three cases are the minimum: an unauthenticated caller, a call signed in but not authorized, and the happy path. Assert the database row, not only the status code: a refused DELETE has to leave the row present. -### Two ways a test can be worthless +### Three ways a test can be worthless -Both have been found in this repo, so they are worth naming. +All three have been found in this repo, so they are worth naming. 1. **A test that cannot fail.** Before you finish, name the specific mutation of the production code your test would catch. If you cannot name one, delete the test. When it @@ -58,7 +61,16 @@ Both have been found in this repo, so they are worth naming. function looks up means deleting an entry from that constant also deletes its own test case. Write expected values by hand as literals. -A third variant is specific to `tests/api/auth-matrix.test.ts`: a route that refuses a +3. **A selector, route pattern or filter that matches nothing.** This one is specific to + `tests/e2e/`, and it fails in the worst direction: an injected failure that never gets + injected leaves the feature working, and the test then waits for an error message that + was never going to appear. Both halves need to be asserted, so count what you + intercepted and assert the count, the way + `tests/e2e/failure-recovery.spec.ts` does with `refusedPuts`. Never hardcode a host in a + `page.route()` glob either: CI publishes MinIO on localhost and the compose stack calls + it `minio-test`, and the pattern that matched locally silently matched nothing on CI. + +A fourth variant is specific to `tests/api/auth-matrix.test.ts`: a route that refuses a malformed request before it reaches its access check produces an entry that passes whether or not the guard exists. The suite catches it by requiring a 401 or a 403 rather than merely a non-2xx, and a 404 counts as suspicious rather than as a refusal, since a fixture diff --git a/tests/e2e/failure-recovery.spec.ts b/tests/e2e/failure-recovery.spec.ts index 4943756..39a91b7 100644 --- a/tests/e2e/failure-recovery.spec.ts +++ b/tests/e2e/failure-recovery.spec.ts @@ -19,6 +19,20 @@ import { db } from '@/lib/db'; const SAMPLE_VIDEO = path.join(REPO_ROOT, 'tests', 'fixtures', 'sample.mp4'); +/** + * Every request to object storage, for `page.route`. + * + * Derived from R2_ENDPOINT rather than hardcoded, because the host differs by + * environment and a pattern that matches nothing fails silently in the worst + * possible way: the PUT succeeds, the upload works, and the test that claims to + * inject a storage failure just waits for an error message that will never + * come. That is exactly what happened on CI, where MinIO is published on + * localhost, while locally it is the `minio-test` compose service. + * + * The fallback is the compose hostname, matching playwright.config.ts. + */ +const STORAGE_GLOB = `${process.env.R2_ENDPOINT ?? 'http://minio-test:9000'}/**`; + test.setTimeout(120_000); /** A video with an active version, cheap enough to make several of. */ @@ -54,11 +68,13 @@ test('an upload that fails at the storage PUT leaves the form up and creates not // Only the bytes are refused. The presign (`r2-init`) and everything else the // app serves still work, so the failure is exactly the one this test claims: // object storage rejected the upload halfway through. - await page.route('http://minio-test:9000/**', async (route) => { + let refusedPuts = 0; + await page.route(STORAGE_GLOB, async (route) => { if (route.request().method() !== 'PUT') { await route.continue(); return; } + refusedPuts += 1; await route.fulfill({ status: 500, contentType: 'text/plain', body: 'storage is down' }); }); @@ -77,6 +93,12 @@ test('an upload that fails at the storage PUT leaves the form up and creates not timeout: 60_000, }); + // The interception really happened. Without this, a STORAGE_GLOB that matches + // nothing turns this test into an assertion about an error message that some + // unrelated failure happened to produce, and the diagnostic points at the + // message rather than at the pattern. + expect(refusedPuts, `no PUT to ${STORAGE_GLOB} was intercepted`).toBeGreaterThan(0); + // Still on the form, so the file list and the title survive for a retry. await expect(page).toHaveURL(new RegExp(`/projects/${project.id}/videos/new$`)); await expect(page.getByLabel('Title')).toHaveValue('Doomed Upload'); @@ -88,7 +110,7 @@ test('an upload that fails at the storage PUT leaves the form up and creates not // Positive control: with storage healthy the very same steps succeed, so the // assertions above are about the injected failure and not about the form // being broken. - await page.unroute('http://minio-test:9000/**'); + await page.unroute(STORAGE_GLOB); await page.getByRole('button', { name: 'Add Video', exact: true }).click(); await expect(page).toHaveURL(new RegExp(`/projects/${project.id}$`), { timeout: 90_000 }); await expect(page.getByRole('heading', { name: 'Doomed Upload', level: 3 })).toBeVisible();