proposal: cmd/go: add flag go test -no-cache

[ghost]

#24573 is closed, but it has all the information. -count=1 to most people does not mean -no-cache. It's just not intuitive.

This is a request to implement -no-cache flag on go test so that this kind of behavior isn't hidden from users.

Alternatively (backwards incompatible), since caching of test results is a bit surprising, instead allow uses to decide to cache or not explicitly with -cache flag, and set the default to not cache.

[ianlancetaylor]

CC @bcmills @jayconrod

[ghostsquad]

for clarity, count=1 has the side effect of disabling caching. But the documentation specifies:

    -count n
        Run each test and benchmark n times (default 1).
        If -cpu is set, run n times for each GOMAXPROCS value.
        Examples are always run once.

so this is not a request to have -no-cache be equivalent to -count=1, but rather can be combined with -count flag if desired.

Not sure if the title should be updated or not.

[ianlancetaylor]

A minor point: we don't currently have any -no-XXX flags, so perhaps -cache=false would be a better fit with existing options.

[bcmills]

I do not think we should switch test caching from opt-out to opt-in. The vast majority of tests should be hermetic and cacheable.

It might make sense to have a -cache flag, but in that case -cache=true should force caching on even if other flags would ordinary render the result uncacheable.

Even so, I'm not sure that the added clarity of an explicit flag would be worth the added cost of having yet another flag for users to consider.

CC @jayconrod @matloob

[myitcv]

Just noting the documentation on this particular point, from go help testflag:

When 'go test' runs in package list mode, 'go test' caches successful
package test results to avoid unnecessary repeated running of tests. To
disable test caching, use any test flag or argument other than the
cacheable flags. The idiomatic way to disable test caching explicitly
is to use -count=1.

[ghostsquad]

If it's decided that caching by default is actually not desirable (what I prefer), that would be great. The community can go about their business and be confident that every time they run go test with any sort of flags, it is actually testing things.

If it's decided that caching by default is a good idea, then it would additionally be nice if how go decided to cache was fixed up a bit more

as an example:

go test ./dir/... uses a cache but go test ./dir/... - bypasses the cache, but does not invalidate the cache. So running (assuming nothing has been previously cached)

# cached (if available)
go test ./dir/...
# not cached
go test ./dir/... -
# cached
go test ./dir/...

[ghostsquad]

Linking this issue to other things I've found in the community regarding cleaning cache and dealing with it

https://stackoverflow.com/questions/48882691/force-retesting-or-disable-test-caching/48882892

[ghostsquad]

@bcmills

The vast majority of tests should be hermetic and cacheable.

Do you have data to support this statement?

[myitcv]

The community

We are all in the Go community 😄

