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.

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

passed reruns do not cache correctly? #351

Open
nfi-hashicorp opened this issue Aug 3, 2023 · 6 comments
Open

passed reruns do not cache correctly? #351

nfi-hashicorp opened this issue Aug 3, 2023 · 6 comments

Comments

@nfi-hashicorp
Copy link
Contributor

nfi-hashicorp commented Aug 3, 2023

This is kind of surprising behavior, unless you know details about how Go does test caching.

When using gotestsum --rerun-fails, if the first run fails but the package ultimately passes, that result doesn't really get cached.

This is because go test doesn't treat -run specially. It doesn't cache the result at a function level at all. I did more investigation and a repro here: https://github.com/nfi-hashicorp/gotestsum-rerun-caching

I'm not sure there's anything to be done about this?

@dnephin
Copy link
Member

dnephin commented Aug 4, 2023

Ya, that is not surprising to me given the description of the test cache (https://pkg.go.dev/cmd/go/internal/test, specifically the paragraph that descries "list mode", and the following one that explains "go test caches successful package test
results"). The -run flag is included in the cache key. It does sound like you could cache individual tests with -run, but then you'd have to run them all individually the next time. The cost of running them individually probably negates the benefit of caching in most cases, but it might be worth trying.

I'm assuming you're working on this for Consul. From my experience Consul would benefit significantly from splitting its two largest packages (agent/consul, and agent if I remember correctly) into smaller packages. That would allow more tests to run in parallel and also make better use of the package cache.

@dnephin
Copy link
Member

dnephin commented Aug 4, 2023

This bit is interesting as well

Tests that [...] consult environment variables only match future runs in which the files and environment variables are unchanged.

In CI it's unlikely the test cache will be useful by default, because the environment variables set by the CI system will be different on each run (ex: build number). Presumably most large projects read environment variables in one place or another.

You would have to run go test with env -i (https://man7.org/linux/man-pages/man1/env.1.html) and be explicit about which environment variables to pass to go test.

@nfi-hashicorp
Copy link
Contributor Author

I'm assuming you're working on this for Consul. From my experience Consul would benefit significantly from splitting its two largest packages (agent/consul, and agent if I remember correctly) into smaller packages. That would allow more tests to run in parallel and also make better use of the package cache.

I do agree it is looking to be a pragmatic idea. I'm struggling to get all the unit tests to pass in the same run at the moment. 😅

@nfi-hashicorp
Copy link
Contributor Author

The cost of running them individually probably negates the benefit of caching in most cases, but it might be worth trying.

Yeah, then I'd have to run them in parallel, all the runner flags, etc. Honestly I'm almost at the point of cloning the caching logic and hacking it into gotestsum somehow. :P

@nfi-hashicorp
Copy link
Contributor Author

In CI it's unlikely the test cache will be useful by default, because the environment variables set by the CI system will be different on each run (ex: build number). Presumably most large projects read environment variables in one place or another.

I could be wrong but I believe it hooks os.Getenv and only cares about the env vars (and files) that your tests access. That said, there's os.Environ() that gives you the whole shebang so 🤷. I am definitely getting some caching happening in Github Actions atm

@dnephin
Copy link
Member

dnephin commented Aug 4, 2023

Cool, maybe it is smart enough to only consider the env vars that are accessed as long as os.Environ is not called. I would expect env vars from the code under test to be significant to caching as well. I don't think any program would really be able to distinguish "test code" from "production code" when its all compiled into the same binary.

Since each package is run as a separate binary, maybe there are enough packages that don't access environment variables that it works more often then I expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants