]> Cypherpunks repositories - gostls13.git/commitdiff
cmd/go: suppress calls to collectDeps for test packages
authorBryan C. Mills <bcmills@google.com>
Mon, 3 Apr 2023 21:02:49 +0000 (17:02 -0400)
committerMichael Matloob <matloob@golang.org>
Tue, 4 Apr 2023 23:08:19 +0000 (23:08 +0000)
Instead, do the cycle checking in recompileForTest once the test
variant packages have been poked in the right places in the dependency
tree(graph?).

(Pair programming with bcmills@.)

For #59157.

Change-Id: I0c644cb9f2c0dac3a5b0189e2aa0eef083c669f6
Reviewed-on: https://go-review.googlesource.com/c/go/+/482237
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>

src/cmd/go/internal/list/list.go
src/cmd/go/internal/load/test.go
src/cmd/go/testdata/script/list_test_cycle.txt [new file with mode: 0644]
src/cmd/go/testdata/script/list_test_err.txt

index 672c3c122f6291191ef810b7f1217f668e81dbdf..a05fca9deea0291e7d0562aea06240a5150a4092 100644 (file)
@@ -653,7 +653,7 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
                                } else {
                                        pmain, ptest, pxtest, err = load.TestPackagesFor(ctx, pkgOpts, p, nil)
                                        if err != nil {
-                                               base.Errorf("can't load test package: %s", err)
+                                               base.Fatalf("can't load test package: %s", err)
                                        }
                                }
                                if pmain != nil {
@@ -770,20 +770,22 @@ func runList(ctx context.Context, cmd *base.Command, args []string) {
                                delete(m, old)
                        }
                }
