Open breml opened 1 week ago
Similar Issues
(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)
re, err := regexp.Compile(tt.regexp)
tt.assertRegexpCompileError(t, err)
if err != nil {
return
}
@earthboundkid I mentioned return
in the description as a potential alternative already.
But as described, this always adds additional conditions, which increases the number of possible code paths (cyclomatic complexity) and with this, it basically raises the question "who tests the test code". It makes the test code needlessly harder to reason about (of course not a single if err != nil
, real world cases I observed can be way more involved). Therefore I think it is advisable to keep the cyclomatic complexity of the test code it self to 1.
I think tt.assertRegexpCompileError(t, err)
needlessly increases the complexity of the test code to begin with. TBH, I had to read the code a bunch to understand it. The solution I would have done is tests := []struct { shouldFail bool
and then
re, err := regexp.Compile(tt.regexp)
if tt.shouldFail {
require.Error(t, err)
return
}
require.NoError(t, err)
match := re.MatchString("some string")
require.True(t, match)
Using a shouldFail boolean is much simpler and easier to read than having a complicated helper closure. There are times when using a closure can clean up some gnarly testing code, but checking if err should be nil or non-nil isn't one of them.
The idea that
re, err := regexp.Compile(tt.regexp)
tt.assertRegexpCompileError(t, err)
match := re.MatchString("some string")
require.True(t, match)
might stop the test with a PASS result during assertRegexpCompileError is surprising at the least. You always have to think maybe code won't continue executing due to a panic or other kind of failure, but a success?, that's something special.
The idea that an explicit return is worse because it "increases the number of possible code paths (cyclomatic complexity)" seems backward to me. The code path exists either way. The return makes it visible. That's better.
We're very unlikely to encourage this kind of hidden successful control flow by adding t.PassNow.
If you really insisted, you could always define something like
package passnow
var errPassNow = errors.New("pass now!")
func AllowPassNow() {
if e := recover(); e != nil && e != ErrPassNow {
panic(e)
}
}
func PassNow() {
panic(errPassNow)
}
and then write the tests like
t.Run(tt.name, func(t *testing.T) {
defer passnow.AllowPassNow()
re, err := regexp.Compile(tt.regexp)
tt.assertRegexpCompileError(t, err)
match := re.MatchString("some string")
require.True(t, match)
})
where calling passnow.PassNow() will panic out to the recover in AllowPassNow.
Then at least each test depending on this hidden control flow is explicitly opting in.
@rsc thanks for your proposed passnow
package. I will consider this idea. I guess it boils down to unsafe
versus panic
.
I understand, that there is very little to no interest to explore this idea so I will save the time to try to counter the arguments.
It is sad, but I guess, I have to accept this.
Proposal Details
Motivation
With popular testing frameworks like
github.com/stretchr/testify/require
there is a pattern to add assertion functions to the test cases in table driven tests. This pattern allows to pass e.g.require.Error
orrequire.NoError
functions to the test case and then use the respective function to assert the result of the function under test. This is a good way to keep the test code free of conditions and to keep the test code clean and easy to read.Sometimes, the function under test does need some setup (for example in integration tests), which might return an error. These errors are provoked and therefore expected, but after this point, it is not save to continue, since the other returned value(s) are not properly initialized (e.g. set to
nil
). In such a case, it would be nice to end the test early and mark it as successful.Example (
regexp
is just used for the sake of the example):Foodnote: while the example explains the use in combination with the test framework
github.com/stretchr/testify/require
, I am convinced, that the added functionality is also useful on its own without the use of any external test framework.Proposal
Add a new method to the
*testing.T
type calledPassNow
with the following signature:The implementation of
PassNow
would be the same as the implementation ofSkipNow
with the sole difference, that the internalc.skipped
field is not not changed totrue
.For the additional methods
Skip
andSkipf
no counterpart is proposed. If additional logging is required, the already existingLog
andLogf
methods can be used.Alternatives
SkipNow
: Thetesting
package provides the(*testing.T).SkipNow
function, which allows you to skip the current test case. But since the test case is then marked as skipped, this solution is not optimal and misleading, e.g. it might break reporting, since the test case would be counted as skipped where pass would be the correct thing.return
: The test function can be ended early withreturn
, but this always requires an additional condition to check if the test should be ended early. One of the main advantages of using a testing framework likegithub.com/stretchr/testify/require
is to avoid conditions in test functions and with this keeping the potential code paths in the test code to a minimum, in the optimal case to 1.Unlocked possibilities
With the
PassNow
method, existing testing frameworks could be extended with functions likeErrorThenPass(t TestingT, err error, msgAndArgs ...interface{})
. If the assertion fails, the function would behave likeError(t TestingT, err error, msgAndArgs ...interface{})
. If the assertion passes, the test would be marked as successful and the test would end immediately.This would allow to replace:
simply with
require.ErrorThenPass(t, err)
. Respectively, the assertion function in the test table would berequire.ErrorThenPass
.Related Work
Package github.com/breml/pass provides a proof of concept implementation of the proposed functionality using
reflect
andunsafe
packages.