Skip to content

assert: add Helper() in TestingT interface #1422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dolmen opened this issue Jul 13, 2023 · 3 comments
Open

assert: add Helper() in TestingT interface #1422

dolmen opened this issue Jul 13, 2023 · 3 comments
Assignees
Labels
Breaking Change TB.Helper Related to hiding testify calls in stack using testing.TB.Helper

Comments

@dolmen
Copy link
Collaborator

dolmen commented Jul 13, 2023

Since Go 1.9 The testing.T/testing.B/testing.F and more generally the testing.TB interface all have an Helper() method that must be used by test helpers to hide them in the caller stack in order to properly report test errors at the location where the user cares.

In testify MRs #554, #1026 added the call to Helper() in every testify function. Here is the code:

 if h, ok := t.(tHelper); ok {
	h.Helper()
}

This works fine, but:

  • this code is verbose
  • every standard use of testify is done with a testing context (testing.TB) that supports Helper(), so the guard
  • the interface guard make every testify function slower than necessary

It would be easier if we could just do:

t.Helper()

However our current definition of assert.TestingT is:

// TestingT is an interface wrapper around *testing.T                                                                                                                                                
type TestingT interface {                                                                                                                                                                            
        Errorf(format string, args ...interface{})                                                                                                                                                   
}

I propose to add the Helper() method to assert.TestingT.

This can be considered a breaking change of the API contract. However:

  • this will not affect any user code that calls the testify methods with values implementing testing.TB.
  • there is a risk that this could affect helpers built around testify functions, however those wrappers should also call Helper() and if they didn't we can safely consider this is a good thing if they have to be fixed when the user will upgrade testify.
  • some mocks of assert.Testing.T in user code may not implement Helper(). I think this is reasonable to force them to be fixed.
  • this is quite similar as when interface testing.TB has been extended to include Helper()

Steps required to accomplish this:

  1. Fix our internal mocks to implement Helper() (not a breaking change)
  2. Implement Helper() on type assert.CollectT (not a breaking change)
  3. Extend assert.TestingT, require.TestingT, mock.TestingT to add Helper() (limited breaking change)
  4. Replace the calls to Helper()

Cc: @fefe982 @boyan-soubachov @brackendawson

@dolmen
Copy link
Collaborator Author

dolmen commented Jul 13, 2023

#262 is the last time we attempted extending assert.TestingT to add FailNow() and this was reverted by #263. That was 9 years ago.

@dolmen
Copy link
Collaborator Author

dolmen commented Jul 13, 2023

Cc: @jasdel because of your comment #262 (comment)

However, project github.com/gucumber/gucumber is dead (replaced by github.com/cucumber/gherkin/go/v26

@brackendawson
Copy link
Collaborator

I've been thinking about this one in the shower for the past week. The advangages here are cosmetic in our implementation and extremely minor perormance gains, the reflection most assertions make will be far slower than this type assertion.

While I acknowledge that there should be no modules out there which would be broken by this change, abiding by the compatability promise is critically important for working in the Go module ecosystem to prevent broken builds by incompatible inderect dependencies. I think we should live with this legacy.

@dolmen dolmen added the TB.Helper Related to hiding testify calls in stack using testing.TB.Helper label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change TB.Helper Related to hiding testify calls in stack using testing.TB.Helper
Projects
None yet
Development

No branches or pull requests

2 participants