Add assertionf assertions like Errorf and Equalf
#356
Conversation
ernesto-jimenez
force-pushed
the
issue-339
branch
from
a0fdc96
to
39e0258
Compare
| @@ -246,7 +248,7 @@ func Fail(t TestingT, failureMessage string, msgAndArgs ...interface{}) bool { | |||
|
|
|||
| // Implements asserts that an object is implemented by the specified interface. | |||
| // | |||
| // assert.Implements(t, (*MyInterface)(nil), new(MyObject), "MyObject") | |||
There was a problem hiding this comment.
Why are the messages removed in the examples? I'd expect the methods without the f suffix to still accept arguments which are appended similar to how fmt.Println does.
There was a problem hiding this comment.
We cannot do it for this version, since it would be a breaking change, but I'm thinking that testify's assertions provide plenty of context around the failure.
I a v2 of testify, we would probably have assertions not take any message at all, and offer the "format" assertions like Errorf to those who want to provide some context.
This has some extra benefits, since people right now make mistakes like this:
assert.Error(t, err, "expected error message")Because they believe that would check that err.Error() == "expected error message" and the code compiles and runs.
There was a problem hiding this comment.
Sorry, I don't mean change the interface, just the examples should suggest formatting.
assert/assertions_test.go
Outdated
| @@ -555,7 +555,7 @@ func TestNoError(t *testing.T) { | |||
| }() | |||
|
|
|||
| if err == nil { // err is not nil here! | |||
| t.Errorf("Error should be nil due to empty interface", err) | |||
| t.Errorf("Error should be nil due to empty interface: %s", err) | |||
There was a problem hiding this comment.
nit: should use %v for types that aren't bytes or strings
same below at line 595
There was a problem hiding this comment.
This is part of our internal tests, so this should be fine.
There was a problem hiding this comment.
Yep, this will work but the %s format specified is not intended to be used for arbitrary types:
From https://golang.org/pkg/fmt/:
%s the uninterpreted bytes of the string or slice
Whereas we really want %v:
%v the value in a default format
require/require.go
Outdated
| // Containsf asserts that the specified string, list(array, slice...) or map contains the | ||
| // specified substring or element. | ||
| // | ||
| // assert.Containsf(t, "Hello World", "World", "extra error message") |
There was a problem hiding this comment.
The examples for Containsf should contain a formatting string + arguments. Otherwise Contains is fine.
There was a problem hiding this comment.
This is part of the code-generated comments.
, "extra error message" is added to the end of usage examples on "formatted assertions".
Would need to figure out what we can add instead that would be a relevant non-confusing message+variable that works for any assertion.
|
Any update on getting this merged in @ernesto-jimenez? |
|
Any updates. Seems like this is still required. |
ernesto-jimenez
force-pushed
the
issue-339
branch
from
39e0258
to
509ffd4
Compare
Related to #339
In Go 1.7, the govet will complain with the following code:
The complain is:
The cause being the fact that
Error's signature ends withmsgAndArgs ...interface{}instead ofmsg string, args ...interface{}.In this PR we add format assertions like
EqualfandErrorf.In a v2 of testify we would remove
msgAndArgsfrom all the assertions andhave people use the format assertions if they want to customise the error.