Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .githooks/pre-push
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
#!/usr/bin/env sh
# Block pushes unless the full test suite passes.

set -eu

echo "🧪 Running npm test before push..."
npm test --silent || {
echo "❌ npm test failed; push aborted."
exit 1
}

echo "✅ npm test passed; proceeding with push."
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ jobs:
- uses: actions/setup-node@v4
with: { node-version: '20' }
- run: npm ci
- name: Unit & property tests
run: npm test
- name: Core test suite
run: npm run test:core
- name: Upload coverage reports
uses: actions/upload-artifact@v4
with:
Expand Down
117 changes: 66 additions & 51 deletions docs/codeCoverageIgnoreGuidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ This document defines how to use coverage-ignore comments in this repository. It
## TL;DR

- `/* istanbul ignore next */` excludes the **next AST node** from coverage.
- Use ignores **sparingly** and **only** for code that is *truly* untestable or irrelevant to product behavior.
- Use ignores **sparingly** and **only** for code that is _truly_ untestable or irrelevant to product behavior.
- Every ignore **must include a reason** right next to it.
- Prefer tests, refactors, or config-level excludes over in-source ignores.

Expand All @@ -30,24 +30,33 @@ Use an ignore only when exercising the code in automated tests is impractical or

1. **Unreachable defensive code**
Exhaustive switch fallthroughs, invariant guards, or “should never happen” paths that exist purely as safety nets.

```ts
type Kind = "A" | "B"
function assertNever(x: never): never { throw new Error("unreachable") }
type Kind = 'A' | 'B';
function assertNever(x: never): never {
throw new Error('unreachable');
}

switch (kind) {
case "A": handleA(); break
case "B": handleB(); break
case 'A':
handleA();
break;
case 'B':
handleB();
break;
/* istanbul ignore next -- defensive, unreachable by construction */
default: assertNever(kind as never)
default:
assertNever(kind as never);
}
```

2. **Platform-/environment-specific branches**
Behavior that cannot be exercised in CI or across all supported OSes without unrealistic setups.

```ts
if (process.platform === "win32") {
if (process.platform === 'win32') {
/* istanbul ignore next -- requires native Windows console; not in CI image */
enableWindowsConsoleMode()
enableWindowsConsoleMode();
}
```

Expand All @@ -61,11 +70,11 @@ Use an ignore only when exercising the code in automated tests is impractical or

## When it is **not** acceptable

* To boost coverage percentages or hide missing tests.
* On **business logic** or any behavior affecting users.
* Broadly before `if`/`switch`/function declarations that mask multiple branches or large regions.
* As a substitute for a **small refactor** that would make testing feasible (e.g., splitting out side effects, injecting dependencies).
* For convenience when a test is mildly inconvenient to write (e.g., mocking a timer or a rejected promise).
- To boost coverage percentages or hide missing tests.
- On **business logic** or any behavior affecting users.
- Broadly before `if`/`switch`/function declarations that mask multiple branches or large regions.
- As a substitute for a **small refactor** that would make testing feasible (e.g., splitting out side effects, injecting dependencies).
- For convenience when a test is mildly inconvenient to write (e.g., mocking a timer or a rejected promise).

---

Expand Down Expand Up @@ -97,10 +106,10 @@ Use an ignore only when exercising the code in automated tests is impractical or

## Preferred alternatives to ignores

* **Write a focused test**: Use dependency injection, seam extraction, or a small adapter to isolate side effects.
* **Refactor for testability**: Split logic from I/O; return values instead of printing; pass a clock/random source.
* **Use config excludes for generated code**: Keep production logic fully measured.
* **Switch directive, not scope**: Prefer `ignore if/else` over `ignore next` when only one branch is untestable.
- **Write a focused test**: Use dependency injection, seam extraction, or a small adapter to isolate side effects.
- **Refactor for testability**: Split logic from I/O; return values instead of printing; pass a clock/random source.
- **Use config excludes for generated code**: Keep production logic fully measured.
- **Switch directive, not scope**: Prefer `ignore if/else` over `ignore next` when only one branch is untestable.

---

