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

proposal: cmd/go: invalidate cache entries for test runs using different timeouts #36134

Open
bcmills opened this issue Dec 13, 2019 · 6 comments
Open
Labels
Milestone

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Dec 13, 2019

In the resolution to #22633, we started explicitly ignoring the test timeout when computing its cache key:

case "-test.timeout":
// Special case: this is cacheable but ignored during the hash.
// Do not add to cacheArgs.

The use-case there was to be able to run a set of tests with a short timeout, and then increase that timeout in case of failure without re-running all of the tests.

However, that behavior seems incorrect to me: a test can today vary its behavior based on its timeout by using flag.Lookup("test.timeout") or examining os.Args directly. We have a pretty comprehensive test that checks for cache invalidation for all kinds of esoteric-but-observable behaviors, such as accessing files or environment variables — it's very surprising for one particular input to escape that check.

In #34707 (comment), I even suggested a situation in which a test might want to explicitly vary its behavior based on the timeout: a test using randomized inputs and running with a short timeout might want to check only a few cursory inputs, while a very long (or even unlimited!) timeout might imply a much more exhaustive search.

We do have a tracing hook that checks whether the test happens to observe external environment variables. Something like that might be reasonable for os.Args, but we don't have that today.

I propose that we revert the special case for -test.timeout, and if that proves to be too unpleasant we should add explicit tracking for access to the flag package and/or os.Args so that we only ignore -test.timeout for tests whose behavior verifiably does not depend on the timeout.

CC @jayconrod @rogpeppe @matloob

@gopherbot gopherbot added this to the Proposal milestone Dec 13, 2019
@gopherbot gopherbot added the Proposal label Dec 13, 2019
@bcmills bcmills changed the title proposal: cmd/go: invalidate test cache entries that use different timeouts proposal: cmd/go: invalidate cache entries for test runs using different timeouts Dec 13, 2019
@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Dec 13, 2019

Have you found any case where this behaviour is an issue currently? I tend to think that it's probably right that adjusting the timeout doesn't invalidate existing cached tests. When tests take a long time, it's frustrating to have them run again needlessly. If a test varies its behaviour based on the timeout, I think it probably deserves what it gets.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Dec 13, 2019

I wrote a bunch of regression tests for #32471 and they failed because the tests were being cached when I didn't expect.

It may be annoying to re-run tests when you change flags, but the general solution to that is to not request those tests when you know you're changing flags...

@rogpeppe

This comment has been minimized.

Copy link
Contributor

@rogpeppe rogpeppe commented Dec 17, 2019

In the past, I've worked a lot on projects that had very slow tests. Often there would be one or two packages that timed out and required a -test.timeout flag, so when running tests from the top level you'd use that flag. But within other subdirectories (still slow but not slow enough to time out), you wouldn't need that flag.

If this proposal was accepted, you could run all the tests from the top level with go test -timeout 20m ./... but then when running tests in subdirectories with just go test ./..., the cache wouldn't apply and all those tests would need to be run again. Speaking for myself, I think I'd find that quite frustrating.

In #34707 (comment), I even suggested a situation in which a test might want to explicitly vary its behavior based on the timeout: a test using randomized inputs and running with a short timeout might want to check only a few cursory inputs, while a very long (or even unlimited!) timeout might imply a much more exhaustive search.

That doesn't seem like a good idea to me. I'd suggest that it would be better to have an environment variable or a flag that controls that behaviour - I think it would be deeply unintuitive for a test to take longer when the timeout is increased.

For your regression tests, perhaps a better solution might be for a test to be able to explicitly mark itself as non-cacheable.

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Dec 17, 2019

Often there would be one or two packages that timed out and required a -test.timeout flag […]. But within other subdirectories (still slow but not slow enough to time out), you wouldn't need that flag.

It seems to me that GOFLAGS already addresses that use-case reasonably well: if you know that many of the tests you will be running require -timeout 20m, you can set GOFLAGS=-timeout=20m to apply that globally. If the behavior of the tests really does not depend on the timeout flag, then they will produce the same results either way.

(And you can always terminate the faster tests earlier by sending them SIGINT or SIGQUIT if you believe they're hung, or run them with an explicitly-shorter timeout while you're iterating on them.)

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Dec 17, 2019

For your regression tests, perhaps a better solution might be for a test to be able to explicitly mark itself as non-cacheable.

That is #23799. (But see specifically #23799 (comment).)

@bcmills

This comment has been minimized.

Copy link
Member Author

@bcmills bcmills commented Dec 17, 2019

Another problem with ignoring -timeout for tests is that it hides test failures.

If I know that my CI system runs with -timeout=5m, and I run go test -timeout=5m ./... and the tests all report as passing, then I will be very surprised when I commit the change and push it to CI and discover that it consistently times out.

That suggests, at the very least, a much more subtle caching policy: a passing test with a shorter timeout generally implies passing with a longer timeout too, and a passing test with a long timeout should continue pass with any timeout shorter than its observed running time, but a passing result for -timeout=10m should not imply a passing result for -timeout=10s, because the test would not necessarily pass if run in that configuration with a clean cache.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.