diff --git a/AGENTS.md b/AGENTS.md index 17091d115..aecdf60e3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,10 +54,10 @@ isn't universal, that's called out in the item itself. - **Fix root causes, not symptoms — refactor over patch.** No `as any` / `// @ts-ignore` / try-catch swallow / defensive skips to make errors disappear (宁愿重构也不要打补丁). If a test fails, fix the code, not the test — the narrow exceptions (a wrong test contract; a test that never carried value) are in [`docs/references/develop-testing.md`](docs/references/develop-testing.md#writing-meaningful-tests-what-to-clean-up--not-write). - **Confirm before you fix.** Before touching a reported bug, reproduce it and confirm it actually exists — never fix from assumption. Capture the reproduction, then fix, **in that order** (确定 bug 存在 → 写测试或记录验证证据 → 修复); how to reproduce and what counts as capture are in [`docs/verification.md`](docs/verification.md) and the TDD entry below. -- **TDD/BDD first, for changes that alter observable behavior.** Write failing tests **before** implementing new or changed behavior, using BDD-style Chinese `describe`/`it` titles. Two narrow exceptions — neither a blanket file/task category — are in [`docs/references/develop-testing.md`](docs/references/develop-testing.md#when-tdd-doesnt-apply). (Runner, mocks, and how to run tests are in `docs/develop.md`.) +- **TDD/BDD first, for changes that alter observable behavior.** Write failing tests **before** implementing new or changed behavior, using BDD-style `describe`/`it` titles (Chinese or English). Two narrow exceptions — neither a blanket file/task category — are in [`docs/references/develop-testing.md`](docs/references/develop-testing.md#when-tdd-doesnt-apply). (Runner, mocks, and how to run tests are in `docs/develop.md`.) - **SOLID, high cohesion & low coupling — applied to the existing extension points.** Persistence is a small backend taxonomy (`Repo` / `DAO` / `OPFSRepo` / a few custom repos), not one pattern to default to — pick by matching an existing entity with the same needs; see [`docs/references/architecture-data.md`](docs/references/architecture-data.md#adding-an-entity). For messages, use `Group.on(...)`. Not every service takes the same constructor shape — context services vs. the Agent subsystem differ; see [`docs/references/architecture-services.md`](docs/references/architecture-services.md#adding-a-service). Depend on narrow interfaces (`IMessageQueue`, not `MessageQueue`). - **Direct replacement over adapter sandwiches.** When swapping a backend/library, replace in place — no `interface Foo + LegacyImpl + NewImpl` unless both must coexist at runtime. -- **Scope discipline — stay in your lane.** Bug fix ≠ cleanup PR. Touch only the files the task requires; leave unrelated files untouched (不要动和任务不相干的文件). Don't add helpers, abstractions, validation, or backwards-compat shims you don't need today. Three similar lines beats a premature abstraction. Don't remove or narrow currently supported behavior just to simplify a fix — only do so when the task or an already-verified contract explicitly calls for that change. +- **Scope discipline — stay in your lane.** Bug fix ≠ cleanup PR. Touch only the files the task requires; leave unrelated files untouched (不要动和任务不相干的文件). Don't add helpers, abstractions, validation, or backwards-compat shims you don't need today. Three similar lines beats a premature abstraction. Don't remove or narrow currently supported behavior just to simplify a fix — only do so when the task or an already-verified contract explicitly calls for that change. This rule also governs test cleanup — [`docs/references/develop-testing.md`](docs/references/develop-testing.md#scope--cleanup-boundary) operationalizes it for tests, it does not carve out an exception. - **No dead code or `// removed` markers** — git remembers. Delete unused code outright. - **Comments explain "why", not "what".** Do not use ephemeral review labels such as `finding N` or review-round identifiers in comments or test names. Permanent issue or PR references are allowed when useful, but must supplement—not replace—the explanation. Do not restate code, duplicate enclosing documentation, or leave stale comments after code changes. See [`docs/develop.md`](docs/develop.md#comment-discipline) for the full policy. diff --git a/docs/DOC-MAINTENANCE.md b/docs/DOC-MAINTENANCE.md index 88c39fa4d..c66c277f7 100644 --- a/docs/DOC-MAINTENANCE.md +++ b/docs/DOC-MAINTENANCE.md @@ -89,7 +89,7 @@ of sanitization patterns can otherwise look like matches — so don't rely on a | Doc | Owns | | --- | --- | | [`../AGENTS.md`](../AGENTS.md) | Engineering principles + architecture quick-map. Single source of truth; `CLAUDE.md` only `@import`s it. | -| [`develop.md`](./develop.md) | The concrete "how": commands, structure, style, i18n, commit/PR; testing mechanics split to [`references/develop-testing.md`](./references/develop-testing.md). | +| [`develop.md`](./develop.md) | The concrete "how": commands, structure, style, i18n, commit/PR; testing (design, cleanup policy, mechanics) split to [`references/develop-testing.md`](./references/develop-testing.md). | | [`pull-request.md`](./pull-request.md) | Detailed PR description structure and guidance for agents and contributors; the human-facing template remains lightweight. | | [`design.md`](./design.md) | The design system: theme mechanism, shadcn component selection, new-page recipe; tokens split to [`references/design-tokens.md`](./references/design-tokens.md), component palette to [`references/design-components.md`](./references/design-components.md), layout/motion/state/a11y patterns to [`references/design-patterns.md`](./references/design-patterns.md). | | [`verification.md`](./verification.md) | Lightweight end-to-end functional verification — throwaway scratch scripts driving the real built extension; report template split to [`references/verification-report-template.md`](./references/verification-report-template.md), debugging FAQ to [`references/verification-debugging.md`](./references/verification-debugging.md). | diff --git a/docs/README.md b/docs/README.md index 13fc84aa4..8db124fa5 100644 --- a/docs/README.md +++ b/docs/README.md @@ -7,7 +7,7 @@ | 文档 | 说明 | | --- | --- | | [`../AGENTS.md`](../AGENTS.md) | 工程原则、架构速览、AI/贡献者约定的单一信息源 —— 但仅相对 `CLAUDE.md`(其仅导入它)成立;`.github/copilot-instructions.md` 是 Copilot 的独立入口,与本文件共享的事实需在两边都改动时做一致性核对(parity review)。 | -| [`develop.md`](./develop.md) | 开发规范:命令、目录结构、编码风格、UI/主题、i18n、提交/PR 流程;测试机制(含 Vitest 性能)拆到 [`references/develop-testing.md`](./references/develop-testing.md)。**写代码前先读。** | +| [`develop.md`](./develop.md) | 开发规范:命令、目录结构、编码风格、UI/主题、i18n、提交/PR 流程;测试设计/清理口径与运行机制(含 Vitest 性能)拆到 [`references/develop-testing.md`](./references/develop-testing.md)。**写代码前先读。** | | [`pull-request.md`](./pull-request.md) | PR 描述指南:代理与贡献者使用的详细章节、按变更类型取舍规则、验证与审查信息要求。 | | [`design.md`](./design.md) | 设计系统参考:主题机制、shadcn 组件选型、新建页面配方总览;令牌完整值拆到 [`references/design-tokens.md`](./references/design-tokens.md),组件清单拆到 [`references/design-components.md`](./references/design-components.md),布局/响应式/动效/状态/无障碍范式拆到 [`references/design-patterns.md`](./references/design-patterns.md)。**做页面/对话框/区块前先读。** | | [`verification.md`](./verification.md) | 功能验证指南:用一次性 scratch 脚本驱动真实扩展做端到端验证(不跑全量 E2E、不加永久用例);报告模板拆到 [`references/verification-report-template.md`](./references/verification-report-template.md),调试 FAQ 拆到 [`references/verification-debugging.md`](./references/verification-debugging.md)。**验证改动是否真正跑通时读。** | diff --git a/docs/develop.md b/docs/develop.md index 691760815..065debd84 100644 --- a/docs/develop.md +++ b/docs/develop.md @@ -69,12 +69,15 @@ it as settled fact: (`bg-white`, `text-gray-500`, `dark:bg-gray-800`, `bg-[#fff]`); use design tokens (`bg-background`/ `text-foreground`/…) so light & dark both work. -Two conventions are enforced via built-in rules in `eslint.config.mjs`: `no-restricted-imports` bans +Three conventions are enforced via built-in rules in `eslint.config.mjs`: `no-restricted-imports` bans `@radix-ui/react-*` single packages (use the merged `radix-ui`) and the `sonner` `toast` export (use `notify`); -`no-restricted-syntax` bans `forwardRef` across `src/pages/**` (use React 19 `function` + ref-prop). +`no-restricted-syntax` bans `forwardRef` across `src/pages/**` (use React 19 `function` + ref-prop); and a +file-scoped `no-restricted-imports` on `tests/vitest.setup.ts` bans `./utils` / `@App/app/service*` / +`@App/pages/store*` so global test setup stays lightweight (as a per-file rule replacement it also drops the +sonner/radix restriction there — the file imports neither). `eslint-rules/harness.test.mjs` covers exactly four of these: `no-i18n-default-value`, `no-raw-color-classname`, the `radix-ui` pattern of `no-restricted-imports`, and `no-restricted-syntax` — not `require-last-error-check`, -and not the `sonner` pattern of `no-restricted-imports`. +not the `sonner` pattern of `no-restricted-imports`, and not the `tests/vitest.setup.ts` scope. `src/pages/components/ui/toast.ts` has an override that turns `no-restricted-imports` **entirely off** for that one file — not just the `sonner` half of it. Only the `sonner` exception is intentional: this is the one place @@ -131,7 +134,7 @@ React 19 + shadcn/ui (Radix UI primitives, "new-york" style) + Tailwind CSS v4 + This project uses Vitest for unit tests and Playwright for end-to-end tests. -> Mechanics, meaningful-test guidance, and Vitest performance hygiene live in [testing.md](./references/develop-testing.md). To verify a change end-to-end without growing the suite, see [verification.md](./verification.md). +> Test design, meaningful-test & cleanup guidance, run mechanics, and Vitest performance hygiene live in [testing.md](./references/develop-testing.md). To verify a change end-to-end without growing the suite, see [verification.md](./verification.md). ## i18n diff --git a/docs/references/develop-testing.md b/docs/references/develop-testing.md index 034fd47a5..00135f99a 100644 --- a/docs/references/develop-testing.md +++ b/docs/references/develop-testing.md @@ -3,37 +3,130 @@ > The **TDD/BDD-first principle** (write failing tests before implementation; fix code not tests) lives in > [AGENTS.md § Engineering Principles](../../AGENTS.md#engineering-principles). This section is the mechanics. -Vitest + happy-dom. Per-test budgets live in `vitest.config.ts` per project: non-UI projects (`fast`, -`isolated`) use 340ms; the `ui` project (`src/pages/**/*.test.{ts,tsx}` — React renders, including -`renderHook` tests in `.ts` files) uses 850ms because a render + interaction case genuinely costs 100–200ms -solo under coverage and worker parallelism multiplies that (fake-timer countdown cases have been observed at -~630ms under full local load). Don't pass `--test-timeout` on the CLI — it would override every project's -budget at once. Chrome APIs are mocked via -`@Packages/chrome-extension-mock` (`tests/vitest.setup.ts`). `MockMessage` is available for message-system tests. -`happy-dom` is patched via `patches/` (see `pnpm-workspace.yaml` `patchedDependencies`) to build its -invalid-selector `DOMException` lazily — the upstream eager construction captures a deep stack on every -`matches()`/`querySelector()` call, which is measurably slower at TSX-suite scale. No specific percentage is -tracked here since it isn't tied to a reproducible command/environment; if you need a number, measure -before/after this patch in the same environment using the JSON-report method below rather than trusting a -historical figure. +This guide owns how contributors design, write, review, clean up, and run automated tests. A test is not useful +merely because it raises coverage: it must protect an observable contract, fail for a relevant regression, and +cost less to understand and maintain than the confidence it provides. -- Co-locate `*.test.ts`/`*.test.tsx` next to source (or place in `tests`). -- BDD-style Chinese `describe`/`it` titles. Use `describe.concurrent()` / `it.concurrent()` where independent. -- Single file: `pnpm test -- --run path/to/file.test.ts`. -- Playwright tests are `*.spec.ts` files in `e2e`; they run with one worker and retain failure artifacts. Run targeted tests while iterating, then run `pnpm run lint` plus the relevant full suite before a PR. +## Applicability gate — read this first + +Not every section below applies to every change. Before designing or reviewing tests, check which of these the +**changed contract** actually touches. Skip a row silently if it doesn't apply — do not mark it "N/A" in a PR +description or commit message; that's ceremony, not evidence. + +| Does the contract involve… | If yes, use | +|---|---| +| A threshold, count, size, or other boundary value | [Cover the behavior space](#cover-the-behavior-space-deliberately) — boundary cases | +| An invalid input, rejected dependency, or failure path | [Cover the behavior space](#cover-the-behavior-space-deliberately) — invalid/failure cases | +| State held across calls, async work, or lifecycle (mount/unmount, subscribe/dispose) | [Cover the behavior space](#cover-the-behavior-space-deliberately) — state transitions | +| Ordering, concurrency, or overlapping operations | [Cover the behavior space](#cover-the-behavior-space-deliberately) — ordering/concurrency | +| Legacy input forms, cross-browser behavior, untrusted input, or access scope | [Cover the behavior space](#cover-the-behavior-space-deliberately) — compatibility/security | +| A real browser API, extension process boundary, or multiple components wired together | [Boundary selection](#choosing-a-test-boundary) — integration/E2E row | +| A mechanical source-text convention (an import ban, a naming rule) | [Writing meaningful tests](#writing-meaningful-tests-what-to-clean-up--not-write) — file-content assertion | +| A test that looks low-value but sits outside the files/behavior this task changes | [Scope & cleanup boundary](#scope--cleanup-boundary) | + +If none of these apply, a normal-case test plus the one or two boundaries the contract actually has is enough — +don't manufacture coverage for inapplicable categories. + +## Designing a test before writing it + +Start from behavior, not from the current implementation. Before writing assertions, state four things: + +1. **Contract** — what callers or users can observe. +2. **Trigger** — the input, event, state, or sequence that exercises the contract. +3. **Outcome** — the returned value, rendered state, persisted data, emitted message, or external call that must + result. +4. **Regression** — a plausible wrong implementation that this test would reject. + +If the regression cannot be named, the proposed test is probably asserting an implementation detail or a +tautology. For a reported bug, first reproduce and capture the failure as required by +[verification.md](../verification.md); then make the smallest test that fails for that confirmed cause. + +### Choosing a test boundary + +Choose the narrowest boundary that still observes the real contract: + +| Contract characteristic | Test boundary | +|---|---| +| Parsing, mapping, validation, selection, or state-transition logic | Pure unit test | +| Conditional UI, accessibility derivation, interaction, or variant-to-token mapping | Focused component render | +| Persistence, messages, retries, ordering, or lifecycle crossing an object boundary | Service or repository test | +| A real browser API, extension context, build entry, worker boundary, or several components wired together | Integration or E2E test — do not force this into a heavily mocked unit test to make it cheap | +| Permanent automation is genuinely infeasible or costs more than the regression risk warrants | [Throwaway verification](../verification.md) | + +### Cover the behavior space deliberately + +A behavior-changing test set normally starts with the **normal case** and then adds the boundaries and failure +paths that can change the outcome, gated by the [applicability check](#applicability-gate--read-this-first) above. +Do not stop after one happy-path example, and do not mechanically enumerate inputs that all execute the same +branch. + +| Category | What to cover | +|---|---| +| Normal case | Representative valid input completes successfully and produces the intended observable result. | +| Boundary cases | Empty and single-item inputs; first/last item; exact size, time, or count limit; values just below and above a threshold; missing optional fields; duplicate items; Unicode or special paths only when the code branches on them. | +| Invalid/failure cases | Malformed input, rejected dependency, permission denial, timeout, cancellation, partial data, unavailable capability. Assert whether the contract rejects, reports, retries, rolls back, or preserves prior state. | +| State transitions | Before/after state, repeated calls, idempotency, cleanup, unsubscribe/dispose, whether stale async work can overwrite newer work. | +| Ordering/concurrency | Out-of-order completion, overlapping operations, deduplication, exactly-once effects — only when production code promises them. | +| Compatibility/security | Legacy accepted forms, cross-browser branches, untrusted URLs or paths, access scope, payload limits — only when part of the contract. | + +Select cases by distinct equivalence classes and branches, not by sample count: + +- If `value === limit`, `value < limit`, and `value > limit` produce three different outcomes, cover all three — + that's three distinct branches, not three samples of one. +- If ten ordinary strings take the same path, one representative string is enough — that's one equivalence class, + however many inputs it has. +- An empty array earns its own test only when emptiness changes behavior; it's redundant when it follows the exact + same branch and assertion as a non-empty array. + +For bug fixes, include a regression case matching the confirmed failure conditions closely enough that +reintroducing the cause makes it fail. Also keep a normal-case assertion when the fix could accidentally narrow +existing supported behavior. + +### Assert outcomes, not incidental structure + +Prefer assertions at the public boundary: + +- Assert returned domain values, persisted records, visible state, accessibility attributes, messages, or the + minimum necessary collaborator call. +- Assert a collaborator call when the call itself is the contract ("do not write before approval", "publish + exactly once"); do not assert every internal call made along the way. +- Prefer exact assertions for structured output. Use broad `toContain`/truthiness assertions only when the + omitted details are intentionally outside the contract. +- A test name must describe what the body actually triggers and observes. Use BDD-style `describe`/`it` titles — + Chinese and English are both fine; avoid vague names such as `works`, `test1`, or a bug label without the + behavior. +- One test may contain several related assertions for one behavior. Don't split every property into a separate + setup-heavy test, and don't combine unrelated contracts into one scenario. + +### Mocks and fixtures + +Mock at external or expensive boundaries, not at every internal function. A useful mock makes the test +deterministic while preserving the production path under test. -### When TDD doesn't apply +- Prefer the repository's shared mocks — `@Packages/chrome-extension-mock` (Chrome APIs), `MockMessage` from + `@Packages/message/mock_message`, `@Tests/mocks/pageStores.ts` (page stores) — over hand-built partial objects. +- Give a mock only the behavior needed by the scenario, but keep it structurally compatible with the narrow + interface the subject consumes. +- Don't render a page with many live child sections and maintain unrelated client mocks just to assert a static + button count. Test the category generator, focused section behavior, or a real integration boundary. +- Assert how our code transforms, routes, persists, or reacts to what a mock returns — never that the mock + returned what it was configured to return (that's testing the mock, a no-value category below). +- Fixtures should be small enough that the meaningful difference is visible. Builders are useful when defaults + are stable and scenarios override only relevant fields; avoid builders that hide the input responsible for a + regression. + +## When TDD doesn't apply Two exceptions to the TDD/BDD-first principle, neither a blanket file/task category — write a failing test for everything else: - **Genuinely behavior-preserving work** — refactors, type cleanup, dead-code removal, mechanical renames, or a config/dependency change confirmed not to alter behavior. Verify it instead of testing it. - **Automated coverage is genuinely infeasible** — a pure visual/animation tweak, a bug reproducible only in a specific browser version or extension lifecycle stage, a copy/translation wording change, platform behavior that can't be automated reliably. Verify it manually and record the evidence instead of committing a pass-through or low-value test just to satisfy the rule — [`../verification.md`](../verification.md)'s scratch-extension workflow when the change needs the built extension or browser APIs, a simpler noted check otherwise. -### Writing meaningful tests (what to clean up / not write) +## Writing meaningful tests (what to clean up / not write) **Two distinct situations — don't conflate them.** A test that fails because *its own asserted contract* is wrong (a stale fixture, an assertion that was incorrect from the start, a contract that legitimately changed) — fix the test and say why. A test that never carried value regardless of pass/fail (see below) — clean it up independent of whether it's currently failing. Neither is license to weaken a valid regression test just to make CI pass. -A test earns its place by exercising **our own logic** and failing on a real regression. Don't write the "tests nothing" kinds below — and clean them up when you find them (delete the test; don't touch business logic): +A test earns its place by exercising **our own logic** and failing on a real regression. Don't write the "tests nothing" kinds below — and clean them up when you find them inside the [cleanup boundary](#scope--cleanup-boundary) (delete the test; don't touch business logic): - **Tautology** — asserting a constant equals its own literal definition (source `const FOO = [Type.BAR]`, test `expect(FOO).toEqual([Type.BAR])`). - **Genuine duplicate** — a whole file/block near-verbatim identical to another, differing only by irrelevant suffixes. @@ -41,7 +134,7 @@ A test earns its place by exercising **our own logic** and failing on a real reg - **Pure pass-through render** — `render()` that only asserts `x` shows up, with no branching / variant mapping / derived logic in the component. - **Testing the mock or framework, not our code** — configuring a `vi.fn()` then asserting it returned what it was fed; asserting a third-party lib's or the JS language's own semantics. - **Mislabeled** — the test name claims a behavior the body never triggers (e.g. claims to test abort but never calls abort). Worse than no test: it gives false confidence. -- **File-content assertion that belongs in a lint rule** — reading a source file and grepping its text for a token/string is a mechanical convention; express it as an ESLint rule, not a unit test. +- **File-content assertion that belongs in a lint rule** — reading a source file and grepping its text for a token/string is a mechanical convention; express it as an ESLint rule or a structural-harness check (see [develop.md § ESLint custom rules](../develop.md#eslint-custom-rules)), and land the verified replacement guard **before** deleting the Vitest test — e.g. `tests/vitest.setup.ts`'s lightweight-setup constraint is a file-scoped `no-restricted-imports` in `eslint.config.mjs`. Conversely, keep these — they look thin but carry real value: @@ -50,12 +143,118 @@ Conversely, keep these — they look thin but carry real value: - `instanceof` / `name` guards on custom `Error` subclasses, security-blocklist completeness, and similar regression guards. - The **only** coverage of a component / sub-component — deleting it removes coverage, not noise. -> **Verify each against the source before deleting.** Many "looks meaningless" tests actually exercise a real branch; judging in bulk from a scan over-flags heavily. Confirm the behavior genuinely exists / is covered elsewhere before removing anything. +### Gray-area calls + +When a case doesn't obviously fall on one side, use these: + +| Question | Call it this way | +|---|---| +| Observable contract vs. implementation detail? | If a caller/user could notice the value changing, it's the contract. If only the source structure changed (variable name, internal helper split), it's implementation detail — not worth its own test. | +| Distinct equivalence class vs. another sample? | Distinct only if a plausible bug would make this specific input produce a *different* outcome than the other cases already covered. Otherwise it's another sample of the same class. | +| Minimum necessary collaborator call vs. internal call assertion? | Assert a collaborator call only when *not* calling it (or calling it wrong) is itself the bug the test guards against. Otherwise assert the outcome, not the call. | +| Valuable thin test vs. pass-through test? | Thin but valuable if the component branches, maps, or derives something (see "keep" list above). Pass-through if it renders a prop with zero conditional logic in between. | + +### Scope & cleanup boundary + +`AGENTS.md`'s scope-discipline principle ("bug fix ≠ cleanup PR; touch only the files the task requires") governs +test cleanup exactly as it governs production code. This section operationalizes it for tests — it does not carve +out an exception. + +| Situation | Action | +|---|---| +| A no-value test (per the categories above) sits in a file this task is already changing, or directly covers the behavior this task changes | Clean it up as part of this PR — it's in scope. | +| A no-value test sits in a file/behavior this task does *not* otherwise touch | Don't delete it in this PR. Record it as an out-of-scope finding (e.g. a follow-up issue or task) instead. | +| You notice a repository-wide pattern (the same no-value shape recurring across many unrelated files) | Don't bulk-clean it here. Open a separate issue/PR scoped to that pattern. | +| A cleanup you already started turns out to span many unrelated files | Split it into its own PR rather than growing the current one. | +| A replacement lint/structural guard is the direct substitute for a Vitest test you're removing *in this task* | In scope — landing the guard is part of "replace before delete" for this test, not a repo-wide lint rollout. | +| A replacement lint/structural guard would also need to cover other, currently-untested files | Out of scope for this task; note it as a follow-up. | + +### Cleaning up tests safely + +Tests are production dependencies: stale or meaningless tests should be removed, but a failing or slow test is not +automatically meaningless. Classify before editing: + +| Symptom | Classification | Action | +|---|---|---| +| Asserted contract still valid; production violates it | Production regression | Fix the production code. | +| Requirements legitimately changed, or the assertion was wrong from the start | Wrong/obsolete contract | Update or replace the test; record the contract change. | +| Timing, leaked global state, nondeterministic ordering, or an uncontrolled dependency changes the result | Flaky test | Reproduce the flake and fix its cause; don't add retries or a large timeout without evidence. | +| Real browser/process/I/O work exceeds a pure-unit budget under CI contention | Misclassified integration work | Move it to the appropriate project or give that case a measured budget; don't delete the behavior or relax every test globally. | +| Matches a no-value category above; removing it loses no distinct regression detection | No-value test | Delete it (subject to the [scope boundary](#scope--cleanup-boundary) above) rather than preserving it for coverage numbers. | +| A file-content assertion protects a real convention, just in the wrong mechanism | Valuable constraint, wrong mechanism | Migrate per the file-content-assertion rule above — replacement guard first. | + +Before deleting or consolidating a test, verify each against the source, one by one — judging in bulk from a scan +over-flags heavily, and many "looks meaningless" tests actually exercise a real branch: + +1. Read the production path it claims to cover; don't judge from its name or line count. +2. Search for the same contract in nearby unit tests, caller tests, integration tests, E2E tests, lint rules, and + structural harnesses. +3. Identify the mutation/regression the test rejects. If another test would fail for the same regression, show + why; similar setup or output alone does not prove duplication. +4. Check whether the test is the only coverage of a conditional, error path, security boundary, lifecycle event, + browser variant, or historical regression. +5. Delete or consolidate only the redundant signal, not assertions that cover distinct branches. Parameterize + repeated setup when it makes the behavior matrix clearer. +6. Run the focused remaining tests, then the relevant full suite. For timing or concurrency issues, reproduce the + CI combination — see [Vitest Performance Hygiene](#vitest-performance-hygiene). + +## Author & reviewer checklists + +Two short checklists, not one merged list — the author executes steps, the reviewer checks for evidence. Fill in +the bracketed fields with the actual value; a checked box with no field filled in is not evidence. + +### Author + +- [ ] `Regression rejected:` — the plausible broken implementation this test would catch (not "missing mocks" or + unrelated setup). +- [ ] `Distinct branch:` — for each case beyond the normal one, the outcome it changes vs. the cases already + covered. +- [ ] `Selected boundary because:` — why this test boundary (pure unit / component / service / integration / + E2E / throwaway), per [Choosing a test boundary](#choosing-a-test-boundary). +- [ ] `Existing coverage searched:` — where (nearby unit/caller/integration/E2E/lint) you looked before adding or + deleting a test. +- [ ] `Replacement guard:` — if a test was removed per the file-content-assertion rule, the lint/structural rule + that now covers it, or "n/a". +- [ ] `Out-of-scope findings recorded:` — any no-value test noticed outside this task's boundary, or "none". +- [ ] `Focused/full suite actually run:` — the literal command(s) run, not "tests pass". + +### Reviewer + +- [ ] The named regression is real and the test would actually fail for it — not merely missing setup. +- [ ] Each new case is a distinct branch/equivalence class, not another sample of one already covered. +- [ ] The test boundary matches [Choosing a test boundary](#choosing-a-test-boundary); nothing browser/process-level + is forced into a mocked unit test, and nothing simple got a full integration/E2E render. +- [ ] Any deleted test is in scope per [Scope & cleanup boundary](#scope--cleanup-boundary), and — if it protected + a mechanical convention — the replacement guard exists and is verified, not just proposed. +- [ ] Assertions observe the contract at the public boundary, and the title matches the trigger and outcome. +- [ ] Commands in "focused/full suite actually run" were actually run (spot-check by re-running one). + +## Running tests + +Vitest + happy-dom. Per-test budgets live in `vitest.config.ts` per project: non-UI projects (`fast`, +`isolated`) use 340ms; the `ui` project (`src/pages/**/*.test.{ts,tsx}` — React renders, including +`renderHook` tests in `.ts` files) uses 850ms because a render + interaction case genuinely costs 100–200ms +solo under coverage and worker parallelism multiplies that (fake-timer countdown cases have been observed at +~630ms under full local load). Don't pass `--test-timeout` on the CLI — it would override every project's +budget at once. Chrome APIs are mocked via +`@Packages/chrome-extension-mock` (`tests/vitest.setup.ts`). `MockMessage` is available for message-system tests. +`happy-dom` is patched via `patches/` (see `pnpm-workspace.yaml` `patchedDependencies`) to build its +invalid-selector `DOMException` lazily — the upstream eager construction captures a deep stack on every +`matches()`/`querySelector()` call, which is measurably slower at TSX-suite scale. No specific percentage is +tracked here since it isn't tied to a reproducible command/environment; if you need a number, measure +before/after this patch in the same environment using the JSON-report method below rather than trusting a +historical figure. + +- Co-locate `*.test.ts`/`*.test.tsx` next to source (or place in `tests`). +- Use `describe.concurrent()` / `it.concurrent()` where independent. +- Single file: `pnpm test -- --run path/to/file.test.ts`. +- Playwright tests are `*.spec.ts` files in `e2e`; they run with one worker and retain failure artifacts. Run targeted tests while iterating, then run `pnpm run lint` plus the relevant full suite before a PR. -### Vitest Performance Hygiene +## Vitest Performance Hygiene - Keep `tests/vitest.setup.ts` lightweight. Shared setup should only install global browser/chrome mocks; heavier - feature helpers belong in opt-in test utilities. + feature helpers belong in opt-in test utilities. Enforced by a file-scoped `no-restricted-imports` in + `eslint.config.mjs`. - For files that use one fixed UI language, prefer `initTestLanguage()` from `tests/initTestLanguage.ts` in `beforeAll` over repeated `initLanguage()` calls inside every test. Tests that intentionally switch languages should keep explicit language setup. @@ -114,5 +313,5 @@ jq -r '.testResults[] | .name as $file | .assertionResults[] | [.duration, $file ``` > To **verify a change works end-to-end without growing the suite** — drive the real built extension with a -> throwaway scratch script — see [`VERIFICATION.md`](../verification.md). That is lightweight verification, not +> throwaway scratch script — see [`verification.md`](../verification.md). That is lightweight verification, not > the committed test suite owned by this section. diff --git a/eslint.config.mjs b/eslint.config.mjs index 78024e366..215f0a05a 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -97,6 +97,31 @@ export default [ files: ["src/pages/components/ui/toast.ts"], rules: { "no-restricted-imports": "off" }, }, + { + // 全局测试 setup 每个测试文件都要加载，引入重型模块会拖慢整个套件 + // （见 docs/references/develop-testing.md § Vitest Performance Hygiene）。 + files: ["tests/vitest.setup.ts"], + rules: { + "no-restricted-imports": [ + "error", + { + patterns: [ + { + group: [ + "./utils", + "./utils/*", + "@App/app/service", + "@App/app/service/*", + "@App/pages/store", + "@App/pages/store/*", + ], + message: "全局 setup 只安装浏览器/chrome mock，重型 helper 放到按需 import 的测试工具里。", + }, + ], + }, + ], + }, + }, { // new-ui 页面禁止 className 写原始颜色，必须走设计令牌以适配亮/暗主题。 files: ["src/pages/**/*.tsx"], diff --git a/scripts/git-staged-snapshot.test.mjs b/scripts/git-staged-snapshot.test.mjs index 8e9c941eb..591476103 100644 --- a/scripts/git-staged-snapshot.test.mjs +++ b/scripts/git-staged-snapshot.test.mjs @@ -24,8 +24,6 @@ function makeTempRepo() { tmpDirs.push(dir); mkdirSync(dir, { recursive: true }); git(dir, ["init", "-q"]); - git(dir, ["config", "user.email", "test@example.com"]); - git(dir, ["config", "user.name", "Test"]); return dir; } @@ -36,72 +34,48 @@ afterEach(() => { }); describe("materializeStagedSnapshot", () => { - it("暂存坏内容、工作区改回好内容时，快照应反映暂存区（坏）内容", () => { + it("工作区与暂存区内容不同时应物化暂存区内容", () => { const repo = makeTempRepo(); - writeFileSync(path.join(repo, "a.json"), JSON.stringify({ good: true })); - git(repo, ["add", "a.json"]); - git(repo, ["commit", "-q", "-m", "init"]); + const source = path.join(repo, "a.json"); - // 暂存"坏"内容 - writeFileSync(path.join(repo, "a.json"), JSON.stringify({ bad: true })); + writeFileSync(source, JSON.stringify({ source: "index" })); git(repo, ["add", "a.json"]); - // 工作区恢复为"好"内容（不影响索引） - writeFileSync(path.join(repo, "a.json"), JSON.stringify({ good: true })); + writeFileSync(source, JSON.stringify({ source: "working-tree" })); const dest = path.join(repo, "..", `snapshot-${Math.random().toString(36).slice(2)}`); tmpDirs.push(dest); materializeStagedSnapshot(repo, dest); const snapshotContent = JSON.parse(readFileSync(path.join(dest, "a.json"), "utf8")); - expect(snapshotContent).toEqual({ bad: true }); - }); - - it("未暂存的工作区改动不应出现在快照中", () => { - const repo = makeTempRepo(); - writeFileSync(path.join(repo, "a.json"), JSON.stringify({ v: 1 })); - git(repo, ["add", "a.json"]); - git(repo, ["commit", "-q", "-m", "init"]); - - // 只改工作区，不 git add - writeFileSync(path.join(repo, "a.json"), JSON.stringify({ v: 2 })); - - const dest = path.join(repo, "..", `snapshot-${Math.random().toString(36).slice(2)}`); - tmpDirs.push(dest); - materializeStagedSnapshot(repo, dest); - - const snapshotContent = JSON.parse(readFileSync(path.join(dest, "a.json"), "utf8")); - expect(snapshotContent).toEqual({ v: 1 }); + expect(snapshotContent).toEqual({ source: "index" }); }); // 与 check-i18n.mjs 同源的入口守卫问题：脚本路径含空格 / 非 ASCII 字符时 CLI 分支不执行， // 快照目录留空。pre-commit 里它与 check-i18n 用 `&&` 串联，两者一起静默放行坏提交。 describe("CLI 入口守卫（脚本路径含空格 / 非 ASCII 字符）", () => { - // 真实 spawn node 子进程（实测 200ms+），不适用 vitest.config.ts 给单元测试定的 340ms 预算。 + // 真实启动 Node 和 Git 子进程，不适用 vitest.config.ts 给进程内单元测试定的 340ms 预算。 const CLI_TIMEOUT = 15_000; - for (const dirName of ["with space", "中文目录"]) { - it( - `目录名 "${dirName}" 下仍应真正物化暂存快照`, - () => { - const repo = makeTempRepo(); - writeFileSync(path.join(repo, "a.json"), JSON.stringify({ v: 1 })); - git(repo, ["add", "a.json"]); - git(repo, ["commit", "-q", "-m", "init"]); - - const host = path.join(os.tmpdir(), `snapshot-cli-${Math.random().toString(36).slice(2)}`); - tmpDirs.push(host); - const scriptDir = path.join(host, dirName); - mkdirSync(scriptDir, { recursive: true }); - const scriptPath = path.join(scriptDir, "git-staged-snapshot.mjs"); - copyFileSync(path.join(SCRIPTS_DIR, "git-staged-snapshot.mjs"), scriptPath); - - const dest = path.join(host, "dest"); - execFileSync(process.execPath, [scriptPath, repo, dest], { stdio: "pipe" }); - - expect(existsSync(path.join(dest, "a.json"))).toBe(true); - }, - CLI_TIMEOUT - ); - } + it( + "目录名同时包含空格和非 ASCII 字符时仍应真正物化暂存快照", + () => { + const repo = makeTempRepo(); + writeFileSync(path.join(repo, "a.json"), JSON.stringify({ v: 1 })); + git(repo, ["add", "a.json"]); + + const host = path.join(os.tmpdir(), `snapshot-cli-${Math.random().toString(36).slice(2)}`); + tmpDirs.push(host); + const scriptDir = path.join(host, "测试 中文目录"); + mkdirSync(scriptDir, { recursive: true }); + const scriptPath = path.join(scriptDir, "git-staged-snapshot.mjs"); + copyFileSync(path.join(SCRIPTS_DIR, "git-staged-snapshot.mjs"), scriptPath); + + const dest = path.join(host, "dest"); + execFileSync(process.execPath, [scriptPath, repo, dest], { stdio: "pipe" }); + + expect(existsSync(path.join(dest, "a.json"))).toBe(true); + }, + CLI_TIMEOUT + ); }); }); diff --git a/src/pages/components/NameAvatar.test.tsx b/src/pages/components/NameAvatar.test.tsx index 5137cff71..1b6b9c967 100644 --- a/src/pages/components/NameAvatar.test.tsx +++ b/src/pages/components/NameAvatar.test.tsx @@ -5,10 +5,6 @@ import { getNameAvatarTone, NameAvatar } from "./NameAvatar"; afterEach(cleanup); describe("NameAvatar 名称头像", () => { - it("相同名称稳定映射到同一组设计令牌", () => { - expect(getNameAvatarTone("ScriptCat")).toEqual(getNameAvatarTone("ScriptCat")); - }); - it("返回 label token 类名而不是硬编码颜色", () => { const tone = getNameAvatarTone("ScriptCat"); expect(tone.bg).toMatch(/^bg-label-(green|blue|purple|orange|rose|teal|amber|indigo)-bg$/); @@ -16,6 +12,13 @@ describe("NameAvatar 名称头像", () => { expect(`${tone.bg} ${tone.text}`).not.toMatch(/#|hsl\(|rgb\(|dark:/); }); + it("哈希为负的种子仍映射到有效令牌", () => { + // "Tampermonkey" 的字符串哈希为负数，覆盖取模索引的负值保护；若退化为单次取模会取到 undefined + const tone = getNameAvatarTone("Tampermonkey"); + expect(tone).toBeDefined(); + expect(tone.bg).toMatch(/^bg-label-\w+-bg$/); + }); + it("渲染时使用尺寸参数和设计令牌类", () => { const { getByText } = render( diff --git a/src/pages/components/ui/empty-state.test.tsx b/src/pages/components/ui/empty-state.test.tsx index d378cb1ef..85feee551 100644 --- a/src/pages/components/ui/empty-state.test.tsx +++ b/src/pages/components/ui/empty-state.test.tsx @@ -1,13 +1,18 @@ -import { render, screen } from "@testing-library/react"; +import { cleanup, render, screen } from "@testing-library/react"; import { Inbox } from "lucide-react"; -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; import { EmptyState } from "./empty-state"; +afterEach(cleanup); + describe("基础空状态组件", () => { - it("渲染图标、标题与说明", () => { - render(); + it("非紧凑模式渲染大图标、加粗标题与说明", () => { + render(); - expect(screen.getByText("暂无数据")).toBeInTheDocument(); + const root = screen.getByTestId("empty"); + expect(root).toHaveClass("gap-3"); + expect(root.querySelector("svg")).toHaveClass("size-10"); + expect(screen.getByText("暂无数据")).toHaveClass("font-medium"); expect(screen.getByText("稍后再试")).toBeInTheDocument(); }); diff --git a/src/pages/options/routes/Agent/components/AgentEmptyState.test.tsx b/src/pages/options/routes/Agent/components/AgentEmptyState.test.tsx deleted file mode 100644 index 87db9d90e..000000000 --- a/src/pages/options/routes/Agent/components/AgentEmptyState.test.tsx +++ /dev/null @@ -1,20 +0,0 @@ -import { describe, it, expect, afterEach } from "vitest"; -import { render, cleanup, screen } from "@testing-library/react"; -import { Server } from "lucide-react"; -import { AgentEmptyState } from "./AgentEmptyState"; - -afterEach(() => cleanup()); - -describe("AgentEmptyState 空状态", () => { - it("复用统一状态屏语义", () => { - render(); - const root = screen.getByTestId("empty-state"); - expect(root).toHaveAttribute("data-slot", "state-screen"); - expect(root).toHaveAttribute("role", "status"); - }); - it("无 action 时仍正常渲染", () => { - render(); - expect(screen.getByText("空")).toBeInTheDocument(); - expect(screen.getByText("说明")).toBeInTheDocument(); - }); -}); diff --git a/src/pages/options/routes/Setting/index.test.tsx b/src/pages/options/routes/Setting/index.test.tsx deleted file mode 100644 index 641f402a4..000000000 --- a/src/pages/options/routes/Setting/index.test.tsx +++ /dev/null @@ -1,56 +0,0 @@ -import { describe, it, expect, vi, beforeAll, afterEach, beforeEach } from "vitest"; -import { render, cleanup } from "@testing-library/react"; -import { MemoryRouter } from "react-router-dom"; -import { initTestLanguage } from "@Tests/initTestLanguage"; -import { mockIntersectionObserver } from "@Tests/mockIntersectionObserver"; -import { mockMatchMedia } from "@Tests/mockMatchMedia"; - -const { get, set } = vi.hoisted(() => ({ - get: vi.fn((key: string) => { - if (key === "cloud_sync") - return Promise.resolve({ enable: false, syncDelete: false, syncStatus: true, filesystem: "webdav", params: {} }); - if (key === "cat_file_storage") return Promise.resolve({ status: "unset", filesystem: "webdav", params: {} }); - if (key === "editor_preferences") - return Promise.resolve({ version: 1, fontSize: 14, mouseWheelScrollSensitivity: 1, smoothScrolling: true }); - if (key === "editor_config") return Promise.resolve("{}"); - if (key === "editor_type_definition") return Promise.resolve("declare const GM_info: unknown;"); - if (key === "enable_eslint") return Promise.resolve(false); - if (key === "eslint_config") return Promise.resolve("{}"); - return Promise.resolve("scriptcat"); - }), - set: vi.fn(), -})); -vi.mock("@App/pages/store/global", async () => { - const { createGlobalStoreMock } = await import("@Tests/mocks/pageStores.ts"); - return createGlobalStoreMock({ systemConfig: { get, set } }); -}); -vi.mock("./sections/GeneralSection", () => ({ GeneralSection: () => null })); -vi.mock("./sections/InterfaceSection", () => ({ InterfaceSection: () => null })); -vi.mock("./sections/SyncSection", () => ({ SyncSection: () => null })); -vi.mock("./sections/UpdateSection", () => ({ UpdateSection: () => null })); -vi.mock("./sections/RuntimeSection", () => ({ RuntimeSection: () => null })); -vi.mock("./sections/SecuritySection", () => ({ SecuritySection: () => null })); -vi.mock("./sections/DeveloperSection", () => ({ DeveloperSection: () => null })); -vi.mock("./sections/DeveloperMonacoEditor", () => ({ - DeveloperMonacoEditor: ({ ariaLabel }: { ariaLabel: string }) =>