Expand Down Expand Up @@ -135,14 +144,14 @@ Jest example (if using V8 coverage):
// jest.config.js
module.exports = {
collectCoverage: true,
coverageProvider: "v8",
coverageProvider: 'v8',
coveragePathIgnorePatterns: [
"/node_modules/",
"/dist/",
"/build/",
"\\.gen\\."
]
}
'/node_modules/',
'/dist/',
'/build/',
'\\.gen\\.',
],
};
```

> Align comment style with the active provider: `istanbul` for Babel/nyc instrumentation; `c8` for V8.
Expand All @@ -155,28 +164,32 @@ module.exports = {

```js
// scripts/check-coverage-ignores.mjs
import { readFileSync } from "node:fs";
import { globby } from "globby";
import { readFileSync } from 'node:fs';
import { globby } from 'globby';

const files = await globby(["src/**/*.{ts,tsx,js,jsx}"], { gitignore: true });
const files = await globby(['src/**/*.{ts,tsx,js,jsx}'], { gitignore: true });

const offenders = [];
const re = /(istanbul|c8)\s+ignore\s+(next|if|else|file)/;

for (const f of files) {
const lines = readFileSync(f, "utf8").split("\n");
const lines = readFileSync(f, 'utf8').split('\n');
for (let i = 0; i < lines.length; i++) {
if (re.test(lines[i])) {
const hasReason =
/--\s*[A-Za-z0-9]/.test(lines[i]) || (i > 0 && /--\s*[A-Za-z0-9]/.test(lines[i - 1]));
if (!hasReason) offenders.push(`${f}:${i + 1}: missing reason after ignore comment`);
/--\s*[A-Za-z0-9]/.test(lines[i]) ||
(i > 0 && /--\s*[A-Za-z0-9]/.test(lines[i - 1]));
if (!hasReason)
offenders.push(`${f}:${i + 1}: missing reason after ignore comment`);
}
}
}

if (offenders.length) {
console.error("Coverage ignore comments require an inline reason (use `-- reason`).");
console.error(offenders.join("\n"));
console.error(
'Coverage ignore comments require an inline reason (use `-- reason`).'
);
console.error(offenders.join('\n'));
process.exit(1);
}
```
Expand All @@ -200,7 +213,13 @@ Optional ESLint guard (warn on any usage):
"no-restricted-comments": [
"warn",
{
"terms": ["istanbul ignore next", "istanbul ignore if", "istanbul ignore else", "istanbul ignore file", "c8 ignore next"],
"terms": [
"istanbul ignore next",
"istanbul ignore if",
"istanbul ignore else",
"istanbul ignore file",
"c8 ignore next"
Comment on lines +217 to +221
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The strings in the terms array are still using double quotes. This is inconsistent with the goal of this pull request to standardize on single quotes. For consistency, these should be updated to use single quotes.

Suggested change
"istanbul ignore next",
"istanbul ignore if",
"istanbul ignore else",
"istanbul ignore file",
"c8 ignore next"
'istanbul ignore next',
'istanbul ignore if',
'istanbul ignore else',
'istanbul ignore file',
'c8 ignore next'

],
"location": "anywhere",
"message": "Coverage ignore detected: add `-- reason` and ensure policy compliance."
}
Expand All @@ -217,11 +236,10 @@ Optional ESLint guard (warn on any usage):

```ts
if (cacheEnabled) {
warmCache()
}
/* istanbul ignore else -- cold path is a telemetry-only fallback */
else {
coldStartWithTelemetry()
warmCache();
} else {
/* istanbul ignore else -- cold path is a telemetry-only fallback */
coldStartWithTelemetry();
}
```

Expand All @@ -231,7 +249,7 @@ else {
// Calls a native API that only exists on macOS ≥ 13:
if (isDarwin13Plus()) {
/* istanbul ignore next -- native API unavailable in CI runners */
enableFancyTerminal()
enableFancyTerminal();
}
```

