Skip to content

Commit f1d5ce0

Browse files
author
Jay Conrod
committed
cmd/go: make go list error behavior consistent in tests
"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 golang#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]>
1 parent 1ab9f68 commit f1d5ce0

5 files changed

Lines changed: 253 additions & 95 deletions

File tree

‎src/cmd/go/internal/list/list.go‎

Lines changed: 21 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -447,37 +447,34 @@ func runList(cmd *base.Command, args []string) {
447447
continue
448448
}
449449
if len(p.TestGoFiles)+len(p.XTestGoFiles) > 0 {
450-
pmain, ptest, pxtest, err := load.TestPackagesFor(p, nil)
451-
if err != nil {
452-
if *listE {
453-
pkgs = append(pkgs, &load.Package{
454-
PackagePublic: load.PackagePublic{
455-
ImportPath: p.ImportPath + ".test",
456-
Error: &load.PackageError{Err: err.Error()},
457-
},
458-
})
459-
continue
450+
var pmain, ptest, pxtest *load.Package
451+
var err error
452+
if *listE {
453+
pmain, ptest, pxtest = load.TestPackagesAndErrors(p, nil)
454+
} else {
455+
pmain, ptest, pxtest, err = load.TestPackagesFor(p, nil)
456+
if err != nil {
457+
base.Errorf("can't load test package: %s", err)
460458
}
461-
base.Errorf("can't load test package: %s", err)
462-
continue
463459
}
464-
pkgs = append(pkgs, pmain)
465-
if ptest != nil {
460+
if pmain != nil {
461+
pkgs = append(pkgs, pmain)
462+
data := *pmain.Internal.TestmainGo
463+
h := cache.NewHash("testmain")
464+
h.Write([]byte("testmain\n"))
465+
h.Write(data)
466+
out, _, err := c.Put(h.Sum(), bytes.NewReader(data))
467+
if err != nil {
468+
base.Fatalf("%s", err)
469+
}
470+
pmain.GoFiles[0] = c.OutputFile(out)
471+
}
472+
if ptest != nil && ptest != p {
466473
pkgs = append(pkgs, ptest)
467474
}
468475
if pxtest != nil {
469476
pkgs = append(pkgs, pxtest)
470477
}
471-
472-
data := *pmain.Internal.TestmainGo
473-
h := cache.NewHash("testmain")
474-
h.Write([]byte("testmain\n"))
475-
h.Write(data)
476-
out, _, err := c.Put(h.Sum(), bytes.NewReader(data))
477-
if err != nil {
478-
base.Fatalf("%s", err)
479-
}
480-
pmain.GoFiles[0] = c.OutputFile(out)
481478
}
482479
}
483480
}

‎src/cmd/go/internal/load/pkg.go‎

Lines changed: 43 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1424,41 +1424,7 @@ func (p *Package) load(stk *ImportStack, bp *build.Package, err error) {
14241424
}
14251425
}
14261426
p.Internal.Imports = imports
1427-
1428-
deps := make(map[string]*Package)
1429-
var q []*Package
1430-
q = append(q, imports...)
1431-
for i := 0; i < len(q); i++ {
1432-
p1 := q[i]
1433-
path := p1.ImportPath
1434-
// The same import path could produce an error or not,
1435-
// depending on what tries to import it.
1436-
// Prefer to record entries with errors, so we can report them.
1437-
p0 := deps[path]
1438-
if p0 == nil || p1.Error != nil && (p0.Error == nil || len(p0.Error.ImportStack) > len(p1.Error.ImportStack)) {
1439-
deps[path] = p1
1440-
for _, p2 := range p1.Internal.Imports {
1441-
if deps[p2.ImportPath] != p2 {
1442-
q = append(q, p2)
1443-
}
1444-
}
1445-
}
1446-
}
1447-
1448-
p.Deps = make([]string, 0, len(deps))
1449-
for dep := range deps {
1450-
p.Deps = append(p.Deps, dep)
1451-
}
1452-
sort.Strings(p.Deps)
1453-
for _, dep := range p.Deps {
1454-
p1 := deps[dep]
1455-
if p1 == nil {
1456-
panic("impossible: missing entry in package cache for " + dep + " imported by " + p.ImportPath)
1457-
}
1458-
if p1.Error != nil {
1459-
p.DepsErrors = append(p.DepsErrors, p1.Error)
1460-
}
1461-
}
1427+
p.collectDeps()
14621428

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

