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: add flag go test -no-cache #39056

Closed
ghost opened this issue May 13, 2020 · 37 comments
Closed

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

ghost opened this issue May 13, 2020 · 37 comments

Comments

@ghost
Copy link

ghost commented May 13, 2020

#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.

@ALTree ALTree changed the title feature request: add flag go test -no-cache proposal: add flag go test -no-cache (equivalent to -count=1) May 13, 2020
@gopherbot gopherbot added this to the Proposal milestone May 13, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: add flag go test -no-cache (equivalent to -count=1) proposal: cmd/go: add flag go test -no-cache (equivalent to -count=1) May 13, 2020
@ianlancetaylor
Copy link
Contributor

CC @bcmills @jayconrod

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 13, 2020
@ghostsquad
Copy link

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 ianlancetaylor changed the title proposal: cmd/go: add flag go test -no-cache (equivalent to -count=1) proposal: cmd/go: add flag go test -no-cache May 13, 2020
@ianlancetaylor
Copy link
Contributor

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

bcmills commented May 13, 2020

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

myitcv commented May 13, 2020

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

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

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

@bcmills

The vast majority of tests should be hermetic and cacheable.

Do you have data to support this statement?

@myitcv
Copy link
Member

myitcv commented May 13, 2020

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

ghostsquad commented May 13, 2020

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

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

bcmills commented May 13, 2020

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

That is #23799.

@myitcv
Copy link
Member

myitcv commented May 13, 2020

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

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

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

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

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

@ianlancetaylor
Copy link
Contributor

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

mvdan commented May 15, 2020

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

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

@seankhliao
Copy link
Member

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

rsc commented May 20, 2020

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.

@rsc rsc moved this from Incoming to Active in Proposals (old) May 20, 2020
@ghostsquad
Copy link

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

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

mvdan commented May 20, 2020

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

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

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

bcmills commented May 21, 2020

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

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

rsc commented May 27, 2020

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

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

@rsc
Copy link
Contributor

rsc commented Jun 3, 2020

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

@rsc
Copy link
Contributor

rsc commented Jun 3, 2020

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

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Jun 3, 2020
@ghostsquad
Copy link

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

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

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

rsc commented Jun 10, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Jun 10, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Jun 10, 2020
@golang golang locked and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.