Просмотр исходного кода

docs: update testing and TDD documentation; remove obsolete TDD command and enhance essential test skills

Yuki Takei 1 неделя назад
Родитель
Сommit
4cbd0e1ab9

+ 0 - 287
.claude/commands/tdd.md

@@ -1,287 +0,0 @@
----
-name: tdd
-description: Enforce test-driven development workflow. Scaffold interfaces, generate tests FIRST, then implement minimal code to pass. Ensure 80%+ coverage.
----
-
-# TDD Command
-
-This command enforces test-driven development methodology for GROWI.
-
----
-
-## ⚠️ CRITICAL: MANDATORY TDD PROTOCOL
-
-**THIS IS NOT OPTIONAL. THIS IS A TEAM REQUIREMENT.**
-
-### Non-Negotiable Rules
-
-1. **ALL phases of TDD MUST be followed: RED → GREEN → REFACTOR**
-2. **Tests MUST be written BEFORE any implementation code (RED phase)**
-3. **Implementation MUST be minimal and only to pass tests (GREEN phase)**
-4. **Tests MUST remain passing during refactoring (REFACTOR phase)**
-5. **NEVER skip any phase of the TDD cycle**
-6. **If you believe you cannot follow TDD, STOP and ask the user IMMEDIATELY**
-
-### Violation Protocol
-
-If you find yourself about to deviate from the TDD cycle:
-
-1. **STOP IMMEDIATELY** - Do not proceed with any phase out of order
-2. **ASK THE USER** - Explain why you think following TDD is difficult
-3. **WAIT FOR APPROVAL** - Do not proceed without explicit user consent
-4. **DOCUMENT THE EXCEPTION** - If approved, document why TDD was skipped
-
-**Example of required user confirmation:**
-
-```
-"This task requires writing integration tests with complex setup
-(database mocks, authentication, etc.).
-
-I have two options:
-
-Option A: Follow strict TDD (recommended)
-- RED: Write failing integration test first
-- GREEN: Implement minimal code to pass
-- REFACTOR: Improve code while keeping tests green
-- May take 15-30 minutes for test setup
-- Ensures proper test coverage
-
-Option B: Skip TDD for this specific change
-- Implement directly
-- Verify with existing test suite only
-- Risk: No specific test for this change
-
-Which approach would you prefer?"
-```
-
-**Never assume. Always ask.**
-
----
-
-## Pre-Implementation Checklist
-
-Before writing ANY code, confirm:
-
-- [ ] I have read and understood the task requirements
-- [ ] I am starting with Step 1: Write a FAILING test (RED phase)
-- [ ] I have NOT written any implementation code yet
-- [ ] I will follow all phases: RED → GREEN → REFACTOR
-- [ ] If I cannot follow the complete TDD cycle, I have ASKED THE USER for guidance
-
-**If any checkbox is unchecked, STOP and ask the user.**
-
----
-
-## What This Command Does
-
-1. **Scaffold Interfaces** - Define types/interfaces first
-2. **Generate Tests First** - Write failing tests (RED)
-3. **Implement Minimal Code** - Write just enough to pass (GREEN)
-4. **Refactor** - Improve code while keeping tests green (REFACTOR)
-5. **Verify Coverage** - Ensure 80%+ test coverage
-
-## When to Use
-
-Use `/tdd` when:
-- Implementing new features
-- Adding new functions/components
-- Fixing bugs (write test that reproduces bug first)
-- Refactoring existing code
-- Building critical business logic
-
-## TDD Cycle
-
-```
-RED → GREEN → REFACTOR → REPEAT
-
-RED:      Write a failing test
-GREEN:    Write minimal code to pass
-REFACTOR: Improve code, keep tests passing
-REPEAT:   Next feature/scenario
-```
-
-## Example Usage
-
-```
-User: /tdd I need a function to validate page paths
-
-Agent:
-```
-
-# TDD Session: Page Path Validator
-
-## Step 1: Define Interface (SCAFFOLD)
-
-```typescript
-// src/utils/page-path-validator.ts
-export interface ValidationResult {
-  isValid: boolean;
-  errorMessage?: string;
-}
-
-export function validatePagePath(path: string): ValidationResult {
-  throw new Error('Not implemented');
-}
-```
-
-## Step 2: Write Failing Test (RED)
-
-**⚠️ CHECKPOINT: Confirm you have NOT written any implementation code yet.**
-
-```typescript
-// src/utils/page-path-validator.spec.ts
-describe('validatePagePath', () => {
-  it('should return valid for correct path', () => {
-    const result = validatePagePath('/user/docs/page');
-    expect(result.isValid).toBe(true);
-  });
-
-  it('should reject path without leading slash', () => {
-    const result = validatePagePath('user/docs');
-    expect(result.isValid).toBe(false);
-    expect(result.errorMessage).toBeDefined();
-  });
-
-  it('should reject empty path', () => {
-    const result = validatePagePath('');
-    expect(result.isValid).toBe(false);
-  });
-});
-```
-
-## Step 3: Run Tests - Verify FAIL
-
-**⚠️ MANDATORY: Tests MUST fail before proceeding to implementation.**
-
-```bash
-turbo run test --filter @growi/app -- src/utils/page-path-validator.spec.ts
-
-FAIL src/utils/page-path-validator.spec.ts
-  ✕ should return valid for correct path
-    Error: Not implemented
-```
-
-**✅ CHECKPOINT PASSED: Tests fail as expected. Ready to implement.**
-
-**❌ If tests pass or don't run: STOP. Fix the test first.**
-
-## Step 4: Implement Minimal Code (GREEN)
-
-**⚠️ CHECKPOINT: Only write the MINIMUM code needed to pass the tests.**
-
-```typescript
-export function validatePagePath(path: string): ValidationResult {
-  if (!path) {
-    return { isValid: false, errorMessage: 'Path cannot be empty' };
-  }
-  if (!path.startsWith('/')) {
-    return { isValid: false, errorMessage: 'Path must start with /' };
-  }
-  return { isValid: true };
-}
-```
-
-## Step 5: Run Tests - Verify PASS
-
-**⚠️ MANDATORY: ALL tests MUST pass before proceeding to refactoring.**
-
-```bash
-turbo run test --filter @growi/app -- src/utils/page-path-validator.spec.ts
-
-PASS  ✓ All tests passing!
-```
-
-**✅ CHECKPOINT PASSED: Ready to refactor if needed.**
-
-**❌ If tests fail: Fix implementation, do NOT move to refactoring.**
-
-## Step 6: Check Coverage
-
-**⚠️ MANDATORY: Verify test coverage meets requirements (80% minimum).**
-
-```bash
-cd {package_dir} && pnpm vitest run --coverage src/utils/page-path-validator.spec.ts
-
-Coverage: 100% ✅ (Target: 80%)
-```
-
-**✅ TDD CYCLE COMPLETE: All phases completed successfully.**
-
-- ✅ RED: Failing tests written
-- ✅ GREEN: Implementation passes tests
-- ✅ REFACTOR: Code improved (if needed)
-- ✅ COVERAGE: 80%+ achieved
-
-## TDD Best Practices
-
-**DO:**
-- ✅ Write the test FIRST, before any implementation
-- ✅ Run tests and verify they FAIL before implementing
-- ✅ Write minimal code to make tests pass
-- ✅ Refactor only after tests are green
-- ✅ Add edge cases and error scenarios
-- ✅ Aim for 80%+ coverage (100% for critical code)
-- ✅ Use `vitest-mock-extended` for type-safe mocks
-
-**DON'T:**
-- ❌ Write implementation before tests
-- ❌ Skip running tests after each change
-- ❌ Write too much code at once
-- ❌ Ignore failing tests
-- ❌ Test implementation details (test behavior)
-- ❌ Mock everything (prefer integration tests)
-
-## Test Types to Include
-
-**Unit Tests** (`*.spec.ts`):
-- Happy path scenarios
-- Edge cases (empty, null, max values)
-- Error conditions
-- Boundary values
-
-**Integration Tests** (`*.integ.ts`):
-- API endpoints
-- Database operations
-- External service calls
-
-**Component Tests** (`*.spec.tsx`):
-- React components with hooks
-- User interactions
-- Jotai state integration
-
-## Coverage Requirements
-
-- **80% minimum** for all code
-- **100% required** for:
-  - Authentication/authorization logic
-  - Security-critical code
-  - Core business logic (page operations, permissions)
-  - Data validation utilities
-
-## Important Notes
-
-**MANDATORY - NO EXCEPTIONS**: The complete TDD cycle MUST be followed:
-
-1. **RED** - Write failing test FIRST
-2. **GREEN** - Implement minimal code to pass the test
-3. **REFACTOR** - Improve code while keeping tests green
-
-**Absolute Requirements:**
-- ❌ NEVER skip the RED phase
-- ❌ NEVER skip the GREEN phase
-- ❌ NEVER skip the REFACTOR phase
-- ❌ NEVER write implementation code before tests
-- ❌ NEVER proceed without explicit user approval if you cannot follow TDD
-
-**If you violate these rules:**
-1. STOP immediately
-2. Discard any implementation code written before tests
-3. Inform the user of the violation
-4. Start over with RED phase
-
-**This is a team development standard. Violations are not acceptable.**
-
-## Related Skills
-
-This command uses patterns from:
-- **growi-testing-patterns** - Vitest, React Testing Library, vitest-mock-extended

