Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions openspec/changes/specify-go-code-standards/design.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,33 +108,39 @@ 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

- **WHEN** a test needs a library call to fail
- **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

Expand Down