GinkgoT() should implements testing.TB

[wingyplus]

I found that I cannot parse GinkgoT() as an argument to my test helper code that use both testing and benchmark code because it doesn't have Helper() method.

For example:

package gink

import (
	"testing"

	"github.com/onsi/ginkgo"
	"github.com/onsi/gomega"
)

func Test(t *testing.T) {
	gomega.RegisterFailHandler(ginkgo.Fail)
	ginkgo.RunSpecs(t, "suite")
}

var _ = ginkgo.Describe("my suite", func() {
	ginkgo.It("under test", func() {
		myTestHelperCode(ginkgo.GinkgoT())
	})
})

// I use this function across my test code and benchmark.
func myTestHelperCode(tb testing.TB) {
	tb.Helper()
}

The result from go test:

$ go test .
# github.com/wingyplus/gink [github.com/wingyplus/gink.test]
./gink_test.go:17:34: cannot use ginkgo.GinkgoT() (type ginkgo.GinkgoTInterface) as type testing.TB in argument to myTestHelperCode:
	ginkgo.GinkgoTInterface does not implement testing.TB (missing Helper method)
FAIL	github.com/wingyplus/gink [build failed]
FAIL

[jmalloc]

Hi all,

Apologies in advance for the wall-of-text!

I too have been looking for functionality along these lines. I am hopeful that you might (re)consider updating GinkgoTInterface to be compatible with testing.TB.

I figured it was also worth pointing out that testing.TB itself contains an unexported method, and as such can never be implemented by anything outside the testing package 😞, though I don't believe that invalidates the argument for supporting such a change.

My use case is that I maintain a test runner / assertion library for use with a particular application framework. It has it's own equivalent to GinkgoTInterface which allows it to be used under Ginkgo, the standard test runner, or anything else that can supply such an interface.

This assertion library also supports custom user-supplied assertions. I would like to be able to supply something compatible with testing.TB to my users such that their custom assertions could make use of any other package that uses its own subset of TB.

We use Ginkgo and Gomega widely within my organisation, and as such I have been reluctant to require any methods that Ginkgo can not supply, but this is becoming increasingly more difficult as I seek to allow integration with third-party testing/assertion libraries that are unknown to me.

Obviously this is just one scenario, but I hope it makes the case for a generalised approach to supporting this.

For completeness, the following methods from testing.TB are absent from GinkgoTInterface as of Go 1.14:

• Name() string
• Helper()
• Cleanup(func())

Additionally, the Parallel() method from testing.T, which is present on GinkgoTInterface, is not on testing.TB.

I think Name() and Cleanup() could have meaningful implementations within the context of Ginkgo. Helper() could likely be a no-op, as per #585. Of course, I would be willing to submit a PRs should such a changes be welcomed :)

Edit: see also #565, #659.

[williammartin]

Hi @jmalloc thanks for the comment. I think we should definitely do something about this @onsi @blgm

[onsi]

hey @jmalloc - this sounds good to me. Would love a PR if you're up for it.

[alex-slynko]

Hey @jmalloc
Do you need any help with implementing this?

[leetrout]

Subscribing- also happy to help- I'm under a deadline so I'm going to make a global of *testing.T since I hit the missing Name() issue.

I was trying to use terratest:

ginkgo.GinkgoTInterface does not implement "github.com/gruntwork-io/terratest/modules/testing".TestingT (missing Name method)

[wingyplus]

I think we can start by implements all method that available in testing.TB and then consider that which one should add the behaviour or noop. What do you all think?

[jmalloc]

Sorry everyone, I haven't had any time to dedicate to this recently and unfortunately I don't see that changing in the very near term. It usually takes me a while to get to these things. I am still willing to do the PR of course, but if someone gets there before me then that's all the better 👍

I think we can start by implements all method that available in testing.TB and then consider that which one should add the behaviour or noop. What do you all think?

This is a good step.

