-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add assertion
f assertions like Errorf and Equalf
#356
Conversation
a0fdc96
to
39e0258
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just some minor comments.
For examples, I think they should encourage always leaving some context. For the formatting methods, the context should use a formatting string, but the non-formatting methods should just a have a static string.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should use %v
for types that aren't bytes or strings
same below at line 595
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of our internal tests, so this should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples for Containsf
should contain a formatting string + arguments. Otherwise Contains
is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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
Equalf
andErrorf
.In a v2 of testify we would remove
msgAndArgs
from all the assertions andhave people use the format assertions if they want to customise the error.