Calling t.Fatal inside a testscript command results in a panic

[datacharmer]

When t.Fatal (or a derived quickest or testify assertion) is called on a failing comparison within a custom command in testscriptit panics. By contrast, when using testscript.Fatalf, the test exits regularly.

How to reproduce:

// go.mod
module testscript-explore

go 1.18

require (
	github.com/rogpeppe/go-internal v1.8.2-0.20220706194532-9d15b660d1d6
)

// t-integration_test.go
package main

import (
	"testing"

	"github.com/rogpeppe/go-internal/testscript"
)

func TestTIntegration(t *testing.T) {
	testscript.Run(t, testscript.Params{
		Dir: "testdata",
		Cmds: map[string]func(ts *testscript.TestScript, neg bool, args []string){
			"wants_two_args": func(ts *testscript.TestScript, neg bool, args []string) {
				if len(args) < 2 {
					t.Fatal("needs two arguments")
				}
				if args[0] != args[1] {
					t.Fatalf("want: %s - got %s", args[0], args[1])
				}
			},
		},
	})
}

# testdata/t-integration.txt
env other=one

# this one passes
wants_two_args one $other

# this one fails
wants_two_args

sample run

$ go test
--- FAIL: TestTIntegration (0.00s)
    t-bug_test.go:15: needs two arguments
    --- FAIL: TestTIntegration/qt-integration (0.00s)
        testscript.go:428:
            # this one passes (0.000s)
            # this one fails (0.000s)
            > wants_two_args

        testing.go:1336: test executed panic(nil) or runtime.Goexit: subtest may have called FailNow on a parent test
FAIL
exit status 1
FAIL	testscript-explore/t-bug	0.605s

Using ts.Fatalf works as expected

go test
--- FAIL: TestTIntegration (0.00s)
    --- FAIL: TestTIntegration/t-integration (0.00s)
        testscript.go:428:
            # this one passes (0.000s)
            # this one fails (0.000s)
            > wants_two_args
            FAIL: testdata/t-integration.txt:7: needs two arguments

FAIL
exit status 1
FAIL	testscript-explore/t-bug	0.567s

I don't know if this is a bug or an incorrect usage from my side.
I'd like it to know how to address it, as I plan to use quickest or a similar testing library in a larger project where this usage would be common (See this blog post for more detail.)

[bitfield]

Another repro:

exec go mod init blowup
exec go get github.com/rogpeppe/go-internal/testscript@v1.8.2-0.20220728141844-24313847d41c
! exec go test
! stdout 'subtest may have called FailNow on a parent test'

-- testdata/script/blowup.txtar --
blowup

-- main_test.go --
package main_test

import (
	"testing"

	"github.com/rogpeppe/go-internal/testscript"
)

func TestBlowup(t *testing.T) {
	testscript.Run(t, testscript.Params{
		Dir: "testdata/script",
		Cmds: map[string]func(ts *testscript.TestScript, neg bool, args []string){
			"blowup": func(ts *testscript.TestScript, neg bool, args []string) {
				t.Fatal("oh no")
			},
		},
	})
}

[myitcv]

I think you're looking for https://pkg.go.dev/github.com/rogpeppe/go-internal@v1.8.1/testscript#TestScript.Fatalf.

[myitcv]

@bitfield this fixes your example:

exec go mod init blowup
exec go get github.com/rogpeppe/go-internal/testscript@v1.8.2-0.20220728141844-24313847d41c
! exec go test
! stdout 'subtest may have called FailNow on a parent test'

-- testdata/script/blowup.txtar --
blowup

-- main_test.go --
package main_test

import (
	"testing"

	"github.com/rogpeppe/go-internal/testscript"
)

func TestBlowup(t *testing.T) {
	testscript.Run(t, testscript.Params{
		Dir: "testdata/script",
		Cmds: map[string]func(ts *testscript.TestScript, neg bool, args []string){
			"blowup": func(ts *testscript.TestScript, neg bool, args []string) {
				ts.Fatalf("oh no")
			},
		},
	})
}

[datacharmer]

@myitcv
As stated in the issue above, testscript.Fatalf works as expected.
What doesn't not work is the insertion of a *testing.T inside a command. Now, if the issue were only Fatalf, it would be solved already. The problem is that I want to use quicktest or testify within a custom command. When I do that, the testing library ultimately calls t.Fatalf and the panic occurs.

