x/tools/gopls: integration testing may not test cases with old go versions

hyangah commented 1 month ago

The go directive in gopls's go.mod is set to 1.23.1 per #65917. The assumption is that gopls built with go1.23.1+ can be still used to analyze codebase with slightly older go versions. (e.g. go1.22.x, go1.21.x)

Gopls integration tests run on workspaces created in temp directories outside golang.org/x/tools/gopls module and their go.mod files have go version old enough to prevent toolchain switch. With this setup, I hoped running gopls tests on go1.X builders (X < 23) is sufficient to test our assumption.

But it looks like this is not true.

CL 404134 prepends GOROOT/bin to PATH in go test, as mentioned in #68005.

In CL 616055 I attempted to verify the go version.

$ GOTOOLCHAIN=local go version
go version go1.22.6 darwin/amd64

$ go version
go version go1.23.1 darwin/amd64

$ go test ./internal/test/integration/workspace -run TestCheckGoVersion
--- FAIL: TestCheckGoVersion (0.02s)
    workspace_test.go:1287: /Users/hakim/sdk/go1.23.1/bin/go <nil>
    workspace_test.go:1288: go version in PATH = 23
...

$ go test ./internal/test/integration/workspace -run TestViewGoVersion
...
--- FAIL: TestViewGoVersion (0.28s)
    --- FAIL: TestViewGoVersion/default (0.28s)
        workspace_test.go:1320: SummarizeViews() mismatch (-want +got):
              []command.View{
                {
                    ... // 1 ignored and 2 identical fields
                    Folder:     "file:///var/folders/5p/zn7ykc111kn3lm09h_47mz2w001py5/T/gopls-te"...,
                    EnvOverlay: nil,
            -       GoVersion:  "",
            +       GoVersion:  "go1.23.1",
                },
              }
              ...

I don't know how we can recover the original PATH inside tests reliably.

go test also sets GOROOT environment variable, too, but we can unset it.

@golang/tools-team

gabyhelp commented 1 month ago

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

findleyr commented 1 month ago

Thanks for investigating! This definitely needs to be fixed (see also #69321).

gopherbot commented 2 weeks ago

Change https://go.dev/cl/623175 mentions this issue: gopls/internal/test: fix path to local go in integration tests

golang / go

x/tools/gopls: integration testing may not test cases with old go versions #69630