cmd/go,testing: GOWORK value does not invalidate test cache

[findleyr]

This may or may not be a bug. While investigating a gopls test that fails when using a go.work file, I noticed that the value of GOWORK is apparently not used in the test cache key:

$ go clean -cache
$ go test ./internal/lsp/safetoken
--- FAIL: TestGoplsSourceDoesNotCallTokenFileMethods (0.02s)
    safetoken_test.go:89: err: exit status 1: stderr: go: -mod may only be set to readonly when in workspace mode, but it is set to "mod"
        	Remove the -mod flag to use the default readonly value,
        	or set GOWORK=off to disable workspace mode.
        
FAIL
FAIL	golang.org/x/tools/gopls/internal/lsp/safetoken	0.036s
FAIL
$ GOWORK=off go test ./internal/lsp/safetoken
ok  	golang.org/x/tools/gopls/internal/lsp/safetoken	3.252s
$ go test ./internal/lsp/safetoken
ok  	golang.org/x/tools/gopls/internal/lsp/safetoken	(cached)

Observe that we get a cache hit for the successful test run when GOWORK=off is set, even when GOWORK=off is not set.

If I had to guess, I'd say that there is an assumption that GOWORK values affect the build list, and therefore if GOWORK changes the build list will change, and therefore we don't need to hash the actual GOWORK value into the cache key. However, as we see here this may not be sufficient if the test in question actually invokes the Go command. (note that my GOWORK uses exactly x/tools and x/tools/gopls, so there is likely no difference in the build list when run from the gopls module, which has a replace directive for x/tools).

@bcmills @matloob what do you think? Should the value of GOWORK be part of the cache key? If not, please feel free to close this issue.

[bcmills]

In general the cache key for a test only includes the factors that determine what's in the resulting binary and the environment variables and files explicitly accessed by the test itself. (And #33976 means that the binary doesn't depend on the build list as much as it probably should either.)

So if all of the packages are loaded from the same source files with or without go/work, this may be WAI.

But in this case, I guess the environment variable gets threaded through x/tools/go/packages and then read by the go command without being explicitly accessed by the test? I see that both os.Getenv and os.LookupEnv record accesses to keys, but Environ does not. Perhaps it should.

• https://cs.opensource.google/go/go/+/master:src/os/env.go;l=98-115;drc=b2faff18ce28edad98303d2c3134dec1331fd7b5
• https://cs.opensource.google/go/go/+/master:src/os/env.go;l=98-115;drc=b2faff18ce28edad98303d2c3134dec1331fd7b5

[bcmills]

I've filed #60057 to consider the more general os.Environ behavior.

[bcmills]

In the meantime, a workaround might be to do something like this in a test function:

for _, kv := range os.Environ() {
	if k, _, ok := strings.Cut(kv, "="); ok {
		_ = os.Getenv(k)
	}
}

Note that #59849 suggests this has to be in the test function itself rather than in TestMain.

[findleyr]

Thanks! I think you have confirmed that this is WAI, and I will defer discussion to #60057.

In the meantime, a workaround might be to do something like this in a test function:

Thanks for the suggestion, but I am loath to replace all of our calls to os.Environ.

Note that #59849 suggests this has to be in the test function itself rather than in TestMain.

That seems like a consequence of #44625 (comment).

Let's close this as unactionable, thanks.