Message argument checking is inconsistent between functions

[prashantv]

I recently ran into an issue where we accidentally passed two different errors to require.NoError:

require.NoError(t, err, TestFunc(), "TestFunc failed")

It was incorrectly testing the previous err instead of the output of calling TestFunc(). I've noticed that testify is inconsistent between whether it checks the arguments before checking if the condition failed, or whether it's only checked if the condition fails.

I've written a couple of simple tests to show the difference:

func TestRequireErr(t *testing.T) {
    var actualErr error
    wrongErr := errors.New("err1")
    require.Error(t, wrongErr, actualErr, "Message")
}

func TestRequireNoErr(t *testing.T) {
    var wrongErr error
    actualErr := errors.New("err1")
    require.NoError(t, wrongErr, actualErr, "Message")
}

In both cases, we're passing two errors -- the second is what we actually want to compare. However, the first error we pass would pass the assertion, while the second is the one we intended, and would cause the assertion to fail.

The output of TestRequireErr:

=== RUN   TestRequireErr
--- FAIL: TestRequireErr (0.00s)
panic: interface conversion: interface is nil, not string [recovered]
        panic: interface conversion: interface is nil, not string

The output of TestRequireNoErr:

=== RUN   TestRequireNoErr
--- PASS: TestRequireNoErr (0.00s)

I think both assertions should be consistent -- and ideally, they both ensure that the msg is actually a string regardless of the condition, since the compiler can't enforce this.

[ernesto-jimenez]

Hey @prashantv,

It was incorrectly testing the previous err instead of the output of calling TestFunc().

That's how it is supposed to work. Error and NoError just take one variable and check whether the err is nil or not.

I've noticed that testify is inconsistent between whether it checks the arguments before checking if the condition failed, or whether it's only checked if the condition fails.

Sorry, I'm having a hard time understanding what you mean by this. Can you elaborate a bit more?

[prashantv]

That's how it is supposed to work. Error and NoError just take one variable and check whether the err is nil or not.

Yes, I understand that, but my issue is more that when the user makes a mistake and passes multiple errors, the handling is different based on whether it's Error or NoError.

If you pass two errors by accident to Error, it will always panic, since it tries to format the message before the condition check. However, if you pass two errors to NoError, if the first error is nil, it won't try to format the message, and so won't cause any issues.

This is an inconsistency, and I personally think both methods should ensure that when you do:

assert.Error(t, err, someArg)

The method should always ensure that someArg is a string. This is normally something the compiler would catch, but because the message and args are optional, the compiler can't do this check, so I think the library should check to catch these sorts of bugs.

[ernesto-jimenez]

@prashantv sorry it took long to reply, I have been very busy.

Would you like to issue a PR changing every assertion to fail when the first element of msgAndArgs is not a string?

[prashantv]

@ernesto-jimenez Sure, I can take that on.

[ernesto-jimenez]

awesome, thanks!

[bzon]

This seems related to #119.

[ossan-dev]

Hey guys,
correct me if I'm wrong but I didn't get the issue.
I repro everything locally with the following code (I wrote one test for each function we're dealing with):

package example

import (
	"errors"
	"testing"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

func TestRequireError(t *testing.T) {
	err := errors.New("custom error 1")
	_ = err
	// require.Error(t, nil, err, "err should be present") // panics interface conversion err
	// require.Error(t, nil, nil, "err should be present") // panics interface conversion err
	require.Error(t, nil, "context: %s", "err should be present")
}

func TestRequireNoError(t *testing.T) {
	err := errors.New("custom error 2")
	_ = err
	// require.NoError(t, err, nil, "err should NOT be present") // panics interface conversion err
	// require.NoError(t, err, err, "err should NOT be present") // panics interface conversion err
	require.NoError(t, err, "context: %s", "err should NOT be present")
}

func TestAssertError(t *testing.T) {
	err := errors.New("custom error 3")
	_ = err
	// assert.Error(t, nil, err, "err should be present") // panics interface conversion err
	// assert.Error(t, nil, nil, "err should be present") // panics interface conversion err
	assert.Error(t, nil, "context: %s", "err should be present")
}

To me, the behavior is consistent among them. If not, can you bring me in the right direction so I can file a PR to fix that mismatched behavior?
Thanks

[myusko]

Is an issue still relevant? cc @arjunmahishi

[arjunmahishi]

@myusko I'm not sure is this is still relevant. I think most of the functions are handling this in a pretty consistent way. Can you help in verifying that?

We need to make sure that all the assertions using the Fail function to handle the msgAndArgs.

---

Looks like none of the http assertions are using the msgAndArgs at all. I'll file a separate issue for that.

[myusko]

@myusko I'm not sure is this is still relevant. I think most of the functions are handling this in a pretty consistent way. Can you help in verifying that?

We need to make sure that all the assertions using the Fail function to handle the msgAndArgs.

Looks like none of the http assertions are using the msgAndArgs at all. I'll file a separate issue for that.

Yup, I can help with that, but seems like you already even created a PR, sonic's speed, hehe.

[brackendawson]

It seems like this issue has mostly been worked around. I like this suggestion would entirely fix the problem but would need v2.