ScenarioFn uses common testing interface

[sirockin]

Resolves #278

• Adds a new interface TF which exposes common testing methods
• Adds f1.Register to replace f1.New using ScenarioFunc and RunFunc which each use testing.TF rather than testing.T

Most of the complications are due to preserving backwards-compatibility with the existing f1.New method.

We had to remove t.Parallel() from an existing (previously-unique) test after adding a new one to the same module since this triggered race-detection. We have confirmed that this was an previously-existing issue (by simply duplicating the existing test). It may be worth raising an issue on this.

[nvloff-f3]

@sirockin would you mind updating the PR with master again to pull #281 so that we can run tests on the PR please 🙏

Edit: pulling the branch locally I can see that there are linting failures that would need to be resolved

[sirockin]

@sirockin would you mind updating the PR with master again to pull #281 so that we can run tests on the PR please 🙏

@nvloff-f3, thanks for the feedback and sorry about the lint errors. I've merged from main and attempted to fix lint errors as best as I can. Unfortunately there seem to be some conflicting linting rules when using //nolint:

I get:

==> Running golangci-lint...
pkg/f1/f1_deprecated_scenarios_test.go:5:1: directive `// nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition` should be written without leading space as `//nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition` (nolintlint)
// nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition
^
pkg/f1/f1_scenarios_test.go:5:1: directive `// nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition` should be written without leading space as `//nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition` (nolintlint)
// nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition
^
pkg/f1/testing/t.go:40:1: directive `// nolint: interfacebloat // we are deliberately implementing a subset of the testing.TB interface` should be written without leading space as `//nolint: interfacebloat // we are deliberately implementing a subset of the testing.TB interface` (nolintlint)
// nolint: interfacebloat // we are deliberately implementing a subset of the testing.TB interface

But when fix the space I then get:

==> Running golangci-lint...
pkg/f1/f1_deprecated_scenarios_test.go:5: File is not `gofmt`-ed with `-s` (gofmt)
//nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition
pkg/f1/f1_scenarios_test.go:5: File is not `gofmt`-ed with `-s` (gofmt)
//nolint: paralleltest // running tests in parallel reveals a pre-existing a race condition
pkg/f1/testing/t.go:40: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/form3tech-oss/f1) (gci)
//nolint: interfacebloat // we are deliberately implementing a subset of the testing.TB interface
make: *** [lint] Error 1

[sirockin]

@nvloff-f3 I have also now replicated (I think) all the tests which previously used the deprecated Run() function.

[sirockin]

We remove these since the t.Logger() function is no longer available

[nvloff-f3]

Hi @sirockin I've been thinking about this and I have some concerns I want to share.

I don't think the deprecated APIs can ever be removed. We have thousands of f1 scenarios at Form3, we can't realistically change all of them. This would mean that we must forever keep two separate APIs for f1, which is not something we're comfortable maintaining, especially given how we have no use case for the new APIs.

Looking at go's testing package the testing.TB is just defined, but concrete types are passed to user defined functions. Where the interface is there just to allow users to write helpers that can be reused between tests and benchmarks. In that way I'm even more reluctant to introduce the new APIs, as they are not really necessary.

Let's take the following use case:

package main

import (
	stdtesting "testing"

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

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

func emptyScenario(*f1testing.T) f1testing.RunFn {
	runFn := func(t *f1testing.T) {
		myTest(t)
	}

	return runFn
}

func TestEmpty(t *stdtesting.T) {
	myTest(t)
}

func myTest(t stdtesting.TB) {
	t.Error("test")
}

This does not compile due to testing.TB forbidding interface implementations:

benchcmd/main.go:18:10: cannot use t (variable of type *"github.com/form3tech-oss/f1/v2/pkg/f1/testing".T) as "testing".TB value in argument to myTest: *"github.com/form3tech-oss/f1/v2/pkg/f1/testing".T does not implement "testing".TB (missing method private)

Now if we introduce a copy of testing.TB interface in f1 that does allow users to implement it - it would mean they must use that interface across the test suite. Which would make f1 a generic testing library, and we're not looking to take f1 in that direction.

This now compiles

package main

import (
	stdtesting "testing"

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

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

func emptyScenario(*f1testing.T) f1testing.RunFn {
	runFn := func(t *f1testing.T) {
		myTest(t)
	}

	return runFn
}

func TestEmpty(t *stdtesting.T) {
	myTest(t)
}

// all the shared test code must now use this interface
func myTest(t f1testing.TF) {
	t.Error("test")
}

However, this interface doesn't have to be in f1, I think it's better to leave it up to users to decide.

All of that being said, I think if we can get around the breaking change of Error and Fatal signature changes - I'm happy to make f1's testing.T compatible with testing.TB. However, I'm reluctant to add the generic interfaces in f1, as I don't think it's a good idea for users to tie their test suites to this package.

[nvloff-f3]

The best idea I have to avoid the API breakage is to introduce t.TBCompatible() that would return a struct embedding the t and implementing the extra methods.

This way we can define all the new ones on testing.T, have testing.TBCompat proxy everything to the underlying T, except for the Error and Fatal calls. Then in a future where we want to make this the default T API TBCompatible() will just return itself.

While I don't think introducing the breaking change is too bad, we don't want to bump the module version and we don't want to remove the goreleaser CI check.

[sirockin]

Thanks for the useful and extensive feedback, @nvloff-f3. Your points make very good sense.

Making t.Error() and t.Fatal() compatible with testing.TB would definitely unblock our use-case

I would still argue that the .TF interface could be usefully included in the package but happy to let that one slide.

Closing this in favour of: #286