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.

Sign up for GitHub

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

Jump to bottom

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

Open
fmeum opened this issue May 24, 2022 · 9 comments
Open

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

fmeum opened this issue May 24, 2022 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Milestone
Backlog

Comments

@fmeum
Copy link

fmeum commented May 24, 2022 •
edited

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
Copy link
Member

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

@seankhliao seankhliao changed the title cmd/go: test data cache invalidation logic is unsound cmd/go: test cache not invalidated when modifying testdata subdirectory May 24, 2022
@fmeum
Copy link
Author

fmeum commented May 24, 2022

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
Copy link
Member

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
Copy link
Contributor

bcmills commented May 24, 2022

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.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 24, 2022
@bcmills bcmills added this to the Backlog milestone May 24, 2022
@fmeum
Copy link
Author

fmeum commented May 24, 2022

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
Copy link
Contributor

bcmills commented May 24, 2022

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
Copy link
Contributor

bcmills commented May 24, 2022

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
Copy link
Author

fmeum commented May 24, 2022

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
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Unfortunate
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
3 participants
@fmeum @bcmills @seankhliao