-
-
Notifications
You must be signed in to change notification settings - Fork 4
fix: Add testing-guidelines and fix skill-writer #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| --- | ||
| name: testing-guidelines | ||
| description: Guide for writing tests. Use when adding new functionality, fixing bugs, or when tests are needed. Emphasizes integration tests, real-world fixtures, and regression coverage. | ||
| --- | ||
|
|
||
| # Testing Guidelines | ||
|
|
||
| Follow these principles when writing tests for this codebase. | ||
|
|
||
| ## Core Principles | ||
|
|
||
| ### 1. Mock External Services, Use Real Fixtures | ||
|
|
||
| **ALWAYS** mock third-party network services. **ALWAYS** use fixtures based on real-world data. | ||
|
|
||
| - Fixtures must be scrubbed of PII (use dummy data like `foo@example.com`, `user-123`) | ||
| - Capture real API responses, then sanitize them | ||
| - Never make actual network calls in tests | ||
|
|
||
| ### 2. Prefer Integration Tests Over Unit Tests | ||
|
|
||
| Focus on **end-to-end style tests** that validate inputs and outputs, not implementation details. | ||
|
|
||
| - Test the public interface, not internal methods | ||
| - Unit tests are valuable for edge cases in pure functions, but integration tests are the priority | ||
| - If refactoring breaks tests but behavior is unchanged, the tests were too coupled to implementation | ||
|
|
||
| ### 3. Minimize Edge Case Testing | ||
|
|
||
| Don't test every variant of a problem. | ||
|
|
||
| - Cover the **common path** thoroughly | ||
| - Skip exhaustive input permutations | ||
| - Skip unlikely edge cases that add maintenance burden without value | ||
| - One representative test per category of input is usually sufficient | ||
|
|
||
| ### 4. Always Add Regression Tests for Bugs | ||
|
|
||
| When a bug is identified, **ALWAYS** add a test that would have caught it. | ||
|
|
||
| - The test should fail before the fix and pass after | ||
| - Name it descriptively to document the bug | ||
| - This prevents the same bug from recurring | ||
|
|
||
| ### 5. Cover Every User Entry Point | ||
|
|
||
| **ALWAYS** have at least one basic test for each customer/user entry point. | ||
|
|
||
| - CLI commands, API endpoints, public functions | ||
| - Test the common/happy path first | ||
| - This proves the entry point works at all | ||
|
|
||
| ### 6. Tests Validate Before Manual QA | ||
|
|
||
| Tests are how we validate **ANY** functionality works before manual testing. | ||
|
|
||
| - Write tests first or alongside code, not as an afterthought | ||
| - If you can't test it, reconsider the design | ||
| - Passing tests should give confidence to ship | ||
|
|
||
| ## Technical Guidelines | ||
|
|
||
| ### File Organization | ||
|
|
||
| - Test files use `*.test.ts` extension | ||
| - Co-locate tests with source: `foo.ts` → `foo.test.ts` | ||
|
|
||
| ### Test Isolation | ||
|
|
||
| Every test must: | ||
| - Run independently without affecting other tests | ||
| - Use temporary directories for file operations | ||
| - Clean up resources in `afterEach` hooks | ||
|
|
||
| ```typescript | ||
| import { describe, it, expect, beforeEach, afterEach } from 'vitest'; | ||
| import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; | ||
| import { join } from 'node:path'; | ||
| import { tmpdir } from 'node:os'; | ||
|
|
||
| describe('my feature', () => { | ||
| let tempDir: string; | ||
|
|
||
| beforeEach(() => { | ||
| tempDir = join(tmpdir(), `warden-test-${Date.now()}`); | ||
| mkdirSync(tempDir, { recursive: true }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| rmSync(tempDir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it('does something with files', () => { | ||
| writeFileSync(join(tempDir, 'test.ts'), 'content'); | ||
| // ... test code | ||
| }); | ||
| }); | ||
| ``` | ||
|
|
||
| ### Pure Function Tests | ||
|
|
||
| For pure functions without side effects, no special setup is needed: | ||
|
|
||
| ```typescript | ||
| import { describe, it, expect } from 'vitest'; | ||
| import { matchGlob } from './matcher.js'; | ||
|
|
||
| describe('matchGlob', () => { | ||
| it('matches exact paths', () => { | ||
| expect(matchGlob('src/index.ts', 'src/index.ts')).toBe(true); | ||
| }); | ||
| }); | ||
| ``` | ||
|
|
||
| ## Running Tests | ||
|
|
||
| ```bash | ||
| pnpm test # Run all tests in watch mode | ||
| pnpm test:run # Run all tests once | ||
| ``` | ||
|
|
||
| ## Checklist Before Submitting | ||
|
|
||
| - [ ] New entry points have at least one happy-path test | ||
| - [ ] Bug fixes include a regression test | ||
| - [ ] External services are mocked with sanitized fixtures | ||
| - [ ] Tests validate behavior, not implementation | ||
| - [ ] No shared state between tests |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,3 +11,9 @@ name = "find-bugs" | |
| event = "pull_request" | ||
| actions = ["opened", "synchronize", "reopened"] | ||
| skill = "find-bugs" | ||
|
|
||
| [[triggers]] | ||
| name = "testing-guidelines" | ||
| event = "pull_request" | ||
| actions = ["opened", "synchronize", "reopened"] | ||
| skill = "testing-guidelines" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trigger references skill not designed for PR analysisMedium Severity The new Additional Locations (1) |
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skill name mismatches directory naming convention
Low Severity
The
skill-writerskill usesname: Skill Writer(title case with space) in the frontmatter, but its directory isskill-writer(kebab-case). All other skills follow the convention where the frontmatter name matches the directory name exactly (e.g.,find-bugs,security-review,testing-guidelines). This mismatch breaks skill discovery:discoverAllSkillsstores skills by frontmatter name, so looking up by"skill-writer"fails since the key is"Skill Writer". The name in the frontmatter should beskill-writerto match the directory.