proposal: testing: add (*testing.T).PassNow method

[breml]

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 or require.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):

package main

import (
	"regexp"
	"testing"

	"github.com/stretchr/testify/require"
)

func TestSomething(t *testing.T) {
	tests := []struct {
		name                     string
		regexp                   string
		assertRegexpCompileError require.ErrorAssertionFunc
		wantMatch                bool
	}{
		{
			name:                     "valid regexp",
			regexp:                   `.*`,
			assertRegexpCompileError: require.NoError,
			wantMatch:                true,
		},
		{
			name:   "expected compile error",
			regexp: `.*[`,
			assertRegexpCompileError: func(tt require.TestingT, err error, i ...interface{}) {
				require.Error(t, err)
				// PassNow marks the test as successful and stops the execution
				// for this test case. Continuing the test would not work, since
				// re is nil.
				tt.PassNow()
			},
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			re, err := regexp.Compile(tt.regexp)
			tt.assertRegexpCompileError(t, err)

			match := re.MatchString("some string")
			require.True(t, match)
		})
	}
}

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 called PassNow with the following signature:

// PassNow marks the test as being successful and stops its execution
// by calling runtime.Goexit.
// If a test fails (see Error, Errorf, Fail) and is then ended
// using this function, it is still considered to have failed.
// Execution will continue at the next test or benchmark. See also FailNow.
// PassNow must be called from the goroutine running the test, not from
// other goroutines created during the test. Calling PassNow does not stop
// those other goroutines.
func (t *T) PassNow()

The implementation of PassNow would be the same as the implementation of SkipNow with the sole difference, that the internal c.skipped field is not not changed to true.

For the additional methods Skip and Skipf no counterpart is proposed. If additional logging is required, the already existing Log and Logf methods can be used.

Alternatives

1. SkipNow: The testing 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.
2. return: The test function can be ended early with return, 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 like github.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.
3. Separate test functions: The test cases could be split into separate test functions, but this can lead to code duplication and does make the test code potentially harder to maintain. This is in particular the case for integration tests, where setting up the test dependencies might be involved and the setup is the same for all test cases.

Unlocked possibilities

With the PassNow method, existing testing frameworks could be extended with functions like ErrorThenPass(t TestingT, err error, msgAndArgs ...interface{}). If the assertion fails, the function would behave like Error(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:

func(tt require.TestingT, err error, i ...interface{}) {
	require.Error(t, err)
	tt.PassNow()
}

simply with require.ErrorThenPass(t, err). Respectively, the assertion function in the test table would be require.ErrorThenPass.

Related Work

Package github.com/breml/pass provides a proof of concept implementation of the proposed functionality using reflect and unsafe packages.

[gabyhelp]

Similar Issues

• proposal: testing: verify failure (rather than skip) #28488
• proposal: testing: add conditional-error helpers to T and B #45650
• proposal: provide TestResult.Test function, analogous to BenchmarkResult.Benchmark #25889
• Proposal: testing.MustRun #22353
• proposal: testing: Collect test failure logs on the testing.common struct and expose collected logs via a new method on testing.TB #61860

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[earthboundkid]

re, err := regexp.Compile(tt.regexp)
tt.assertRegexpCompileError(t, err)
if err != nil {
	return
}

[breml]

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

[earthboundkid]

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.

[rsc]

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.

[breml]

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

[rsc]

This proposal has been declined as infeasible.
— rsc for the proposal review group