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

testing: start test log for caching before TestMain #24122

Open
bakatz opened this Issue Feb 25, 2018 · 22 comments

Comments

Projects
None yet
6 participants
@bakatz

bakatz commented Feb 25, 2018

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

go version go1.10 windows/amd64

Does this issue reproduce with the latest release?

Yes

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

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\ben\AppData\Local\go-build
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\ben\dev\go
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\ben\AppData\Local\Temp\go-build167965298=/tmp/go-build -gno-record-gcc-switches

What did you do?

  1. Create a test_something.go file whose tests require a main_test.go file for setup/teardown
  2. Create several tests in test_something.go
  3. Run all of the tests
  4. Observe that the code in main_test is executed
  5. Change a non-go file that the test depends on

What did you expect to see?

main_test is executed again and a cache miss occurs.

What did you see instead?

main_test is not executed again and a cache hit occurs. See https://github.com/bakatz/golangissue21422 for more info.

@bakatz bakatz changed the title from go test doesn't run test_main when a cache hit occurs to testing: go test doesn't run test_main when a cache hit occurs Feb 25, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 25, 2018

What is test_main? Do you mean TestMain?

@bakatz

This comment has been minimized.

bakatz commented Feb 25, 2018

@ianlancetaylor whoops, yes! Meant main_test.go, i.e. TestMain. Will update the issue now.

EDIT: Issue updated.

@bakatz bakatz changed the title from testing: go test doesn't run test_main when a cache hit occurs to testing: go test doesn't run MainTest code when a cache hit occurs Feb 25, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Feb 25, 2018

Thanks. For complete clarity, can you provide a small self-contained example?

@bakatz

This comment has been minimized.

bakatz commented Feb 25, 2018

Sure, give me a few minutes to throw together some sample code and I'll update you when it's ready.

@bakatz

This comment has been minimized.

bakatz commented Feb 25, 2018

My fault: as it turns out I can no longer reproduce this issue today. It occurred yesterday with the same exact version of go, set of tests, etc.

I'll reopen this issue if needed once I figure out what set of steps caused the original failure (clearly the steps I gave are not accurate)

Thanks

@bakatz bakatz closed this Feb 25, 2018

@bakatz

This comment has been minimized.

bakatz commented Feb 27, 2018

@ianlancetaylor Figured it out. Basically, the issue is with SQL migrations. My TestMain is responsible for running these migrations when the test run starts, but since .sql files are not imported by any .go files, I'm assuming that the go test runner has no idea about these migration files and doesn't know to invalidate the cache and run the TestMain code in order to run the migrations. This means that every time I add a new migration, it won't be applied unless I run go test -count=1 ./... manually

To be fair, I don't think it should be Go's responsibility to keep track of extraneous files in the repository and know which ones are critical to the testing process, but I also think it could make sense to always run TestMain (but not necessarily all other tests), even when there's a cache hit, in order to avoid issues like this. Alternatively, the CLI flag that I mentioned in my first post would be useful for me. What do you think?

@bakatz bakatz reopened this Feb 27, 2018

@ysmolsky

This comment has been minimized.

Member

ysmolsky commented Feb 27, 2018

@bakatz can you please provide an example of code with the issue?

@bakatz

This comment has been minimized.

bakatz commented Mar 1, 2018

@ysmolsky sure, apologies on the delay on this; been really busy at work. I'll provide an example repo when I next have a free moment. Should be within the next day or two

@andybons andybons changed the title from testing: go test doesn't run MainTest code when a cache hit occurs to cmd/go: go test doesn't run MainTest code when a cache hit occurs Mar 13, 2018

@andybons

This comment has been minimized.

Member

andybons commented Mar 26, 2018

Ping on that info, @bakatz

@andybons andybons added this to the Go1.11 milestone Mar 26, 2018

@bakatz

This comment has been minimized.

bakatz commented Mar 28, 2018

@andybons Whoops, I completely forgot about providing a repo for this issue. I've just reproduced the issue with a self-contained piece of code and I'm pushing up the code to github right now. I'll provide a link to the repo once the code is available. Thanks for the reminder

@bakatz

This comment has been minimized.

bakatz commented Mar 28, 2018

@andybons here is the self-contained repository which you can use to reproduce the bug. See README.md for instructions: https://github.com/bakatz/golangissue21422

EDIT: I've also updated the original issue's "steps to reproduce" since I now know exactly how to reproduce the issue.