[datacharmer]

I initially filed the issue against quicktest, but then I realised that the problem is within testscript.

Here you can see the failure using quicktest: frankban/quicktest#143

[mvdan]

Reiterating what I said on Slack, to not lose it: the problem is that the example uses the testing.T of the parent test. testscript runs every script as a parallel sub-test, so if you do need any testing.T at all, it would be the one for the sub-test - which testscript currently does not give access to:

go-internal/testscript/testscript.go

Lines 247 to 250 in 2431384

	t.Run(name, func(t T) {
	t.Parallel()
	ts := &TestScript{
	t: t,

[myitcv]

@datacharmer - apologies, I skim read your initial description far too quickly.

Ah I see. I think this probably falls outside the scope of testscript commands. The commands you are supplying via Cmds are effectively mini-main programs. Closing over the *testing.T is not something you should therefore do. Hence using quicktest is also not supported in this way.

[myitcv]

Glad that @mvdan is commenting here too - I'll defer to him and @rogpeppe. They are both closer to the API.

[datacharmer]

Thanks @myitcv , @mvdan for your answers.
I understand that under current circumstances using a testing library that uses *testing.T within a custom command is not supported.

What about future improvements?

• Would you consider exporting the variable with a getter (ts.T())?
• Or, if someone were to propose a PR with such a getter, do you know of a reason for not doing it?

[mvdan]

One reason not to do it that comes to mind is that the UX will be worse; note that the current Fatalf method does more than simply forwarding the call to testing.T.

go-internal/testscript/testscript.go

Lines 785 to 788 in 2431384

	func (ts *TestScript) Fatalf(format string, args ...interface{}) {
	fmt.Fprintf(&ts.log, "FAIL: %s:%d: %s\n", ts.file, ts.lineno, fmt.Sprintf(format, args...))
	ts.t.FailNow()
	}

The same applies to Logf.

I'm still not sure whether or not we should do this to be honest. My only general feeling is that I've been okay without using any testing libraries or frameworks when writing testscript commands. After all, they are little main-like programs, they aren't really meant to be tests themselves.

[datacharmer]

Fair enough.
I have a workaround in my project that does not involve modifying testscript code, i.e. writing a few quick assertion functions that use the Testscript object to fail. With this, I can use the library to my satisfaction.
I think, however, that the documentation should mention to avoid using t inside a custom command.

[mvdan]

Yeah, I think mentioning this in the docs is worthwhile. An alternative, to avoid the footgun entirely, would be to recommend shadowing the parent testing.T parameter with the TestScript parameter below:

func TestTIntegration(t *testing.T) {
	testscript.Run(t, testscript.Params{
		Dir: "testdata",
		Cmds: map[string]func(t *testscript.TestScript, neg bool, args []string){
			"wants_two_args": func(t *testscript.TestScript, neg bool, args []string) {
				if len(args) < 2 {
					t.Fatal("needs two arguments") // NOTE: this would then need to be Fatalf
				}
				if args[0] != args[1] {
					t.Fatalf("want: %s - got %s", args[0], args[1])
				}
			},
		},
	})
}

[bitfield]

The commands you are supplying via Cmds are effectively mini-main programs. Closing over the *testing.T is not something you should therefore do.

This makes sense, but I can see a danger of some confusion, because custom commands in testscript (as opposed to custom programs) can be assertions, just like cmp or stdout. They need the ability to fail the test.

Now we know that the right way to do that is to call ts.Fatalf, but users don't know that. They're used to calling methods on the t, and that's a natural thing to do if the t is in scope. Also, as @datacharmer points out, it's reasonable to expect that you can use the t to construct some outboard test runner like a quicktest.C, or to pass it to testify's assert.Equal, for example.

As ever, when you're the one in this situation making a forgivable mistake, being told that you're holding it wrong is somehow unsatisfactory. It's a good idea to mention in the docs that this won't work, as you say, but even that isn't really enough, because (sad to relate) people don't read documentation until they've already had a problem.

We can't avoid the t being in scope, or closures being created over it, but we could show an example of a custom command that fails the test with ts.Fatalf (I'm happy to have a go at this: at the moment the docs have very little to say about custom commands in general).

Beyond that, we probably need an executive decision: going forward, will testscript support using non-stdlib test frameworks in custom commands, or not? If yes, we should figure out a way to make something like a ts.T() method work as expected. If no, we can state that clearly up front.