From 85bdd05c0542e9274b1b5ffc3b329a7865fda5e2 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Mon, 5 Feb 2018 23:57:41 -0500 Subject: [PATCH] cmd/go: rebuild as needed for tests of packages that add methods If A's external test package imports B, which imports A, and A's (internal) test code also adds something to A that invalidates anything in the export data from a build of A without its test code, then strictly speaking we need to rebuild B against the test-augmented version of A before using it to build A's external test package. We've been skating by without doing this for a very long time, but I knew we'd need to handle it better eventually, I planned for it in the new build cache simplifications, and the code was ready. Now that we have a real-world test case that needs it, turn on the "proper rebuilding" code. It doesn't really matter how much things slow down, since a real-world test cases that caused an internal compiler error before is now handled correctly, but it appears to be small: I wasn't able to measure an effect on "go test -a -c fmt". And of course most builds won't use -a and will be cached well. Fixes #6204. Fixes #23701. Change-Id: I2cd60cf400d1928428979ab05831f48ff7cee6ca Reviewed-on: https://go-review.googlesource.com/92215 Run-TryBot: Russ Cox TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/cmd/go/go_test.go | 38 ++++++++++++++++++++++++++++++++ src/cmd/go/internal/test/test.go | 9 +------- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/cmd/go/go_test.go b/src/cmd/go/go_test.go index 7db62da34e..92600b6238 100644 --- a/src/cmd/go/go_test.go +++ b/src/cmd/go/go_test.go @@ -5439,6 +5439,44 @@ func TestTestVet(t *testing.T) { tg.grepStdout(`ok\s+vetfail/p2`, "did not run vetfail/p2") } +func TestTestRebuild(t *testing.T) { + tg := testgo(t) + defer tg.cleanup() + tg.parallel() + + // golang.org/issue/23701. + // b_test imports b with augmented method from export_test.go. + // b_test also imports a, which imports b. + // Must not accidentally see un-augmented b propagate through a to b_test. + tg.tempFile("src/a/a.go", `package a + import "b" + type Type struct{} + func (*Type) M() b.T {return 0} + `) + tg.tempFile("src/b/b.go", `package b + type T int + type I interface {M() T} + `) + tg.tempFile("src/b/export_test.go", `package b + func (*T) Method() *T { return nil } + `) + tg.tempFile("src/b/b_test.go", `package b_test + import ( + "testing" + "a" + . "b" + ) + func TestBroken(t *testing.T) { + x := new(T) + x.Method() + _ = new(a.Type) + } + `) + + tg.setenv("GOPATH", tg.path(".")) + tg.run("test", "b") +} + func TestInstallDeps(t *testing.T) { tooSlow(t) tg := testgo(t) diff --git a/src/cmd/go/internal/test/test.go b/src/cmd/go/internal/test/test.go index bf684809e3..a99c6a5ec2 100644 --- a/src/cmd/go/internal/test/test.go +++ b/src/cmd/go/internal/test/test.go @@ -897,7 +897,7 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin t.ImportXtest = true } - if ptest != p && localCover { + 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 @@ -906,13 +906,6 @@ func builderTest(b *work.Builder, p *load.Package) (buildAction, runAction, prin // 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. - // TODO(rsc): Once we get export metadata changes - // handled properly, look into the expense of dropping - // "&& localCover" above. - // - // This will cause extra compilation, so for now we only do it - // when testCover is set. The conditions are more general, though, - // and we may find that we need to do it always in the future. recompileForTest(pmain, p, ptest, pxtest) }