proposal: testing: less syntactic overhead to synctest tests

[rogpeppe]

Proposal Details

When writing tests using testing/synctest, I find myself writing very many tests that look like this:

func TestSomething(t *testing.T) {
	synctest.Test(t, func(t *testing.T) {
		...
	})
}

This means that essentially all of my test code has a double level of indentation,
and the synctest wrapper is annoying to write every time.

I wonder if it might be more convenient to have a way to mark
a given test as "synctest" and have the testing package be responsible
for doing the wrapping.

One possibility might be to use a doc-comment-style annotation:

//testing:synctest
func TestSomething(t *testing.T)

This would be picked up by the testing harness and become equivalent
to the earlier TestSomething function.

[seankhliao]

You can write it like this?

func TestSomething(t *testing.T) { synctest.Test(t, testSomething) }
func testSomething(t *testing.T) {
    ...
}

net/http does something similar https://go.googlesource.com/net/+/346cc6157ee53301dea14e57a45c22368ab46e55/http2/server_test.go#4943

I don't see why we'd accept a special case of decorators which we've generally rejected in the past #36669

[gabyhelp]

Related Issues

• testing/synctest: replace Run with Test #73567 (closed)
• proposal: testing/synctest: create bubbles with Start rather than Run #73062 (closed)
• proposal: net/http/httptest: synctest support #76608
• testing/synctest: experimental package for testing concurrent code #69687 (closed)

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

[neild]

I also dislike the double layer of indentation.

You can add an extra function as @seankhliao describes, but this is verbose and makes it easy to have cut-and-paste errors which result in a test accidentally not running:

func TestB(t *testing.T) { synctest.Test(t, testA) } // oops, TestB is running testA
func testB(t *testing.T) { ... }
}

I don't think a doc comment is the right approach; it's too much semantic change for a comment.

A different function signature could work, but I don't know what the new signature would be. Something like this, maybe?

package synctest

// synctest.T is a type alias to testing.T.
type T = testing.T

func TestSomething(t *synctest.T) {
  // This test runs in a synctest bubble.
}

That seems a bit magic; I don't know that I like making a distinction between different aliases for the same type.

It'd also be nice to handle the case where you want to chain T.Run and synctest.Test:

func Test(t *testing.T) {
  t.Run("subtest", func(t *testing.T) {
    synctest.Test(t, func(t *testing.T) {
      // this is getting very indented
    }
  }
}

Perhaps a synctest.Subtest (although I'm not really happy with the name)?

func Test(t *testing.T) {
  synctest.Subtest("subtest", func(t *testing.T) {
    // ...
  }
}

[rogpeppe]

I agree about your reservations about doc comments and different semantics for type aliases.
Another possibility might be to have a different naming convention for the test function itself, I guess,

func SyncTestSomething(t *testing.T) {

I considered that before and didn't much like it (the convention of having func Test* is strong)
but maybe better than those other alternatives.

It'd also be nice to handle the case where you want to chain T.Run and synctest.Test:

At least in this case it's easy to write a little helper oneself to avoid the overhead,
which is essentially your synctest.Subtest.

func runSyncTest(t *testing.T, name string, f func(t *testing.T)) {
    t.Run(name, func(t *testing.T) {
      synctest.Test(t, f) 
    })
}

Given it's so trivial to write, I'd probably not prioritize it.

[myaaaaaaaaa]

How about a catchall sandbox mode that enables other isolation features by default, not just synctest bubbles?

func SandboxTestSomething(t *testing.T)

We could then enable other, bubble-like isolation features by default. For example:

• Always run the garbage collector on test cleanup, allowing finalizers to run. This means runtime.Pinner can reliably panic on memory leaks.
• Change finalizers for files, network connections, etc to panic in a SandboxTest if they're not closed, helping to catch leaks in system resources.

[doggedOwl]

I like the proposal of rogpeppe to have the convention func SyncTestSomething(t *testing.T) {
and in a similar way we could also have a t.RunSyncTest() ?

[rogpeppe]

Ideally I'd prefer an API like:

func TestFoo(t *testing.T) {
    t.Sync()
    ...
}

but I'm not sure it's possible to make that work nicely. One hacky way might be to make it call runtime.Goexit and then re-run the function, I guess.

[apparentlymart]

While I can certainly agree that I found it a little annoying to have an extra level of nesting, I don't personally think this minor ergonomic quirk is significant enough to justify adding additional complexity to the overall test system.

The current style is not hugely different than what it looks like to start a new goroutine using a go statement, with the same option of either writing an inline closure or calling a separate named function. synctest.Run is effectively a special kind of go that also creates a bubble. It seems reasonable to me that the two have a similar code shape.

[neild]

I think we went through a version of the synctest proposal that placed the current goroutine into a bubble (as with t.Sync), but the clarity of having the bubble lifetime clearly defined by a single function call won out.