test: lay out tests by the code they test

.golangci.yml

@@ -202,7 +202,7 @@ linters:
       # liveForCalls is a context.Context: it embeds the one it wraps to answer Err
       # on a schedule, which is how a context decorator is written, not a context
       # held for later.
-      - path: compilers/openapi/budgets_test\.go
+      - path: compilers/openapi/budgets_internal_test\.go
         linters:
           - containedctx
         source: ^\s*context\.Context$

CLAUDE.md

@@ -301,10 +301,12 @@ below are the ones most likely to bite in this codebase — the full guide gover
 - **Naming:** MixedCaps; scope-proportional length; no `Get` prefix on getters; initialisms in
   one consistent case; package names short lowercase nouns; never shadow builtins or imports.
 - **Testing:** table-driven and flat; `TestFunc_Scenario` names; `testify/require` for
-  preconditions, `assert` for values; compare with `cmp.Diff`, never `reflect.DeepEqual`;
-  golden files for complex expected output; `t.Helper()` in helpers; `t.Cleanup()` over
-  `defer`; external test packages (`package foo_test`) preferred; where goroutines exist, the
-  goroutine leak check under Testing strategy above (the styleguide names `goleak`; #568).
+  preconditions, `assert` for values; compare with `cmp.Diff`, never `reflect.DeepEqual`; golden
+  files for complex expected output; `t.Helper()` in helpers; `t.Cleanup()` over `defer`; external
+  test packages (`package foo_test`) preferred, reaching internals through the package's
+  `export_test.go`, and an in-package test file named `foo_internal_test.go` (CONTRIBUTING's "Where
+  a test goes"); where goroutines exist, the goroutine leak check under Testing strategy above (the
+  styleguide names `goleak`; #568).
 - **Packages:** one package per directory; `internal/` aggressively for implementation detail;
   no `utils`/`helpers`/`common`; `doc.go` for package docs; imports in three `gci` groups
   (stdlib, external, local); no dot imports.

CONTRIBUTING.md

@@ -45,6 +45,34 @@ Read the documents for your layer, and skip the rest until you need them.
   as `ir/nofloat_test.go` is, and in a package of one source file a test file needs no prefix.
   *Fails if missed:* `make coverage`, for a test in another package than its code, since coverage
   counts only a package's own tests. Nothing fails for a test in the wrong file of its package.
+- **A test is external, `package x_test`, unless it cannot be.** It reaches what it must of the
+  package's internals through the package's `export_test.go`, which names each one once, under its
+  exported spelling or, where that is taken, a name that says what it adds, as
+  `irtest.CompareGoldenOn` does. A test that needs an unexported field or method, a generic, or a
+  variable it assigns stays inside the package, in `x_internal_test.go`, and so do a package's
+  tests when moving them out would take more than ten aliases, enough to make a second API, as
+  `schema`'s and `operation`'s would. A fixture both sides use is declared once, exported, in
+  `export_test.go`, as `schema.Lowerer` is. A `cmd` program's tests are all inside it, under plain
+  names, since a `main` package cannot be imported.
+  *Fails if missed:* nothing. A test written inside the package that could be external compiles and
+  runs.
+- **A test is named for what it tests.** A `TestFunc_Scenario` name is a function's or a type's, and
+  the file follows from it as above. A test that pins a construct or a lowering rule may take the
+  construct's name instead, as the `TestPathItemRefSiblings` tests do, and then sits in the file of
+  the construct or of the entry point it runs through. A function that asserts on a test's behalf
+  calls `t.Helper`, except one a table runs as a case's body, as the conformance corpus's assertions
+  and the canaries are: it is the case, so its failure reports the line in it that fails.
+  *Fails if missed:* nothing checks a test's name or its `t.Helper`.
+- **A helper is written once.** One that several test files of a package share sits in one of
+  them or, when it is new, in the package's `helpers_test.go`, or `helpers_internal_test.go` inside
+  the package. One that several packages share is in `compilers/openapi/internal/openapitest` or
+  `ir/irtest`, as `openapitest.SchemaFromYAML` and `irtest.RepoRoot` are, when it builds or reads
+  what those packages serve, a spec and its diagnostics or the IR. One that belongs there but
+  cannot live there, as `parseFull`, which compiles through `openapi.New`, is written in each
+  package that needs it, and each copy says why. A helper outside what either serves, such as a
+  file read or a stub compiler, is written where it is needed, and so is one whose body is a single
+  expression, such as `ir.TypeRef{Target: id}`, which holds no behaviour for a copy to drift from.
+  *Fails if missed:* nothing. A copy compiles beside the original.
 - **An end-to-end test of the OpenAPI compiler goes in `compilers/openapi/e2e`,** in a file named
   for the construct or the property it checks: a test of what the compiler does as a whole, which no
   one package decides, such as the conformance corpus, the goldens, fuzzing, benchmarks and a

cmd/morphic/compile_test.go

@@ -89,15 +89,7 @@ func TestRun_SpecProblemsExitOne(t *testing.T) {
 
 func TestRun_DiagnosticsGateExitCode(t *testing.T) {
 	t.Parallel()
-	// A parseable spec that produces at least one warning-or-info diagnostic
-	// but no errors: validation-only keyword.
-	spec := writeFile(t, "warn.yaml", `openapi: 3.1.0
-info: {title: T, version: "1"}
-paths: {}
-components:
-  schemas:
-    S: {type: object, not: {required: [x]}}
-`)
+	spec := writeFile(t, "warn.yaml", warnSpec)
 	var stdout, stderr bytes.Buffer
 	require.Equal(t, 0, run([]string{"compile", spec}, &stdout, &stderr),
 		"info diagnostics must not fail the default gate")
@@ -407,3 +399,28 @@ func TestRun_CompactOutputKeepsDocumentOrder(t *testing.T) {
 	assert.Less(t, bytes.Index(compact, []byte("Alpha")), bytes.Index(compact, []byte("Zeta")),
 		"schemas must be emitted in sorted-key order, not declaration order")
 }
+
+// TestSpecOptions_DisabledPasses pins that --skip-validate adds validate to
+// whatever --disable-pass named, and that names reach the engine as typed.
+func TestSpecOptions_DisabledPasses(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name string
+		opts specOptions
+		want []string
+	}{
+		{"neither flag", specOptions{}, nil},
+		{"--skip-validate", specOptions{skipValidate: true}, []string{"validate"}},
+		{"--disable-pass validate", specOptions{disable: passFlag{"validate"}}, []string{"validate"}},
+		{"both", specOptions{skipValidate: true, disable: passFlag{"validate"}},
+			[]string{"validate", "validate"}},
+		{"another name with --skip-validate", specOptions{skipValidate: true, disable: passFlag{"Nope"}},
+			[]string{"Nope", "validate"}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Empty(t, cmp.Diff(tt.want, tt.opts.disabledPasses()))
+		})
+	}
+}

cmd/morphic/options_test.go

@@ -89,28 +89,3 @@ func TestPassFlag_SetKeepsNamesAsTyped(t *testing.T) {
 	assert.Empty(t, cmp.Diff(passFlag{"validate", "validate", ""}, got))
 	assert.Equal(t, "validate validate ", got.String())
 }
-
-// TestSpecOptions_DisabledPasses pins that --skip-validate adds validate to
-// whatever --disable-pass named, and that names reach the engine as typed.
-func TestSpecOptions_DisabledPasses(t *testing.T) {
-	t.Parallel()
-	tests := []struct {
-		name string
-		opts specOptions
-		want []string
-	}{
-		{"neither flag", specOptions{}, nil},
-		{"--skip-validate", specOptions{skipValidate: true}, []string{"validate"}},
-		{"--disable-pass validate", specOptions{disable: passFlag{"validate"}}, []string{"validate"}},
-		{"both", specOptions{skipValidate: true, disable: passFlag{"validate"}},
-			[]string{"validate", "validate"}},
-		{"another name with --skip-validate", specOptions{skipValidate: true, disable: passFlag{"Nope"}},
-			[]string{"Nope", "validate"}},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-			assert.Empty(t, cmp.Diff(tt.want, tt.opts.disabledPasses()))
-		})
-	}
-}

