chrisleekr / chrisleekr/github-app

test(testing): add ESLint no-restricted-syntax gate for .only / .skip; current runtime-only catch is unreliable under Bun #16611

Open
#248 0 comments 0 reactions 0 assignees View on GitHub
research type: test 🧪
Dominant language
TypeScript
Stars
1
Forks
0
Avg merge
6h 39m
Merged PRs (30d)
27

Description

## Finding

The test runner has a single line of defence against silent test loss from accidentally-committed `it.only` / `test.only` / `describe.only` markers, and it sits at CI runtime, not at lint time. `scripts/test-isolated.sh:33-35` greps Bun's per-file summary for a leading-whitespace ` N skip` line and treats any non-zero skip count as failure. That works for explicit `.skip(...)` markers but is unreliable for `.only`: Bun's own tracker confirms the skip count does not consistently reflect tests filtered out by `.only` (see [`oven-sh/bun#16611`](https://github.com/oven-sh/bun/issues/16611), still open). So a developer who pushes `it.only("...", ...)` for local debugging can land a PR where only one assertion in that file actually executes and CI stays green, while the rest of the file goes dark.

There is no static gate. `eslint.config.mjs:139-165` already uses `no-restricted-syntax` to enforce a similar invariant for `src/workflows/ship/scoped/triage.ts` (forbidding issue-mutation Octokit calls) and `eslint.config.mjs:168-186` defines a relaxed-rules block for `**/*.test.ts`. Neither block restricts `.only` / `.skip`. The same shape, a `no-restricted-syntax` selector scoped to test globs, would catch the leftover-`.only` mistake at edit time (in-IDE), pre-commit (husky + lint-staged), and `bun run lint` in CI, well before `bun run test` ever runs. This also closes the gap created by `bun run test:fast` (`package.json:49`, documented in `docs/operate/setup.md:70`), which is `bun test` directly and therefore does not invoke the per-file skip-detection wrapper at all.

The project already prefers static gates that shift runtime invariants left, see issue #201 (`scripts/check-test-globs.ts`, fails CI when a `*.test.ts` is not reachable by the runner glob), issue #173 (`scripts/check-runner-pins.ts`, fails on `*-latest` runner aliases), and issue #203 (`scripts/check-no-destructive-actions.ts`, fails on force-push / `gh pr merge` outside the agent). A `no-only-tests` ESLint rule is the same defensive shape applied to the test suite itself.

## Diagram

```mermaid
flowchart TD
Dev[Developer commits
it.only added by accident] --> PreCommit[pre-commit hook
husky + lint-staged]
PreCommit -->|ESLint runs| Lint{no-restricted-syntax
covers .only?}
Lint -->|NO rule today| Pass1[Lint passes
commit succeeds]:::warn
Pass1 --> Push[Push to PR]
Push --> CI[CI: bun run test]
CI --> Runner[scripts/test-isolated.sh
per-file Bun process]
Runner --> Detect{skip via regex
line ' N skip'?}
Detect -->|Bun #16611:
skip count wrong
under .only| Miss[CI may pass
silent test loss]:::fail
Detect -->|If Bun emits skip| Catch[CI fails as
expected]:::ok

classDef warn fill:#f39c12;color:#000000
classDef fail fill:#c0392b;color:#ffffff
classDef ok fill:#196f3d;color:#ffffff
```

## Rationale

Silent test loss is the worst class of test-suite regression because the signal is inverted: CI is green, but the proof that the change is safe is fictitious. With 150 `*.test.ts` files in the repo today (`find . -name '*.test.ts' -not -path '*/node_modules/*' -not -path '*/dist/*' | wc -l`), and 46 of them using `mock.module()` (per-file isolation by design of `scripts/test-isolated.sh:1-5`), the blast radius of one stray `.only` is everything below it in that file. Combined with the existing 90 % `coverageThreshold` in `bunfig.toml`, an accidental `.only` could also drop a file under the gate and still escape the per-file runner's only=fail heuristic.

The catch is cheap, ESLint `no-restricted-syntax` runs in milliseconds and is already invoked by `bun run lint`, by `bun run check`, by `bun run check` in CI (`.github/workflows/ci.yml`), and by husky pre-commit on staged files. Shifting the detection from one fragile runtime regex to a deterministic AST selector eliminates the dependence on Bun resolving [`oven-sh/bun#16611`](https://github.com/oven-sh/bun/issues/16611) upstream. The funbox community plugin [`eslint-plugin-no-only-tests`](https://github.com/funbox/eslint-plugin-no-only-tests) exists, but the repo convention is to add a single targeted `no-restricted-syntax` block per invariant (see `eslint.config.mjs:139-165` for the triage-mutation block), so no new dependency is needed.

## References

**Internal**:
- `scripts/test-isolated.sh:25-35` (the ` 0 fail` / ` N skip` regex pair the runtime catch depends on)
- `scripts/test-isolated.sh:12` (the `tests=( ... )` glob array, paired with `scripts/check-test-globs.ts` per issue #201)
- `eslint.config.mjs:139-165` (precedent for `no-restricted-syntax` invariant scoping)
- `eslint.config.mjs:168-186` (`**/*.test.ts` rule-relaxation block where the new selector would live)
- `package.json:49` (`test:fast` alias that bypasses the runtime skip detection)
- `docs/operate/setup.md:70` (`bun run test:fast` documented as a recommended dev workflow)
- `bunfig.toml` (`coverageThreshold = { lines = 0.9, functions = 0.9 }`, the gate `.only` could silently dodge)
- CLAUDE.md, "Test-glob + destructive-action guards run in CI" section (the shift-left precedent these guards represent)

**External**:
- [`oven-sh/bun#16611`](https://github.com/oven-sh/bun/issues/16611) bun test: warn when tests are skipped because of `test.only()` (open; confirms Bun does not reliably surface skip count under `.only`)
- [`oven-sh/bun#10850`](https://github.com/oven-sh/bun/issues/10850) test runner runs all tests above the one which is marked as `.only` (related Bun runner bug)
- [`oven-sh/bun#7701`](https://github.com/oven-sh/bun/issues/7701) `.only` tests unexpected behavior when running `bun test` without `-only` flag
- [ESLint `no-restricted-syntax` rule reference](https://eslint.org/docs/latest/rules/no-restricted-syntax) (the rule shape already used in `eslint.config.mjs`)
- [`funbox/eslint-plugin-no-only-tests`](https://github.com/funbox/eslint-plugin-no-only-tests) (community plugin; cited as prior art, not proposed as a dependency)

## Suggested Next Steps

1. In `eslint.config.mjs:168-186` (the existing `**/*.test.ts` block), add a `no-restricted-syntax` rule with three selectors covering `CallExpression[callee.property.name='only'][callee.object.name=/^(it|test|describe)$/]`, the equivalent `MemberExpression` access (`it.only.each`, etc.), and a permissive selector for `.skip` paired with a per-line eslint-disable comment requirement so legitimate `describe.skipIf(...)` usages (e.g. `test/integration/repo-knowledge.test.ts:48`) remain allowed.
2. Add a small unit test under `test/scripts/` (mirroring `test/scripts/check-runner-pins.test.ts`) that lints a fixture containing `it.only(...)` and asserts the rule fires; tie it into the existing eslint-tests pattern.
3. Update `docs/operate/setup.md` to note that the lint gate covers `.only` and that `bun run test:fast` should not be used for verifying a finished branch (because it skips the per-file isolation that backstops mock bleed).
4. Optionally, fix the `docs/operate/setup.md:69` description that says `bun test` invokes `scripts/test-isolated.sh`; that is true only via `bun run test`, the bare `bun test` invocation hits Bun's built-in runner directly and bypasses the wrapper.

## Areas Evaluated

- `scripts/test-isolated.sh` (per-file Bun process model, fail/skip regex detection at lines 25-35, glob source-of-truth at line 12)
- `scripts/check-test-globs.ts` (existing static guard derived from the runner globs; issue #201 precedent)
- `bunfig.toml` (test preload, 30 s timeout, `coverageThreshold`)
- `test/preload.ts` (env-pinning before every test file)
- `eslint.config.mjs` (`no-restricted-syntax` precedent in the triage block; `**/*.test.ts` relaxed-rules block; no `.only`/`.skip` restriction)
- `package.json` scripts (`test`, `test:fast`, `test:coverage`, `check`, `lint`)
- `.github/workflows/ci.yml` (test invocation `bun run test`; no separate coverage gate)
- Sample mock-using files (`test/webhook/router.test.ts`, `test/webhook/events/issue-comment.test.ts`, `test/integration/repo-knowledge.test.ts`)
- Existing `research`-labelled issues to rule out duplication (closest precedents are `scripts/test-isolated.sh` glob check #201 and the destructive-action check #203; both target a different gate class)
- Bun upstream tracker for `.only` reporting reliability

*Generated by the scheduled research action on 2026-06-22*

Contributor guide

Open the contributing guide

Research direction

Start in eslint.config.mjs:168-186, then compare the existing invariant rule at lines 139-165. Review the lint-test pattern in test/scripts/check-runner-pins.test.ts and run the relevant lint checks. Done means test files flag accidental .only/.skip usage, the fixture test passes, and docs/operate/setup.md reflects the gate and test:fast limitation.

Written by the indexing model from the issue text.

Assessment

Tech stack
bun, typescript
Domain
ci-cd, testing, tooling
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.