suite.Run: validate Test* methods signature #1512
Conversation
Barugoo
changed the title
| @@ -184,6 +184,11 @@ func Run(t *testing.T, suite TestingSuite) { | |||
| failOnPanic(t, r) | |||
| }() | |||
|
|
|||
| if method.Type.NumIn() > 1 || method.Type.NumOut() > 0 { | |||
| msg := fmt.Sprintf("testify: suite method '%s' shouldn't have any arguments or returning values\n", method.Name) | |||
| panic(msg) | |||
There was a problem hiding this comment.
Not sure about using panic here tho, seems appropriate to me, but i will appreciate if you take a look @dolmen
There was a problem hiding this comment.
Isn't it better to use t.Error(msg)? Then you can directly see the TestMethod violating the contract. I find panic always quite hard to read.
There was a problem hiding this comment.
@wvell the motivation to add panic here instead of t.Error is the source of this condition. I.e. It is not the tested code returning unexpected result, but rather the programmer who messed up the signature of the Test* method itself. In that case panic is more fitting IMHO
There was a problem hiding this comment.
Yeah fair point, I just hate reading panics.
Thanks for putting in the work.
There was a problem hiding this comment.
While I'm a strong advocate of panic to catch programmer's mistake, I think a panic is not appropriate in this case as the stack trace will not be useful to the user.
Instead Run is being called with *testing.T, so let's just use t.Fatal (or t.Fatalf) to report the issue.
However, the main problem with this implementation is that this check must be done much much earlier, line 151, just after the successful call to methodFilter.
| @@ -184,6 +184,11 @@ func Run(t *testing.T, suite TestingSuite) { | |||
| failOnPanic(t, r) | |||
| }() | |||
|
|
|||
| if method.Type.NumIn() > 1 || method.Type.NumOut() > 0 { | |||
| msg := fmt.Sprintf("testify: suite method '%s' shouldn't have any arguments or returning values\n", method.Name) | |||
| panic(msg) | |||
There was a problem hiding this comment.
While I'm a strong advocate of panic to catch programmer's mistake, I think a panic is not appropriate in this case as the stack trace will not be useful to the user.
Instead Run is being called with *testing.T, so let's just use t.Fatal (or t.Fatalf) to report the issue.
However, the main problem with this implementation is that this check must be done much much earlier, line 151, just after the successful call to methodFilter.
dolmen
changed the title
Summary
Added panic in suite.Run if Test* method has args or returning values
P.S. This is my first ever opensource contribution, so I'm very open to any code logic/styling criticism!
Changes
testify: suite method <MethodName> shouldn't have any arguments or returning valuesMotivation
As @wvell mentioned in issue #1508 currently,
suite.Runreturns an ambiguous error when executing with instance of Suite that has incorrectTest*methods signatures.Related issues
Closes #1508