1497+
// collectDeps populates p.Deps and p.DepsErrors by iterating over
1498+
// p.Internal.Imports.
1499+
//
1500+
// TODO(jayconrod): collectDeps iterates over transitive imports for every
1501+
// package. We should only need to visit direct imports.
1502+
func (p *Package) collectDeps() {
1503+
deps := make(map[string]*Package)
1504+
var q []*Package
1505+
q = append(q, p.Internal.Imports...)
1506+
for i := 0; i < len(q); i++ {
1507+
p1 := q[i]
1508+
path := p1.ImportPath
1509+
// The same import path could produce an error or not,
1510+
// depending on what tries to import it.
1511+
// Prefer to record entries with errors, so we can report them.
1512+
p0 := deps[path]
1513+
if p0 == nil || p1.Error != nil && (p0.Error == nil || len(p0.Error.ImportStack) > len(p1.Error.ImportStack)) {
1514+
deps[path] = p1
1515+
for _, p2 := range p1.Internal.Imports {
1516+
if deps[p2.ImportPath] != p2 {
1517+
q = append(q, p2)
1518+
}
1519+
}
1520+
}
1521+
}
1522+
1523+
p.Deps = make([]string, 0, len(deps))
1524+
for dep := range deps {
1525+
p.Deps = append(p.Deps, dep)
1526+
}
1527+
sort.Strings(p.Deps)
1528+
for _, dep := range p.Deps {
1529+
p1 := deps[dep]
1530+
if p1 == nil {
1531+
panic("impossible: missing entry in package cache for " + dep + " imported by " + p.ImportPath)
1532+
}
1533+
if p1.Error != nil {
1534+
p.DepsErrors = append(p.DepsErrors, p1.Error)
1535+
}
1536+
}
1537+
}
1538+
15311539
// SafeArg reports whether arg is a "safe" command-line argument,
15321540
// meaning that when it appears in a command-line, it probably
15331541
// doesn't have some special meaning other than its own name.

‎src/cmd/go/internal/load/test.go‎

Lines changed: 64 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -39,44 +39,74 @@ type TestCover struct {
3939
DeclVars func(*Package, ...string) map[string]*CoverVar
4040
}
4141