Also to note that test caching has been present since Go 1.10 (https://golang.org/doc/go1.10)

[ghostsquad]

It might make sense to have a -cache flag, but in that case -cache=true should force caching on even if other flags would ordinary render the result uncacheable.

I support giving users the ability to force behavior that the author didn't originally intend, as long as they understand the particular dangers of doing so.

I think caching of test results by default is more dangerous than anything else, simply because it's impossible to know what each test is actually doing, so it would make sense to let the author tell the tooling, not the other way around.

Even when following best practices such as using "golden files" which are read from disk (for doing such things as HTTP responses or whatever), it's not clear how Go would detect if the file being read has changed or not.

It might additionally be useful to add to the testing package an option that forces cache to be disabled, so that you can mix/match within a single test run. Any test that is potentially flaky, or accesses it's "environment" in some way, being disk, network, or even time itself would fall into this category.

[ghostsquad]

Also to note that test caching has been present since Go 1.10 (golang.org/doc/go1.10)

I'm not sure how the date/period that test caching was introduced is relevant to the conversation. Can you explain?

[bcmills]

it would make sense to let the author tell the tooling, not the other way around.

That is #23799.

[myitcv]

I'm not sure how the date/period that test caching was introduced is relevant to the conversation. Can you explain?

Test caching has been on by default since Feb 2018. Many of the points you are raising here have been previously discussed in other issues. But, like @bcmills, as someone who has followed the issue tracker relatively closely since that date, the vast majority of people (judging by the relatively small number of issues raised in the tracker) have not had issues with test caching, indeed users benefit significantly from it.

That's not to say there aren't cases that test caching does not cover well, and #23799 touches on some of them.

[ghostsquad]

@myitcv ultimately, at least in scope of this issue, I really just care about intuitiveness of the tooling/flags/options. So I'm 100% ok to keep this focused on just the -count=1 flag is when it comes to how to relates to caching.

[ghostsquad]

Now that I think more on this. Some tests (if cached), would run 0 times. So this flag forces tests to run N times. So in some ways, this flag makes perfect sense. But it seems it's just not intuitive enough.

[ianlancetaylor]

Even when following best practices such as using "golden files" which are read from disk (for doing such things as HTTP responses or whatever), it's not clear how Go would detect if the file being read has changed or not.

The test caching logs all files opened by the test and records their size and modification time. If either is different when the test is run again, the cache is invalid, and the test is re-run.

[ghostsquad]

@ianlancetaylor ah, so the biggest concern then is tests that make network requests?

[ianlancetaylor]

It seems to me that the concern in this issue is that -count=1 is too hard to discover.

On a related topic, see #23799.

[mvdan]

It seems to me that the concern in this issue is that -count=1 is too hard to discover.

I get the same impression. Perhaps we could point to its effect on the cache more prominently, though it's already in go help test.

[ghostsquad]

@seankhliao you thumbs-down this request. What's your opinion on this?

[seankhliao]

I disagree with the addition of a new flag for this, but I do agree there may be a discoverability issue

Perhaps one of the example output lines in go help testflag can be changed to (cached) with a note somewhere close by and/or include a copy-pasteable example in go help testflag

[rsc]

We really try hard not to have "there's more than one way to do it" in Go.
This is clearly documented.
We also don't have any -no-foo flags, as already pointed out.
It seems fine if people want to update docs somewhere, but let's not add more than one way to do this.

[ghostsquad]

The point is that count represents the number of times to run something. I think most people think/believe that the default of this is already 1. Why would it be any other value? Thus, even when you read the documentation on how to disable cache, and you see -count=1 it's confusing. A user might be thinking "well, wait, if that's not the default, how many times are my tests running?" which of course is in the grey area of somewhere between 0 & 1 depending on some magic/invisible criteria of whether or not the test is cacheable.

-count=1 has the side effect based on the semantics of what it means of disabling cache. I'm assuming as does every other value provided to -count, but that is also not explained.

From this perspective, -count=1 does not immediately mean "disable caching" to users, unless of course they have some deeper understanding that your test may or may not actually run without explicitly telling go test to run it.

Like the (cached) test output, it might be useful to just simple say something like this at the bottom if any tests were cached:

Some of your tests were not rerun, but are being reported from cache
To disable caching (per run), use -count=N where N>0
Or clear your test cache with go clean -testcache

[kortschak]

A perfectly reasonable way to interpret -count=n that would intuitively mean what @ghostsquad wants it "Run the tests n times." This means that it cannot be using previously cached results. Admittedly this is a retrospective justification, but it fits with a reasonable intuition.

[mvdan]

I agree with what @kortschak says, and the docs already say Run each test and benchmark n times, but they also say (default 1) which is counter-intuitive since adding -count=1 changes the behavior.

[kortschak]

To clarify the documentation something to the effect that without the -count switch tests are run zero or one time, depending on cached successes. Obviously that text is awful, but a nicer version should suffice.

[ghostsquad]

From a UI/UX perspective, there's no valid value for -count that performs the "run if not cached" behavior. The only way to achieve that is to exclude this option entirely. When options represent "boolean" behavior, it makes sense to support including the option, excluding it, or explicitly setting it via --option=false. Unfortunately, -count does not follow this pattern, which may also be the reason why it is confusing to users.

[bcmills]

@ghostsquad, explicitly passing --count= or --count '' (that is, an argument that is the empty string) should produce the default behavior. (If it does not, which is quite possible, please file a separate issue.)

[ghostsquad]

@bcmills if that's true, this feels definitely like a hack, and doesn't support the user experience issue. A user would likely not know that an empty string is a valid value for an option that very much sounds like it should be an integer.

[rsc]

@bcmills, I don't believe -count= (empty string) should work, fwiw. If it doesn't today, we probably shouldn't make it work. The argument is an integer, not a string.

The overall sentiment above though seems to be to leave things as they are, perhaps with better docs, rather than add a second flag with the same meaning. Do I have that right?

[ghostsquad]

Better docs, and even add output to the test command would be tremendously helpful.

[rsc]

Based on the discussion above, this seems like a likely accept.

[rsc]

Based on the discussion above, this seems like a likely decline.
(Sorry, mistyped earlier.)

[ghostsquad]

@rsc should I file a separate issue to update the documentation and output of go test in order to help discoverability of the -count flag? What about the issue that there's no reasonable valid value for -count that keeps the caching behavior?

[ianlancetaylor]

@ghostsquad Yes, I think this issue is long enough that a separate issue would be appropriate. Thanks.

Please suggest specific changes if you can.

Personally I don't see the fact that there is no value for -count that sets the default caching behavior as a serious problem. I don't see much reason to think that that is a source of confusion for users.

[jimmyfrasche]

Define -count=0 as using the cached results, updating the cache as necessary, and define count > 0 as running the test exactly that many times even if cached.

[rsc]

No change in consensus, so declined.