chore(claude): add testing guidelines and fxa-test-* skills by nshirley · Pull Request #20482 · mozilla/fxa

nshirley · 2026-04-29T17:03:41Z

Because:

There were no general testing guidelines for Claude to reference when writing or reviewing tests, and no dedicated skill for test authoring, repair, or independence validation.

This commit:

Adds Section 8 (Testing Guidelines) to CLAUDE.md with 10 behavioral rules
Adds fxa-test-draft skill for writing tests against a changeset
Adds fxa-test-repair skill for auditing and repairing existing test files
Adds fxa-test-independence skill for validating test isolation
Adds fxa-testing-shared/GUIDELINES.md as a single source of truth for rules and examples shared across the test skills

Checklist

Put an x in the boxes that apply

My commit is GPG signed.
If applicable, I have modified or added tests which pass locally.
I have added necessary documentation (if appropriate).
I have verified that my changes render correctly in RTL (if appropriate).
I have manually reviewed all AI generated code.

How to review (Optional)

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

Any other information that is important to this pull request.

Because: there were no general testing guidelines for Claude to reference when writing or reviewing tests, and no dedicated skill for test authoring, repair, or independence validation. This commit: - Adds Section 8 (Testing Guidelines) to CLAUDE.md with 10 behavioral rules - Adds fxa-test-draft skill for writing tests against a changeset - Adds fxa-test-repair skill for auditing and repairing existing test files - Adds fxa-test-independence skill for validating test isolation - Adds fxa-testing-shared/GUIDELINES.md as a single source of truth for rules and examples shared across the test skills

nshirley · 2026-04-30T15:48:49Z

+| `/fxa-test-independence`        | Validating tests pass both as a full suite and individually in isolation                    |
+| `/fxa-test-repair`              | Auditing a test file for guideline violations and producing a prioritized repair plan       |
+
+## 8) Testing Guidelines


I tried to keep this as short and concise as possible, but wanted to include something here so that it's always "top of mind" to Claude. If we don't think this needs to be here, owing to the fact that it's included in everything and context space is limited, I can find somewhere else or a way to tighten it up even further

nshirley · 2026-04-30T15:52:48Z

This acts as shared 'golden goal' of testing. Since each of the new test skills should understand these things, I didn't want to have to copy/paste and keep our rules/guidelines updated between all of them. Now, those skills just point here and we have a single point to update when we want to add new rules

nshirley · 2026-04-30T15:56:43Z

+
+# FXA Test Repair
+
+Review a test file for violations of the FXA testing guidelines and produce a prioritized repair plan with before/after code suggestions. This skill does not modify files — all repairs are presented for engineer review.


In the spirit of AI adoption, I wonder if we should include a hint/suggestion that says "Suggest a separate PR to fix tests if the size becomes too large" or similar. I don't mind the idea of test only PRs that address, fix or otherwise make tests more-better. Anyone have any other thoughts?

nshirley · 2026-04-30T16:00:34Z

+- Are **not** already test files (`*.spec.ts`, `*.test.ts`, `*.test.tsx`)
+- Are **not** generated, config, or migration files
+
+For each source file, check whether a co-located test file already exists and what it already covers.


Probably need to note that there may be tests in a non-co-located test file for some projects.

nshirley · 2026-04-30T16:04:48Z