+ 39 - 1
.claude/rules/testing.md

@@ -35,4 +35,42 @@ pnpm vitest run yjs.integ --repeat=10
 turbo run test --filter @growi/app
 ```
 
-For testing patterns (mocking, assertions, structure), see the `.claude/skills/learned/essential-test-patterns` skill.
+## Essential Test Skills (MANDATORY)
+
+Whenever you **write** a test OR **review** a change that adds/modifies tests, you
+MUST consult both skills first — they are not optional background reading:
+
+| Skill | Apply it to |
+|-------|-------------|
+| **essential-test-design** (`.claude/skills/essential-test-design/SKILL.md`) | *What* to assert — test the observable contract, not the mechanism. Catches brittle implementation-spies and assertion-free "it didn't throw" tests. |
+| **essential-test-patterns** (`.claude/skills/essential-test-patterns/SKILL.md`) | *How* to build the test — Vitest globals, RTL, Jotai scopes, type-safe mocking, module mocking strategy. |
+
+This applies in every context, including review-time skills (`kiro-review`,
+`kiro-validate-impl`, `kiro-verify-completion`): a test diff is not "good" until it
+has been checked against essential-test-design (contract) and essential-test-patterns
+(mechanics).
+
+## Type-Safe Mocks — avoid type assertions (`as any`, `as unknown as T`, `as T`)
+
+Mocking an interface/class? Use `mock<T>()` / `mock<T>({ ...overrides })` from
+`vitest-mock-extended` (already a dependency). It returns a real `T` (no cast),
+type-checks the overrides against the type, and auto-stubs everything you don't
+specify — so it is both **type-safe** and **shorter** than a hand-built object.
+
+Every assertion form defeats the type checker in some way and lets the mock drift
+out of sync with the real type silently — prefer `mock<T>()` over all of them:
+
+```typescript
+// ❌ all of these escape type checking — the mock can rot as Crowi changes
+const crowi = { searchService: { searchKeyword: vi.fn() } } as unknown as Crowi;
+const crowi = { searchService: { searchKeyword: vi.fn() } } as Crowi;
+const crowi = { searchService: { searchKeyword: vi.fn() } } as any;
+
+// ✅ type-checked against Crowi, auto-stubs the rest
+const crowi = mock<Crowi>({ searchService: { searchKeyword: vi.fn() } });
+```
+
+A type assertion in a test is only acceptable when removing it would cost more than
+it saves (no type exists for the target, or one field needs real behavior). For the
+full tolerance framework (4 tiers) and the `mock<T>` patterns, see the **Type-Safe
+Mocking** section of the `essential-test-patterns` skill.

+ 0 - 0
.claude/skills/learned/essential-test-design/SKILL.md → .claude/skills/essential-test-design/SKILL.md


+ 72 - 1
.claude/skills/learned/essential-test-patterns/SKILL.md → .claude/skills/essential-test-patterns/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: essential-test-patterns
-description: GROWI testing patterns with Vitest, React Testing Library, and vitest-mock-extended.
+description: GROWI testing patterns with Vitest, React Testing Library, and vitest-mock-extended (type-safe mocking, avoid type assertions). Auto-invoked when writing or reviewing tests.
 ---
 
 # GROWI Testing Patterns
@@ -116,6 +116,77 @@ mockProps.onSubmit?.mockImplementation((value) => {
 - ✅ **Deep mocking**: Automatically mocks nested objects
 - ✅ **Vitest integration**: Works seamlessly with `vi.fn()`
 
+### `mock<T>()` vs `mockDeep<T>()` — and `mock<T>({ ...overrides })`
+
+```typescript
+import { mock, mockDeep } from 'vitest-mock-extended';
+
+// Auto-stub everything, override just the members the test touches.
+// The override object is type-checked against Crowi (PartialDeep<Crowi>),
+// and the return value IS a Crowi — no cast needed.
+const crowi = mock<Crowi>({
+  searchService: { searchKeyword: vi.fn() },
+});
+
+// mockDeep<T>() recursively proxies nested access (foo.bar.baz.method()
+// all return mocks lazily). Use when the code-under-test reaches deep into
+// nested members you don't want to enumerate.
+const configManager = mockDeep<IConfigManagerForApp>();
+```
+
+- `mock<T>(overrides?)` — shallow auto-stub + a typed partial override. The common
+  case for service/Crowi-style dependencies.
+- `mockDeep<T>()` — recursive proxy for arbitrarily deep nested access.
+
+### Avoid type assertions for mocks — `as any`, `as unknown as T`, `as T`
+
+All three assertion forms **disable the type checker** for the mock, just in
+different ways, so prefer `mock<T>()` over every one of them:
+
+- `as unknown as Crowi` — the usual escape hatch; compiles even when the real type
+  drifts (a method is renamed or removed), leaving the mock silently wrong, and lets
+  you assign members that do not exist.
+- `as Crowi` — same problem when it compiles; TypeScript only blocks it when the
+  object structurally clashes, which then pushes people to `as unknown as` or `as any`.
+- `as any` — the broadest hole: erases the type entirely, so *nothing* about the
+  mock is checked and IDE autocomplete dies too.
+
+`mock<T>({ ...overrides })` is both safer (the override is checked, so drift is a
+compile error) and shorter (no need to hand-build the object).
+
+```typescript
+// ❌ WRONG — every one of these escapes type checking and survives API drift
+const crowi = { searchService: { searchKeyword: vi.fn() } } as unknown as Crowi;
+const crowi = { searchService: { searchKeyword: vi.fn() } } as Crowi;
+const crowi = { searchService: { searchKeyword: vi.fn() } } as any;
+
+// ✅ CORRECT — type-checked, auto-stubs the rest, returns a real Crowi
+const crowi = mock<Crowi>({
+  searchService: { searchKeyword: vi.fn() },
+});
+```
+
+Anti-pattern to watch for: hand-writing a `Pick<T, ...>` shim type plus a builder
+function and *still* casting at the call site. That is more code than `mock<T>()`
+and is not type-safe — the worst of both. Replace it with `mock<T>({ ... })`.
+
+### Tolerance framework: when a type assertion is acceptable
+
+The deciding question is the cost: **how many lines (and how much clarity) does
+removing the assertion cost?**
+
+| Tier | Situation | Rule |
+|------|-----------|------|
+| **1 — Avoid (cost ≤ 0)** | Mocking an interface/class. `mock<T>()` is 1 line, type-safe, *shorter* than the manual object. | **No assertion.** Use `mock<T>()`. No exceptions. |
+| **2 — Localize (small cost)** | One field needs *real* behavior `mock<T>` can't give (e.g. a working `EventEmitter` whose listeners actually fire), and its type doesn't quite match. | Allowed, but **confine the cast to that one field**: `mock<Crowi>({ events: { page: realEmitter as unknown as PageEvent } })` — never cast the whole object. |
+| **3 — Allow + comment (large cost)** | No type exists for the target (untyped JS module, untyped third-party lib). `mock<T>` can't be built. | A 1-line cast is fine — **writing a dozens-of-lines shim just to delete it is overkill**. Leave a `// WHY:` comment (e.g. `// PageEvent is a JS file typed as 'any' in Crowi`). |
+| **4 — Forbidden** | A hand-built/`Pick<>` partial object that ends in a cast anyway. | Replace with `mock<T>()`. There is no reason to keep it. |
+
+Rule of thumb: if deleting the cast costs **≤ 0 lines**, it's mandatory (Tier 1);
+if it can be **localized to one field**, do that (Tier 2); if the **type itself is
+missing** and avoidance would cost dozens of lines, a commented 1-line cast is fine
+(Tier 3); a shim that still casts is never fine (Tier 4).
+
 ## React Testing Library Patterns
 
 ### Basic Component Test

