/cc @bcmills @jayconrod

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

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.

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.

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.

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.

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.

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.

@cuonglm, see

@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?

@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.

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

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

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”.