Skip to content
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

changed dependency to interface #867

Merged
merged 4 commits into from Jan 29, 2020
Merged

Conversation

npxcomplete
Copy link
Contributor

I encountered a problem when consuming testify through gobuffalo. I wanted to boot the development environment using a tests fixture from their fixture tooling. They use testify suites to integrate fixtures into controller tests. Since setting up a concrete T is quite laborious given much of what go test uses to do it is hidden, it makes more sense to me to provide a stub which can only work if testify consumes an interface.

@glesica
Copy link
Collaborator

glesica commented Jan 11, 2020

I don’t think we want to do this because there could be methods added to ‘testing.T’ in the future. It also makes the signature different than what people might normally expect to see. Anyone else have thoughts?

@npxcomplete
Copy link
Contributor Author

It's worth acknowledging that "this" is idiomatic in go, and is already done in both the require and assert packages. Perhaps you could outline what makes this case special in that it should accept a concrete type?

@glesica
Copy link
Collaborator

glesica commented Jan 12, 2020

If we've done it in the past and no one has complained, then sure. I was just concerned that the change would fix your thing but break someone else's thing. Let's give it until Tuesday or so to give people time to share their thoughts and then I'll go ahead and merge it. Thanks!

Copy link
Collaborator

@boyan-soubachov boyan-soubachov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npxcomplete 's argument makes sense and this change would bring the suite package inline with require and assert. Happy to approve this as long as we implement only the minimum necessary of that interface (i.e. without the Helper() function).

suite/suite.go Outdated
@@ -16,21 +16,29 @@ import (
var allTestsFilter = func(_, _ string) (bool, error) { return true, nil }
var matchMethod = flag.String("testify.m", "", "regular expression to select tests of the testify suite to run")

type TestingT interface {
Run(name string, f func(t *testing.T)) bool
Helper()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't limit the interface unnecessarily. If I'm not mistaken, Helper() isn't used anywhere in Suite; would it not make sense to leave it out for now until absolutely necessary?

@boyan-soubachov boyan-soubachov merged commit ea72eb9 into stretchr:master Jan 29, 2020
boyan-soubachov added a commit to boyan-soubachov/testify that referenced this pull request Feb 19, 2020
@boyan-soubachov boyan-soubachov mentioned this pull request Feb 19, 2020
boyan-soubachov added a commit that referenced this pull request Feb 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants