testr: use an interface to make it work with *testing.B and *testing.F

[jeandeaual]

The testr logger currently only works with *testing.T and is not usable in benchmarks and fuzz tests.
By using an interface instead, it can be made to accept *testing.T, *testing.B and *testing.F.

This is similar to what's done in other libraries, for example:

• https://github.com/kubernetes/klog/blob/a585df903e8653af246bcb8291cc856af2df9cfd/ktesting/testinglogger.go#L47
• https://github.com/stretchr/testify/blob/6990a05d54c2287be8787a411d77815643347339/require/requirements.go#L4
• https://github.com/ent/ent/blob/762e6aeff9d92f4b2af45932274401674462297f/entc/gen/template/enttest.tmpl#L32

This changes the return type of GetUnderlying though, so I'm not sure if that's OK.

[pohly]

This makes sense to me. It's the same approach that I took in https://github.com/kubernetes/klog/blob/a585df903e8653af246bcb8291cc856af2df9cfd/ktesting/testinglogger.go#L41-L65

It is an API break not just because of the Underlier change, but also because the function signature of the New function changes. One option to avoid this would be to have yet another constructor that takes TestingT plus options and have a second Underlier which is implemented by the struct returned by that (and only that) constructor.

Most of the common code can still be shared by the two structs, but the devil will be in the details. 🤷

[pohly]

Logf is not needed. Perhaps drop it to make it easier to satisfy the interface with something other than testing.T/B/F?

[jeandeaual]

It is being used in the implementation, but you're right, it's not strictly required so I removed it.

[jeandeaual]

It is an API break not just because of the Underlier change, but also because the function signature of the New function changes. One option to avoid this would be to have yet another constructor that takes TestingT plus options and have a second Underlier which is implemented by the struct returned by that (and only that) constructor.

Most of the common code can still be shared by the two structs, but the devil will be in the details. 🤷

That makes sense.

As you suggested, I created a new struct and created a new constructor that takes TestingT and left the existing implementation as-is. This seems to be working fine and doesn't change the API.

I'm not sure about the naming though. Do you have any suggestion?

[pohly]

I think a single NewWithInterface may be okay. It matches the NewWithOptions pattern.

We only need a single variant of it. That we have New and NewWithOptions is because of historic reasons, we don't need to repeat that.

[jeandeaual]

I see, thanks for the explanation.
I renamed the variant with the Options parameter NewWithInterface and removed the other constructor.

[pohly]

You can squash all commits into one already during the review, GitHub's force-push links work for me.

[pohly]

Nit: this works, but doesn't look like idiomatic Go. How about this:

Suggested change

	if underlier, ok := log.GetSink().(UnderlierInterface); ok {
	if underlierT, ok := underlier.GetUnderlying().(*testing.T); ok {
	if t != underlierT {
	t.Error("invalid underlier")
	}
	} else {
	t.Error("couldn't get underlying *testing.T")
	}
	} else {
	t.Error("couldn't get underlier")
	}
	underlier, ok := log.GetSink().(UnderlierInterface)
	if !ok {
	t.Fatal("couldn't get underlier")
	}
	underlierT, ok := underlier.GetUnderlying().(*testing.T)
	if !ok {
	t.Fatal("couldn't get underlying *testing.T")
	}
	if t != underlierT {
	t.Error("invalid underlier")
	}

The same applies above and below.

[jeandeaual]

This does look better. I updated the tests to follow your suggestion.

[pohly]

This looks good to me now, but I'll give @thockin some time to also have another look because this touches code that he wrote initially.

[thockin]

Can you please un-linewrap the args? This is inconsistent with anything else in the codebase

[jeandeaual]

Sorry for that.

[thockin]

Also please add comments on these funcs to explain why they exist

[jeandeaual]

I added some comments to the new functions. They are here to prevent code duplication between testlogger and testloggerInterface.

[thockin]

do we really need this? What is it proving?

[jeandeaual]

@thockin This is mostly to test that NewWithInterface can be called with a *testing.B in addition to *testing.T.
But it's true that it is not really useful to benchmark this library, so I can remove it if you'd prefer.

[thockin]

I see - I think it is important to have test-cases for B and F.

It's probably enough to have something like:

b := testing.B{}
log := NewWithInterface(b)

and same for F, just to prove it compiles.

[jeandeaual]

I added test cases for *testing.B and *testing.F.
I put the one for *testing.F in a separate file since fuzzing is only available since Go 1.18 but the test cases are run on the CI pipeline with Go 1.16+.