Expand All @@ -252,18 +270,15 @@ nyc.exclude += ["src/generated/**"] // in package.json nyc config
```

2. **Classify**

* ✅ Legitimate (add/verify reason, minimize scope)
* 🟡 Replaceable (write a test or refactor)
* 🔴 Remove/ban (business logic, overly broad)
- ✅ Legitimate (add/verify reason, minimize scope)
- 🟡 Replaceable (write a test or refactor)
- 🔴 Remove/ban (business logic, overly broad)

3. **Refactor & test**

* Extract logic from side effects; inject collaborators; mock clocks/randomness.
- Extract logic from side effects; inject collaborators; mock clocks/randomness.

4. **Guard**

* Add CI script and ESLint rule to prevent regressions.
- Add CI script and ESLint rule to prevent regressions.

---

Expand All @@ -282,7 +297,7 @@ A: Use one approach consistently. If switching to V8 coverage, update directives

## Checklist for new code

* [ ] Coverage added for changed behavior.
* [ ] No new `istanbul`/`c8` ignores **unless** justified and minimal.
* [ ] Each ignore has `-- reason` and (optionally) a ticket reference.
* [ ] Generated/vendor code excluded via config, not inline comments.
- [ ] Coverage added for changed behavior.
- [ ] No new `istanbul`/`c8` ignores **unless** justified and minimal.
- [ ] Each ignore has `-- reason` and (optionally) a ticket reference.
- [ ] Generated/vendor code excluded via config, not inline comments.
55 changes: 24 additions & 31 deletions docs/tests-and-lint-as-precommit-hook.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
Instructions to add a **native Git pre-commit hook** that blocks commits unless **lint** and **tests** pass:
Instructions to add a **native Git pre-push hook** that blocks pushes unless your selected test command succeeds.

---

## 0) Prereqs (once)

Ensure `package.json` has lint and test scripts defined.
Confirm the command you plan to run (default `npm test`) already covers the checks you want to enforce before pushing.

- In this template, `npm test` runs `test:core` (format + lint + dependency checks + unit/property tests) followed by `test:e2e:index` (Playwright).
- If you only want the lighter core suite at push time, swap in `npm run test:core --silent` in Step 2.

---

Expand All @@ -15,59 +18,49 @@ mkdir -p .githooks
git config core.hooksPath .githooks
```

> This tells Git to use `.githooks/` (which is tracked in the repo) instead of the untracked `.git/hooks/`.
This tells Git to use the tracked `.githooks/` directory instead of `.git/hooks/`.

---

## 2) Create the pre-commit hook
## 2) Create the pre-push hook

Create `.githooks/pre-commit` with the following content:
Create `.githooks/pre-push` with the following content:

```bash
#!/usr/bin/env sh
# Abort the commit if lint or tests fail.
# Works on macOS/Linux and in Git Bash on Windows.

set -eu

echo "🔎 Linting..."
npm run -s lint || { echo "❌ Lint failed"; exit 1; }

echo "🧪 Running tests..."
npm test --silent || { echo "❌ Tests failed"; exit 1; }
echo "Running npm test before push..."
npm test --silent || {
echo "npm test failed; push aborted."
exit 1
}

echo "✅ Pre-commit checks passed"
echo "npm test passed; proceeding with push."
```

Make it executable (choose one):
Mark it executable (choose one):

```bash
# Portable way (works on Windows too):
git update-index --chmod=+x .githooks/pre-commit

# Or the POSIX way:
chmod +x .githooks/pre-commit
```

Commit the hook:

```bash
git add .githooks/pre-commit
git commit -m "chore: add native pre-commit hook for lint and tests"
git update-index --chmod=+x .githooks/pre-push
# or
chmod +x .githooks/pre-push
```

---

## 3) Quick self-test

```bash
git commit --allow-empty -m "test hook"
# Expect the hook to run; commit only succeeds if lint & tests pass.
./.githooks/pre-push
```

Running the hook manually should execute the same checks Git will run before a push.

---

## Additional Notes

- Developers can bypass local hooks with `--no-verify`, so keep CI with required checks as the final gate.
- For faster commits, consider moving long-running suites to a `pre-push` hook or to CI.
- Developers can bypass local hooks with `--no-verify`, so keep CI as the final gate.
- If you want a lighter-weight push check, swap `npm test --silent` for `npm run test:core --silent` to skip e2e tests.
- Re-run the `git config core.hooksPath .githooks` command on every machine or clone so Git picks up the tracked hooks.
Loading