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([]) + }) +})