stretchr / testify

A toolkit with common assertions and mocks that plays nicely with the standard library
MIT License
22.5k stars 1.56k forks source link

Misleading Error() docs #1609

Open andig opened 3 weeks ago

andig commented 3 weeks ago

Description

The error docs read:

// Error asserts that a function returned an error (i.e. not `nil`).
//
//    actualObj, err := SomeFunction()
//    if assert.Error(t, err) {
//         assert.Equal(t, expectedError, err)
//    }

This indicates that Error() has a bool return value which it hasn't. There is no way to check expectedError like this.

Expected behavior

Error docs should show actual usage. It would be nice if Error() actually allowed for checking the specific error value.

hendrywiranto commented 3 weeks ago

hello @andig

is this the Error() that you meant? https://github.com/stretchr/testify/blob/master/assert/assertions.go#L1596 Error has a bool return value, please do elaborate more if I misunderstood your meanings.

andig commented 3 weeks ago

Thank you for the quick reply. Seems I was actually looking at require.Error(). Thinking about it, the if case doesn't really make sense for require, so only the comment is off?

// Error asserts that a function returned an error (i.e. not `nil`).
//
//    actualObj, err := SomeFunction()
//    if assert.Error(t, err) {
//         assert.Equal(t, expectedError, err)
//    }
func Error(t TestingT, err error, msgAndArgs ...interface{}) {
    if h, ok := t.(tHelper); ok {
        h.Helper()
    }
    if assert.Error(t, err, msgAndArgs...) {
        return
    }
    t.FailNow()
}
hendrywiranto commented 3 weeks ago

yea I think so, but I checked the docs for require, they all used assert as example in the comments, I don't know if it's intended or not I think if we want to change we need to adjust all the comments on the package.