From d9c16d0840abcf9d65b317612e62a9445a54f765 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Fri, 29 May 2026 01:13:11 +0200 Subject: [PATCH] feat(lint): add bun:test expect-expect oxlint plugin oxlint ships `jest/expect-expect`, but its Rust implementation only recognizes `jest`/`vitest` imports and silently ignores `bun:test`, so assertion-less tests passed silently and accumulated over time. Adds a custom oxlint JS plugin (`lint/expect-expect.ts`, rule `bun-test/expect-expect`) registered via `jsPlugins` in `.oxlintrc.json` and scoped to `tests/**/*.test.ts`. It fails the build for any runnable `test()`/`it()` (incl. `test.skipIf(...)()`, `test.each(...)()`) whose body contains no `expect()` call, while ignoring `test.skip`/`test.todo`. Also fixes the 44 pre-existing assertion-less smoke tests it surfaced by making their implicit "does not throw" contracts explicit, and documents the new convention in ARCH-005. Closes #353 Signed-off-by: Rhuan Barreto --- .archgate/adrs/ARCH-005-testing-standards.md | 8 + .../agent-memory/archgate-developer/MEMORY.md | 4 + .oxlintrc.json | 5 + lint/expect-expect.ts | 177 +++++++++++ tests/helpers/auth.test.ts | 3 +- tests/helpers/credential-store-impl.test.ts | 3 +- tests/helpers/git.test.ts | 25 +- tests/helpers/platform.test.ts | 10 +- tests/helpers/sentry.test.ts | 44 +-- tests/helpers/telemetry.test.ts | 285 ++++++++++-------- 10 files changed, 402 insertions(+), 162 deletions(-) create mode 100644 lint/expect-expect.ts diff --git a/.archgate/adrs/ARCH-005-testing-standards.md b/.archgate/adrs/ARCH-005-testing-standards.md index dc86f6ac..8aad64f5 100644 --- a/.archgate/adrs/ARCH-005-testing-standards.md +++ b/.archgate/adrs/ARCH-005-testing-standards.md @@ -44,6 +44,10 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - **When a test creates a temp git repo and needs to call `git commit`, configure local user identity first** — CI runners have no global git config, so commits fail without explicit local identity. Set it with `await Bun.$\`git config user.email "test@test.com"\`.cwd(tempDir).quiet()`and`await Bun.$\`git config user.name "Test"\`.cwd(tempDir).quiet()`immediately after`git init`. - Test public module interfaces, not private implementation details - Use descriptive test names that explain the expected behavior +- **Every test MUST contain at least one `expect()` assertion** — enforced by the custom `bun-test/expect-expect` oxlint plugin at `lint/expect-expect.ts` (registered via `jsPlugins` in `.oxlintrc.json`, enabled for `tests/**/*.test.ts`). `bun run lint` fails on any runnable `test()`/`it()` whose body contains no `expect()`. oxlint's built-in `jest/expect-expect` does not cover `bun:test`, which is why this plugin exists. +- **Make implicit "does not throw" contracts explicit** — for a smoke test that merely invokes a function, assert the contract: `expect(() => fn()).not.toThrow()` for synchronous calls, or `await expect(promise).resolves.toBeUndefined()` for async calls. Calling a function with no assertion provides false confidence and is blocked by the assertion rule. +- **Use `test.skip` or `test.todo` for intentionally empty or disabled tests** — the `bun-test/expect-expect` rule deliberately ignores `.skip` and `.todo` so placeholders remain explicit and self-documenting. +- **When adding the first `expect()` to a previously assertion-less test file, add `expect` to the `bun:test` import** — older smoke-test files (e.g., `sentry.test.ts`) historically omitted it because their bodies never asserted. - **When mocking `fetch` in tests, assign directly to `globalThis.fetch`** — use `globalThis.fetch = mockFn as unknown as typeof fetch`. Restore in `afterEach` via `mock.restore()` (from `bun:test`) or by reassigning the original reference before the test. - **Wrap `spyOn` / `mockImplementation` calls in `try/finally` to guarantee `mockRestore()` runs** — when `expect()` assertions fail, they throw immediately, skipping any `mockRestore()` that follows. The un-restored spy leaks into subsequent tests, causing false positives or false negatives. Pattern: `const spy = spyOn(...).mockImplementation(() => {}); try { /* assertions */ } finally { spy.mockRestore(); }`. Alternatively, create and restore spies in `beforeEach`/`afterEach` hooks instead of inline. @@ -56,6 +60,8 @@ Use Bun's built-in test runner (`bun test`) for all tests. Test files go in `tes - **Don't rely on globally-configured git identity in temp git repos** — always set `user.email` and `user.name` locally in any repo that makes commits. Omitting this works locally (where developers have global git config) but fails silently in CI, producing a cryptic `ShellPromise` error with no indication that git identity is the cause. - **Don't let tests send real events to Sentry** — set `Bun.env.NODE_ENV = "test"` in `beforeEach` (and restore in `afterEach`) for any test that initializes Sentry. The Sentry SDK is configured with `enabled: Bun.env.NODE_ENV !== "test"` to prevent test noise from polluting production error tracking. - Don't skip tests without a tracking issue +- **Don't write assertion-less tests** — a `test()`/`it()` body with no `expect()` call silently passes and gives false confidence. The `bun-test/expect-expect` oxlint plugin blocks these at lint time. +- **Don't use a bare early `return` or an empty callback body to conditionally skip a test** — use `test.skipIf(condition)`, `test.skip`, or `test.todo` so the skip is explicit and the assertion rule can recognize it. Bare returns and empty bodies are exactly how assertion-less tests accumulated silently before the rule existed. - Don't import test utilities from `node:test` — use Bun's built-in `bun:test` module - **Don't use `mock.module("node:fetch", ...)` to intercept HTTP fetch calls** — in Bun, the runtime fetch is `globalThis.fetch` and `mock.module` targeting `node:fetch` does not intercept it. The mock silently has no effect: the real network is hit, making tests non-deterministic and dependent on external services. Assign `globalThis.fetch` directly instead (see Do's above). - **Don't place `mockRestore()` after assertions without `try/finally` protection** — if the assertion throws, the spy is never restored and contaminates subsequent tests. This is especially dangerous when spying on globals like `console.warn` or `globalThis.fetch`, where a leaked spy silently suppresses or redirects output for every test that follows. @@ -199,6 +205,7 @@ globalThis.fetch = (() => ### Automated Enforcement - **Archgate rule** `ARCH-005/test-mirrors-src`: Scans all source files in `src/` and verifies a corresponding `.test.ts` file exists in `tests/`. Severity: `error`. +- **oxlint plugin** `bun-test/expect-expect` (`lint/expect-expect.ts`): A custom oxlint JS plugin enabled for `tests/**/*.test.ts` that fails the build for any runnable `test()`/`it()` (including `test.skipIf(...)()`, `test.each(...)()`) whose body contains no `expect()` call. It ignores `test.skip` and `test.todo`. Runs as part of `bun run lint` (and therefore `bun run validate` and CI). oxlint's built-in `jest/expect-expect` only recognizes `jest`/`vitest` imports, so it does not cover `bun:test` — this plugin fills that gap. - **CI pipeline**: `bun test --timeout 60000` runs on every pull request. Test failures and per-test timeouts block merge. All workflow jobs have `timeout-minutes` set to prevent indefinite hangs. - **Coverage threshold**: The `Coverage Report` job enforces a 90% minimum line coverage. If total coverage drops below 90%, the job fails and the `Validate Code` gate blocks the PR. @@ -213,6 +220,7 @@ Code reviewers MUST verify: 5. Tests that call `git commit` on a temp repo configure `user.email` and `user.name` locally before committing 6. Tests that mock HTTP fetch assign `globalThis.fetch` directly — no `mock.module("node:fetch", ...)` usage 7. Tests that use `spyOn` or `mockImplementation` inline (not in `beforeEach`/`afterEach`) wrap the spy lifecycle in `try/finally` to guarantee `mockRestore()` runs even when assertions fail +8. Every test asserts something with `expect()` — no smoke tests that merely call a function; "does not throw" contracts are made explicit via `expect(() => fn()).not.toThrow()` or `await expect(promise).resolves.toBeUndefined()` ## References diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 55e3940d..3f78b375 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -30,6 +30,10 @@ Skipping steps 2 or 3 is a workflow violation. The user should NEVER have to inv ## Patterns & Fixes +- **Custom oxlint JS plugins live in `lint/*.ts`, registered via `jsPlugins` in `.oxlintrc.json`** — The repo has a custom plugin `lint/expect-expect.ts` (rule id `bun-test/expect-expect`) that fails the build for any runnable `bun:test` `test()`/`it()` (incl. `test.skipIf(...)()`, `test.each(...)()`) whose body has no `expect()` call; it ignores `test.skip`/`test.todo`. It exists because oxlint's built-in `jest/expect-expect` only recognizes `jest`/`vitest` imports and silently skips `bun:test`. The plugin uses the ESLint-compatible default-export shape (`{ meta:{name}, rules:{ "x": { create(ctx){ return { CallExpression(node){...} } } } } }`) and runs as native TS under Bun (no build step). Important scoping: `lint/` is NOT in `tsconfig.json` `include` (so `tsc` does not typecheck it) and NOT in `knip.json` `project` (so knip ignores it) — but `oxlint --deny-warnings .` DOES lint it, so the plugin file must itself pass all oxlint rules. To enable a jsPlugin rule only for tests, list it under an `overrides` entry for `tests/**/*.test.ts` (the plugin is loaded top-level via `jsPlugins`, the rule is turned on in the override). The convention is now documented in ARCH-005. +- **Converting assertion-less smoke tests: assert the "does not throw" contract explicitly** — `expect(() => fn()).not.toThrow()` for sync void calls, `await expect(promise).resolves.toBeUndefined()` for async void calls. Watch for two gotchas: (1) older smoke-test files may NOT import `expect` from `bun:test` (their bodies never asserted) — add it or you get `ReferenceError: expect is not defined` at runtime, not at lint time; (2) a Windows-only `test.skipIf(process.platform !== "win32")` test that was a no-op placeholder can often be made real by stubbing `Bun.spawn`/`Bun.which` in a `try/finally` (see `tests/helpers/git.test.ts` forcing the `wsl which git` fallback to miss). +- **oxlint `unicorn/no-array-callback-reference`** — Don't pass a bare function reference to `.map()`/`.find()`/`.filter()` etc. (e.g. `args.map(asNode)`); the rule wants the element-only contract made explicit. Wrap in an arrow: `args.map((x) => asNode(x)).find((x) => isFn(x))`. +- **oxlint `require-unicode-regexp`** — Regex literals need the `u` flag: `/Git is not installed/u`, not `/Git is not installed/`. Applies in tests too (e.g. `expect(...).rejects.toThrow(/.../u)`). - **YAML double-quoted strings require escaped backslashes for Windows paths in tests** — YAML interprets `\` as an escape character inside double-quoted strings. Writing `cwd: "E:\project"` silently corrupts the parsed value because `\p` is not a valid escape sequence. Fix: use `JSON.stringify(path)` to produce properly escaped YAML values (e.g., `cwd: ${JSON.stringify(cwd)}`). JSON and YAML double-quoted strings share the same escape syntax. Encountered in Copilot CLI session-context tests (`workspace.yaml` with Windows paths). - **`git commit` in temp repos requires local identity** — CI runners have no global `user.email`/`user.name` configured. Any test that runs `git commit` on a temp repo MUST call `git config user.email` and `git config user.name` locally after `git init`. Fails with a cryptic `ShellPromise` error in CI; passes locally. Also captured in ARCH-005 Do's. - **Never use `bunx prettier` directly** — Always use `bun run format` (to fix) or `bun run format:check` (to verify). Using `bunx prettier` can fail or use a different version than the project's devDependency. The same applies to all dev tools: prefer `bun run