testing: add func Testing() bool

[misha-ridge]

This is a proposal to add a build tag during compilation of unit tests.

Background & motivation

Sometimes it is handy to have some code not available for unit testing, e.g. some volatile data that should not be relied upon in tests. Code reviews help to catch accidental or intentional misuses, but it's exhausting to check that no test code refers to this data directly or indirectly.

There are workarounds that allow distinguishing if the code is running tests, but they are inadequate:

• Checking presence of "test.v" flag does not work for code running from global initializers or package-level init() clauses.
• Opening currently running executable to match some string that is present only in internals of testing package may produce false positives, slow, and OS-dependent.
• Manually specifying build tags for testing (likely via shell wrappers) forces every developer of the project to learn that go test is not the right tool to run tests, and creates toil for integrating with IDEs.

Proposal

Add a build tag that is available during compilation of unit tests. This will trivially allow marking some data/functions as "not available in unit tests".

[misha-ridge]

flag.Lookup("test.v") idiom is seen in the wild: https://github.com/search?q=flag.Lookup%28%22test.v%22%29&type=code

[ianlancetaylor]

This may be a dup of #14668.

[misha-ridge]

It is. New information in this ticket:

• a new legitimate use-case
• survey of existing codebases that shows that inadequate workarounds do get applied

[rsc]

I'm trying to understand why you would want to create untestable code.
The justification on StackOverflow is "I want to use a different configuration file." during a test.
The comment above says "some volatile data that should not be relied upon in tests".
Are there more examples?

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[misha-ridge]

The simplified situation that caused me to open the ticket:

shared/config:
  prod_env.go
  dev_env.go
service1/
  main.go (selects one of environments and passes it down the call stack)
service2/
  main.go (ditto)
lib/a/
  something.go
  something_test.go
  env_test.go // contains test environment that must be used instead in tests

I'd like to ensure that something_test.go (and anything that it transitively depends on) does not reference shared/config, because this data changes over time, so changes in shared/config cause breakage of unrelated unit tests that accidentally depend on current state of environment data. Instead tests and system under test should use test environment.

It's a pretty non-fun situation when an urgent change in environment forces the person who needs to do it to chase all the implicit assumptions in tests that were violated.

[ChrisHines]

Another possible use case: Excluding code used for production resiliency that hinders local testing and debugging. The specific example I've seen is disabling the use of github.com/mitchellh/panicwrap when running tests.

[ianlancetaylor]

I just want to note that you could get the same effect by running go test -tags test. And you could enforce the use of the -tags option by adding a file tags_test.go.

//go:build !test
func init() { log.Fatal("running a test without -tags test") }

So it doesn't seem that hard for people to get this functionality already. The question is whether this is a common enough need to add to the standard tools. Note in particular that actually using the build tag will tend to make the build cache less effective, so it's not something we necessarily want to encourage.

[misha-ridge]

I have mentioned it in the bug description:

Manually specifying build tags for testing (likely via shell wrappers) forces every developer of the project to learn that go test is not the right tool to run tests, and creates toil for integrating with IDEs.

[bcmills]

Excluding code used for production resiliency that hinders local testing and debugging.

For that kind of use case, why not make the debugging-hostile behavior opt-in instead of opt-out?
(For example: why not have func main enable it explicitly after parsing flags, so that you can also debug your production binary without recompiling it if the need arises?)

[ChrisHines]

Excluding code used for production resiliency that hinders local testing and debugging.

For that kind of use case, why not make the debugging-hostile behavior opt-out instead of opt-in? (For example: why not have func main enable it explicitly after parsing flags, so that you can also debug your production binary without recompiling it if the need arises?)

The situation I've seen is that the panicwrap logic is integrated with a centralized package used to setup standard behaviors all services in an organization should have. Part of that is configuring a standard logger that catches and logs all panics in a format the log aggregation system can accept. The shared library then has code like this to check if it's running in a test.

	// make sure that this isn't called in a test env
	if strings.HasSuffix(os.Args[0], ".test") {
		return
	}
	for _, arg := range os.Args[1:] {
		if strings.HasPrefix(arg, "-test.") {
			return
		}
	}

I'm not sure it's the best approach. Right now I am only trying to report on examples I've seen as additional data points.

[aclements]

One situation where I've wanted something like this is to support test hooks in the runtime package tests that can be fully optimized away when not building for testing. Such test hooks could be deeply embedded in the runtime code to let tests, for example, check that they covered some corner case. However, this is generally performance-sensitive code and code that is shipped with every Go binary, so ideally these hooks would leave no trace outside of a runtime test binary. There are similar patterns in other std packages, such as the test hook functions in the net package.

An alternative solution to my problem is that the compiler could detect when an unexported package global is never written to and constant-propagate the zero value of that global. It's not clear to me that that optimization is generally worthwhile, though.

[bcmills]

An alternative solution to my problem is that the compiler could detect when an unexported package global is never written to and constant-propagate the zero value of that global. It's not clear to me that that optimization is generally worthwhile, though.

