Skip to content

Commit

Permalink
cmd/go: make go list error behavior consistent in tests
Browse files Browse the repository at this point in the history 
"go list -test" constructs a package graph, then creates test packages
for the target. If it encounters an error (for example, a syntax error
in a test file or a test function with the wrong signature), it
reports the error and exits without printing the test packages or
their dependencies, even if the -e flag is given. This is a problem
for tools that operate on test files while users are editing them. For
example, autocomplete may not work while the user is typing.

With this change, a new function, load.TestPackagesAndErrors replaces
TestPackagesFor. The new function attaches errors to the returned test
packages instead of returning immediately. "go list -test" calls this
when the -e flag is set. TestPackagesFor now returns the same error as
before, but it returns non-nil packages so that "go list -test"
without -e can print partial results.

Fixes #28491

Change-Id: I141765c4574eae424d872eb9bf7dd63fdfb85efb
Reviewed-on: https://go-review.googlesource.com/c/go/+/164357
Run-TryBot: Jay Conrod <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Bryan C. Mills <[email protected]>
  • Loading branch information
Jay Conrod committed Mar 8, 2019
1 parent 1ab9f68 commit f1d5ce0
Show file tree
Hide file tree
Showing 5 changed files with 253 additions and 95 deletions.
45 changes: 21 additions & 24 deletions src/cmd/go/internal/list/list.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,37 +447,34 @@ func runList(cmd *base.Command, args []string) {
continue
}
if len(p.TestGoFiles)+len(p.XTestGoFiles) > 0 {
pmain, ptest, pxtest, err := load.TestPackagesFor(p, nil)
if err != nil {
if *listE {
pkgs = append(pkgs, &load.Package{
PackagePublic: load.PackagePublic{
ImportPath: p.ImportPath + ".test",
Error: &load.PackageError{Err: err.Error()},
},
})
continue
var pmain, ptest, pxtest *load.Package
var err error
if *listE {
pmain, ptest, pxtest = load.TestPackagesAndErrors(p, nil)
} else {
pmain, ptest, pxtest, err = load.TestPackagesFor(p, nil)
if err != nil {
base.Errorf("can't load test package: %s", err)
}
base.Errorf("can't load test package: %s", err)
continue
}
pkgs = append(pkgs, pmain)
if ptest != nil {
if pmain != nil {
pkgs = append(pkgs, pmain)
data := *pmain.Internal.TestmainGo
h := cache.NewHash("testmain")
h.Write([]byte("testmain\n"))
h.Write(data)
out, _, err := c.Put(h.Sum(), bytes.NewReader(data))
if err != nil {
base.Fatalf("%s", err)
}
pmain.GoFiles[0] = c.OutputFile(out)
}
if ptest != nil && ptest != p {
pkgs = append(pkgs, ptest)
}
if pxtest != nil {
pkgs = append(pkgs, pxtest)
}

data := *pmain.Internal.TestmainGo
h := cache.NewHash("testmain")
h.Write([]byte("testmain\n"))
h.Write(data)
out, _, err := c.Put(h.Sum(), bytes.NewReader(data))
if err != nil {
base.Fatalf("%s", err)
}
pmain.GoFiles[0] = c.OutputFile(out)
}
}
}
Expand Down
78 changes: 43 additions & 35 deletions src/cmd/go/internal/load/pkg.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1424,41 +1424,7 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) {
}
}
p.Internal.Imports = imports

deps := make(map[string]*Package)
var q []*Package
q = append(q, imports...)
for i := 0; i < len(q); i++ {
p1 := q[i]
path := p1.ImportPath
// The same import path could produce an error or not,
// depending on what tries to import it.
// Prefer to record entries with errors, so we can report them.
p0 := deps[path]
if p0 == nil || p1.Error != nil && (p0.Error == nil || len(p0.Error.ImportStack) > len(p1.Error.ImportStack)) {
deps[path] = p1
for _, p2 := range p1.Internal.Imports {
if deps[p2.ImportPath] != p2 {
q = append(q, p2)
}
}
}
}

p.Deps = make([]string, 0, len(deps))
for dep := range deps {
p.Deps = append(p.Deps, dep)
}
sort.Strings(p.Deps)
for _, dep := range p.Deps {
p1 := deps[dep]
if p1 == nil {
panic("impossible: missing entry in package cache for " + dep + " imported by " + p.ImportPath)
}
if p1.Error != nil {
p.DepsErrors = append(p.DepsErrors, p1.Error)
}
}
p.collectDeps()

// unsafe is a fake package.
if p.Standard && (p.ImportPath == "unsafe" || cfg.BuildContext.Compiler == "gccgo") {
Expand Down Expand Up @@ -1528,6 +1494,48 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) {
}
}

// collectDeps populates p.Deps and p.DepsErrors by iterating over
// p.Internal.Imports.
//
// TODO(jayconrod): collectDeps iterates over transitive imports for every
// package. We should only need to visit direct imports.
func (p *Package) collectDeps() {
deps := make(map[string]*Package)
var q []*Package
q = append(q, p.Internal.Imports...)
for i := 0; i < len(q); i++ {
p1 := q[i]
path := p1.ImportPath
// The same import path could produce an error or not,
// depending on what tries to import it.
// Prefer to record entries with errors, so we can report them.
p0 := deps[path]
if p0 == nil || p1.Error != nil && (p0.Error == nil || len(p0.Error.ImportStack) > len(p1.Error.ImportStack)) {
deps[path] = p1
for _, p2 := range p1.Internal.Imports {
if deps[p2.ImportPath] != p2 {
q = append(q, p2)
}
}
}
}

p.Deps = make([]string, 0, len(deps))
for dep := range deps {
p.Deps = append(p.Deps, dep)
}
sort.Strings(p.Deps)
for _, dep := range p.Deps {
p1 := deps[dep]
if p1 == nil {
panic("impossible: missing entry in package cache for " + dep + " imported by " + p.ImportPath)
}
if p1.Error != nil {
p.DepsErrors = append(p.DepsErrors, p1.Error)
}
}
}

// SafeArg reports whether arg is a "safe" command-line argument,
// meaning that when it appears in a command-line, it probably
// doesn't have some special meaning other than its own name.
Expand Down
99 changes: 64 additions & 35 deletions src/cmd/go/internal/load/test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,44 +39,74 @@ type TestCover struct {
DeclVars func(*Package, ...string) map[string]*CoverVar
}

// TestPackagesFor returns three packages:
// TestPackagesFor is like TestPackagesAndErrors but it returns
// an error if the test packages or their dependencies have errors.
// Only test packages without errors are returned.
func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Package, err error) {
pmain, ptest, pxtest = TestPackagesAndErrors(p, cover)
for _, p1 := range []*Package{ptest, pxtest, pmain} {
if p1 == nil {
// pxtest may be nil
continue
}
if p1.Error != nil {
err = p1.Error
break
}
if len(p1.DepsErrors) > 0 {
perr := p1.DepsErrors[0]
perr.Pos = "" // show full import stack
err = perr
break
}
}
if pmain.Error != nil || len(pmain.DepsErrors) > 0 {
pmain = nil
}
if ptest.Error != nil || len(ptest.DepsErrors) > 0 {
ptest = nil
}
if pxtest != nil && (pxtest.Error != nil || len(pxtest.DepsErrors) > 0) {
pxtest = nil
}
return pmain, ptest, pxtest, err
}

