From 629a7be6d0a4808cd8e7af805986652d81bb975f Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 30 Apr 2024 23:07:49 -0400 Subject: [PATCH] go/analysis/analysistest: stricter errors and GOWORK setting This change causes analysistest to report complete failure to load one or more of the packages named by the load patterns. (This is indicated by a missing Package.Name.) Previously, these errors were silently ignored, meaning that tests were providing less coverage than they seemed. In particular, the stdversion test was unable to load the specified modules because there was no GOWORK file to locate them. (Worse: the user's GOWORK environment was affecting the test.) So, we now allow tests to specify a go.work file in the root of the test tree. If present, we honor it, and, crucially, if absent, we set GOWORK=off. Also, two other tests in gopls/internal/analysis were silently doing nothing (!). Change-Id: I4ee7ae2a636497a64f1e43eb05a4a414ceaa5f4a Reviewed-on: https://go-review.googlesource.com/c/tools/+/582595 Auto-Submit: Alan Donovan Reviewed-by: Tim King LUCI-TryBot-Result: Go LUCI --- go/analysis/analysistest/analysistest.go | 22 ++++++++++++++++--- .../passes/stdversion/testdata/test.txtar | 7 ++++++ .../testdata/src/{a => }/typeparams/a.go | 0 .../src/{a => }/typeparams/a.go.golden | 0 .../analysis/stubmethods/stubmethods_test.go | 2 +- .../testdata/src/typeparams/implement.go | 2 +- 6 files changed, 28 insertions(+), 5 deletions(-) rename gopls/internal/analysis/fillreturns/testdata/src/{a => }/typeparams/a.go (100%) rename gopls/internal/analysis/fillreturns/testdata/src/{a => }/typeparams/a.go.golden (100%) diff --git a/go/analysis/analysistest/analysistest.go b/go/analysis/analysistest/analysistest.go index 95db20f4be3..368cb1bd2d9 100644 --- a/go/analysis/analysistest/analysistest.go +++ b/go/analysis/analysistest/analysistest.go @@ -369,11 +369,16 @@ type Result = checker.TestAnalyzerResult // loadPackages returns an error if any package had an error, or the pattern // matched no packages. func loadPackages(a *analysis.Analyzer, dir string, patterns ...string) ([]*packages.Package, error) { - env := []string{"GOPATH=" + dir, "GO111MODULE=off"} // GOPATH mode + env := []string{"GOPATH=" + dir, "GO111MODULE=off", "GOWORK=off"} // GOPATH mode // Undocumented module mode. Will be replaced by something better. if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { - env = []string{"GO111MODULE=on", "GOPROXY=off"} // module mode + gowork := filepath.Join(dir, "go.work") + if _, err := os.Stat(gowork); err != nil { + gowork = "off" + } + + env = []string{"GO111MODULE=on", "GOPROXY=off", "GOWORK=" + gowork} // module mode } // packages.Load loads the real standard library, not a minimal @@ -397,12 +402,23 @@ func loadPackages(a *analysis.Analyzer, dir string, patterns ...string) ([]*pack return nil, err } + // If any named package couldn't be loaded at all + // (e.g. the Name field is unset), fail fast. + for _, pkg := range pkgs { + if pkg.Name == "" { + return nil, fmt.Errorf("failed to load %q: Errors=%v", + pkg.PkgPath, pkg.Errors) + } + } + // Do NOT print errors if the analyzer will continue running. // It is incredibly confusing for tests to be printing to stderr // willy-nilly instead of their test logs, especially when the // errors are expected and are going to be fixed. if !a.RunDespiteErrors { - packages.PrintErrors(pkgs) + if packages.PrintErrors(pkgs) > 0 { + return nil, fmt.Errorf("there were package loading errors (and RunDespiteErrors is false)") + } } if len(pkgs) == 0 { diff --git a/go/analysis/passes/stdversion/testdata/test.txtar b/go/analysis/passes/stdversion/testdata/test.txtar index 796e1594042..9e66cee8c63 100644 --- a/go/analysis/passes/stdversion/testdata/test.txtar +++ b/go/analysis/passes/stdversion/testdata/test.txtar @@ -8,6 +8,13 @@ See also gopls/internal/test/marker/testdata/diagnostics/stdversion.txt which runs the same test within the gopls analysis driver, to ensure coverage of per-file Go version support. +-- go.work -- +go 1.21 + +use . +use sub +use old + -- go.mod -- module example.com diff --git a/gopls/internal/analysis/fillreturns/testdata/src/a/typeparams/a.go b/gopls/internal/analysis/fillreturns/testdata/src/typeparams/a.go similarity index 100% rename from gopls/internal/analysis/fillreturns/testdata/src/a/typeparams/a.go rename to gopls/internal/analysis/fillreturns/testdata/src/typeparams/a.go diff --git a/gopls/internal/analysis/fillreturns/testdata/src/a/typeparams/a.go.golden b/gopls/internal/analysis/fillreturns/testdata/src/typeparams/a.go.golden similarity index 100% rename from gopls/internal/analysis/fillreturns/testdata/src/a/typeparams/a.go.golden rename to gopls/internal/analysis/fillreturns/testdata/src/typeparams/a.go.golden diff --git a/gopls/internal/analysis/stubmethods/stubmethods_test.go b/gopls/internal/analysis/stubmethods/stubmethods_test.go index 86328ae4606..9c744c9b7a3 100644 --- a/gopls/internal/analysis/stubmethods/stubmethods_test.go +++ b/gopls/internal/analysis/stubmethods/stubmethods_test.go @@ -13,5 +13,5 @@ import ( func Test(t *testing.T) { testdata := analysistest.TestData() - analysistest.Run(t, testdata, stubmethods.Analyzer, "a") + analysistest.Run(t, testdata, stubmethods.Analyzer, "typeparams") } diff --git a/gopls/internal/analysis/stubmethods/testdata/src/typeparams/implement.go b/gopls/internal/analysis/stubmethods/testdata/src/typeparams/implement.go index be20e1d9904..7b6f2911ea9 100644 --- a/gopls/internal/analysis/stubmethods/testdata/src/typeparams/implement.go +++ b/gopls/internal/analysis/stubmethods/testdata/src/typeparams/implement.go @@ -4,7 +4,7 @@ package stubmethods -var _ I = Y{} // want "Implement I" +var _ I = Y{} // want "does not implement I" type I interface{ F() }