From 9a7a00567ae4d5ed98ac34f1e5695546306e3909 Mon Sep 17 00:00:00 2001 From: postlog Date: Sun, 28 Jun 2026 18:29:14 +0300 Subject: [PATCH 1/4] chore: move the unit-test convention into a go-unit-tests skill Extract the unit-test rules from AGENTS.md into a tracked .claude/skills/go-unit-tests skill and have AGENTS.md reference it as the source of truth, instead of duplicating the table-driven / testify / gomock-from-contract.go convention inline. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/go-unit-tests/SKILL.md | 244 ++++++++++++++++++++++++++ AGENTS.md | 60 +------ CHANGELOG.md | 7 + 3 files changed, 260 insertions(+), 51 deletions(-) create mode 100644 .claude/skills/go-unit-tests/SKILL.md diff --git a/.claude/skills/go-unit-tests/SKILL.md b/.claude/skills/go-unit-tests/SKILL.md new file mode 100644 index 0000000..6e7b31e --- /dev/null +++ b/.claude/skills/go-unit-tests/SKILL.md @@ -0,0 +1,244 @@ +--- +name: go-unit-tests +description: >- + Write Go unit tests in a strict table-driven style using testify (require/assert) + and gomock (go.uber.org/mock) with mockgen-generated mocks from a per-package + contract.go. Use this skill whenever the user asks to write, add, generate, or fix + unit tests for Go code — for a handler, service, client, repository, or any exported + method — even if they don't say the words "table-driven" or "testify". Also use it + when reviewing or refactoring existing Go tests toward this convention, or when the + user mentions gomock, mockgen, contract.go, Mock* types, or *_test.go files in a Go + backend. +--- + +# Go Unit Tests + +This skill captures one specific, opinionated way of writing Go unit tests. The goal +is that every test you write looks like it was written by the same person: a strict +table-driven structure, `testify` for checks, and `gomock` mocks generated from a +per-package `contract.go`. Consistency here matters more than cleverness — a reviewer +should be able to read any test in the codebase without re-learning the layout. + +## The shape of every test + +One test function per **exported method**, named `Test{Type}_{Method}`. The table holds +the cases for **that one method** — it is not a place to dump cases for several methods. +Each method gets its own `Test{Type}_{Method}` with its own table. And do **not** split a +single method's cases the other way either, into `TestX_Success` / `TestX_Error` — all +cases for a method live together in its one table. Private helpers are never tested +directly; exercise them through the exported method that uses them. + +**When NOT to use a table.** The table earns its keep when a method has several distinct +cases worth enumerating. If a method is trivial or has effectively one case — say a +`CopyMap` that just copies a map, or a pure getter — a table is just ceremony. Write a +single straight-line test that sets up, calls the method, and asserts the behavior +directly. Reach for the table when there's real branching to cover; don't force one onto a +one-case method. + +The canonical template for the common (multi-case) shape — this is the target, match it: + +```go +func TestClient_Method(t *testing.T) { + targetErr := errors.New("test") + + tt := []struct { + name string + // input fields for this method go here + + // one build function per dependency, named after the dependency. + // Any of them may be left nil in a case where that dep isn't touched. + buildClientMock func(m *MockClient) + buildRepoMock func(m *MockusersRepo) + + result SomeOut + err error + wantErr bool // only when err is NOT a sentinel (see "Checking errors") + }{ + {name: "empty"}, + { + name: "success.one_user", + buildClientMock: func(m *MockClient) { m.EXPECT().X(gomock.Any()).Return(out, nil) }, + buildRepoMock: func(m *MockusersRepo) { m.EXPECT().GetByID(gomock.Any(), int64(7)).Return(u, nil) }, + result: SomeOut{ /* expected mapping */ }, + }, + { + name: "error.downstream", + buildClientMock: func(m *MockClient) { m.EXPECT().X(gomock.Any()).Return(nil, targetErr) }, + err: targetErr, + }, + } + + t.Parallel() + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + + client := NewMockClient(ctrl) + if tc.buildClientMock != nil { + tc.buildClientMock(client) + } + + repo := NewMockusersRepo(ctrl) + if tc.buildRepoMock != nil { + tc.buildRepoMock(repo) + } + + s := New(client, repo) + result, err := s.Method(context.Background() /*, tc.input */) + + require.ErrorIs(t, err, tc.err) + assert.Equal(t, tc.result, result) + }) + } +} +``` + +Why this shape: the table is the contract of the method made visible — a reader scans +`name` + `buildMock` + `result`/`err` and sees every branch at once. Per-method (not +per-outcome) functions keep that whole picture in one place. + +For a trivial method, the straight-line form is the right call — no table: + +```go +func TestCopyMap(t *testing.T) { + src := map[string]int{"a": 1, "b": 2} + + got := CopyMap(src) + + assert.Equal(t, src, got) // same contents + got["a"] = 99 + assert.Equal(t, 1, src["a"]) // and it's a real copy, not an alias +} +``` + +## Rules + +**Case names use dots, not spaces.** `success.one_user`, `error.invalid_user_id`, +`empty_fields`. They become subtest names (`go test -run`), so spaces are awkward and a +flat dotted namespace reads well in `-v` output. + +**Always cover, at minimum:** an `empty` case (when a zero/empty input is meaningful), a +`success` case that asserts the actual mapping/output (not just "no error"), and an +`error.*` case driven by a failing dependency. Add domain edge cases as the method +warrants — don't pad with cases that don't test a distinct branch. + +**Parallelism: `t.Parallel()` both on the table and inside `t.Run`.** The one exception +is when dependency codegen isn't goroutine-safe (a data race in the generated `New()` or +declarations) — then drop the parallel construction and say why in a short comment. + +**Mocks come from the package's own `contract.go` via mockgen.** Each package declares +its dependencies as a **private** interface in `contract.go` describing exactly the methods +it needs (interface segregation). **Name the interface after the concrete dependency it +points at, not after an abstract role.** A repository is `Repo` (`usersRepo`, +`nodesRepo`, `configsRepo`, `routingRepo`); a service is `Service` (`fleetService`, +`provisioningService`, `nodesService`); a client is `Client` (`panelClient`). +Role names that hide whether the thing is a repo, a service or a client — `deleter`, +`creator`, `subLinker`, `configResolver`, `mihomoReader` — are the wrong call: someone +reading the test then has to open `contract.go` to learn that `deleter` is "the provisioning +service." The name should answer *what is this dependency* on sight, so the test reads on its +own. Keep these interfaces unexported by default — they exist only for this package to depend +on and to generate mocks from, so there's no reason to export them. The only reason to export +one is a DI container or codegen tool that needs to import the interface (dig, wire, etc.); +absent that, lowercase it. The generated `Mock*` types live next to it in `contract_mocks.go` +(mockgen names them `Mock`, so a private `nodesRepo` interface gives +`MocknodesRepo` / `NewMocknodesRepo`). +In a test, use only the **local** `Mock*` from that package — never pull another package's +`clients.MockClient` into a service test. Each layer mocks its own contract. + +If `contract.go` doesn't exist yet for the dependencies you need to mock, create it with +the generation directive and run `go generate`: + +```go +// contract.go +//go:generate go tool mockgen -source=contract.go -destination contract_mocks.go -package $GOPACKAGE +package mypkg + +import "context" + +type usersRepo interface { + GetByID(ctx context.Context, id int64) (entity.User, error) +} +``` + +Then `go generate ./...` (mockgen is wired as a `go tool`). + +## Checking errors and results + +- Errors: `require.ErrorIs(t, err, tc.err)` against a **sentinel** error (the domain + defines `var ErrFoo = errors.New(...)`). Use `require.ErrorAs` for typed errors and + `require.NoError` when no error is expected. `require` (not `assert`) for errors so the + test stops before dereferencing a bad result. +- Use `wantErr bool` / `require.ErrorContains` **only** when there's no concrete sentinel + to match — prefer matching a sentinel whenever one exists. +- Results: `assert.Equal(t, tc.result, result)` — assert the whole expected value so the + mapping is verified, not just that something non-nil came back. + +## Readability and test helpers + +- **Group the table struct fields with blank lines**: name on its own, then the + input/expected-output fields (`in` / `expectedOut` read well), then the `buildXxxMock` + funcs. The visual grouping makes a long table scannable. +- **Use a `Must` helper for setup that returns `(T, error)`.** Many repos keep a generic + `func Must[T any](v T, err error) T` in an internal `utils/testing` package to unwrap + fixture construction without an `if err != nil` in every test. If one exists, use it + (`hash := testingUtils.Must(hashOf(data))`); building the *fixtures* shouldn't be where a + test's error handling lives — that belongs in the assertions on the method under test. +- **Assert concrete expected arguments in `EXPECT` when the arguments are the point.** If + the test is partly verifying that the method maps its input into the right call, spell out + the full expected struct/slice in `m.EXPECT().Save(gomock.Any(), wantArg).Return(...)` + rather than `gomock.Any()` — that's what catches a broken mapping. Use `gomock.Any()` only + for arguments that genuinely aren't under test (typically `context.Context`). + +## Setting up gomock expectations + +Inside `buildMock`/`buildMocks`, set expectations with `m.EXPECT()`: + +```go +m.EXPECT().DeleteUser(gomock.Any(), int64(7)).Return(nil) +``` + +Match concrete arguments when the value is part of what you're testing (e.g. that the +right id is passed through); use `gomock.Any()` for arguments that aren't the point of the +case (like a `context.Context`). **One build function per dependency**, named after the dependency it sets up +(`buildClientMock`, `buildRepoMock`), each taking only its own `Mock*`. Don't collapse +several deps into one combined `buildMocks(...)` — separate functions keep each case +readable and let a case leave a dependency's function `nil` when that case never touches +it (an untouched mock simply has no expectations set, so gomock won't fail on it). + +## HTTP / external-API clients + +Clients in `internal/clients//` are thin adapters — one method per file +(`add_client.go`), one test file per method. Don't hit the real network: mock HTTP with +`httptest.Server` or an `http.RoundTripper` mock, feed it the dependency's real wire +response (with its quirks), and assert the client maps it to the right `entity.*` value. +The test verifies the DTO→domain mapping, which is the only logic a client should have. + +## Workflow when asked to write tests + +1. Read the code under test and its package's `contract.go` (if present) to learn the + real dependency interfaces and the `entity.*` types involved. +2. Identify the exported methods that need tests and the sentinel errors they can return. +3. Write one `Test{Type}_{Method}` per method using the template above. +4. If mocks are missing, add/extend `contract.go` and run `go generate ./...`. +5. Run `go test ./...` (or the specific package) and make it green. Report the result — + if a test reveals a real bug in the code, surface it rather than bending the test to pass. + +## A note on scope + +This is the convention for **unit** tests — pure logic with mocked dependencies. Two +neighbors are deliberately out of scope, and forcing the mock-and-table template onto them +makes things worse, not better: + +- **Integration / API tests** are a separate thing (testify suites under a build tag, run + against real dependencies). If the user clearly wants an integration test against a live + service, say so and don't bend the unit-test template onto it. +- **Repository tests, in a codebase that already tests them against a real store.** Many Go + backends (subgen among them) run repository tests under a `//go:build integration` tag + against a real database via a `dbtest`-style helper — because the thing worth testing in a + repository *is* the SQL and the constraint-to-sentinel mapping, which a mocked DB can't + exercise. There, a repository test is an integration test; **don't mock the database.** The + exception is a pure-logic helper that merely lives under `repository/` (an error-class + detector that inspects a driver error, a small mapper) — no I/O, so it's an ordinary unit + test. When unsure which kind a package wants, glance at its sibling `*_test.go`: the build + tag and whether they spin up `dbtest` tell you immediately. diff --git a/AGENTS.md b/AGENTS.md index 6d718a3..eeaabd6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -455,57 +455,15 @@ panel's format, and the service layer is trivially mocked via `contract.go`. ## Unit tests -A strict table-driven structure. One `Test{Type}_{Method}` per **method** (do not -split into `Test*_Success` / `Test*_Error`). - -```go -func TestClient_Method(t *testing.T) { - targetErr := errors.New("test") - - tt := []struct { - name string - // input - buildMock func(mock *MockClient) // buildMocks(...) if there are several dependencies - result SomeOut - err error - wantErr bool // only if err is not a sentinel - }{ - {name: "empty"}, - {name: "success.one_user", buildMock: func(m *MockClient) { /* EXPECT */ }, result: SomeOut{ /*…*/ }}, - {name: "error.downstream", buildMock: func(m *MockClient) { m.EXPECT().X(gomock.Any()).Return(nil, targetErr) }, err: targetErr}, - } - - t.Parallel() - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - - mock := NewMockClient(ctrl) - if tc.buildMock != nil { - tc.buildMock(mock) - } - - c := New(mock) - result, err := c.Method(context.Background(), /* in */) - - require.ErrorIs(t, err, tc.err) - assert.Equal(t, tc.result, result) - }) - } -} -``` - -| Rule | Essence | -|---|---| -| `contract.go` + mockgen | Dependency interfaces — in the package's `contract.go`; `//go:generate go tool mockgen -source=contract.go` for services, entity and clients | -| Public API | We test only exported methods; private helpers — through the public one | -| Table-driven | One `Test{Type}_{Method}` per method; `tt []struct{ name, … }`; do **not** split into `*_Success`/`*_Error` | -| Case naming | `name` **without spaces**: `success.one_user`, `error.invalid_user_id`, `empty_fields` | -| Parallelism | `t.Parallel()` on the table **and** in `t.Run`. Exception: if the dependency codegen is not thread-safe (a data race in the generated `New()`/declaration) — without parallel construction | -| Branches | At a minimum: `empty` (if applicable), `success` with a mapping check, `error` from downstream; domain edge cases — by sense | -| Mocks | `mockgen` by one's own package `contract.go` → `Mock*`; do **not** drag a foreign `clients.MockClient` into a service/entity test | -| Assertions | `require.ErrorIs` / `ErrorAs` / `NoError` for errors; `assert.Equal` for the result; `wantErr` / `ErrorContains` — only when there is no specific sentinel | +The unit-test convention lives in the **`go-unit-tests` skill** +(`.claude/skills/go-unit-tests/SKILL.md`) — it is the source of truth for how unit tests are +written and reviewed here, so they are not duplicated in this document. In short: a strict +table-driven structure (one `Test{Type}_{Method}` per method, never split into +`*_Success`/`*_Error`), `testify` `require`/`assert`, and `gomock` mocks generated from a +per-package `contract.go`; dotted case names, `t.Parallel()` on the table and each subtest, +one build-func per dependency, sentinel checks via `require.ErrorIs`, and the judgment of when +*not* to reach for a table. Write and review unit tests by that skill. Integration / API tests +are a separate thing — see below. ## Integration / API tests diff --git a/CHANGELOG.md b/CHANGELOG.md index 4181471..9b9b948 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,13 @@ ADR catalog was removed; older entries below still mention their ADR by number). format are in [`AGENTS.md`](AGENTS.md) (section "Documenting changes"). There are no versions/tags: the service is not released, deploy is continuous. +## 2026-06-28 — Move the unit-test convention into a skill (#122) + +Extracted the unit-test rules out of `AGENTS.md` into a tracked **`go-unit-tests` skill** +(`.claude/skills/go-unit-tests/`); `AGENTS.md` now references the skill as the source of truth +for the table-driven / `testify` / `gomock`-from-`contract.go` convention instead of +duplicating it inline. + ## 2026-06-28 — Migrate to OpenSpec (#121) Adopted [OpenSpec](https://github.com/Fission-AI/OpenSpec) (the `spec-driven` schema) as the From 568299867727b0dc3306f77810895ccc56866a46 Mon Sep 17 00:00:00 2001 From: postlog Date: Sun, 28 Jun 2026 18:56:21 +0300 Subject: [PATCH 2/4] chore: scenario-focused skill description; trim AGENTS.md unit-tests to a pointer Rewrite the go-unit-tests skill description to say WHEN to apply the skill (triggering scenarios) instead of summarizing its rules, and reduce the AGENTS.md "Unit tests" section to a single sentence pointing at the skill. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/go-unit-tests/SKILL.md | 17 +++++++++-------- AGENTS.md | 10 +--------- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/.claude/skills/go-unit-tests/SKILL.md b/.claude/skills/go-unit-tests/SKILL.md index 6e7b31e..6170268 100644 --- a/.claude/skills/go-unit-tests/SKILL.md +++ b/.claude/skills/go-unit-tests/SKILL.md @@ -1,14 +1,15 @@ --- name: go-unit-tests description: >- - Write Go unit tests in a strict table-driven style using testify (require/assert) - and gomock (go.uber.org/mock) with mockgen-generated mocks from a per-package - contract.go. Use this skill whenever the user asks to write, add, generate, or fix - unit tests for Go code — for a handler, service, client, repository, or any exported - method — even if they don't say the words "table-driven" or "testify". Also use it - when reviewing or refactoring existing Go tests toward this convention, or when the - user mentions gomock, mockgen, contract.go, Mock* types, or *_test.go files in a Go - backend. + Use this skill whenever unit tests are involved in this Go backend — writing, adding, + generating, fixing, extending, or reviewing them — for a handler, service, client, or + any exported method. Trigger it when the user asks for a test or a `*_test.go` file, + wants more coverage for some function, needs a missing or failing unit test made to + pass, adds a case to a table-driven test, or needs a `contract.go` / mock created or + regenerated for a test — even if they never say "table-driven", "testify", "gomock", + or name this skill. Also trigger on mentions of gomock, mockgen, contract.go, or Mock* + types, and when refactoring existing Go tests toward the repo's convention. Reach for + it by default so every test in the codebase stays consistent rather than ad hoc. --- # Go Unit Tests diff --git a/AGENTS.md b/AGENTS.md index eeaabd6..d87cd20 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -455,15 +455,7 @@ panel's format, and the service layer is trivially mocked via `contract.go`. ## Unit tests -The unit-test convention lives in the **`go-unit-tests` skill** -(`.claude/skills/go-unit-tests/SKILL.md`) — it is the source of truth for how unit tests are -written and reviewed here, so they are not duplicated in this document. In short: a strict -table-driven structure (one `Test{Type}_{Method}` per method, never split into -`*_Success`/`*_Error`), `testify` `require`/`assert`, and `gomock` mocks generated from a -per-package `contract.go`; dotted case names, `t.Parallel()` on the table and each subtest, -one build-func per dependency, sentinel checks via `require.ErrorIs`, and the judgment of when -*not* to reach for a table. Write and review unit tests by that skill. Integration / API tests -are a separate thing — see below. +The rules for writing unit tests are described in the **`go-unit-tests` skill** (`.claude/skills/go-unit-tests/`). ## Integration / API tests From 78890d745e014a42116629ed373d6005739154b7 Mon Sep 17 00:00:00 2001 From: postlog Date: Sun, 28 Jun 2026 19:02:37 +0300 Subject: [PATCH 3/4] chore: move the scope note to the top of the skill and trim it Move "A note on scope" to the beginning of the skill and reduce it to a single statement that these rules apply in full only to unit tests; drop the detailed Integration/API and repository-tests subsections. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/go-unit-tests/SKILL.md | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/.claude/skills/go-unit-tests/SKILL.md b/.claude/skills/go-unit-tests/SKILL.md index 6170268..b8081df 100644 --- a/.claude/skills/go-unit-tests/SKILL.md +++ b/.claude/skills/go-unit-tests/SKILL.md @@ -20,6 +20,13 @@ table-driven structure, `testify` for checks, and `gomock` mocks generated from per-package `contract.go`. Consistency here matters more than cleverness — a reviewer should be able to read any test in the codebase without re-learning the layout. +## A note on scope + +These rules are for **unit** tests — pure logic with mocked dependencies — and apply in full +only to them. Other kinds of tests (integration / API tests, or repository tests that run +against a real store under a build tag) follow their own conventions; don't force this +mock-and-table template onto them. + ## The shape of every test One test function per **exported method**, named `Test{Type}_{Method}`. The table holds @@ -224,22 +231,3 @@ The test verifies the DTO→domain mapping, which is the only logic a client sho 4. If mocks are missing, add/extend `contract.go` and run `go generate ./...`. 5. Run `go test ./...` (or the specific package) and make it green. Report the result — if a test reveals a real bug in the code, surface it rather than bending the test to pass. - -## A note on scope - -This is the convention for **unit** tests — pure logic with mocked dependencies. Two -neighbors are deliberately out of scope, and forcing the mock-and-table template onto them -makes things worse, not better: - -- **Integration / API tests** are a separate thing (testify suites under a build tag, run - against real dependencies). If the user clearly wants an integration test against a live - service, say so and don't bend the unit-test template onto it. -- **Repository tests, in a codebase that already tests them against a real store.** Many Go - backends (subgen among them) run repository tests under a `//go:build integration` tag - against a real database via a `dbtest`-style helper — because the thing worth testing in a - repository *is* the SQL and the constraint-to-sentinel mapping, which a mocked DB can't - exercise. There, a repository test is an integration test; **don't mock the database.** The - exception is a pure-logic helper that merely lives under `repository/` (an error-class - detector that inspects a driver error, a small mapper) — no I/O, so it's an ordinary unit - test. When unsure which kind a package wants, glance at its sibling `*_test.go`: the build - tag and whether they spin up `dbtest` tell you immediately. From 2385cd4a0677e7e544ae8e6ed70d77d71f6f270c Mon Sep 17 00:00:00 2001 From: postlog Date: Sun, 28 Jun 2026 19:05:12 +0300 Subject: [PATCH 4/4] chore: manual wording edits to the go-unit-tests skill Tighten the intro and the scope note (cover integration / API / e2e generically). Co-Authored-By: Claude Opus 4.8 --- .claude/skills/go-unit-tests/SKILL.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/.claude/skills/go-unit-tests/SKILL.md b/.claude/skills/go-unit-tests/SKILL.md index b8081df..e5d7f79 100644 --- a/.claude/skills/go-unit-tests/SKILL.md +++ b/.claude/skills/go-unit-tests/SKILL.md @@ -15,17 +15,15 @@ description: >- # Go Unit Tests This skill captures one specific, opinionated way of writing Go unit tests. The goal -is that every test you write looks like it was written by the same person: a strict -table-driven structure, `testify` for checks, and `gomock` mocks generated from a -per-package `contract.go`. Consistency here matters more than cleverness — a reviewer +is that every test you write looks like it was written by the same person. +Consistency here matters more than cleverness — a reviewer should be able to read any test in the codebase without re-learning the layout. ## A note on scope These rules are for **unit** tests — pure logic with mocked dependencies — and apply in full -only to them. Other kinds of tests (integration / API tests, or repository tests that run -against a real store under a build tag) follow their own conventions; don't force this -mock-and-table template onto them. +only to them. Other kinds of tests (integration / API tests / e2e / etc.) follow their own conventions; don't force this +template onto them. ## The shape of every test