Repository navigation
Description
Problem
#1169 brought the tree to two styleguide rules and left a check behind each. Neither check can
see every breach of its rule.
1. TestTestHelpers_StartWithHelper misses helpers by how they are spelled
The test reads parsed source and matches spellings: a first parameter of type testing.T,
packages named assert and require, and a call written as a bare name. Planted in the tree at
main @bd874baa, each of these helpers asserts, is called directly, has no t.Helper(), and goes
unreported:
package zzprobe
import (
"testing"
tst "testing"
)
func secondParam(name string, t *testing.T) { t.Fatal(name) } // testing parameter not first
func aliased(t *tst.T) { t.Fatal(1) } // testing imported under another name
func generic[V any](t *testing.T, v V) { t.Fatal(v) } // called as generic[int](t, 1)
func TestProbe(t *testing.T) {
secondParam("x", t)
aliased(t)
generic[int](t, 1)
}$ go test ./internal/archtest -run TestTestHelpers_StartWithHelper -count=1
ok github.com/dexpace/morphic/internal/archtest 0.153sThe same holds for more shapes:
- a helper taking
*testing.Bor*testing.F, both of which haveHelpertoo; - a helper failing through the second name of one field (
func check(a, t *testing.T)); - a call written through parentheses (
(check)(t)); - a testify package imported under another name (
a "github.com/stretchr/testify/assert").
In the other direction, a package that is not testify but is imported as assert is taken for
testify.
2. Nothing reports a missing interface assertion
interface-assertions-stay-narrow (.claude/skills/morphic-checking-style/references/departures.md)
asks every exported type that implements an interface the module exports to carry a var _
assertion. Its Probe is a grep that lists the assertions present:
$ git grep -nE '^(var )?[[:space:]]*_ +[A-Za-z.*]+ += ' -- '*.go' ':!*_test.go' ':!*testdata*'Nothing on main reads whether an assertion is missing. Delete
var _ compilers.DocumentReader = Disk{} from engine/reader/disk.go and the grep just prints one
line fewer. If the type's methods later drift, the build fails only where some other code happens
to use the pair. A type-checked scan at @bd874baa (every production package, 284 exported types against the 10
exported interfaces) finds no gap today, and it does report the deleted assertion.
What fixing this buys
Each rule has a check because a reader cannot be trusted to apply it every time, and #1178 shows
why the helper rule needed one of its own: thelper reads signatures, and of the 298 helpers it
reported, none was a breach. A check that misses a shape brings the problem back in a quieter form. The test stays green, so a reviewer who trusts it stops looking,
and the next breach of that shape lands on main unseen. CLAUDE.md names this failure under
"Verify by executing": a check that runs is not a check that reaches. Fixing both closes the
shapes above:
- A test helper keeps reporting its caller's line, whichever testing parameter it fails through
and however its file imports testing or testify. A red test then points at the case that failed,
not at the inside of a helper. - An exported implementation keeps its contract visible. Code inside a package need not
exercise an exported pair, so an assertion is often the only thing that fails the build when a
method changes signature. Without a check, the first exported type added without one breaks
that silently, and it stays broken until a downstream caller stops compiling. - Review costs less. A reviewer need not re-check those shapes by hand, which the review
skill's "never re-check by eye what the gate enforces" assumes of what the gate holds.
Neither gap lets a defect through on main today. The fix makes sure the next breach of these
shapes is caught when it lands, not after.
Cause
At @bd874baa: testingParam in internal/archtest/testhelper_test.go reads the first name of
params[0] only, accepts T and TB alone, and compares the package name to the literal
"testing". asserts compares to the literals "assert" and "require". The call walk accepts
only an *ast.Ident callee. The second rule has no check at all.
Direction
Proposal:
- Read the names each file imports
testingand testify by, all of them when a path is imported
twice. Take every parameter of typeT,B,ForTB, in any position, skip benchmarks and
fuzz targets as tests are skipped, and read the callee through an instantiation and through
parentheses. - Add
TestInterfaceAssertions_CoverEveryExportedPairtointernal/archtest. Over the existing
productionLoad, report each exported, non-generic type that implements an exported interface
its package declares or imports from the module, when no package-level var of an interface
implementing that one holds it. Asserting an embedding interface then covers what it embeds.
Point the rule's Probe at it.
Only the package's own and directly imported interfaces are compared: a type cannot assert an
interface from elsewhere without a new import, so this is the rule's own scope.
Out of scope
- The helper check stays a parsed reader. A local variable that shadows a testify import name is
still read as testify, but revive'simport-shadowingalready refuses such a local. A testing
parameter copied before use (tb := t; tb.Fatal(1)) is still missed. - Generic interfaces and generic types are left out of the assertion check, as only an
instantiation implements anything. The one generic exported interface,
compilers/openapi/internal/resolve.Referenced, is a constraint over library types.
Priority: P4, housekeeping (a test change; no lowering, validation, IR or emit-contract behaviour
changes). Probes above run at morphic @bd874baa.
Activity