Make Error and Fatal functions compatible with testing.T

[sirockin]

What

Making testing.Error and testing.Fatal functions compatible with testing.T by changing the single error argument to a variable argument list accepting any.

Why

Facilitates reusing test scenarios by extracting common testing interface

Testing

The only tests which currently exist for the two functions confirm that the methods fail the load test do not test the logging functionality. We update these with test cases to cover different arguments being passed in.

Example code

main.go:

package main

import (
	"github.com/form3tech-oss/f1/v2/pkg/f1"
	"github.com/form3tech-oss/f1/v2/pkg/f1/testing"
)

// TF is the interface common to T and testing.TB
type TF interface {
	Cleanup(fn func())
	Error(args ...any)
	Errorf(format string, args ...any)
	Fail()
	FailNow()
	Failed() bool
	Fatal(args ...any)
	Fatalf(format string, args ...any)
	Log(args ...any)
	Logf(format string, args ...any)
	Name() string
}

// Check T implements TF
var _ TF = (*testing.T)(nil)

func main() {
	f1.New().
		Add("Foo", setupFoo).
		Execute()
}

func setupFoo(t *testing.T) testing.RunFn {
	// Do once-only setup here

	return func(t *testing.T) {
		FooTest(t)
	}
}

// Foo can be run as a load test or a standard go test
func FooTest(TF) {
	// Do the test here
}

main_test.go:

package main

import (
	"testing"
)

// Run  FooTest as a standard go test 
func TestFoo(t *testing.T) {
	main.FooTest(t)
}

Resolves #278

[sirockin]

I'm surprised the CI pipeline is failing on interface compatibility given the changes are non-breaking.

[nvloff-f3]

@sirockin This is a breaking change for any users that may have already done what we're trying here - create an abstraction to share code between f1 and other parts of their codebase.

We want to avoid breaking users as much as possible hence my comment on the other PR: #279 (comment)

I'm open to ideas, but I still think adding a TB() method to T that returns a compatible object is a reasonable approach.

[nvloff-f3]

This would have different logging depending on the number of arguments.

I think if we just add a new log Attr helper that merges a list of errors, we can handle any number of arguments in a consistent way

[sirockin]

Thanks for the quick review and useful feedback, @nvloff-f3 .

I'm still not exactly clear on what you are looking for though. The original comment said // Error is equivalent to Log followed by Fail. (consistent with behaviour of testing.T) so I was aiming to get as close to that as possible.

Please could you clarify the behaviour you would like to see like to see with:

• a single argument of type error
• a single argument of type other than error
• more than one argument, all of type error
• more than one argument, not all of type error

It would be great if we could create some tests for these scenarios but given the way that concrete logger instances are embedded into t, that may be difficult to achieve.

It could also be argued that rather than introducing a helper to log the arguments, we should amend Log so that it does what we want then use that in the methods. This would simplify code, make behaviour consistent across these methods and make the original comments for Error and Fatal accurate. However it would mean that we couldn't force logger.Error to be called if standard printf arguments were supplied to Error or Fatal.

Anyway let me know your thoughts and I'll prepare a commit.

Thanks again!

[sirockin]

@sirockin This is a breaking change for any users that may have already done what we're trying here - create an abstraction to share code between f1 and other parts of their codebase.

I may be being dense (it's early in the morning for me 😂 ) but I'm still not clear on why this would be a breaking change. The two methods will still behave exactly as previously for the only case which was previously compilable: a single error argument.