I notice that the TestingT interface from terratest only requires the Name() method. Perhaps we could add that first in it's own PR, then at least @leetrout could move forward. I think it would be ideal if this method were to return the "full path" of Describe / It names for the current test. If this is difficult to achieve, perhaps in the short-term it could simply return a static string, like "ginkgo". Obviously it's up to the Ginkgo maintainers as to whether that's a good enough solution.

In my opinion, Cleanup() in particular needs special attention. It's probably best that its behavior matches the standard Go testing framework as closely as possible. If we were to make Cleanup() a noop that could lead to some very confusing and difficult to diagnose issues within the test suites running on these other frameworks.

[onsi]

hey all I don't know if you've had a chance to see the Ginkgo 2.0 proposal - but this is in scope (and should land on master before most of the 2.0 stuff)

[onsi]

i've started working on this - I have Name() returning the full test name (i.e. the concatenated strings of all Describe, Context, and the It block itself).

Cleanup() is going to be challenging to implement. Ginkgo separates test structure creation from the test runtime and Cleanup() would require adding an AfterEach while the test is running. This isn't something that would easy to support, unfortunately, without some ugly hacker to support it just for GinkgoT(). I'm curious how many folks on this thread are using frameworks that use Cleanup(). (FWIW I agree that not having this could lead to confusing behavior... I'm just not quite sure it's worth the effort to implement in practice yet).

[paulburlumi]

I'm curious how many folks on this thread are using frameworks that use Cleanup(). (FWIW I agree that not having this could lead to confusing behavior... I'm just not quite sure it's worth the effort to implement in practice yet).

@onsi, one recent example where GinkgoT() failed because Cleanup() was missing was:

https://github.com/hashicorp/consul/blob/9fab3fe9905f4d621312eea192f556772603dd85/sdk/testutil/testlog.go#L38

Tracking this back you can see that t here is actually a testing.TB.

https://github.com/hashicorp/consul/blob/9fab3fe9905f4d621312eea192f556772603dd85/sdk/testutil/server.go#L223

[onsi]

ok I've pushed a commit to master that should implement all the relevant interfaces. Cleanup() is currently a no-op and I'm weighing adding a new Ginkgo method to enable users to dynamically register AfterEaches within a running test. That could then be used to implement Cleanup().

[leetrout]

Thanks @onsi -- working as expected with terratest.

[paulburlumi]

Thanks @onsi. I've just retested the updated commit with the consul testutil package. i.e.

$ go get github.com/onsi/ginkgo@b5fe44d9e12817942846c49d35804c5100e05327

So now have the following in my go.mod file:

github.com/hashicorp/consul/api v1.6.0
github.com/hashicorp/consul/sdk v0.6.0
github.com/onsi/ginkgo v1.14.2-0.20200909164806-b5fe44d9e128

When I use GinkgoT() as follows:

ts, err := testutil.NewTestServerConfigT(ginkgo.GinkgoT(), nil)

I now see the following error when running the tests:

cannot use ginkgo.GinkgoT() (type ginkgo.GinkgoTInterface) as type testing.TB in argument to testutil.NewTestServerConfigT:
        ginkgo.GinkgoTInterface does not implement testing.TB (missing testing.private method)

Looking at the definition of testing.TB I see:

// TB is the interface common to T and B.
type TB interface {
	Cleanup(func())
	Error(args ...interface{})
	Errorf(format string, args ...interface{})
	Fail()
	FailNow()
	Failed() bool
	Fatal(args ...interface{})
	Fatalf(format string, args ...interface{})
	Helper()
	Log(args ...interface{})
	Logf(format string, args ...interface{})
	Name() string
	Skip(args ...interface{})
	SkipNow()
	Skipf(format string, args ...interface{})
	Skipped() bool
	TempDir() string

	// A private method to prevent users implementing the
	// interface and so future additions to it will not
	// violate Go 1 compatibility.
	private()
}

Am I misreading something here? It looks like it is impossible to implement testing.TB by design?
How might I resolve this problem? Workarounds and suggestions welcome!

[paulburlumi]

I've just looked at the implementation of terratest..

