diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index f6542dd0..e7d44ce8 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -61,6 +61,7 @@ Go through each changed file and check for violations. Flag only actual problems - [ ] New functionality has tests (prefer integration tests) - [ ] Bug fixes have an integration test that reproduces the bug (fails before fix, passes after) +- [ ] E2E/integration tests assert observable behavior only (CLI output, exit codes, files, requests received by mocks) — not internal details; external systems may be mocked at the boundary - [ ] Interactive tests use PTY (`github.com/creack/pty`) - [ ] No unchecked errors outside of test files diff --git a/CLAUDE.md b/CLAUDE.md index e9f84f0e..032ff8c4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -232,6 +232,9 @@ When drafting Slack messages, PR descriptions, review replies, release notes, or # Testing +- **TDD is mandatory for every feature and bug fix (red → green → refactor).** First write an end-to-end integration test that reproduces the bug or specifies the new behavior, and run it to confirm it fails for the expected reason. Only then implement the change, and run the test again to confirm it passes. Never write the implementation first and backfill the test. +- E2E tests must assert only **observable behavior** — what a user sees through the CLI (exit codes, output, files written, requests the emulator/wrapped tool receives) — never internal details (function calls, internal state, log internals). If it's unclear what the observable behavior is, ask the user before writing the test. +- To keep e2e tests faithful, it's fine (and encouraged) to provide observable mocks of *external* systems — a mock auth/license endpoint, a fake `aws`/`az` binary on `PATH`, a fake browser opener. These mocks define the boundary between lstk and the outside world; do not mock lstk's own internals in e2e tests. Existing examples: `fakeBrowserOpener` in `test/integration/login_test.go`, mock platform-API servers. - Prefer integration tests to cover most cases. Use unit tests when integration tests are not practical. - Assertions use `github.com/stretchr/testify` (`require` for fatal checks). Snapshots use the in-house `internal/snap` helper: `snap.Match(t, s)` stores snapshots in one archive per test file (`__snapshots__/.snap`): a `[TestName_N]` header line per Match call, the value verbatim, and a `---` terminator on its own line (so a value and the same value plus one final newline are indistinguishable; other newlines are preserved exactly). Missing snapshots are created locally but fail in CI; `UPDATE_SNAPS=true go test` rewrites them. The format has no escaping, so a value containing a line that is exactly `---` is rejected (round-trip guard) — sanitize such values before matching. `snap.MatchJSON(t, raw, "data.currentVersion", ...)` snapshots a JSON document in canonical pretty-printed form, masking the values at the given dotted paths with `` (a path that doesn't resolve fails the test; objects only, no array indexing). A package using snapshots must wire `func TestMain(m *testing.M) { os.Exit(snap.Clean(m)) }` — obsolete snapshots then fail the run (or are deleted under `UPDATE_SNAPS=true`); cleanup is skipped on filtered (`-run`/`-skip`) or failed runs. Sanitize volatile values before matching with label-anchored regexes, not value-shaped ones (see `sanitizeSnapshot` in `internal/output`; RE2 has no lookahead, so a bare semver pattern corrupts IPv4 strings). - **When fixing a bug, always add an integration test** that fails before the fix and passes after. This prevents regressions and documents the exact scenario that was broken.