compilers/openapi/budgets_test.go → compilers/openapi/budgets_internal_test.go

@@ -93,7 +93,7 @@ func TestCompile_EnumBudgetBindsOnlyPastTheLimit(t *testing.T) {
 				assert.Len(t, enum.Members, tc.members)
 				return
 			}
-			assertHasErrorCode(t, diags, diag.BudgetExceeded)
+			openapitest.AssertHasCode(t, diags, diag.BudgetExceeded, ir.SeverityError)
 			assert.Equal(t, "enum declares 5 members, past the 4-member budget; lowered as any",
 				messageOf(t, diags, diag.BudgetExceeded),
 				"the refusal names what was observed and what the budget was")
@@ -124,7 +124,7 @@ func TestCompile_SourceByteBudgetBindsOnlyPastTheLimit(t *testing.T) {
 				return
 			}
 			assert.Nil(t, doc, "an over-budget source is refused before it is parsed")
-			assertHasErrorCode(t, diags, diag.BudgetExceeded)
+			openapitest.AssertHasCode(t, diags, diag.BudgetExceeded, ir.SeverityError)
 			assert.Equal(t,
 				fmt.Sprintf("source document is %d bytes, past the %d-byte budget", len(src), tc.limit),
 				messageOf(t, diags, diag.BudgetExceeded))
@@ -137,7 +137,7 @@ func TestCompile_SourceNodeBudgetRefusesADocumentPastIt(t *testing.T) {
 	doc, diags := compileBounded(t, enumSpec(3), Limits{MaxSourceNodes: 1})
 
 	assert.Nil(t, doc, "an over-budget node count is refused before the model is built")
-	assertHasErrorCode(t, diags, diag.BudgetExceeded)
+	openapitest.AssertHasCode(t, diags, diag.BudgetExceeded, ir.SeverityError)
 	assert.Contains(t, messageOf(t, diags, diag.BudgetExceeded), "past the 1-node budget")
 }
 
@@ -204,7 +204,7 @@ func TestCompile_AliasBudgetBindsAtItsDefault(t *testing.T) {
 	doc, diags := compileBounded(t, aliasFanOutSpec(20_000), Limits{})
 
 	assert.Nil(t, doc, "an over-budget expansion is refused before the model is built")
-	assertHasErrorCode(t, diags, diag.BudgetExceeded)
+	openapitest.AssertHasCode(t, diags, diag.BudgetExceeded, ir.SeverityError)
 	assert.Contains(t, messageOf(t, diags, diag.BudgetExceeded),
 		fmt.Sprintf("past the %d-node alias budget", DefaultMaxAliasSurplus))
 }
@@ -215,7 +215,7 @@ func TestCompile_AliasBudgetIsTheCallersToSet(t *testing.T) {
 
 	doc, diags := compileBounded(t, src, Limits{MaxAliasSurplus: 1})
 	assert.Nil(t, doc)
-	assertHasErrorCode(t, diags, diag.BudgetExceeded)
+	openapitest.AssertHasCode(t, diags, diag.BudgetExceeded, ir.SeverityError)
 	assert.Contains(t, messageOf(t, diags, diag.BudgetExceeded), "past the 1-node alias budget")
 
 	doc, diags = compileBounded(t, src, Limits{MaxAliasSurplus: -1})
@@ -233,7 +233,7 @@ func TestCompile_AnUnboundedAliasBudgetStillRefusesABomb(t *testing.T) {
 
 	doc, diags := compileBounded(t, string(data), Limits{MaxAliasSurplus: -1})
 	assert.Nil(t, doc)
-	assertHasErrorCode(t, diags, diag.AliasAmplification)
+	openapitest.AssertHasCode(t, diags, diag.AliasAmplification, ir.SeverityError)
 	assert.NotContains(t, messageOf(t, diags, diag.AliasAmplification), "budget",
 		"the refusal must not name a budget, since raising one would not admit the document")
 }

compilers/openapi/detect_test.go

@@ -100,3 +100,28 @@ func TestDetect_ReadsTheVersionTheCompileReads(t *testing.T) {
 		})
 	}
 }
+
+// TestDetectionBudget_ReadsTheCallersOptions pins the budget detection takes
+// from each kind of options, another compiler's included, which mean this
+// one's default as compilers.Compiler.Detect requires rather than no budget.
+// Detect's own tests exceed only a budget the caller sets: a source past the
+// default costs more than the gate's race run should pay, so another
+// compiler's options read as no budget would pass them all.
+func TestDetectionBudget_ReadsTheCallersOptions(t *testing.T) {
+	t.Parallel()
+	for _, tc := range []struct {
+		name string
+		opts compilers.Options
+		want int
+	}{
+		{"no options", compilers.Options{}, openapi.DefaultMaxSourceBytes},
+		{"another compiler's options", compilers.Options{FormatOptions: "another compiler's options"}, openapi.DefaultMaxSourceBytes},
+		{"a budget of its own", compilers.Options{FormatOptions: openapi.Options{Limits: openapi.Limits{MaxSourceBytes: 7}}}, 7},
+		{"the budget turned off", compilers.Options{FormatOptions: openapi.Options{Limits: openapi.Limits{MaxSourceBytes: -1}}}, 0},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tc.want, openapi.DetectionBudget(tc.opts))
+		})
+	}
+}

compilers/openapi/e2e/allof_visibility_test.go

@@ -93,7 +93,7 @@ var wantReadOnlyVisibility = ir.Visibility{
 func thingIDVisibility(t *testing.T, name, spec string) (ir.Visibility, []ir.Diagnostic) {
 	t.Helper()
 	doc, diags := compileAnnotationSpec(t, name, spec)
-	m, ok := doc.Types[namedID("Thing")].(*ir.Model)
+	m, ok := doc.Types[componentID("Thing")].(*ir.Model)
 	require.True(t, ok, "Thing must own a Model node")
 	p, ok := propByWire(m, "id")
 	require.True(t, ok, "Thing must carry a merged \"id\" property")

compilers/openapi/e2e/amplification_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/dexpace/morphic/compilers"
 	"github.com/dexpace/morphic/compilers/openapi"
 	"github.com/dexpace/morphic/compilers/openapi/internal/diag"
+	"github.com/dexpace/morphic/compilers/openapi/internal/openapitest"
 	"github.com/dexpace/morphic/ir"
 )
 
@@ -38,7 +39,7 @@ func TestCompile_AliasBombDoesNotExhaustMemory(t *testing.T) {
 	case r := <-done:
 		require.NoError(t, r.err, "an alias bomb is a spec problem, not a Go error")
 		assert.Nil(t, r.doc, "the compiler refuses to lower an amplifying document")
-		assertHasErrorCode(t, r.diags, diag.AliasAmplification)
+		openapitest.AssertHasCode(t, r.diags, diag.AliasAmplification, ir.SeverityError)
 	case <-time.After(bound):
 		t.Fatalf("Compile did not return within %v — the bomb likely reached soa.Unmarshal", bound)
 	}

compilers/openapi/e2e/annotations_carrier_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/dexpace/morphic/compilers/openapi/internal/openapitest"
 	"github.com/dexpace/morphic/internal/harness"
 	"github.com/dexpace/morphic/ir"
 )
@@ -52,7 +53,7 @@ paths:
 // carriedProperty returns the single property carrierPropertySpec declares.
 func carriedProperty(t *testing.T, doc *ir.Document) ir.Property {
 	t.Helper()
-	m, ok := doc.Types[namedID("S")].(*ir.Model)
+	m, ok := doc.Types[componentID("S")].(*ir.Model)
 	require.True(t, ok, "component S must lower to a model")
 	p, ok := propByWire(m, "f")
 	require.True(t, ok, "the model must carry property f")
@@ -64,7 +65,7 @@ func carriedParameter(t *testing.T, doc *ir.Document) ir.Parameter {
 	t.Helper()
 	op, ok := opByName(doc, "op")
 	require.True(t, ok, "the operation must lower")
-	p, ok := paramByName(op, "q")
+	p, ok := openapitest.ParamBySource(op, "q")
 	require.True(t, ok, "the operation must carry parameter q")
 	return p
 }