Repository navigation
test: move end-to-end tests out and name tests for their code - #1109
Merged
Merged
Conversation
The root package held its four source files among 53 test files, most of which compile a whole spec through openapi.New and read the IR, the conformance corpus, goldens, fuzz targets and benchmarks among them. They cover nothing under internal/, since coverage counts a package's own tests, and they buried the unit tests of the root's own code. They now live in compilers/openapi/e2e, a package of test files only. The files move whole, so history follows them; the edits are the package clause, one more ../ in each repository path, the openapi. qualifier on the seven files that were in-package, and the FuzzCycleDetector seed corpus moving beside its target. What the root keeps is the tests of its own files. Three of them borrowed fixtures from files that moved: sourcetable_test.go now declares its own document and overlays, and openapi_test.go and budgets_test.go their own paths. Two exported defaults were covered only by end-to-end tests and get a unit test each. defs_test.go's copy of propByWire, which existed only because it could not reach the conformance one, is gone. The lowering-spec checks name each package by its repository path, and the docs and commands that named the old location point at the new one.
The helpers several end-to-end files share were declared in whichever file needed them first: the petstore path in fuzz_test.go, the IR readers in conformance_test.go, the on-disk compile in confinement_test.go. They now sit in helpers_test.go in reading order, fixture paths first, then compiling, then reading the IR, then reading diagnostics. What stays where it was is not a helper: a conformance case's assertion stays with its family, the corpus's own reader with conformance_test.go, the lowering-spec checker with its tests, and a fixture one construct's files share with that construct. The declarations are byte-identical.
conformance_test.go was 4,659 lines: the corpus runner, its table and 138 assertions. Adding or finding a case meant scrolling past every other construct's. The runner, the table and the corpus's own reader stay in conformance_test.go. Each case's assertion moves to the family file of its construct (conformance_types_test.go, conformance_composition_test.go, conformance_params_test.go and so on), with every helper only that family uses, beside the conformance_unmodeled_test.go that already grouped the Unmodeled assertions. The table keeps its order and the declarations are byte-identical.
Four root files drove a single internal lowering through a fixture only the root declared, so they covered nothing in the package they tested and kept a second copy of every package's fixture alive at the root. The constraint tests move to internal/schema, whose ConstraintsOf reports what they pin; the component-schema and property tests go there too, and the service test to internal/operation, each onto the fixture of the same name that package already had. The unpreservable tests, which compile whole documents, move to e2e, and the reason sweep among them reads a full compile rather than a partial one. Five tests are deleted rather than moved, because a test in the package under test already pins what they did. compile's TestTypes_InternIsIdempotentAndRecordsBeforeBuilding and TestTypes_PrimRefInternsOnceAndNamesNoSource kill every mutant of Intern and PrimRef the root's TestIntern_ and TestPrimID_ kill, and one they let through. annotation's TestAt_ tests kill each mutant of At the root's three TestSiteAt_ tests kill. With them goes the root's lowerer fixture, which nothing uses any more.
Thirty-seven test files held tests of one source file under a name that sorted nowhere near it: load's entry_internal_test.go tested Load in load.go, engine's document_test.go tested Engine in engine.go, bind's bind_test.go tested table.go. A reader looking for a file's tests could not find them by name. Each is renamed to start with the file it tests, keeping its topic (load_entry_internal_test.go, engine_document_test.go, table_test.go), so it sorts beside that file. A file is renamed only when every test in it names a declaration of one source file; one whose tests name several files is a scenario and keeps its name, as do main_test.go files, test-only packages and packages with one source file. Only the comment in engine/main_test.go that named a renamed file changes.
TestDocuments_NameOnlyTestsThatExist read a span such as `openapi.TestCompile_X` as no test name at all, since its pattern began at Test, so a citation naming the package a test used to live in passed unread. Moving tests between packages is exactly when that goes stale. declaredTests now records the package declaring each test, an external test package under the package it tests, and a span qualified as pkg.TestName must name a test of package pkg. The lowering specification's citation shape, `package.TestName`, joins the placeholders.
The renaming pass read a test's subject from a function or type its name names, so a test named for a method or an initialism missed: oas's TestCheck_ tests are of Checker.Check in engine.go, nodeview's of View methods in nodeview.go, lowering's within tests of Ctx.InScope in lowering.go, the root's SourceTable tests of Compiler.SourceTable in openapi.go, and keydoc's TestYAMLComment_ of yamlComment in cite.go. Their files are renamed to sort beside those files. Two files held the tests of two source files under one name. compile_test.go becomes types_test.go and gives its Diags tests to diags_test.go; harness's internal_test.go becomes harness_internal_test.go and gives the in-package tests of path.go to path_internal_test.go, with the stub compiler both use in helpers_internal_test.go. The declarations are byte-identical.
The moves left comments describing where code used to be: a helper's "above" that meant the file it left, two comments naming a test file in another package by its old home, the e2e package comment, which claimed every test there compiles a spec, and a design note that placed the whole-compiler suites at the top level. A test moved into internal/schema was still named for fillPropertyExamples, which is fillPropertyAnnotations now, and its comment said the function resolved the example itself; it is renamed and says what does. The bomb fixture path both packages declare now says what the fixture is. Comments name a test or a symbol rather than a file where one serves, so the next move leaves them true.
CONTRIBUTING gains a "Where a test goes" section before the recipes: a unit test sits in the _test.go named after the file declaring what its name says it tests, a scenario or a whole-package check in a file named for it, and an end-to-end test of the OpenAPI compiler in compilers/openapi/e2e, whose helpers_test.go holds what several of its files share. Each rule names the check that fails when it is missed, or says nothing does. The conformance step now says the assertion goes in its construct's family file, and the root guidance's layout lists e2e/.
TestXML_NodeTypeIsReadAtEveryVersion sat in operation's tests, yet the document it compiles declares no path: what it pins is a property's XML.NodeType, which the schema lowering attaches, and the later-version-construct warning load's rule table reports below 3.2. Neither is the operation package's code. It moves to the end-to-end later_version_test.go, beside the other later-version constructs, and compiles through the public face in place of operation's fixture. operation's coverage is unchanged, and the test still fails when nodeType is never read.
Two doc comments in the end-to-end package still told a reader to regenerate goldens with `go test ./compilers/openapi -run ...`, which now runs no test; they name compilers/openapi/e2e. TestConformance's comment also sent the reader to TestConformance_UnwitnessedIRFields, and the CLI's readOnlyDirDest to TestWriteCompiled_ReadOnlyDirFails, both gone before this change; they name the census and atomicfile.TestReplace_ReadOnlyDirFails, which hold those facts now. reproducerDir and corpusRoot stayed in the files the helpers reading them left, and reproducerDir's comment still pointed at "the reader below"; both now sit with the other fixture paths in helpers_test.go, the comment naming what derives from it. The placement rule in CONTRIBUTING called any test that compiles a spec through openapi.New end-to-end, which would send internal packages' own tests, which compile whole specs to reach their code, out of the package their coverage counts in. It now turns on whose code decides what a test asserts. docs/architecture.md's package tree gains e2e/.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test files were hard to navigate.
compilers/openapiheld its four source files among 53 test files, most of them end-to-end tests that compile a whole spec. Across the repo, many test files had names that sort next to no source file, andconformance_test.goran to 4,659 lines. This PR keeps unit tests beside the code they test, gives the OpenAPI compiler's end-to-end tests a package of their own, names test files after the file they test, and writes the placement rule down. No non-test Go file changes.compilers/openapi/e2e, a package of test files only, now holds the end-to-end tests: the conformance corpus, the lowering-specification checks, goldens, fuzz targets with their seed corpus, benchmarks, and the whole-compiler scenarios. Files move whole, sogit log --followand blame track them. The root keeps only the tests of its own four files. Two exported defaults (DefaultStreamingMediaTypes,DefaultExtensionPromotions) had been covered only by end-to-end tests, so each gets a root unit test.sourcetable_test.gogets fixtures of its own instead of borrowingoverlay_test.go's.helpers_test.go; until now each lived in whichever file first needed it.conformance_test.gokeeps the runner, the case table and the corpus reader. Each case's assertion moves to aconformance_<family>_test.gofile for its construct (types, composition, params, content, and so on).TestXML_NodeTypeIsReadAtEveryVersionmoves fromoperation's tests, whose code it never reaches, to e2e'slater_version_test.go, as test: an XML nodeType test lowers no operation yet sits in operation's #1074 proposed.internal/schema, the service test tointernal/operation, the unpreservable tests to e2e. Five root tests are deleted rather than moved, because a test in the package under test already pins what they did:TestIntern_IdempotentOnSamePointerandTestPrimID_SecondCallReuses:compile'sTestTypes_InternIsIdempotentAndRecordsBeforeBuildingandTestTypes_PrimRefInternsOnceAndNamesNoSourcekill all three mutants planted inIntern/PrimRef(a rebuild on revisit, a fresh ID per call, a re-minted ID on a second reach). The root tests kill only two of them.TestSiteAt_*tests:annotation'sTestAt_*kill each mutant ofAtthey kill (an empty$refread as a reference, a reference site with no node, every site a declaration).load/entry_internal_test.go→load_entry_internal_test.go. 47 files are renamed and two are split:compile_test.gointotypes_test.goanddiags_test.go, and harness'sinternal_test.goby the file each test covers. Files named for a scenario several source files decide, and whole-package checks such asir/nofloat_test.go, keep their names.pkg.TestName) in Markdown. Its pattern used to start atTest, so a citation naming the package a test had moved out of passed unread.docs/compilers/openapi.mdcitations, the root guidance's layout anddocs/architecture.md's package tree all point at the new homes. Comments the moves would have made false are corrected, commands in Go comments included; where a symbol or test name can stand in for a file name, they now use it. Two comments in moved files cited tests deleted before this change (TestConformance_UnwitnessedIRFields,TestWriteCompiled_ReadOnlyDirFails) and now name what holds those facts today.Deliberately left alone:
integrationorperformancepackage.engine's tests are the integration layer's own and count toward its coverage. A benchmark of unexported code has to sit in its package, so benchmarks stay beside what they measure, and only the compiler's whole-document benchmark moves with e2e.stream,nesting,plus_refs,documentbudget; resolve'sbeside; merge'sconflict; the CLI'scommand,completionandhelptests).auth_test.gokeeps itsTestAuth_tests, as decided whenauth.gowas split.Test plan
main:go test -list '.*' ./...lists 3,481 tests onmainand 3,478 here. The difference is the two root unit tests added, the five dominated tests deleted, andTestFillPropertyExamples_RefSiblingExampleBindsTheSiterenamed toTestFillPropertyAnnotations_…, since the function it tests isfillPropertyAnnotationsnow.go test -json -race ./...: 12,274 passing tests and subtests onmain(fuzz seeds included) against 12,271 here, differing in the same names only. The fourTestLoad_AFileURIReadsTheFileItNamessubtests are named after a random temp directory, so they differ in name only.go run ./cmd/morphic-covergateover that run's profile: all 15,144 statements covered, the same total across the same 52 packages asmain's profile, so no package's statements left the count.make fmt build vet lint lint-planted bench-smokeexit 0, with golangci-lint at the pinned version.make fuzz FUZZTIME=2sfinds and runs the three fuzz targets in their new package.git diff -M --name-statusreports R100); the root's SourceTable tests also carry the fixtures they gained, andmain_edge_cases_test.goa corrected test citation. The files moved into e2e keep at least 76% similarity, the rest being the helpers that moved out of them and, inlater_version_test.go, the nodeType test that moved in.docs/micro-compiler-design.md§4.1 saysTestConstraints_HoistedSubSchemaBadBoundSingleErrorholds, killed;TestPreserve_AllReasonsReachable;nodeTypenever read, killed by the nodeType test in its new home.TestLoweringSpec_TestsAreDeclaredstill goes red on a stale citation.go test ./compilers/openapi/e2e -run 'TestGolden|TestConformance' -updateleavestestdata/unchanged, and a deleted conformance golden comes back byte-identical.go testcommand quoted in a Markdown document or a Go comment lists at least one test with its-runfilter, and every test name a Go comment cites is declared.mainmerged with chore: add a review agent built from past PR fixes #888.Closes #1074