go 1.10 test cache broken by override of getenv, lookupenv

[gedge]

Background

when running go test ./... and the tested code uses environment variables, go-1.10 now caches the access to those env vars: https://github.com/golang/go/commit/29be20a111d87b41b91e79a59fd3df95062ce91a#diff-b30ffd1cf7612ccebac97fced83d5106R84

Symptoms

Tests can appear to pass when they should fail! They are not run due to prior cached successes.

(When they've previously been run successfully and that result is cached, and only the env vars have since changed, and the env vars are used only by envconfig.)

Cause

syscall.Getenv is not compatible with the cacheing used by go1.10 - it does not log accesses to env vars. So separate runs of go test ./... (with differing env vars) are treated as cached and appear to pass (if passed previously and cached) when they should fail.

Quick fixes

one of:

• replace the lookupEnv call in envconfig.go with os.LookupEnv and it solves the problem
• use go clean -cache a lot more. 😄
• use go test -count=1 ./... to force cache-invalidation

Fix

use go1.5 tag in env_*.go - see patch below

[teepark]

I still don't see how this can come about. There is no environment variable cache, it is a test result cache where the cache key hash includes environment variables accessed via os.LookupEnv. But:

1. package and test code is also a part of the cache key
2. envconfig tests explicitly set all env vars in code before accessing them

The environment variables that envconfig reads in tests don't change unless there is also a change to the test code that sets them up, which would invalidate the cache. So there shouldn't be any difference between accessing them via syscall or os -- cache invalidation should always result from the code change anyway. Even tests that check the absence of a variable are calling os.Clearenv first.

Can you provide steps to reproduce the incorrect test result? I'm not actually against preferring os.LookupEnv in environments that have it (that's probably a good idea anyway), but I still want to understand the problem you're having.

[gedge]

Firstly, I'm not talking about the tests in this repo. I'm talking about code that I've written that uses envconfig and I am testing my code. This affects all code that uses envconfig.

Secondly, when my code builds, it is not running under Google App Engine, so the appengine tag is not set (why is it using that, and not go1.5, I don't know). So my code is built using the version of lookupEnv that is from env_syscall.go which uses syscall.Getenv(...) which (when used by go test ./...) does not add env var accesses to the cached info.

https://github.com/golang/go/blob/master/src/syscall/env_unix.go#L71

So, when I re-run my test without code changes, but with changed env var values, the cache seems still valid (envconfig has by-passed the cache's view of env var accesses, so go test ./... doesn't see that my env vars have changed).

For example code, see the last commit in https://github.com/gedge/envconfig/tree/show-cache-fails - run that script

[gedge]

hi @jprobinson - can you please confirm that 9aca109c9aec4633fced9717c4a09ecab3d33111 used appengine tag where it should have used go1.5 - otherwise, I'm very confused. Thanks.

[teepark]

@gedge at the time we had already moved over to syscall but realized that function isn't available on appengine, hence the appengine tag. The strategy was "use syscall anywhere we can, fall back to os". This 1.10 change is a good impetus to change over to favoring os. Thanks for all the clarifications.