@bakatz bakatz changed the title from cmd/go: go test doesn't run MainTest code when a cache hit occurs to cmd/go: go test doesn't run MainTest code when a cache hit occurs and a non-go file is changed Mar 28, 2018

@bakatz

This comment has been minimized.

bakatz commented Jun 28, 2018

Hey @andybons - I'd like to discuss how this issue should be fixed and then just try to fix it myself since I have some free time at the moment. I have a couple questions:

  1. Do you think that some sort of default behavior is appropriate here? i.e. always run TestMain even if a cache hit occurs?
  2. If not, what about adding a flag to the go test command that forces TestMain to be run?

Thanks!

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 28, 2018

I do not think we should always run TestMain. If the test is cached, then it is cached. The only question here is whether we should consider the test to be cached or not.

The current guideline we use is that if the test opens any files under GOPATH, then if those files change then the test is no longer cached. This is documented at https://golang.org/cmd/go: "Tests that open files within the package's source root (usually $GOPATH) or that consult environment variables only match future runs in which the files and environment variables are unchanged."

It sounds like your test is opening a .sql file that is not under GOPATH, and that that file is changing. If that is correct, then we do expect the test caching to fail for your case. The workaround for this sort of test is to run it with -test.count=1, which disables caching.

We could also consider changing the current cache plan if we can come up with a simple and efficient mechanism.

@bakatz

This comment has been minimized.

bakatz commented Jun 29, 2018

@ianlancetaylor Gotcha. That is not correct RE: "opening a .sql file that is not under $GOPATH" -- all of my files are under the directory $GOPATH/src/golangissue21422 in the case of the repository that reproduces this issue.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 29, 2018

@bakatz What exactly is changing that causes the cached test result to be invalid?

@bakatz

This comment has been minimized.

bakatz commented Jun 29, 2018

@ianlancetaylor Adding a new .sql file or updating an existing .sql file within some subdirectory under $GOPATH (i.e. $GOPATH/src/someproject/migrations/somenewmigration.sql). To explain the "real world" situation a bit more: say I have a failing test that is written for an API that isn't working correctly because the database is missing a column. In order to fix this test, I go and add the missing column via a new .sql migration script and re-run the test. The test comes back as cached, so the new .sql migration script that has been added to the filesystem doesn't run, and the test continues to "fail" (even though it's just returning the cached failed result). I then have to re-run the test with -count=1 in order to force the new migration script to run. Let me know if this helps.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 29, 2018

@bakatz Thanks. That sounds like a bug. Can you provide the output of GODEBUG=gocachehash=1 go test on this issue or in a gist or something? Note: the output will be long.

@ianlancetaylor ianlancetaylor removed this from the Go1.11 milestone Jun 29, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Jun 29, 2018

@gopherbot gopherbot removed the NeedsFix label Jun 29, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 29, 2018

Oh, sorry, you provided a repro above. I'll take a look.

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 29, 2018

I see the problem. The checks for whether changed files affect the hash only affect tests themselves. Files opened in TestMain do not affect whether the test is cached. You could fix your tests by having the tests themselves open the file, e.g., via

var migrationContents []byte
var setMigrationContentsOnce sync.Once

func setMigrationContents(t *testing.T) {
 	migrationContents, err := ioutil.ReadFile("./some_migration.sql")
	if err != nil {
		t.Fatal(err)
	}
}

func TestSomething(t *testing.,T) {
	setMigrationContentsOnce.Do(setMigrationContents)
}

@ianlancetaylor ianlancetaylor changed the title from cmd/go: go test doesn't run MainTest code when a cache hit occurs and a non-go file is changed to testing: start test log for caching before TestMain Jun 29, 2018

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 29, 2018

I'll change this issue to consider whether we should check file I/O for caching in TestMain. That won't be until 1.12, though.

@bakatz

This comment has been minimized.

bakatz commented Jun 29, 2018

@ianlancetaylor thanks for looking into this. The sync.Once workaround is pretty neat; didn't know sync.Once existed till now! Regarding the decision though: is there an argument for not having a file I/O check RE: TestMain? It seems like the behavior here should be the same as it is for any other regular test, but maybe I'm missing something?

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jun 29, 2018

As I see it, the argument against is basically that, at least given the current implementation, it would mean that code before TestMain would have to look through the flags, without actually calling flag.Parse. And that might be a little error prone. I think it's pretty clearly desirable if there is a clean and safe way to implement it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment