From 804d03281c04096fca7f73dc33d1d62e09a86892 Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Tue, 3 Apr 2018 00:35:46 -0400 Subject: [PATCH] cmd/go: rebuild as needed when vetting test packages If A's external test package imports B, which imports A, and A's internal test code adds something to A that invalidates anything in A's export data, then we need to build B against the test-augmented version of A before using it to build A's external test package. https://golang.org/cl/92215 taught 'go test' to do this rebuilding properly, but 'go vet' was not taught the same trick when it learned to vet test packages in https://golang.org/cl/87636. This commit moves the necessary logic into the load.TestPackagesFor function so it can be shared by 'go test' and 'go vet'. Fixes #23701. Change-Id: I1086d447eca02933af53de693384eac99a08d9bd Reviewed-on: https://go-review.googlesource.com/104315 Run-TryBot: Russ Cox TryBot-Result: Gobot Gobot Reviewed-by: Russ Cox --- src/cmd/go/go_test.go | 3 +- src/cmd/go/internal/load/pkg.go | 53 ++++++++++++++++++++++++++++++++ src/cmd/go/internal/test/test.go | 50 ------------------------------ 3 files changed, 55 insertions(+), 51 deletions(-) diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index aa354027f40..ffedcff3d9b 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -5497,7 +5497,7 @@ func TestTestVet(t *testing.T) { tg.grepStdout(`ok\s+vetfail/p2`, "did not run vetfail/p2") } -func TestTestRebuild(t *testing.T) { +func TestTestVetRebuild(t *testing.T) { tg := testgo(t) defer tg.cleanup() tg.parallel() @@ -5533,6 +5533,7 @@ func TestTestRebuild(t *testing.T) { tg.setenv("GOPATH", tg.path(".")) tg.run("test", "b") + tg.run("vet", "b") } func TestInstallDeps(t *testing.T) { diff --git a/src/cmd/go/internal/load/pkg.go b/src/cmd/go/internal/load/pkg.go index 02cbb94bc7c..590de6c49b9 100644 --- a/src/cmd/go/internal/load/pkg.go +++ b/src/cmd/go/internal/load/pkg.go @@ -1712,6 +1712,18 @@ func TestPackagesFor(p *Package, forceTest bool) (ptest, pxtest *Package, err er } } + if p != ptest && pxtest != nil { + // We have made modifications to the package p being tested + // and are rebuilding p (as ptest). + // Arrange to rebuild all packages q such that + // pxtest depends on q and q depends on p. + // This makes sure that q sees the modifications to p. + // Strictly speaking, the rebuild is only necessary if the + // modifications to p change its export metadata, but + // determining that is a bit tricky, so we rebuild always. + recompileForTest(p, ptest, pxtest) + } + return ptest, pxtest, nil } @@ -1732,3 +1744,44 @@ Search: } return stk } + +func recompileForTest(preal, ptest, pxtest *Package) { + // The "test copy" of preal is ptest. + // For each package that depends on preal, make a "test copy" + // that depends on ptest. And so on, up the dependency tree. + testCopy := map[*Package]*Package{preal: ptest} + // Only pxtest and its dependencies can legally depend on p. + // If ptest or its dependencies depended on p, the dependency + // would be circular. + for _, p := range PackageList([]*Package{pxtest}) { + if p == preal { + continue + } + // Copy on write. + didSplit := p == pxtest + split := func() { + if didSplit { + return + } + didSplit = true + if testCopy[p] != nil { + panic("recompileForTest loop") + } + p1 := new(Package) + testCopy[p] = p1 + *p1 = *p + p1.Internal.Imports = make([]*Package, len(p.Internal.Imports)) + copy(p1.Internal.Imports, p.Internal.Imports) + p = p1 + p.Target = "" + } + + // Update p.Internal.Imports to use test copies. + for i, imp := range p.Internal.Imports { + if p1 := testCopy[imp]; p1 != nil && p1 != imp { + split() + p.Internal.Imports[i] = p1 + } + } + } +} diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index 42bff352c50..7f14ce3cd73 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -911,18 +911,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin t.ImportXtest = true } - if ptest != p { - // We have made modifications to the package p being tested - // and are rebuilding p (as ptest). - // Arrange to rebuild all packages q such that - // the test depends on q and q depends on p. - // This makes sure that q sees the modifications to p. - // Strictly speaking, the rebuild is only necessary if the - // modifications to p change its export metadata, but - // determining that is a bit tricky, so we rebuild always. - recompileForTest(pmain, p, ptest, pxtest) - } - for _, cp := range pmain.Internal.Imports { if len(cp.Internal.CoverVars) > 0 { t.Cover = append(t.Cover, coverInfo{cp, cp.Internal.CoverVars}) @@ -1058,44 +1046,6 @@ func addTestVet(b *work.Builder, p *load.Package, runAction, installAction *work } } -func recompileForTest(pmain, preal, ptest, pxtest *load.Package) { - // The "test copy" of preal is ptest. - // For each package that depends on preal, make a "test copy" - // that depends on ptest. And so on, up the dependency tree. - testCopy := map[*load.Package]*load.Package{preal: ptest} - for _, p := range load.PackageList([]*load.Package{pmain}) { - if p == preal { - continue - } - // Copy on write. - didSplit := p == pmain || p == pxtest - split := func() { - if didSplit { - return - } - didSplit = true - if testCopy[p] != nil { - panic("recompileForTest loop") - } - p1 := new(load.Package) - testCopy[p] = p1 - *p1 = *p - p1.Internal.Imports = make([]*load.Package, len(p.Internal.Imports)) - copy(p1.Internal.Imports, p.Internal.Imports) - p = p1 - p.Target = "" - } - - // Update p.Internal.Imports to use test copies. - for i, imp := range p.Internal.Imports { - if p1 := testCopy[imp]; p1 != nil && p1 != imp { - split() - p.Internal.Imports[i] = p1 - } - } - } -} - // isTestFile reports whether the source file is a set of tests and should therefore // be excluded from coverage analysis. func isTestFile(file string) bool {