I would expect that optimization to be generally worthwhile: I've seen that pattern in a lot of packages, both to inject test-only helpers and to reconfigure defaults (for example, shortening timeouts so that a test can actually provoke them).

Even better would be to also apply that optimization at link-time for exported package globals. 😁

[aclements]

@rsc and I chatted a bit about this and realized there are two somewhat different possible designs:

1. Package-local: Set a "test" build tag for just the package under test. This would allow the package to introduce internal test-only alternatives. It would have minimal impact on the build graph. And it would mean that the "_test.go" naming convention would more closely parallel the "_$GOOS_$GOARCH.go" naming convention. However, this would be the only build tag that's applied only to a single package, rather than globally.
2. Global: Set a "testing" build tag globally when building for go test. This would work like other global build tags. It would allow packages to export test-only code like mocks, though this would also introduce complexity into documentation tools and API checking tools (not more than other build tags, but this one would be very visible). It's possible to consider this option now because of the build cache; before the build cache, this would have required rebuilding most packages on every go test. However, it still would have a somewhat significant impact on the build graph, like any other build tag used in many packages. E.g., if we were to use this tag in std packages even just for unexported APIs, go test would need to rebuild a decent chunk of std, though the results of that would be cached after the first go test and the cache changes would probably peter out after a layer or two of the import graph.

I lean toward option 1, but I can see arguments either way.

I would expect that optimization to be generally worthwhile: I've seen that pattern in a lot of packages, both to inject test-only helpers and to reconfigure defaults (for example, shortening timeouts so that a test can actually provoke them).

I'd been thinking this optimization would only work for things that are zero-initialized and never written to, but I suppose it would also work with any other constant initializer. We just need to be able to prove that it cannot be read before it's "written to" and that it's never written to again. So it's less limited than I was thinking.

[misha-ridge]

Set a "test" build tag for just the package under test.

This will not solve the problem that caused me to open the ticket, described here: #52600 (comment) - files in shared/config will not be excluded from compilation if they contain //go:build !test.

This will definitely require a lot of explicit documentation about a surprising behaviour of this specific build flag.

[rsc]

Talked about this with @aclements and others at proposal review today. We generally agree that the package-local option is off the table. It's too different and not worth the hassle.

It may be that for runtime testing hooks, the best we can do is a 100% predictable branch if f != nil { f() }. That will not compile to nothing in production programs, but it also doesn't compile to much: just five or so instructions plus whatever argument preparation is needed (but won't ever execute).

For the more general "global" testing build tag, the effect would be to have to compile packages built while testing differently from packages built for production. That's not hard - we already do it for go test -race, for example. It would cache automatically thanks to the build cache, and there are ways to reduce the impact further. So efficiency is not a concern.

The big concern is the impact on APIs and tooling. Adding this build tag would mean that any API compatibility checks would need to treat test and non-test builds as completely separate configurations that might have completely different APIs, just as Windows and Linux builds are. It would mean being able to define exported API that is only available in a test binary (test-only code), and it would also mean being able to define exported API that is only available outside a test binary (untestable code). It is not at all clear to us that this is a feature that makes sense for the Go ecosystem. The possibility of defining test-only or non-test-only API introduces significant complexity that both tooling and developers will have to understand. That cost would need to be matched by a very large benefit, and I don't see it here.

A while back we arranged that programs can import "testing" without causing any side-effects like flag registration. Perhaps it would suffice to add a new function testing.Testing() bool, analogous to Short and Verbose, to ask whether the current binary is a test. Then the top comment's enumeration of other checks could be replaced by this simpler call.

[dottedmag]

It would mean being able to define exported API that is only available in a test binary (test-only code)

As an aside: by itself it might not be a bad idea: exposing hooks for testing only to unit tests might be useful.

Perhaps it would suffice to add a new function testing.Testing() bool, analogous to Short and Verbose, to ask whether the current binary is a test.

This is a great solution for my problem. With testing.Testing() I can easily guard non-unit-tests code with

// file/that/should/not/be/used/from/testing.go

func prodEnvironmentData() *Environment {
    if testing.Testing() {
        log.Fatal("Using production data in unit tests")
    }
    ....
}

[ChrisHines]

I agree with your concerns about the build tag approach, @rsc. I think a testing.Testing() bool function is a fine solution. It would certainly improve the use case I posted above.

[rsc]

Retitled. Sounds like people are in favor of func Testing() bool, and we can make it inline to a constant, along with similar functionality for the runtime internals.

Have all concerns about this proposal been addressed?

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[twmb]

This may be a silly question that has an obvious compiler answer, but why have func Testing() bool vs. a constant const Testing bool?

[ianlancetaylor]

It's not a silly question. A function is easier to implement. A constant would require either something magical, or it would require build tags which is what we are trying to avoid. With a function we can just have an internal variable that is set to true by the test's main function, which is generated by go test.

[ianlancetaylor]

The natural implementation of this is going to produce a testing.Testing function that returns false when called by an init function, and returns true when called by a test or in TestMain. @misha-ridge I note that detecting a test in an init function was one of the cases that you explicitly called out in the original report. Is it going to be acceptable for testing.Testing to return false in a test program before the tests start running?