-               // Recompute deps lists using new strings, from the leaves up.
-               for _, p := range all {
-                       deps := make(map[string]bool)
-                       for _, p1 := range p.Internal.Imports {
-                               deps[p1.ImportPath] = true
-                               for _, d := range p1.Deps {
-                                       deps[d] = true
+               if !pkgOpts.SuppressDeps {
+                       // Recompute deps lists using new strings, from the leaves up.
+                       for _, p := range all {
+                               deps := make(map[string]bool)
+                               for _, p1 := range p.Internal.Imports {
+                                       deps[p1.ImportPath] = true
+                                       for _, d := range p1.Deps {
+                                               deps[d] = true
+                                       }
                                }
+                               p.Deps = make([]string, 0, len(deps))
+                               for d := range deps {
+                                       p.Deps = append(p.Deps, d)
+                               }
+                               sort.Strings(p.Deps)
                        }
-                       p.Deps = make([]string, 0, len(deps))
-                       for d := range deps {
-                               p.Deps = append(p.Deps, d)
-                       }
-                       sort.Strings(p.Deps)
                }
        }
 
index 7a40cc6b4544ea7a5ab176f8c802fce9e25f31a9..ec7fe10c350cd7fa1957ccbd579d5b5b7df7b208 100644 (file)
@@ -112,22 +112,12 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
        rawTestImports := str.StringList(p.TestImports)
        for i, path := range p.TestImports {
                p1 := loadImport(ctx, opts, pre, path, p.Dir, p, &stk, p.Internal.Build.TestImportPos[path], ResolveImport)
-               if str.Contains(p1.Deps, p.ImportPath) || p1.ImportPath == p.ImportPath {
-                       // Same error that loadPackage returns (via reusePackage) in pkg.go.
-                       // Can't change that code, because that code is only for loading the
-                       // non-test copy of a package.
-                       ptestErr = &PackageError{
-                               ImportStack:   importCycleStack(p1, p.ImportPath),
-                               Err:           errors.New("import cycle not allowed in test"),
-                               IsImportCycle: true,
-                       }
-               }
                p.TestImports[i] = p1.ImportPath
                imports = append(imports, p1)
        }
        var err error
        p.TestEmbedFiles, testEmbed, err = resolveEmbed(p.Dir, p.TestEmbedPatterns)
-       if err != nil && ptestErr == nil {
+       if err != nil {
                ptestErr = &PackageError{
                        ImportStack: stk.Copy(),
                        Err:         err,
@@ -208,7 +198,9 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
                ptest.Internal.OrigImportPath = p.Internal.OrigImportPath
                ptest.Internal.PGOProfile = p.Internal.PGOProfile
                ptest.Internal.Build.Directives = append(slices.Clip(p.Internal.Build.Directives), p.Internal.Build.TestDirectives...)
-               ptest.collectDeps()
+               if !opts.SuppressDeps {
+                       ptest.collectDeps()
+               }
        } else {
                ptest = p
        }
@@ -250,7 +242,9 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
                if pxtestNeedsPtest {
                        pxtest.Internal.Imports = append(pxtest.Internal.Imports, ptest)
                }
-               pxtest.collectDeps()
+               if !opts.SuppressDeps {
+                       pxtest.collectDeps()
+               }
        }
 
        // Arrange for testing.Testing to report true.
@@ -341,7 +335,9 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
                pmain.Imports = append(pmain.Imports, pxtest.ImportPath)
                t.ImportXtest = true
        }
-       pmain.collectDeps()
+       if !opts.SuppressDeps {
+               pmain.collectDeps()
+       }
 
        // Sort and dedup pmain.Imports.
        // Only matters for go list -test output.
@@ -357,7 +353,10 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
        pmain.Internal.RawImports = str.StringList(pmain.Imports)
 
        // Replace pmain's transitive dependencies with test copies, as necessary.
-       recompileForTest(pmain, p, ptest, pxtest)
+       cycleErr := recompileForTest(pmain, p, ptest, pxtest)
+       if cycleErr != nil {
+               ptest.Error = cycleErr
+       }
 
        if cover != nil {
                if cfg.Experiment.CoverageRedesign {
@@ -403,46 +402,6 @@ func TestPackagesAndErrors(ctx context.Context, opts PackageOpts, p *Package, co
        return pmain, ptest, pxtest
 }
 
-// importCycleStack returns an import stack from p to the package whose import
-// path is target.
-func importCycleStack(p *Package, target string) []string {
-       // importerOf maps each import path to its importer nearest to p.
-       importerOf := map[string]string{p.ImportPath: ""}
-
-       // q is a breadth-first queue of packages to search for target.
-       // Every package added to q has a corresponding entry in pathTo.
-       //
-       // We search breadth-first for two reasons:
-       //
-       //      1. We want to report the shortest cycle.
-       //
-       //      2. If p contains multiple cycles, the first cycle we encounter might not
-       //         contain target. To ensure termination, we have to break all cycles
-       //         other than the first.
-       q := []*Package{p}
-
-       for len(q) > 0 {
-               p := q[0]
-               q = q[1:]
-               if path := p.ImportPath; path == target {
-                       var stk []string
-                       for path != "" {
-                               stk = append(stk, path)
-                               path = importerOf[path]
-                       }
-                       return stk
-               }
-               for _, dep := range p.Internal.Imports {
-                       if _, ok := importerOf[dep.ImportPath]; !ok {
-                               importerOf[dep.ImportPath] = p.ImportPath
-                               q = append(q, dep)
-                       }
-               }
-       }
-
-       panic("lost path to cycle")
-}
-
 // recompileForTest copies and replaces certain packages in pmain's dependency
 // graph. This is necessary for two reasons. First, if ptest is different than
 // preal, packages that import the package under test should get ptest instead
@@ -452,7 +411,7 @@ func importCycleStack(p *Package, target string) []string {
 // clear p.Internal.BuildInfo in the test copy to prevent link conflicts.
 // This may happen if both -coverpkg and the command line patterns include
 // multiple main packages.
-func recompileForTest(pmain, preal, ptest, pxtest *Package) {
+func recompileForTest(pmain, preal, ptest, pxtest *Package) *PackageError {
        // 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.
@@ -462,7 +421,7 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) {
                        continue
                }
                // Copy on write.
-               didSplit := p == pmain || p == pxtest
+               didSplit := p == pmain || p == pxtest || p == ptest
                split := func() {
                        if didSplit {
                                return
@@ -489,6 +448,10 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) {
                for i, imp := range p.Internal.Imports {
                        if p1 := testCopy[imp]; p1 != nil && p1 != imp {
                                split()
+
+                               // If the test dependencies cause a cycle with pmain, this is
+                               // where it is introduced.
+                               // (There are no cycles in the graph until this assignment occurs.)
                                p.Internal.Imports[i] = p1
                        }
                }
@@ -503,6 +466,49 @@ func recompileForTest(pmain, preal, ptest, pxtest *Package) {
                        split()
                }
        }
+
+       // Do search to find cycle.
+       // importerOf maps each import path to its importer nearest to p.
+       importerOf := map[*Package]*Package{}
+       for _, p := range ptest.Internal.Imports {
+               importerOf[p] = nil
+       }
+
+       // q is a breadth-first queue of packages to search for target.
+       // Every package added to q has a corresponding entry in pathTo.
+       //
+       // We search breadth-first for two reasons:
+       //
+       //      1. We want to report the shortest cycle.
+       //
+       //      2. If p contains multiple cycles, the first cycle we encounter might not
+       //         contain target. To ensure termination, we have to break all cycles
+       //         other than the first.
+       q := slices.Clip(ptest.Internal.Imports)
+       for len(q) > 0 {
+               p := q[0]
+               q = q[1:]
+               if p == ptest {
+                       var stk []string
+                       for p != nil {
+                               stk = append(stk, p.ImportPath)
+                               p = importerOf[p]
+                       }
+                       return &PackageError{
+                               ImportStack:   stk,
+                               Err:           errors.New("import cycle not allowed in test"),
+                               IsImportCycle: true,
+                       }
+               }
+               for _, dep := range p.Internal.Imports {
+                       if _, ok := importerOf[dep]; !ok {
+                               importerOf[dep] = p
+                               q = append(q, dep)
+                       }
+               }
+       }
+
+       return nil
 }
 
 // isTestFunc tells whether fn has the type of a testing function. arg
diff --git a/src/cmd/go/testdata/script/list_test_cycle.txt b/src/cmd/go/testdata/script/list_test_cycle.txt
new file mode 100644 (file)
index 0000000..2ab8528
--- /dev/null
@@ -0,0 +1,33 @@
+go list ./p
+stdout 'example/p'
+
+! go list -json=ImportPath -test ./p
+cmp stderr wanterr.txt
+
+! go list -json=ImportPath,Deps -test ./p
+cmp stderr wanterr.txt
+
+! go list -json=ImportPath,Deps -deps -test ./p
+cmp stderr wanterr.txt
+
+! go list -json=ImportPath -deps -test ./p
+cmp stderr wanterr.txt
+
+-- wanterr.txt --
+can't load test package: package example/p
+       imports example/r
+       imports example/q: import cycle not allowed in test
+-- go.mod --
+module example
+go 1.20
+-- p/p.go --
+package p
+-- p/p_test.go --
+package p
+import "example/q"
+-- q/q.go --
+package q
+import "example/r"
+-- r/r.go --
+package r
+import "example/p"
index 25dbb969b01c06a04d1869879168f4a52422923e..02bd6a16d41caf4b20af6cd38cf3748613cab41d 100644 (file)
@@ -3,19 +3,6 @@ env GO111MODULE=off
 # issue 28491: errors in test source files should not prevent
 # "go list -test" from returning useful information.
 
-# go list prints information for package, internal test,
-# external test, but not testmain package when there is a
-# syntax error in test sources.
-! go list -test -deps syntaxerr
-stdout pkgdep
-stdout testdep_a
-stdout testdep_b
-stdout ^syntaxerr$
-stdout '^syntaxerr \[syntaxerr.test\]'
-stdout '^syntaxerr_test \[syntaxerr.test\]'
-! stdout '^syntaxerr\.test'
-stderr 'expected declaration'
-
 # go list -e prints information for all test packages.
 # The syntax error is shown in the package error field.
 go list -e -test -deps -f '{{.ImportPath}} {{.Error | printf "%q"}}' syntaxerr
@@ -30,13 +17,6 @@ stdout 'syntaxerr\.test "[^"]*expected declaration'
 
 [short] stop
 
-# go list prints partial information with test naming error
-! go list -test -deps nameerr
-stdout pkgdep
-stdout testdep_a
-stdout testdep_b
-stderr 'wrong signature for TestBad'
-
 go list -e -test -deps -f '{{.ImportPath}} {{.Error | printf "%q"}}' nameerr
 stdout 'pkgdep <nil>'
 stdout 'testdep_a <nil>'
@@ -48,15 +28,10 @@ stdout 'nameerr\.test "[^"]*wrong signature for TestBad'
 ! go list -test -deps genericerr
 stderr 'wrong signature for TestGeneric, test functions cannot have type parameters'
 
-# go list prints partial information with error if test has cyclic import
-! go list -test -deps cycleerr
-stdout cycleerr
-stderr 'import cycle not allowed in test'
-
 go list -e -test -deps -f '{{.ImportPath}} {{.Error | printf "%q"}}' cycleerr
 stdout 'cycleerr <nil>'
 stdout 'testdep_a <nil>'
-stdout 'testdep_cycle <nil>'
+stdout 'testdep_cycle \[cycleerr.test\] <nil>'
 stdout 'cycleerr \[cycleerr.test\] "[^"]*import cycle not allowed in test'
 ! stderr 'import cycle not allowed in test'