maxatome/go-testdeep

Difficult to call t.Parallel with td.T

abhinav opened this issue · 3 comments

testdeep encourages wrapping the *testing.T into a *td.T. This
unfortunately makes it difficult to call t.Parallel() on the
*testing.T once hidden behind the *td.T.

func TestFoo(tt *testing.T) {
    t := td.NewT(tt)
    
    t.Run("foo", func(t *td.T) {
        // t.Parallel() <<< errors because Parallel() does not exist
        //                  on the testing.TB interface.
        
        t.Cmp(...)
    })
    
    t.Run("bar", func(t *td.T) {
        // t.Parallel()
        
        t.Cmp(...)
    })
}

It's possible to call t.Parallel() on the *testing.T with some
upcasting on the wrapped testing.TB.

func Parallel(t *td.T) {
	tp, ok := t.TB.(interface{ Parallel() })
	if ok {
		tp.Parallel()
	}
}

If you're open to addressing this within testdeep, there are two
straightforward options:

  • Add a top-level func Parallel(*td.T) that does the same as above.
    Usage would then be,

    t.Run("foo", func(t *td.T) {
        td.Parallel(t)
        
        t.Cmp(...)
    })
  • Add a Parallel() method on td.T. Usage:

    t.Run("foo", func(t *td.T) {
        t.Parallel()
        
        t.Cmp(...)
    })

The latter is closest to what people already do with *testing.T so it
might be preferable.


Separately, either choice opens room for a setting in testdeep to
always call t.Parallel() for subtests invoked with a specific
*td.T.Run. For example,

func TestFoo(tt *testing.T) {
    t := td.NewT(tt).AlwaysParallel(true)
    
    t.Run("foo", func(t *td.T) {
        // already parallel
        t.Cmp(...)
    })
    
    t.Run("bar", func(t *td.T) {
        // already parallel
        t.Cmp(...)
    })
}

But that can be a separate discussion.

Hi, good proposals. Let me review your PR and think about it after some rest and I come back to you. Thanks!

#150 just reviewed.

After some thought, I am not sure AlwaysParallel() method is a good idea. I understand the concern, but won't this setting be unusual for the readers? As usually each subtest has to be explicitly set as parallelizable, it could be non-obvious to understand why a test runs in parallel while no Parallel() call is around.

#150 just reviewed.

After some thought, I am not sure AlwaysParallel() method is a good idea. I understand the concern, but won't this setting be unusual for the readers? As usually each subtest has to be explicitly set as parallelizable, it could be non-obvious to understand why a test runs in parallel while no Parallel() call is around.

Reasonable. That was just something that could be possible once Parallel was supported but obviously an optional feature. If you feel that the confusion it would cause is more than its value, we can omit it.