Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
findleyr opened this issue May 8, 2023 · 4 comments
Closed

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

findleyr opened this issue May 8, 2023 · 4 comments

Comments

@findleyr
Copy link
Contributor

findleyr commented May 8, 2023

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
Copy link
Contributor

bcmills commented May 8, 2023

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.

@bcmills bcmills changed the title cmd/go: GOWORK value does not invalidate test cache cmd/go,testing: GOWORK value does not invalidate test cache May 8, 2023
@bcmills
Copy link
Contributor

bcmills commented May 8, 2023

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

@bcmills
Copy link
Contributor

bcmills commented May 8, 2023

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
Copy link
Contributor Author

findleyr commented May 8, 2023

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.

@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants