cmd/go: test cache not invalidated when modifying testdata subdirectory

[fmeum]

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

$ go version
go version go1.18.2 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/fhenneke/.cache/go-build"
GOENV="/home/fhenneke/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/fhenneke/go/pkg/mod"
GONOPROXY="code-intelligence.com/*"
GONOSUMDB="code-intelligence.com/*"
GOOS="linux"
GOPATH="/home/fhenneke/go"
GOPRIVATE="code-intelligence.com/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.2"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/fhenneke/git/cifuzz/go.mod"
GOWORK=""
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-build2057125203=/tmp/go-build -gno-record-gcc-switches"

What did you do?

1. Have some test that accesses a directory testdata with test data that contains a subdirectory subdir that contains a file foo.
2. Run go test ./....
3. Change foo.
4. Run go test ./....

What did you expect to see?

The test is rerun despite caching being enabled since a data dependency changed.

What did you see instead?

The test is not run again.

Root cause

The cache invalidation logic in test.go lists all entries in testdata in

go/src/cmd/go/internal/test/test.go

Line 1737 in 61fc5df

files, err := os.ReadDir(name)

and then calls hashWriteStat on them (in this case, on testdata/subdir). This function records the results of a stat, including the mtime of the directory. But the mtime of a directory is not updated when any of the files it contains changes.

Instead, the logic could do a full recursive walk over the directory and collect the individual mtimes of the contents.

[seankhliao]

how are you using the file in tests/modifying the file?
I can't reproduce the described behavior

[fmeum]

how are you using the file in tests/modifying the file? I can't reproduce the described behavior

In my original example, I am running an external tool on the testdata directory after having accessed the directory from the Go test. I distilled this into a reproducer:

1. Clone https://github.com/fmeum/go-53053
2. Run ./test.sh

[seankhliao]

cc @bcmills @matloob

I see the read of data is via an external command (exec.Command(bash, -c, cat file)) which is why the change isn't tracked

[bcmills]

Indeed. In general the Go test process must touch all of the data that should be used as a cache input, since it doesn't have instrumentation for other programs.

[fmeum]

Indeed. In general the Go test process must touch all of the data that should be used as a cache input, since it doesn't have instrumentation for other programs.

I do not expect Go to pick up changes to files touched by external tools only.

What I find surprising and what led me to open this issue is that touching a directory from a Go test results in certain changes to the contents of this directory to cause cache invalidation, but not all such changes.

[bcmills]

I think probably the best we could do here is provide some library function that touches all of files in the testdata subdirectory for the current package (https://go.dev/play/p/cnjP1PbosLa). Maybe that's worth doing in the testing package by default?

[bcmills]

What I find surprising and what led me to open this issue is that touching a directory from a Go test results in certain changes to the contents of this directory to cause cache invalidation, but not all such changes.

A change should cause the cache to be invalidated if (and only if) it changes the result of a function called by the Go test process. I agree that that is sometimes surprising when subprocesses are involved. 😅

[fmeum]

I think probably the best we could do here is provide some library function that touches all of files in the testdata subdirectory for the current package (https://go.dev/play/p/cnjP1PbosLa). Maybe that's worth doing in the testing package by default?

That sounds great. It's exactly the workaround I have adopted (save for enabling this by default for testing, of course ;-))

[seankhliao]

or maybe only if exec is used? it sounds like a very expensive operation especially if there's a deep hierarchy that's only used by external tools