// Using an interface that describes testing.T instead of the actual implementation
// makes terratest usable in a wider variety of contexts (e.g. use with ginkgo : https://godoc.org/github.com/onsi/ginkgo#GinkgoT)
type TestingT interface {

I assume to make this work for consul I would have modify the testutil package to use a new interface instead of using the actual testing.TB interface itself. sigh.

[onsi]

yes that's correct @paulburlumi

can you point me at the testutil package? i'm curious to see what it does with that instance of TB.

[paulburlumi]

@onsi I've now modified the consul testutil package with hashicorp/consul#8647.
With this change it now appears to be working as expected, given Cleanup is a no-op.

[onsi]

sweet - thanks for pushing that through @paulburlumi

[blgm]

It looks like things are now working for everyone since commit b5fe44d, so I'm going to close this off now. Please let me know if I missed something.

[raulvc]

welp, here we are again, testing from 1.18.1 SDK has this amazing change in testing.TB interface:

	// A private method to prevent users implementing the
	// interface and so future additions to it will not
	// violate Go 1 compatibility.
	private()

[qmilangowin]

Arrgh. Was hopeful for it too. I have test-fixtures (setup and teardown) in the framework I've written at work that use t.Cleanup() and t.Failed() and t.Name() as the framework should be able to use testing.T from that standard library but also would like to use Ginkgo with them. And seems like everything would work if I could pass in BeforeEach/BeforeSuite testing.T in any func.

For now I guess I'll resort to running them in the bootstrapped function that uses testing.T

[ebabani]

If anyone is having problems due to the private() in testing.TB, we ended up working through it by creating simple struct wrapper.

type ginkgoT struct {
    testing.TB
    gt GinkgoTInterface
}

func (g *ginkgoT) Cleanup(f func()) {
    g.gt.Cleanup(f)
}
func (g *ginkgoT) Error(args ...any) {
    g.gt.Error(args)
}
func (g *ginkgoT) Errorf(format string, args ...any) {
    g.gt.Errorf(format, args)
}
func (g *ginkgoT) Fail() {
    g.gt.Fail()
}
func (g *ginkgoT) FailNow() {
    g.gt.FailNow()
}
func (g *ginkgoT) Failed() bool {
    return g.gt.Failed()
}
func (g *ginkgoT) Fatal(args ...any) {
    g.gt.Fatal(args)
}
func (g *ginkgoT) Fatalf(format string, args ...any) {
    g.gt.Fatalf(format, args)
}
func (g *ginkgoT) Helper() {
    g.gt.Helper()
}
func (g *ginkgoT) Log(args ...any) {
    g.gt.Log()
}
func (g *ginkgoT) Logf(format string, args ...any) {
    g.gt.Logf(format, args)
}
func (g *ginkgoT) Name() string {
    return g.gt.Name()
}
func (g *ginkgoT) Setenv(key, value string) {
    g.gt.Setenv(key, value)
}
func (g *ginkgoT) Skip(args ...any) {
    g.gt.Skip(args)
}
func (g *ginkgoT) SkipNow() {
    g.gt.SkipNow()
}
func (g *ginkgoT) Skipf(format string, args ...any) {
    g.gt.Skipf(format, args)
}
func (g *ginkgoT) Skipped() bool {
    return g.gt.Skipped()
}
func (g *ginkgoT) TempDir() string {
    return g.gt.TempDir()
}

// &ginkgoT{gt: GinkgoT()}

For any function that takes in the testing.TB interface we pass in our *ginkgoT struct.

[onsi]

oh snap - that's brilliant. would this make sense to pull in to Ginkgo itself? Something like GinkgoTB() which returns a concrete type that wraps GinkgoT()?

[ebabani]

I think that would work. Only concern would be that if testing.TB were to add a new function, any caller of that function from on ginkgoTB would get a NPE.

https://go.dev/play/p/WK2gl6HctI1

If that's an acceptable tradeoff I can make a PR for this.

[onsi]

I think that's a fine tradeoff and would prompt the introduction of that function to GinkgoT which would be a helpful feedback loop. Yes, please do submit a PR for GinkgoTB() - thanks!

[EronWright]

PR: #1333