testscript: expose current subtest in Env

[frankban]

This way, the setup function could fail in case setting up the test is currently not possible.
Moreover, client code could type assert the returned testscript.T at its own risk, for instance for registering cleanup functions or using test helpers like

tb := env.T().(testing.TB)
c := qt.New(tb)
env.Defer(c.Done)

[myitcv]

Is there a particular reason to expose this rather than closing over the *testing.T?

[frankban]

@myitcv could you expand a bit? Do you mean using the *testing.T being passed to testscript.Run in the setup func? A failure in setting up would make the top level test fail at that point, not the subtest. Also, cleanup functions would be executed only at the end of all tests, not as soon as each subtest completes.

[myitcv]

Sorry, you're quite right, I wasn't thinking about the fact these run as parallel subtests.

I would be supportive of this for exactly the reason you state: cleanup. Because right now (and pre the arrival of https://pkg.go.dev/testing?tab=doc#T.Cleanup) I'm having to do this myself.

[myitcv]

cc @rogpeppe

[rogpeppe]

I'm not sure this is quite right. I think the idea is that you can do:

    t := env.T().(*testing.T)

but that won't work because T isn't implemented by *testing.T (there's an extra Verbose method and the signature of the Run method is different). The doc comment on the T type is misleading.

I can see a couple of possible ways around this. We could change the signature to this:

// T returns the t argument passed to the current test by the T.Run method.
// This enables a Setup method to acquire the current test value.
// If Cleanup is called on the returned value, the function will run
// after any functions passed to Env.Defer.
func (e *Env) T() interface{}

Alternatively, we could factor out the methods in common between T and *testing.T:

// T holds the methods of the *testing.T type that
// are used by testscript along with methods that are
// different because of the requirements of RunT.
type T interface {
	TCommon
	Run(string, func(T))
	// Verbose is usually implemented by the testing package
	// directly rather than on the *testing.T type.
	Verbose() bool
}

// TCommon holds the methods held in common between
// T and *testing.T.
type TCommon interface {
	Skip(...interface{})
	Fatal(...interface{})
	Parallel()
	Log(...interface{})
	FailNow()
}

// T returns the test's current T value. This will be either an instance of T
// or an instance of *testing.T depending on whether Run or RunT was
// used to run the tests.
func (e *Env) T() TCommon

Another possibility that springs to mind is that we could expose the tshim type:

// TShim adapts a *testing.T to make it implement the T
// interface.
type TShim struct {
	*testing.T
}

// T returns the value of T for the current script test.
// If the tests were started by calling `Run`, the original
// *testing.T value is wrapped, but can be retrieved with:
//
//	env.T().(TShim).T
func (e *Env) T() T

What do you think?

[frankban]

Interesting, all of those would work. For my use case env.T().(testing.TB) would be good enough, hence this proposal.

If instead we want to allow access to the subtest's *testing.T, that's different, as you described. But then we exclude testscript.RunT, because if you had a *testing.T in the first place, then you would have used Run. RunT is (I suppose) intended to be used when you don't actually have a *testing.T, but your own type implementing testscript.T.
If that's true, then we could implement something that's very easy to use for the relevant use case, and impossible to use otherwise. Like

// T returns the current subtest, or an error if testcript.RunT was used to run the tests.
// If Cleanup is called on the returned value, 
// the function will run after any functions passed to Env.Defer.
func (e *Env) T() (*testing.T, error)

[rogpeppe]

For my use case env.T().(testing.TB) would be good enough, hence this proposal.

That's a reasonable point. Maybe there's not really a good use case for
making the actual concrete *testing.T value available.

Something like this, perhaps? (the same as your proposal
with an expanded doc comment)

// T returns the t argument passed to the current test by the T.Run method.
// Note that if the tests were started by calling Run,
// the returned value will implement testing.TB. Note that
// despite that, the underlying value will not be of type *testing.T
// because *testing.T does not implement T.
//
// If Cleanup is called on the returned value, the function will run
// after any functions passed to Env.Defer.
func (e *Env) T() T

If at some point, someone does come up with a specific use case that requires
*testing.T, we can always expose TShim or similar.

[frankban]

Cool, I made that change to the docstring.

[rogpeppe]

I think it would be worth checking that it actually implements testing.TB, as documented.

Suggested change

	if ts.Value("t").(T) != ts.t {
	if ts.Value("t").(testing.TB) != ts.t {

[frankban]

Done, in a slightly different way.

[rogpeppe]

LGTM with one final suggestion, thanks!