+For each source file, read nearby test files and shared mocks to understand the conventions in that package:
+
+```bash
+# Find test files in the same directory


well, these aren't useful bash, might be good to include actual examples!

nshirley · 2026-04-30T16:11:06Z

+
+### 10. Leave things better than you found them
+
+When adding tests to an existing file, write new tests to these standards. Don't blindly copy existing patterns that violate these rules. Note violations in existing tests and suggest `/fxa-test-repair` — but don't rewrite them wholesale unless that's explicitly in scope.


Shift-left: Favor moving tests left, business logic that can be tested in unit tests over integration tests should live in unit tests. Maybe also I should include a solid distinction between the purpose of unit, integration, and functional tests.

Copilot

Pull request overview

Adds a shared set of testing guidelines and three Claude “test” skills to standardize how tests are drafted, repaired, and validated for isolation across the FXA repo.

Changes:

Adds a new “Testing Guidelines” section to CLAUDE.md and registers the new /fxa-test-* skills.
Introduces a shared guidelines document (fxa-testing-shared/GUIDELINES.md) used by the new skills.
Adds three new skills: drafting tests from diffs, auditing/repairing tests, and validating test independence.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`CLAUDE.md`	Adds new `/fxa-test-*` skills and a testing-guidelines section for Claude usage.
`.claude/skills/fxa-testing-shared/GUIDELINES.md`	New shared, example-driven testing guidelines referenced by test-related skills.
`.claude/skills/fxa-test-draft/SKILL.md`	New skill for drafting Jest tests from staged/unstaged diffs or a commit range.
`.claude/skills/fxa-test-repair/SKILL.md`	New skill for auditing an existing test file and producing a prioritized repair plan.
`.claude/skills/fxa-test-independence/SKILL.md`	New skill for running per-test isolation checks to detect order dependencies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+|---|----------|---------------|----------|-------------|
+| 1 | High | Independent & order-agnostic | `account.spec.ts:14` | `account` mutated across tests without `beforeEach` reset |
+| 2 | Medium | Assert explicitly | `account.spec.ts:42` | `toBeTruthy()` used where exact value is known |
+| 3 | Low | Independent & order-agnostic | `account.spec.ts:8` | `jest.clearAllMocks()` in `beforeEach` — global config handles this |


+- **Name tests for what they assert.** Test names should make the assertion self-evident; don't assert behavior outside the scope of the name.
+- **Cover all paths.** All happy paths and error cases require tests. Call out when testing is difficult due to code complexity or anti-patterns rather than skipping coverage.
+- **Tests are first-class.** A reader should be able to infer the intent and expected behavior of code from its tests alone, without reading the implementation.
+- **Trust, but verify.** Use tests to verify changes. When tests and implementation disagree, investigate — never assume one is automatically correct over the other.
+- **Mock at system boundaries, not internals.** Mock external dependencies (DB, network, external APIs); use real implementations for internal functions within the same package. Over-mocking produces tests that only verify mock wiring.
+- **Tests must be independent and order-agnostic.** No shared mutable state between tests. Each test owns its setup; use `beforeEach` for resets.
+- **Assert explicitly.** Prefer exact values and shapes over `toBeTruthy()`, `toBeDefined()`, etc. Vague assertions hide regressions.
+- **Test behavior, not implementation.** Test through public interfaces; avoid asserting on private state or calling private methods.
+- **Test async code correctly.** Always `await` async operations; use `jest.useFakeTimers()` + `jest.setSystemTime()` over real timers or `setTimeout` hacks.
+- **Prefer deterministic test values.** Use hardcoded or factory-generated constants for UIDs, tokens, timestamps, and identifiers. Avoid `Math.random()`, `uuid()`, `Date.now()` in test setup — reserve randomness for tests that explicitly exercise randomness-dependent behavior.


+  jest.useFakeTimers();
+  jest.setSystemTime(Date.now() + 3_600_000);


+```bash
+npx jest --testPathPattern="<test-file-path>" --no-coverage
+```


+## Step 5: Run Each Test in Isolation
+
+For each test name extracted in Step 2, run it individually:
+```bash
+npx jest --testPathPattern="<test-file-path>" --testNamePattern="<exact test name>" --no-coverage
+```


+
+### 5. Tests must be independent and order-agnostic
+
+No shared mutable state between tests. Each test owns its setup; use `beforeEach` for resets. `jest.clearAllMocks()` in `beforeEach` is redundant — `clearMocks: true` is set globally.


nshirley commented Apr 30, 2026

View reviewed changes

nshirley marked this pull request as ready for review April 30, 2026 19:26

nshirley requested a review from a team as a code owner April 30, 2026 19:26

Copilot AI review requested due to automatic review settings April 30, 2026 19:26

Copilot started reviewing on behalf of nshirley April 30, 2026 19:26 View session

Copilot AI reviewed Apr 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(claude): add testing guidelines and fxa-test-* skills#20482

chore(claude): add testing guidelines and fxa-test-* skills#20482
nshirley wants to merge 1 commit intomainfrom
worktree-update-testing-skills

nshirley commented Apr 29, 2026 •

edited

Loading

Uh oh!

nshirley Apr 30, 2026

Uh oh!

nshirley Apr 30, 2026

Uh oh!

nshirley Apr 30, 2026 •

edited

Loading

Uh oh!

nshirley Apr 30, 2026

Uh oh!

nshirley Apr 30, 2026

Uh oh!

nshirley Apr 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		# FXA Test Repair

		Review a test file for violations of the FXA testing guidelines and produce a prioritized repair plan with before/after code suggestions. This skill does not modify files — all repairs are presented for engineer review.


		### 10. Leave things better than you found them

		When adding tests to an existing file, write new tests to these standards. Don't blindly copy existing patterns that violate these rules. Note violations in existing tests and suggest `/fxa-test-repair` — but don't rewrite them wholesale unless that's explicitly in scope.

		jest.useFakeTimers();
		jest.setSystemTime(Date.now() + 3_600_000);


		### 5. Tests must be independent and order-agnostic

		No shared mutable state between tests. Each test owns its setup; use `beforeEach` for resets. `jest.clearAllMocks()` in `beforeEach` is redundant — `clearMocks: true` is set globally.

Conversation

nshirley commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

How to review (Optional)

Screenshots (Optional)

Other information (Optional)

Uh oh!

nshirley Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nshirley Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

nshirley Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nshirley commented Apr 29, 2026 •

edited

Loading

nshirley Apr 30, 2026 •

edited

Loading