changed dependency to interface#867
Conversation
|
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? |
|
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? |
|
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! |
boyan-soubachov
left a comment
There was a problem hiding this comment.
@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
|
|
||
| type TestingT interface { | ||
| Run(name string, f func(t *testing.T)) bool | ||
| Helper() |
There was a problem hiding this comment.
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?
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.