From 9165272d54f55ff65a56cbea528976e55bca1fd2 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 11:06:57 +0200 Subject: [PATCH] feat(testing): bootstrap 8 rule blocks in go-testing-guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restructures the 8 MUST sections in docs/go-testing-guide.md into canonical `### RULE` blocks. Same template as PRs #2-5, #8. Rules added (all judgment for this PR; mechanical ast-grep follow-ups tracked separately): - go-testing/no-stdlib-table-tests (MUST) Ginkgo-suite packages must use DescribeTable/Entry, not for/t.Run. - go-testing/no-testing-t-direct (MUST) No bare testing.T in Ginkgo-suite packages — use Describe/It. - go-testing/no-bare-error-call (MUST) Wrap error-returning calls in Gomega matchers (Succeed/HaveOccurred/ MatchError) — errcheck enforces this in precommit. - go-testing/suite-test-file-required (MUST) Every test-containing package needs *_suite_test.go or Ginkgo silently discovers no specs. - go-testing/main-test-with-compiles (MUST) Every binary needs main_test.go with a Compiles It-block backed by gexec.Build. - go-testing/suite-timeout-required (MUST) suiteConfig.Timeout must be set so hung tests fail fast. - go-testing/counterfeiter-mocks-required (MUST) No hand-written mocks — they drift silently. - go-testing/libtime-injection-required (MUST) Cross-references go-time/no-time-now-direct; test-quality scope. rules/index.json: 27 → 42 entries This also picks up 7 agent-cmd/* entries from PR #9 (#9 merged the doc but didn't include the walker output regen — caught locally via make build-index). Net new in this PR: 8 go-testing rules. Pre-emptive checks (lessons from PRs #6, #8): no personal vault paths, no trading-domain terms, no internal contradictions (rule examples don't violate sibling rules). --- docs/go-testing-guide.md | 104 +++++++++++++++++++++++++----- rules/index.json | 135 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 224 insertions(+), 15 deletions(-) diff --git a/docs/go-testing-guide.md b/docs/go-testing-guide.md index e3d4f27..ef7c1af 100644 --- a/docs/go-testing-guide.md +++ b/docs/go-testing-guide.md @@ -15,18 +15,29 @@ Key principles: ## Critical Rules -**MUST NOT use stdlib `testing` table-driven tests.** Always use Ginkgo `DescribeTable`/`Entry`. If a `*_suite_test.go` with Ginkgo imports exists in the package, all tests must use Ginkgo. +### RULE go-testing/no-stdlib-table-tests (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a Go test file in a package that has a Ginkgo `*_suite_test.go` uses `t.Run` inside a `for _, tt := range tests` loop instead of `DescribeTable`/`Entry`. +**Enforcement**: judgment (ast-grep follow-up) +**Why**: Mixed Ginkgo + stdlib tables produce inconsistent reporter output, fragmented runs, and surprises with `--focus` / `--label-filter`. Single-framework enforcement keeps test runs predictable. + +#### Bad ```go -// BAD — stdlib table-driven test +// stdlib table-driven test in a Ginkgo-suite package func TestFoo(t *testing.T) { tests := []struct{ input, want string }{ ... } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ... }) } } +``` + +#### Good -// GOOD — Ginkgo DescribeTable +```go +// Ginkgo DescribeTable var _ = DescribeTable("foo", func(input, expected string) { Expect(foo(input)).To(Equal(expected)) @@ -36,22 +47,60 @@ var _ = DescribeTable("foo", ) ``` -**MUST NOT use `testing.T` directly** in packages that have a Ginkgo test suite. Use `Describe`/`Context`/`It`/`DescribeTable`/`Entry` instead. +### RULE go-testing/no-testing-t-direct (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a Go file in a package that has a Ginkgo `TestSuite` entry-point uses `*testing.T` directly inside test functions other than the suite entry-point itself. +**Enforcement**: judgment (ast-grep follow-up) +**Why**: Direct `testing.T` use bypasses the Ginkgo lifecycle (`BeforeEach`/`AfterEach`/`JustBeforeEach`), produces flaky setup ordering, and breaks `--focus` filtering. Use `Describe`/`Context`/`It`/`DescribeTable`/`Entry` so the suite runs as one coherent test plan. -**MUST NOT call an error-returning function bare in an `It` block.** `errcheck` (run by `make precommit`) will fail. Wrap with a matcher that documents intent: +#### Bad + +```go +func TestUserService(t *testing.T) { // direct testing.T in a Ginkgo-suite package + t.Run("Create", func(t *testing.T) { + // ... + }) +} +``` -- Expecting success: `Expect(someFunc(ctx)).To(Succeed())` -- Expecting failure: `Expect(someFunc(ctx)).To(HaveOccurred())` -- Need the error: `err := someFunc(ctx); Expect(err).To(MatchError(...))` +#### Good ```go -// BAD — errcheck: "Error return value not checked" +var _ = Describe("UserService", func() { + Context("Create", func() { + It("creates a user with valid data", func() { + // ... + }) + }) +}) +``` + +### RULE go-testing/no-bare-error-call (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a Go test file inside an `It` / `BeforeEach` / `JustBeforeEach` / `AfterEach` block calls an error-returning function whose return value is discarded. +**Enforcement**: judgment (ast-grep follow-up — errcheck-equivalent scoped to Ginkgo blocks) +**Why**: `errcheck` (run by `make precommit`) flags discarded errors and breaks the build. Wrapping every error-returning call in a Gomega matcher (`Succeed()` / `HaveOccurred()` / `MatchError(...)`) documents the test's intent at the assertion site instead of relying on silent fall-through. + +Matcher choice by intent: +- Expecting success: `Expect(fn(ctx)).To(Succeed())` +- Expecting failure: `Expect(fn(ctx)).To(HaveOccurred())` +- Need the error for further assertions: `err := fn(ctx); Expect(err).To(MatchError(...))` + +#### Bad + +```go +// errcheck: "Error return value not checked" It("calls Save exactly twice", func() { service.Process(ctx) Expect(store.SaveCallCount()).To(Equal(2)) }) +``` -// GOOD — error explicitly accounted for +#### Good + +```go It("calls Save exactly twice", func() { Expect(service.Process(ctx)).To(HaveOccurred()) Expect(store.SaveCallCount()).To(Equal(2)) @@ -60,7 +109,12 @@ It("calls Save exactly twice", func() { ## Test Suite Setup -**MUST provide a `*_suite_test.go` file in every package with tests.** Without it, Ginkgo specs are not discovered and `make test` silently misses coverage. +### RULE go-testing/suite-test-file-required (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a Go package contains test files (`*_test.go`) but no `*_suite_test.go` file with a `TestSuite` entry-point and `RunSpecs`. +**Enforcement**: judgment (file-existence check; ast-grep follow-up) +**Why**: Without a suite file, Ginkgo specs are not discovered. `make test` exits 0 even though no specs ran — silent coverage loss. The suite file is the single entry-point Go's `testing` package invokes. ### Standard Package Suite @@ -101,7 +155,12 @@ Requirements: ### Main Package Suite (special case) -**MUST include `main_test.go` for every binary project.** Without it, build failures are not caught by `make test`. +### RULE go-testing/main-test-with-compiles (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a Go binary project (package `main` with `main.go`) does not have a `main_test.go` containing a `Compiles` It-block backed by `gexec.Build`. +**Enforcement**: judgment (file-existence + body check; ast-grep follow-up) +**Why**: Without `main_test.go` + a `Compiles` check, build failures in `main.go` are not caught by `make test`. The CI greenlight then deploys a binary that doesn't link. ```go // Copyright (c) 2026 Benjamin Borbe All rights reserved. @@ -177,7 +236,12 @@ var _ = Describe("Product", func() { ## Test Timeouts -**MUST set a suite-level timeout.** Every suite file includes `suiteConfig.Timeout` as a safety net against hanging tests. +### RULE go-testing/suite-timeout-required (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a `*_suite_test.go` file calls `GinkgoConfiguration()` without setting `suiteConfig.Timeout` before `RunSpecs`. +**Enforcement**: judgment (ast-grep follow-up — pattern over suite body) +**Why**: Without a suite-level timeout, a hung test holds the test runner indefinitely. CI eventually kills the job — but only after the job-level timeout (often 30+ minutes), wasting CI minutes and delaying feedback. Suite-level timeout is the safety net that fails fast. Per-spec timeout: @@ -199,7 +263,12 @@ Describe("slow subsystem", func() { ## Mock Generation -**MUST use Counterfeiter-generated mocks.** Never hand-write mocks — they drift from the interface and break silently. +### RULE go-testing/counterfeiter-mocks-required (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a test file declares a hand-written struct that satisfies a production interface and is used in place of a real implementation under test, instead of importing a `mocks/` fake produced by Counterfeiter. +**Enforcement**: judgment (presence of `//counterfeiter:generate` directive + hand-written fake detection; ast-grep follow-up) +**Why**: Hand-written mocks drift from the interface — when the production interface gains a method, the hand-written mock silently keeps satisfying the old surface (test still compiles, doesn't exercise the new contract). Counterfeiter-generated fakes regenerate from the interface, so any drift surfaces at `go generate` time. ### Generate Directive @@ -237,7 +306,12 @@ Expect(actualUser.Name).To(Equal("test")) ## Time Handling -**MUST inject time via `libtime.CurrentDateTimeGetter` from `github.com/bborbe/time`.** Never call `time.Now()` directly in business logic — tests cannot control it. +### RULE go-testing/libtime-injection-required (MUST) + +**Owner**: go-test-quality-assistant +**Applies when**: a Go business-logic file (outside `main.go`, `cmd/**`, `*_test.go`, `vendor/`) reads the current time. Tests cannot control `time.Now()` directly, so dependent code is unverifiable. +**Enforcement**: cross-rule — overlaps with `go-time/no-time-now-direct` (already in `rules/index.json`). This rule scopes the same constraint to test-coverage assessments: a service that doesn't inject time has no testable time-dependent paths. +**Why**: Without `libtime.CurrentDateTimeGetter` injection, every test that depends on time becomes flaky or impossible. `libtime.NewCurrentDateTime()` + `SetNow(fixedTime)` produces deterministic, fast tests for date math, expiry windows, scheduling, etc. ```go import libtime "github.com/bborbe/time" diff --git a/rules/index.json b/rules/index.json index 20e05fc..5b67976 100644 --- a/rules/index.json +++ b/rules/index.json @@ -1,4 +1,67 @@ [ + { + "anchor": "agent-cmd/agent-frontmatter", + "applies_when": "any `agents/*.md` file is created.", + "doc_path": "docs/agent-command-development-guide.md", + "enforcement": "judgment", + "id": "agent-cmd/agent-frontmatter", + "level": "MUST", + "owner": "agent-auditor" + }, + { + "anchor": "agent-cmd/command-frontmatter", + "applies_when": "any `commands/*.md` file is created.", + "doc_path": "docs/agent-command-development-guide.md", + "enforcement": "judgment", + "id": "agent-cmd/command-frontmatter", + "level": "MUST", + "owner": "slash-command-auditor" + }, + { + "anchor": "agent-cmd/command-thin", + "applies_when": "any new `commands/*.md` file is added or substantially changed.", + "doc_path": "docs/agent-command-development-guide.md", + "enforcement": "judgment", + "id": "agent-cmd/command-thin", + "level": "MUST", + "owner": "slash-command-auditor" + }, + { + "anchor": "agent-cmd/gap-driven-feedback", + "applies_when": "an agent depends on documented information that may be incomplete.", + "doc_path": "docs/agent-command-development-guide.md", + "enforcement": "judgment", + "id": "agent-cmd/gap-driven-feedback", + "level": "SHOULD", + "owner": "agent-auditor" + }, + { + "anchor": "agent-cmd/no-user-prompts", + "applies_when": "any agent or command performs work that could prompt the user (writing to `/tmp/`, requesting permissions, asking confirmation) during normal execution.", + "doc_path": "docs/agent-command-development-guide.md", + "enforcement": "judgment", + "id": "agent-cmd/no-user-prompts", + "level": "MUST", + "owner": "agent-auditor" + }, + { + "anchor": "agent-cmd/scripts-in-claude-dir", + "applies_when": "an agent depends on executable scripts (Python, shell) to do real work.", + "doc_path": "docs/agent-command-development-guide.md", + "enforcement": "judgment", + "id": "agent-cmd/scripts-in-claude-dir", + "level": "MUST", + "owner": "agent-auditor" + }, + { + "anchor": "agent-cmd/single-source-of-truth", + "applies_when": "an agent's domain has multiple potential data sources (config files, APIs, generated artifacts, documentation) and one of them is the authoritative, human-maintained source.", + "doc_path": "docs/agent-command-development-guide.md", + "enforcement": "judgment", + "id": "agent-cmd/single-source-of-truth", + "level": "SHOULD", + "owner": "agent-auditor" + }, { "anchor": "go-context/cancel-check-in-loop", "applies_when": "Go `for` loop body lacks a non-blocking `select { case <-ctx.Done(): ...; default: }` check, outside `*_test.go` and `vendor/`.", @@ -215,6 +278,78 @@ "level": "MUST", "owner": "go-security-specialist" }, + { + "anchor": "go-testing/counterfeiter-mocks-required", + "applies_when": "a test file declares a hand-written struct that satisfies a production interface and is used in place of a real implementation under test, instead of importing a `mocks/` fake produced by Counterfeiter.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "judgment (presence of `//counterfeiter:generate` directive + hand-written fake detection; ast-grep follow-up)", + "id": "go-testing/counterfeiter-mocks-required", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, + { + "anchor": "go-testing/libtime-injection-required", + "applies_when": "a Go business-logic file (outside `main.go`, `cmd/**`, `*_test.go`, `vendor/`) reads the current time. Tests cannot control `time.Now()` directly, so dependent code is unverifiable.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "cross-rule — overlaps with `go-time/no-time-now-direct` (already in `rules/index.json`). This rule scopes the same constraint to test-coverage assessments: a service that doesn't inject time has no testable time-dependent paths.", + "id": "go-testing/libtime-injection-required", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, + { + "anchor": "go-testing/main-test-with-compiles", + "applies_when": "a Go binary project (package `main` with `main.go`) does not have a `main_test.go` containing a `Compiles` It-block backed by `gexec.Build`.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "judgment (file-existence + body check; ast-grep follow-up)", + "id": "go-testing/main-test-with-compiles", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, + { + "anchor": "go-testing/no-bare-error-call", + "applies_when": "a Go test file inside an `It` / `BeforeEach` / `JustBeforeEach` / `AfterEach` block calls an error-returning function whose return value is discarded.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "judgment (ast-grep follow-up — errcheck-equivalent scoped to Ginkgo blocks)", + "id": "go-testing/no-bare-error-call", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, + { + "anchor": "go-testing/no-stdlib-table-tests", + "applies_when": "a Go test file in a package that has a Ginkgo `*_suite_test.go` uses `t.Run` inside a `for _, tt := range tests` loop instead of `DescribeTable`/`Entry`.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "judgment (ast-grep follow-up)", + "id": "go-testing/no-stdlib-table-tests", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, + { + "anchor": "go-testing/no-testing-t-direct", + "applies_when": "a Go file in a package that has a Ginkgo `TestSuite` entry-point uses `*testing.T` directly inside test functions other than the suite entry-point itself.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "judgment (ast-grep follow-up)", + "id": "go-testing/no-testing-t-direct", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, + { + "anchor": "go-testing/suite-test-file-required", + "applies_when": "a Go package contains test files (`*_test.go`) but no `*_suite_test.go` file with a `TestSuite` entry-point and `RunSpecs`.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "judgment (file-existence check; ast-grep follow-up)", + "id": "go-testing/suite-test-file-required", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, + { + "anchor": "go-testing/suite-timeout-required", + "applies_when": "a `*_suite_test.go` file calls `GinkgoConfiguration()` without setting `suiteConfig.Timeout` before `RunSpecs`.", + "doc_path": "docs/go-testing-guide.md", + "enforcement": "judgment (ast-grep follow-up — pattern over suite body)", + "id": "go-testing/suite-timeout-required", + "level": "MUST", + "owner": "go-test-quality-assistant" + }, { "anchor": "go-time/inject-getter-not-create", "applies_when": "a factory or constructor file outside `main.go` calls `libtime.NewCurrentDateTime()`.",