From d5e6c9164ae040745ecfef33ad8a05a432cbf29f Mon Sep 17 00:00:00 2001 From: Andrii Kohut Date: Tue, 18 Aug 2026 14:57:22 +0200 Subject: [PATCH] docs: a contributor following CONTRIBUTING ran 64% of the tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cloned the repository fresh and did exactly what the guide says. It reports success: Tasks: 28 successful 573 passed, 608 skipped Every test touching Postgres is guarded by skipIf(!hasDb), so with no DATABASE_URL more than half the suite quietly does not run — scoring, tenant isolation, erasure, the queue. A contributor could change any of it, see every task green, and have exercised none of what they changed. For a project whose whole subject is checks that look green without proving anything, that is the wrong first experience to hand someone. The guide now gives the command that reaches the database compose already started, with REQUIRE_DB=1 so an unreachable database fails loudly rather than skipping. With it: 895 passed, 0 skipped. It also documents `turbo run test --force`, which runs packages concurrently and uncached — how the Prisma upgrade was caught writing to the wrong schema — and says format:check is the step most often forgotten. The changeset list was wrong in both directions: it named `ai`, which is private, and omitted the Vitest and Jest reporters, which publish. Someone following it would either write a changeset nobody needs or miss one that gates a release. A test now checks the list against the workspace. That test first flagged `db`, which the section names as an example of something needing no changeset — the check was reading the prose rather than the list, so it now reads only the list. --- CONTRIBUTING.md | 46 +++++++++++++- apps/api/src/__tests__/contributing.test.ts | 69 +++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) create mode 100644 apps/api/src/__tests__/contributing.test.ts diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 06e8601..49e1c5f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -21,21 +21,61 @@ Trunk-based development. Branch from `main`, keep branches short-lived and scope Types: `feat`, `fix`, `chore`, `refactor`, `docs`, `ci`, `spike`. Commits follow [Conventional Commits](https://www.conventionalcommits.org/). See the [Branching & Git Workflow](https://github.com/AKogut/flakemetry/wiki/Branching-and-Git-Workflow) wiki page for the full rules. +## Running the tests + +**`pnpm test` without a database passes while skipping more than half the suite.** Every test +that touches Postgres is guarded by `describe.skipIf(!hasDb)`, so a run with no `DATABASE_URL` +reports every task green: + +``` +Tasks: 28 successful 573 passed, 608 skipped +``` + +Scoring, tenant isolation, erasure, the queue — none of it ran. Point the tests at the +database `docker compose up` already started, and set `REQUIRE_DB=1` so a database that is +unreachable fails loudly instead of quietly skipping: + +```bash +DATABASE_URL="postgresql://flakemetry:flakemetry@localhost:5432/flakemetry?schema=public" \ + REQUIRE_DB=1 pnpm test +``` + +``` +Tasks: 28 successful 895 passed, 0 skipped +``` + +Each package migrates its own test schema on the way in, so there is nothing to set up first. + +Two things worth knowing when a change looks fine and is not: + +- `pnpm exec turbo run test --force` runs the packages **concurrently and without the cache**. + Serial cached runs hide interference between suites; this is how a Prisma upgrade was caught + writing to the wrong schema. +- Prefer proving a guard by breaking what it guards. A test that passes because it stopped + looking passes exactly as convincingly as one that works. + ## Before opening a pull request ```bash pnpm build pnpm lint pnpm typecheck -pnpm test pnpm format:check +DATABASE_URL="postgresql://flakemetry:flakemetry@localhost:5432/flakemetry?schema=public" REQUIRE_DB=1 pnpm test ``` CI runs the same tasks with turbo affected filtering; all checks must be green before merge. +`pnpm format:check` is a separate step and is the one most often forgotten — run `pnpm format` +before pushing. ## Changesets -Every PR that touches a published package (`@flakemetry/contracts`, `core`, `sdk`, `playwright-reporter`, `ai`, `cli`) must include a changeset: +Every PR that touches a published package must include a changeset. The published ones are: + +`@flakemetry/contracts` · `core` · `sdk` · `cli` · `playwright-reporter` · `vitest-reporter` · `jest-reporter` + +A test in `apps/api` checks this list against the workspace, so it cannot drift from what is +actually published. ```bash pnpm changeset @@ -50,3 +90,5 @@ Releases are automated: merged changesets accumulate into a version PR, and merg - No comments in source code — code should read clearly on its own - Prettier and ESLint are enforced in CI (`pnpm format`, `pnpm lint`) - Tests colocate with the package they cover +- Explain **why** in a comment where the reason is not obvious from the code; do not narrate + what the code already says diff --git a/apps/api/src/__tests__/contributing.test.ts b/apps/api/src/__tests__/contributing.test.ts new file mode 100644 index 0000000..01c690f --- /dev/null +++ b/apps/api/src/__tests__/contributing.test.ts @@ -0,0 +1,69 @@ +import { readdirSync, readFileSync } from 'node:fs' +import { dirname, join, resolve } from 'node:path' +import { fileURLToPath } from 'node:url' + +import { describe, expect, it } from 'vitest' + +const root = resolve(dirname(fileURLToPath(import.meta.url)), '../../../..') + +/** + * The list of packages needing a changeset had drifted in both directions: it named `ai`, + * which is private, and omitted the Vitest and Jest reporters, which are published. A + * contributor following it would either write a changeset nobody needs or miss one that + * gates a release. + */ +const publishedPackages = (): string[] => { + const dir = join(root, 'packages') + const names: string[] = [] + for (const entry of readdirSync(dir)) { + try { + const manifest = JSON.parse(readFileSync(join(dir, entry, 'package.json'), 'utf8')) as { + name?: string + private?: boolean + } + if (manifest.name && !manifest.private) names.push(manifest.name) + } catch { + continue + } + } + return names.sort() +} + +describe('CONTRIBUTING lists the packages that actually publish', () => { + const guide = readFileSync(join(root, 'CONTRIBUTING.md'), 'utf8') + const published = publishedPackages() + + it('reads the workspace it is meant to be checking', () => { + // Guard the guard: an empty list would agree with any document at all. + expect(published.length).toBeGreaterThan(4) + expect(published).toContain('@flakemetry/contracts') + }) + + it('names every published package', () => { + const missing = published.filter((name) => !guide.includes(name.replace('@flakemetry/', ''))) + expect(missing, 'these publish but the changeset section does not mention them').toEqual([]) + }) + + it('does not ask for a changeset on a package that never publishes', () => { + // Only the list itself, not the surrounding prose — the section deliberately names `db` + // as an example of something that needs no changeset, and flagging that would be the + // check misreading its own subject. + const line = + guide.split('\n').find((candidate) => candidate.includes('`@flakemetry/contracts` ·')) ?? '' + expect(line, 'the changeset list was not found — this check has lost its subject').not.toBe('') + + const privateNames = readdirSync(join(root, 'packages')).filter((entry) => { + try { + const manifest = JSON.parse( + readFileSync(join(root, 'packages', entry, 'package.json'), 'utf8'), + ) as { private?: boolean } + return manifest.private === true + } catch { + return false + } + }) + + const wrongly = privateNames.filter((entry) => new RegExp(`\`${entry}\``).test(line)) + expect(wrongly, 'these are private and need no changeset').toEqual([]) + }) +})