+ 8 - 1
.kiro/steering/tdd.md

@@ -1,6 +1,13 @@
 # Test-Driven Development
 
-See: `.claude/commands/tdd.md`, `.claude/skills/learned/essential-test-patterns/SKILL.md` and `.claude/skills/learned/essential-test-design/SKILL.md`
+The RED → GREEN → REFACTOR enforcement workflow lives in the `kiro-impl` skill
+(`.claude/skills/kiro-impl/SKILL.md`), which gates every task on a captured
+failing-test (`RED_PHASE_OUTPUT`) before implementation.
+
+For how to *write* the tests well, see `.claude/skills/essential-test-design/SKILL.md`
+(test the contract, not the mechanism) and `.claude/skills/essential-test-patterns/SKILL.md`
+(Vitest / RTL / type-safe mocking). The `testing` rule (`.claude/rules/testing.md`)
+is always loaded and points to both.
 
 ## cc-sdd Specific Notes
 

+ 1 - 2
AGENTS.md

@@ -42,7 +42,6 @@ GROWI is a team collaboration wiki platform using Markdown, featuring hierarchic
 
 | Command | Description |
 |---------|-------------|
-| **/tdd** | Test-driven development workflow |
 | **/learn** | Extract reusable patterns from sessions |
 
 **apps/app Skills** (load via Skill tool when working in apps/app):
@@ -91,7 +90,7 @@ growi/
     ├── rules/              # Always loaded into every session
     ├── skills/             # Load on demand via Skill tool
     ├── agents/             # Specialized subagents
-    └── commands/           # User-invocable commands (/tdd, /learn)
+    └── commands/           # User-invocable commands (/learn)
 ```
 
 ## Development Guidelines

+ 1 - 1
apps/app/.claude/skills/app-specific-patterns/SKILL.md

@@ -6,7 +6,7 @@ user-invocable: false
 
 # App Specific Patterns (apps/app)
 
-For general testing patterns, see the global `.claude/skills/learned/essential-test-patterns` and `.claude/skills/learned/essential-test-design` skills.
+For general testing patterns, see the global `.claude/skills/essential-test-patterns` and `.claude/skills/essential-test-design` skills.
 
 ## Next.js Pages Router