Repository navigation
proposal: testing: add Testing constant #60737
Description
Activity
There's this on tip https://pkg.go.dev/testing@master#Testing
dup of #52600 ?
Oh, I didn't notice, but unfortunately that API doesn't work for the performance optimizations I'm hoping to get: first, it's implemented as a link-time variable, so the compiler can't act based on it at all; second, as a function it requires inlining to happen before code elimination, and I haven't tested it but it's unlikely that will then inform the inliner weight of the caller.
The first part can be fixed but the second might be a permanent problem if we commit to a function in Go 1.21.
release blocker for new api?
cc @ianlancetaylorNote that the release-blocker label likely means little unless this is milestoned for 1.21. cc @golang/release
Reacted by Dmitri ShuralyovI'm not sure it's worth changing at the last minute, but it might be worth making an explicit decision, yeah.
To test I compiled this program
package main import "testing" func main() { f() } func f() { b := make([]byte, 32) if testing.Testing() { escape(b) println("elided") } println("yes") } var sink []byte func escape(b []byte) { sink = b }where
testing.Testingisfunc Testing() bool { return false }The results were mixed but mostly optimization-averse:
- the
escape()andprintln("elided")calls are not emitted - the
"elided"string is present in RODATA bescapes to the heap- the inliner prices the if body
While if we made it
const Testing = false:- neither the calls nor the
"elided"string are emitted make([]byte, 32) does not escapecan inline f with cost 8 as: func() { b := make([]byte, 32); if testing.Testing { }; println("yes") }
- the
Making
testing.Testinga compile-time constant seems infeasible. That would mean that forgo testwe would have to recompile everything that imports the "testing" package.Reacted by Daniel Martí and Bryan C. MillsI initially had the same reaction as @ianlancetaylor - today, if I test
package foovia external tests inpackage foo_test, runninggo buildfollowed bygo testmeans that I only buildfooand its dependencies once. This change could make some of my daily routine noticeably slower to start running the tests.Worth noting that many people write internal tests at
package foo, meaning thatgo buildfollowed bygo testdoes need to buildfootwice. That's partly why #29258 was filed. But at least one can avoid the double-compile by only using external tests.Another concern I would have is that you would be leaving your code much more exposed (at risk) for compiler bugs, since the code you're testing isn't the same as the code you're shipping (from a compiler IR perspective). Here's some contrived Go code:
package mumble func F(n int, sl []int) int { r := n for i := 0; i < n; i++ { if testing.Testing { DoSomeValidation(sl, i) } r = (sl[i] ^ r) + (r << i & 3) } return r }Let's imagine that there is a bug in the register allocator that has to do with values flowing around loops. If
testing.Testingis true, the IR for the loop looks completely different, might not trigger the bug. Withtesting.Testingfalse, however, loop no longer contains any calls (and has only a single basic block); this IR could be the one that triggers the bug. Running "go test" works fine, but in reality you wind up shipping buggy software as a result of the compiler bug.Reacted by cherrymuiThis #52600 (comment) also pointed out wanting to be able to do this. (CC @aclements.)
That would mean that for
go testwe would have to recompile everything that imports the "testing" package.What proportion of packages import "testing" in non-test code? Are there cases where doing so is unavoidable?
Is it viable for the
testing.Testingconstant to be defined to befalsefor packages other than the one being tested, andtruefor the one being tested (similarly to how _test.go files are used in the package being tested, but not its dependencies)? That would help with avoiding rebuilding, but requires a magical constant. If this isn't the case, it would mean dependencies of packages being tested could subtly change behavior during the test. (But perhaps that's already possible with #52600?) Edit: I see in #52600 (comment) that the package-local option was considered in #52600 too, and dismissed.In other languages, this is usually done with debug mode asserts, but really they are mostly exercised during tests.
It doesn't seem appropriate to tie that kind of checking to whether the program is being run as a test: if you can't easily enable it in a more realistic configuration, it isn't useful for debugging a realistic program, and that's exactly where the edge-cases that trigger a bug are likely to arise.
The sort of use-case you describe seems like a better fit for build constraints. For example, we sometimes put additional checks that slow down the program or cause otherwise-unnecessary escapes behind
//go:build race, since the race detector already causes similar effects.Reacted by cherrymuiI don't think this should be a release-blocker. I think the only open question is whether we should put in the effort to make the
testing.Testing()function added in 1.21 zero-cost in a later release. We certainly could, at a possible cost to build time (though I'm not convinced a priori that's an issue). But that's certainly not a release blocking question. The issues in making the function zero-cost are almost exactly the same as the issues in using a constant for this, so switching to a constant doesn't help.I think we should close this as a dup of #52600 and open a new issue specific to making
testing.Testing()zero-cost. Any objections?I filed #60772 for the proposal and will close this as a duplicate of some combination of #52600 and #60772.
It is worth noting, however, that this use is problematic:
In cryptographic packages, I would like to perform extra consistency checks that are unsuitable for production either for performance reasons (they might be slow or cause inliner or escape analysis issues) or because they are not safe (they might be non-constant time or expose undesirable attack surface). In other languages, this is usually done with debug mode asserts, but really they are mostly exercised during tests.
If these extra checks cause performance problems, then they will affect not just tests of the crypto packages themselves but also any tests importing crypto to do their own work. If I have A imports B imports C imports D imports E imports crypto and I am testing A, it's not at all obvious why crypto should slow down A's tests. The performance problems are even worse when you consider benchmarking A. Everyone will observe that wow crypto is super slow, and they'll have benchmarks to prove it.
- locked and limited conversation to collaborators
on Jun 12, 2024
In cryptographic packages, I would like to perform extra consistency checks that are unsuitable for production either for performance reasons (they might be slow or cause inliner or escape analysis issues) or because they are not safe (they might be non-constant time or expose undesirable attack surface). In other languages, this is usually done with debug mode asserts, but really they are mostly exercised during tests.
I propose we add a constant to
testingthat is alwaystrueif building withgo testandfalseotherwise. I couldn't come up with a better name thantesting.Testing.Then, I could gate all my testing assertions with
if testing.Testingand (depending on the order of optimization passes), count on dead code removal to make the effects of that code disappear in production builds. This feels like a technique that would make sense beyond cryptography, too, although for other packages it might be less important for it to be a constant, so it can be emulated withinit()in a_test.gofile setting a variable.