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: add known variables to go command to the test cache key hash #32285

Closed
cuonglm opened this issue May 28, 2019 · 14 comments
Closed

cmd/go: add known variables to go command to the test cache key hash #32285

cuonglm opened this issue May 28, 2019 · 14 comments
Milestone

Comments

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented May 28, 2019

What version of Go are you using (go version)?

$ go version
go version devel +0f897f916a Tue May 28 02:52:39 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/cuonglm/.cache/go-build"
GOENV="/home/cuonglm/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/cuonglm/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/cuonglm/sources/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/cuonglm/sources/go/pkg/tool/linux_amd64"
GCCGO="/usr/local/bin/gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build895058594=/tmp/go-build -gno-record-gcc-switches"

What did you do?

$ GO111MODULE=on go test cmd/vet
ok  	cmd/vet	2.463s
$ GO111MODULE=off go test cmd/vet
ok  	cmd/vet	(cached)

What did you expect to see?

GO111MODULE=off go test cmd/vet won't be cached, the test fail:

$ GO111MODULE=off go test -count=1 cmd/vet
--- FAIL: TestVet (0.48s)
    --- FAIL: TestVet/unsafeptr (0.14s)
        vet_test.go:153: vet output:
        vet_test.go:154: <nil>
    --- FAIL: TestVet/print (0.25s)
        vet_test.go:162: error check failed: 
            print.go:169: missing error "Warn call has possible formatting directive %s"
            print.go:170: missing error "Warnf call needs 1 arg but has 2 args"
            print.go:171: missing error "Warnf format %r has unknown verb r"
            print.go:172: missing error "Warnf format %#s has unrecognized flag #"
            print.go:173: missing error "Warn2 call has possible formatting directive %s"
            print.go:174: missing error "Warnf2 call needs 1 arg but has 2 args"
            print.go:175: missing error "Warnf2 format %r has unknown verb r"
            print.go:176: missing error "Warnf2 format %#s has unrecognized flag #"
FAIL
FAIL	cmd/vet	1.067s
FAIL

What did you see instead?

Test was cached, test pass.

@julieqiu

This comment has been minimized.

Copy link

@julieqiu julieqiu commented May 28, 2019

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 28, 2019

Change https://golang.org/cl/179040 mentions this issue: cmd/go: add GO111MODULE to test cache hash

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 29, 2019

As noted in #32107 (comment):

The test's dependency on GO111MODULE comes from the combination of being in the standard library (and hence not having any module dependencies), plus executing cmd/go as a subprocess (which adds the dependency on module-mode behavior).

Instead of hard-coding GO111MODULE in particular, I think we should make internal/testenv.GoToolPath touch all of the environment variables listed in cmd/go/internal/cfg.knownEnv. That makes the connection between the variables and the test results more explicit and much more robust: tests that execute the go command are potentially sensitive to all of the variables that the go command reads.

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented May 30, 2019

Reading known environment variables in internal/testenv.GoToolPath seems like a good compromise to me.

I don't think we should add special cases for any specific environment variable that tests might access.

Unfortunately, we can't tell exactly what environment variables a test accesses through subprocesses. In principle, we could avoid caching test results if a test starts a subprocess, but this would prevent caching far more often than necessary.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented May 30, 2019

In principle, we could avoid caching test results if a test starts a subprocess, but this would prevent caching far more often than necessary.

Hmm! We could examine the Env field used to start the subprocess and only touch the variables that get passed through, but I would bet that a significant fraction of subprocesses pass the environment through unaltered.

Another alternative might be to whitelist a set of “known volatile” environment variables (PIDs and such) and key the cache on everything else if a subprocess is started.

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

@cuonglm cuonglm commented Jun 11, 2019

As noted in #32107 (comment):

The test's dependency on GO111MODULE comes from the combination of being in the standard library (and hence not having any module dependencies), plus executing cmd/go as a subprocess (which adds the dependency on module-mode behavior).
Instead of hard-coding GO111MODULE in particular, I think we should make internal/testenv.GoToolPath touch all of the environment variables listed in cmd/go/internal/cfg.knownEnv. That makes the connection between the variables and the test results more explicit and much more robust: tests that execute the go command are potentially sensitive to all of the variables that the go command reads.

Would you mind describing more details here?

What do you mean "internal/testenv.GoToolPathtouch all of the environment variables listed incmd/go/internal/cfg.knownEnv`"?

AFAIK, GoToolPath only returns the path to go binary.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 1, 2019

What do you mean "internal/testenv.GoToolPath touch all of the environment variables listed in cmd/go/internal/cfg.knownEnv"?

I mean that we can iterate over the variables listed in cfg.knownEnv and call os.Getenv for each of them. That will trigger the usual environment-variable testing hooks to record that the test potentially depends on those variables.

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

@cuonglm cuonglm commented Jul 2, 2019

What do you mean "internal/testenv.GoToolPath touch all of the environment variables listed in cmd/go/internal/cfg.knownEnv"?

I mean that we can iterate over the variables listed in cfg.knownEnv and call os.Getenv for each of them. That will trigger the usual environment-variable testing hooks to record that the test potentially depends on those variables.

@bcmills would you mind pointing out which part of code does that thing. It's not clear to me that how can os.Getenv trigger it.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 2, 2019

@cuonglm, see

testlog.Getenv(key)

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

@cuonglm cuonglm commented Jul 3, 2019

@bcmills I modified testenv.GoToolPath:

// GoToolPath reports the path to the Go tool.
// It is a convenience wrapper around GoTool.
// If the tool is unavailable GoToolPath calls t.Skip.
// If the tool should be available and isn't, GoToolPath calls t.Fatal.
func GoToolPath(t testing.TB) string {
	MustHaveGoBuild(t)
	path, err := GoTool()
	if err != nil {
		t.Fatal(err)
	}
	for _, envVar := range strings.Split(knownEnv, "\n") {
		testlog.Getenv(strings.TrimPrefix(envVar, "\t"))
	}
	return path
}

then running the test, no output at all. What did I missed?

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 11, 2019

@cuonglm, it shouldn't produce any extra output — just extra metadata in the cache, so that if you change one or more of those variables to a new combination and re-run the go test command it won't be cached.

@cuonglm

This comment has been minimized.

Copy link
Contributor Author

@cuonglm cuonglm commented Jul 12, 2019

@bcmills Thanks, the test is not cached now when I change GO111MODULE value. I updated the CL with newest change.

@cuonglm cuonglm changed the title cmd/go: add GO111MODULE to the test cache key hash cmd/go: add known variables to go command to the test cache key hash Jul 12, 2019
@gopherbot gopherbot closed this in 4b36588 Jul 15, 2019
@cuonglm

This comment has been minimized.

Copy link
Contributor Author

@cuonglm cuonglm commented Jul 16, 2019

@bcmills @jayconrod should we add a note in changelog for this go test behavior?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 16, 2019

Since it only affects the tests of the standard library, I don't think it needs a release note. (It's a nice improvement for folks working on the Go toolchain, but I don't think most users will notice — users don't often run tests that depend on internal/testenv.)

That said, we probably do need more documentation in the community on the general topic of “how to get your tests to play well with caching”.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.