// TestPackagesAndErrors returns three packages:
// - pmain, the package main corresponding to the test binary (running tests in ptest and pxtest).
// - ptest, the package p compiled with added "package p" test files.
// - pxtest, the result of compiling any "package p_test" (external) test files.
// - pmain, the package main corresponding to the test binary (running tests in ptest and pxtest).
//
// If the package has no "package p_test" test files, pxtest will be nil.
// If the non-test compilation of package p can be reused
// (for example, if there are no "package p" test files and
// package p need not be instrumented for coverage or any other reason),
// then the returned ptest == p.
//
// An error is returned if the testmain source cannot be completely generated
// (for example, due to a syntax error in a test file). No error will be
// returned for errors loading packages, but the Error or DepsError fields
// of the returned packages may be set.
//
// The caller is expected to have checked that len(p.TestGoFiles)+len(p.XTestGoFiles) > 0,
// or else there's no point in any of this.
func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Package, err error) {
func TestPackagesAndErrors(p *Package, cover *TestCover) (pmain, ptest, pxtest *Package) {
var ptestErr, pxtestErr *PackageError
var imports, ximports []*Package
var stk ImportStack
stk.Push(p.ImportPath + " (test)")
rawTestImports := str.StringList(p.TestImports)
for i, path := range p.TestImports {
p1 := LoadImport(path, p.Dir, p, &stk, p.Internal.Build.TestImportPos[path], ResolveImport)
if p1.Error != nil {
return nil, nil, nil, p1.Error
}
if len(p1.DepsErrors) > 0 {
err := p1.DepsErrors[0]
err.Pos = "" // show full import stack
return nil, nil, nil, err
}
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.
err := &PackageError{
ptestErr = &PackageError{
ImportStack: testImportStack(stk[0], p1, p.ImportPath),
Err: "import cycle not allowed in test",
IsImportCycle: true,
}
return nil, nil, nil, err
}
p.TestImports[i] = p1.ImportPath
imports = append(imports, p1)
Expand All @@ -87,14 +117,6 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
rawXTestImports := str.StringList(p.XTestImports)
for i, path := range p.XTestImports {
p1 := LoadImport(path, p.Dir, p, &stk, p.Internal.Build.XTestImportPos[path], ResolveImport)
if p1.Error != nil {
return nil, nil, nil, p1.Error
}
if len(p1.DepsErrors) > 0 {
err := p1.DepsErrors[0]
err.Pos = "" // show full import stack
return nil, nil, nil, err
}
if p1.ImportPath == p.ImportPath {
pxtestNeedsPtest = true
} else {
Expand All @@ -108,6 +130,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
if len(p.TestGoFiles) > 0 || p.Name == "main" || cover != nil && cover.Local {
ptest = new(Package)
*ptest = *p
ptest.Error = ptestErr
ptest.ForTest = p.ImportPath
ptest.GoFiles = nil
ptest.GoFiles = append(ptest.GoFiles, p.GoFiles...)
Expand Down Expand Up @@ -140,6 +163,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
m[k] = append(m[k], v...)
}
ptest.Internal.Build.ImportPos = m
ptest.collectDeps()
} else {
ptest = p
}
Expand All @@ -155,6 +179,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
GoFiles: p.XTestGoFiles,
Imports: p.XTestImports,
ForTest: p.ImportPath,
Error: pxtestErr,
},
Internal: PackageInternal{
LocalPrefix: p.Internal.LocalPrefix,
Expand All @@ -173,6 +198,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
if pxtestNeedsPtest {
pxtest.Internal.Imports = append(pxtest.Internal.Imports, ptest)
}
pxtest.collectDeps()
}

// Build main package.
Expand Down Expand Up @@ -207,9 +233,6 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
pmain.Internal.Imports = append(pmain.Internal.Imports, ptest)
} else {
p1 := LoadImport(dep, "", nil, &stk, nil, 0)
if p1.Error != nil {
return nil, nil, nil, p1.Error
}
pmain.Internal.Imports = append(pmain.Internal.Imports, p1)
}
}
Expand Down Expand Up @@ -240,8 +263,8 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
// The list of imports is used by recompileForTest and by the loop
// afterward that gathers t.Cover information.
t, err := loadTestFuncs(ptest)
if err != nil {
return nil, nil, nil, err
if err != nil && pmain.Error == nil {
pmain.Error = &PackageError{Err: err.Error()}
}
t.Cover = cover
if len(ptest.GoFiles)+len(ptest.CgoFiles) > 0 {
Expand All @@ -254,6 +277,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
pmain.Imports = append(pmain.Imports, pxtest.ImportPath)
t.ImportXtest = true
}
pmain.collectDeps()

// Sort and dedup pmain.Imports.
// Only matters for go list -test output.
Expand Down Expand Up @@ -299,12 +323,14 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
}

data, err := formatTestmain(t)
if err != nil {
return nil, nil, nil, err
if err != nil && pmain.Error == nil {
pmain.Error = &PackageError{Err: err.Error()}
}
if data != nil {
pmain.Internal.TestmainGo = &data
}
pmain.Internal.TestmainGo = &data

return pmain, ptest, pxtest, nil
return pmain, ptest, pxtest
}

func testImportStack(top string, p *Package, target string) []string {
Expand Down Expand Up @@ -420,21 +446,24 @@ type coverInfo struct {
}

// loadTestFuncs returns the testFuncs describing the tests that will be run.
// The returned testFuncs is always non-nil, even if an error occurred while
// processing test files.
func loadTestFuncs(ptest *Package) (*testFuncs, error) {
t := &testFuncs{
Package: ptest,
}
var err error
for _, file := range ptest.TestGoFiles {
if err := t.load(filepath.Join(ptest.Dir, file), "_test", &t.ImportTest, &t.NeedTest); err != nil {
return nil, err
if lerr := t.load(filepath.Join(ptest.Dir, file), "_test", &t.ImportTest, &t.NeedTest); lerr != nil && err == nil {
err = lerr
}
}
for _, file := range ptest.XTestGoFiles {
if err := t.load(filepath.Join(ptest.Dir, file), "_xtest", &t.ImportXtest, &t.NeedXtest); err != nil {
return nil, err
if lerr := t.load(filepath.Join(ptest.Dir, file), "_xtest", &t.ImportXtest, &t.NeedXtest); lerr != nil && err == nil {
err = lerr
}
}
return t, nil
return t, err
}

// formatTestmain returns the content of the _testmain.go file for t.
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/go/testdata/script/list_test_e.txt
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
env GO111MODULE=off

# issue 25980: crash in go list -e -test
go list -e -test -f '{{.Error}}' p
go list -e -test -deps -f '{{.Error}}' p
stdout '^p[/\\]d_test.go:2:8: cannot find package "d" in any of:'

-- p/d.go --
Expand Down
Loading

0 comments on commit f1d5ce0

Please sign in to comment.