diff --git a/openspec/changes/specify-go-code-standards/design.md b/openspec/changes/specify-go-code-standards/design.md index f447697..d19d0c6 100644 --- a/openspec/changes/specify-go-code-standards/design.md +++ b/openspec/changes/specify-go-code-standards/design.md @@ -60,24 +60,28 @@ missing. Rejected — consistency in what a rule permits is not the same as consistency in what a repository does, and the second is not worth a dependency. -### `export_test.go` is constrained by what it may expose - -The restrictive form came from `gohai`, where aliases exposing unexported -functions had produced tests that re-covered paths the caller's own test already -exercised, and pinned intermediate steps so they could not be changed without -rewriting tests. - -The reasoning is not specific to collectors. An alias makes an internal step -directly callable, and a directly callable step attracts a test. The requirement -therefore applies organization-wide. - -It constrains what such a file may contain rather than requiring one to exist, -so the two repositories that do not use the pattern are unaffected, and the two -that use it heavily cannot use it to introduce test-only seams. - -*Alternative considered:* leave it to `gohai`, since only two repositories use -the file. Rejected — a rule stated only where it was learned is a rule the next -repository discovers by making the same mistake. +### `export_test.go` constrains the purpose, not the mechanism + +The first draft of this requirement banned exposing an alias to an unexported +function, allowing only setter functions. That was wrong, and applying it is +what showed why: ten such aliases exist across `gohai` and `osapi`, and the +pattern is idiomatic Go. Twenty-two standard library packages use it — +`net/http` alone exports `DefaultUserAgent`, `NewLoggingConn`, `ExportServeFile` +and more this way. + +The rule came from `gohai`, where aliases had produced tests that re-covered +paths the caller's own test already exercised. That concern is real, but it is a +concern about what the test does, not about how the symbol was exposed. Banning +the mechanism outlawed an idiom in order to prevent a misuse of it. + +The requirement now names the misuse: a test SHALL NOT use an exported alias to +re-cover behavior the caller's test already reaches. Exposing a pure helper with +its own contract — `BytesToString`, `ParseOffset` — is exactly what the pattern +is for, and a scenario says so. + +*Alternative considered:* keep the ban and remove the ten aliases. Rejected — +that would have rewritten eleven working call sites to satisfy a rule the +language's own standard library does not follow. ### The reason is stated with the rule diff --git a/openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md b/openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md index 3e4c947..196f95b 100644 --- a/openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md +++ b/openspec/changes/specify-go-code-standards/specs/go-code-standards/spec.md @@ -108,21 +108,21 @@ server — is not required to introduce mocking. - **THEN** it uses a generated mock or a real implementation, rather than a struct written to satisfy the interface -### Requirement: Test-only seams stay out of production code +### Requirement: A test does not duplicate coverage through an internal seam Where a test needs access to an unexported symbol, the package MAY provide an -`export_test.go` exposing it. That file SHALL expose setters that replace a -value and return a function restoring it. +`export_test.go` exposing it — by alias or by setter. The file carries the +`_test.go` suffix, so nothing it declares ships in the built package. -It SHALL NOT expose an alias to an unexported function. An alias makes an -internal step directly callable, which invites a test that exercises it in -isolation — duplicating coverage the caller's own test already provides, and -pinning an implementation detail so it cannot be changed without rewriting -tests. +What the exposure is for matters more than its form. An exported alias SHALL NOT +be used to test an internal step whose behavior the caller's own test already +covers. Such a test adds no coverage and pins an implementation detail, so +changing how the caller reaches its result means rewriting tests that were never +about the result. -A seam SHALL be placed at the boundary with a dependency, not partway through -the code under test, so that the logic between the entry point and the boundary -runs in every case. +A seam replacing a dependency SHALL be placed at the boundary with that +dependency, not partway through the code under test, so the logic between the +entry point and the boundary runs in every case. #### Scenario: A test needs to force an error from a dependency @@ -130,11 +130,17 @@ runs in every case. - **THEN** it replaces that call at the boundary, and the code between the entry point and the boundary executes -#### Scenario: An internal step looks worth testing directly +#### Scenario: An internal step is exported for a test -- **WHEN** an unexported function seems to warrant its own test -- **THEN** it is covered through the exported surface that calls it, rather than - exposed to be called directly +- **WHEN** an unexported function is exposed through `export_test.go` +- **THEN** the test exercises behavior the caller's test does not already reach, + rather than re-covering the same path through a shorter route + +#### Scenario: An unexported helper is pure and self-contained + +- **WHEN** an unexported function has its own contract, independent of the + callers that use it +- **THEN** exposing and testing it directly is appropriate ### Requirement: Style baseline