[dottedmag]

@ianlancetaylor Alas no, then it's no better than the existing buggy workaround of checking presence of flag "test.v".

The obvious case where it will fail, to follow-up an example in #52600 (comment):

some/package/that/uses/env_test.go

var envForTests = config.ProdEnvironmentData()

[bcmills]

Perhaps it would suffice to add a new function testing.Testing() bool, analogous to Short and Verbose, to ask whether the current binary is a test.

The natural implementation of this is going to produce a testing.Testing function that returns false when called by an init function, and returns true when called by a test or in TestMain.

Those are two very different predicates: one answers “is the current binary a test?”, whereas the other answers “is the current binary running tests?”. That makes a big difference for programs like cmd/go in which the test binary re-execs itself as a command in order to test the command-line interface — a pattern that also happens to be the easiest way to get coverage data for command-line tests.

It's not obvious to me which of those definitions is more useful in practice, but we should absolutely be very explicit about which one it is that we're reporting.

---

FWIW, based on @rsc's description I had assumed that the flag would report the value of a variable set at link time, much like how we inject the data for debug.ReadBuildInfo today.

(In fact, I had sort of assumed that testing.Testing would call out to debug.ReadBuildInfo, but that would require fixing #33976 as well, and presumably wouldn't work in GOPATH mode if that matters. 😅)

[ChrisHines]

(In fact, I had sort of assumed that testing.Testing would call out to debug.ReadBuildInfo, but that would require fixing #33976 as well, and presumably wouldn't work in GOPATH mode if that matters. 😅)

I'm interested in getting #33976 fixed for other reasons, but also, the example I shared in #52600 (comment) is from a project that currently still builds in GOPATH mode, although that is hopefully only a short term condition.

For the case of tests re-executing their own binary it seems straightforward for the parent process to set an environment variable for the child process to know it's special. That's the common pattern I've seen for those types of tests anyway.

[gopherbot]

Change https://go.dev/cl/475496 mentions this issue: testing: add Testing function

[adamluzsi]

Hey there, I hope you don't mind me chiming in a bit late to the conversation! 😊
I was just wondering if it's a good idea to import the testing package into production code?

For instance, let's say you're developing a custom logger and want to use STDOUT as the default output.
However, during testing, you'd prefer to use io.Discard to keep the testing output clean.
In this scenario, the logging package would depend on the testing package,
and it's possible that the entire project could end up depending on the testing package as a result.

I was wondering if it might be more efficient to create a separate testing/testcheck package that doesn't have a dependency on testing. Instead, we could have the testing package set a flag in testing/testcheck to show when tests are being executed. This way, package designers working with STDOUT can easily adhere to testing library conventions and suppress output using io.Dispose, without any unintended consequences from importing the testing package.

I'd really love to hear your thoughts on this approach! 😊

[misha-ridge]

@adamluzsi You're right, there are side-effects of importing testing.

Any program that uses testing.Testing() will end up having the whole suite of testing flags added.

@ianlancetaylor Any chance we could address it before it ends up in a release?

[ianlancetaylor]

Simply importing the "testing" package will not define any flags. The flags are only defined if something calls testing.Init. The testing harness will do that, but it won't happen by itself.

I see that it's possible to introduce a dependency on the testing package via something like logging, but the testing package is not all that large. And the linker will discard unused functions, so if the program only calls testing.Testing there won't be much left.

In any case, if you do think we should do something about this, please open a new issue and mark it as a release blocker for 1.21. Thanks.

[dottedmag]

Right, the flags situation is fixed in https://go-review.googlesource.com/c/go/+/173722 (I was looking at old Go source code accidentally), so importing testing does not have any side-effects, and linker should take care of the rest.

Incidentally this also means that the brittle workarounds applied due to lack of Testing() function are also broken due to this change, which is a good thing, as it will force everyone to use testing.Testing().

@adamluzsi So this is fine?

[adamluzsi]

Yes, of course, and thank you so much for taking your time and helping me understand.
I wrongly thought that importing the testing package might have unintended side effects.
I'm sorry if I caused any panic at this late stage. 🤦

[ladydascalie]

Incidentally this also means that the brittle workarounds applied due to lack of Testing() function are also broken due to this change, which is a good thing, as it will force everyone to use testing.Testing().

How does the behaviour break @dottedmag?
Is it a panic, an error?

I think this is important to know before this break occurs.

[misha-ridge]

@ladydascalie Such code (checking pflag.Lookup("test.v") to detect if the code is running as a part of unit tests) always returns "not in unit tests" nowadays.

[mvdan]

Started using this function in a program that didn't previously import testing, and I notice the binary size increased by about 20KiB. While this is not huge, it is still surprising - is that expected? I would hope that, since the Testing func is simple, and the testing package shouldn't have init funcs keeping types or funcs alive, the size increase should be near zero.

[ianlancetaylor]

The testing package does import a lot of other packages.