42-
// TestPackagesFor returns three packages:
42+
// TestPackagesFor is like TestPackagesAndErrors but it returns
43+
// an error if the test packages or their dependencies have errors.
44+
// Only test packages without errors are returned.
45+
func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Package, err error) {
46+
pmain, ptest, pxtest = TestPackagesAndErrors(p, cover)
47+
for _, p1 := range []*Package{ptest, pxtest, pmain} {
48+
if p1 == nil {
49+
// pxtest may be nil
50+
continue
51+
}
52+
if p1.Error != nil {
53+
err = p1.Error
54+
break
55+
}
56+
if len(p1.DepsErrors) > 0 {
57+
perr := p1.DepsErrors[0]
58+
perr.Pos = "" // show full import stack
59+
err = perr
60+
break
61+
}
62+
}
63+
if pmain.Error != nil || len(pmain.DepsErrors) > 0 {
64+
pmain = nil
65+
}
66+
if ptest.Error != nil || len(ptest.DepsErrors) > 0 {
67+
ptest = nil
68+
}
69+
if pxtest != nil && (pxtest.Error != nil || len(pxtest.DepsErrors) > 0) {
70+
pxtest = nil
71+
}
72+
return pmain, ptest, pxtest, err
73+
}
74+
75+
// TestPackagesAndErrors returns three packages:
76+
// - pmain, the package main corresponding to the test binary (running tests in ptest and pxtest).
4377
// - ptest, the package p compiled with added "package p" test files.
4478
// - pxtest, the result of compiling any "package p_test" (external) test files.
45-
// - pmain, the package main corresponding to the test binary (running tests in ptest and pxtest).
4679
//
4780
// If the package has no "package p_test" test files, pxtest will be nil.
4881
// If the non-test compilation of package p can be reused
4982
// (for example, if there are no "package p" test files and
5083
// package p need not be instrumented for coverage or any other reason),
5184
// then the returned ptest == p.
5285
//
86+
// An error is returned if the testmain source cannot be completely generated
87+
// (for example, due to a syntax error in a test file). No error will be
88+
// returned for errors loading packages, but the Error or DepsError fields
89+
// of the returned packages may be set.
90+
//
5391
// The caller is expected to have checked that len(p.TestGoFiles)+len(p.XTestGoFiles) > 0,
5492
// or else there's no point in any of this.
55-
func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Package, err error) {
93+
func TestPackagesAndErrors(p *Package, cover *TestCover) (pmain, ptest, pxtest *Package) {
94+
var ptestErr, pxtestErr *PackageError
5695
var imports, ximports []*Package
5796
var stk ImportStack
5897
stk.Push(p.ImportPath + " (test)")
5998
rawTestImports := str.StringList(p.TestImports)
6099
for i, path := range p.TestImports {
61100
p1 := LoadImport(path, p.Dir, p, &stk, p.Internal.Build.TestImportPos[path], ResolveImport)
62-
if p1.Error != nil {
63-
return nil, nil, nil, p1.Error
64-
}
65-
if len(p1.DepsErrors) > 0 {
66-
err := p1.DepsErrors[0]
67-
err.Pos = "" // show full import stack
68-
return nil, nil, nil, err
69-
}
70101
if str.Contains(p1.Deps, p.ImportPath) || p1.ImportPath == p.ImportPath {
71102
// Same error that loadPackage returns (via reusePackage) in pkg.go.
72103
// Can't change that code, because that code is only for loading the
73104
// non-test copy of a package.
74-
err := &PackageError{
105+
ptestErr = &PackageError{
75106
ImportStack: testImportStack(stk[0], p1, p.ImportPath),
76107
Err: "import cycle not allowed in test",
77108
IsImportCycle: true,
78109
}
79-
return nil, nil, nil, err
80110
}
81111
p.TestImports[i] = p1.ImportPath
82112
imports = append(imports, p1)
@@ -87,14 +117,6 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
87117
rawXTestImports := str.StringList(p.XTestImports)
88118
for i, path := range p.XTestImports {
89119
p1 := LoadImport(path, p.Dir, p, &stk, p.Internal.Build.XTestImportPos[path], ResolveImport)
90-
if p1.Error != nil {
91-
return nil, nil, nil, p1.Error
92-
}
93-
if len(p1.DepsErrors) > 0 {
94-
err := p1.DepsErrors[0]
95-
err.Pos = "" // show full import stack
96-
return nil, nil, nil, err
97-
}
98120
if p1.ImportPath == p.ImportPath {
99121
pxtestNeedsPtest = true
100122
} else {
@@ -108,6 +130,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
108130
if len(p.TestGoFiles) > 0 || p.Name == "main" || cover != nil && cover.Local {
109131
ptest = new(Package)
110132
*ptest = *p
133+
ptest.Error = ptestErr
111134
ptest.ForTest = p.ImportPath
112135
ptest.GoFiles = nil
113136
ptest.GoFiles = append(ptest.GoFiles, p.GoFiles...)
@@ -140,6 +163,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
140163
m[k] = append(m[k], v...)
141164
}
142165
ptest.Internal.Build.ImportPos = m
166+
ptest.collectDeps()
143167
} else {
144168
ptest = p
145169
}
@@ -155,6 +179,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
155179
GoFiles: p.XTestGoFiles,
156180
Imports: p.XTestImports,
157181
ForTest: p.ImportPath,
182+
Error: pxtestErr,
158183
},
159184
Internal: PackageInternal{
160185
LocalPrefix: p.Internal.LocalPrefix,
@@ -173,6 +198,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
173198
if pxtestNeedsPtest {
174199
pxtest.Internal.Imports = append(pxtest.Internal.Imports, ptest)
175200
}
201+
pxtest.collectDeps()
176202
}
177203

178204
// Build main package.
@@ -207,9 +233,6 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
207233
pmain.Internal.Imports = append(pmain.Internal.Imports, ptest)
208234
} else {
209235
p1 := LoadImport(dep, "", nil, &stk, nil, 0)
210-
if p1.Error != nil {
211-
return nil, nil, nil, p1.Error
212-
}
213236
pmain.Internal.Imports = append(pmain.Internal.Imports, p1)
214237
}
215238
}
@@ -240,8 +263,8 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
240263
// The list of imports is used by recompileForTest and by the loop
241264
// afterward that gathers t.Cover information.
242265
t, err := loadTestFuncs(ptest)
243-
if err != nil {
244-
return nil, nil, nil, err
266+
if err != nil && pmain.Error == nil {
267+
pmain.Error = &PackageError{Err: err.Error()}
245268
}
246269
t.Cover = cover
247270
if len(ptest.GoFiles)+len(ptest.CgoFiles) > 0 {
@@ -254,6 +277,7 @@ func TestPackagesFor(p *Package, cover *TestCover) (pmain, ptest, pxtest *Packag
254277
pmain.Imports = append(pmain.Imports, pxtest.ImportPath)
255278
t.ImportXtest = true
256279
}
280+
pmain.collectDeps()
257281

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

301325
data, err := formatTestmain(t)
302-
if err != nil {
303-
return nil, nil, nil, err
326+
if err != nil && pmain.Error == nil {
327+
pmain.Error = &PackageError{Err: err.Error()}
328+
}
329+
if data != nil {
330+
pmain.Internal.TestmainGo = &data
304331
}
305-
pmain.Internal.TestmainGo = &data
306332

307-
return pmain, ptest, pxtest, nil
333+
return pmain, ptest, pxtest
308334
}
309335

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

422448
// loadTestFuncs returns the testFuncs describing the tests that will be run.
449+
// The returned testFuncs is always non-nil, even if an error occurred while
450+
// processing test files.
423451
func loadTestFuncs(ptest *Package) (*testFuncs, error) {
424452
t := &testFuncs{
425453
Package: ptest,
426454
}
455+
var err error
427456
for _, file := range ptest.TestGoFiles {
428-
if err := t.load(filepath.Join(ptest.Dir, file), "_test", &t.ImportTest, &t.NeedTest); err != nil {
429-
return nil, err
457+
if lerr := t.load(filepath.Join(ptest.Dir, file), "_test", &t.ImportTest, &t.NeedTest); lerr != nil && err == nil {
458+
err = lerr
430459
}
431460
}
432461
for _, file := range ptest.XTestGoFiles {
433-
if err := t.load(filepath.Join(ptest.Dir, file), "_xtest", &t.ImportXtest, &t.NeedXtest); err != nil {
434-
return nil, err
462+
if lerr := t.load(filepath.Join(ptest.Dir, file), "_xtest", &t.ImportXtest, &t.NeedXtest); lerr != nil && err == nil {
463+
err = lerr
435464
}
436465
}
437-
return t, nil
466+
return t, err
438467
}
439468

440469
// formatTestmain returns the content of the _testmain.go file for t.

‎src/cmd/go/testdata/script/list_test_e.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
env GO111MODULE=off
22

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

77
-- p/d.go --

0 commit comments

Comments
 (0)