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: go test cache unaffected by GODEBUG value #65436

Closed
findleyr opened this issue Feb 1, 2024 · 8 comments
Closed

cmd/go: go test cache unaffected by GODEBUG value #65436

findleyr opened this issue Feb 1, 2024 · 8 comments
Assignees
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Feb 1, 2024

As we work on getting x/tools tests passing with GODEBUG=gotypesalias=1 in #65294, I noticed that GODEBUG is not included in the test cache key. It probably should be.

[gopls]> go test -count=1 ./doc
ok      golang.org/x/tools/gopls/doc    1.912s
[gopls]> GODEBUG=gotypesalias=1 go test ./doc
ok      golang.org/x/tools/gopls/doc    (cached)
[gopls]> GODEBUG=gotypesalias=1 go test -count=1 ./doc
--- FAIL: TestGenerated (1.71s)
    generate_test.go:26: documentation needs updating. Run: cd gopls && go generate
FAIL
FAIL    golang.org/x/tools/gopls/doc    1.774s
FAIL

CC @bcmills @matloob

@bcmills
Copy link
Contributor

bcmills commented Feb 1, 2024

Adding GODEBUG to the test cache key seems appropriate — every Go program depends on GODEBUG via the runtime package, and it is likely that the runtime parses that variable in a way that is not visible to the test log.

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. GoCommand cmd/go labels Feb 1, 2024
@bcmills bcmills added this to the Backlog milestone Feb 1, 2024
@bcmills

This comment was marked as outdated.

@bcmills bcmills marked this as a duplicate of #51561 Feb 1, 2024
@bcmills bcmills closed this as not planned Won't fix, can't repro, duplicate, stale Feb 1, 2024
@bcmills

This comment was marked as off-topic.

@bcmills bcmills reopened this Feb 1, 2024
@bcmills bcmills marked this as not a duplicate of #51561 Feb 1, 2024
@bcmills
Copy link
Contributor

bcmills commented Feb 1, 2024

Other environment variables go into the test-output cache key here:
https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/test/test.go;l=1869-1870;drc=e076c1b897b30648db794b66fd10929fe9a9852e

GODEBUG is probably not being recorded their because it is parsed directly by the runtime package without going through the usual os.Getenv hooks. We should probably add it to the cache key explicitly.

@samthanawalla
Copy link
Contributor

samthanawalla commented Feb 27, 2024

A little confused here, -count=1 disables test caching explictly. (link)
This is the current behavior:

$go clean -cache

$GODEBUG=gotypesalias=1 go test ./doc

runtime
../../go/src/runtime/runtime2.go:993:6: internal compiler error: assertion failed
FAIL

$ go test ./doc

ok golang.org/x/tools/gopls/doc 4.267s

$ GODEBUG=gotypesalias=1 go test ./doc

ok golang.org/x/tools/gopls/doc (cached)

$ go test -count=1 ./doc

ok golang.org/x/tools/gopls/doc 4.281s

$ GODEBUG=gotypesalias=1 go test -count=1 ./doc

--- FAIL: TestGenerated (3.88s)
generate_test.go:26: documentation needs updating. Run: cd gopls && go generate
FAIL
FAIL golang.org/x/tools/gopls/doc 3.955s
FAIL

It seems to me something fishy is going on with the first line: GODEBUG=gotypesalias=1 go test ./doc fails but then succeeds using the cached test with GODEBUG=gotypesalias=1 omitted.

Do we want GODEBUG=gotypesalias=1 go test ./doc to not use the cached result from go test ./doc?

@samthanawalla samthanawalla self-assigned this Feb 27, 2024
@bcmills
Copy link
Contributor

bcmills commented Feb 27, 2024

Do we want GODEBUG=gotypesalias=1 go test ./doc to not use the cached result from go test ./doc?

Yep, exactly. The mismatched GODEBUG setting should cause the cache key for the new test run not to match the prior run.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/570259 mentions this issue: cmd/go: respect GODEBUG in test cache

@rsc
Copy link
Contributor

rsc commented Mar 9, 2024

Sorry if you were working on this @samthanawalla. I ran into this problem myself and had to fix it to make progress debugging something else. I didn't see this issue until after I'd written the CL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants