The Wayback Machine - https://web.archive.org/web/20220624134117/https://github.com/stretchr/testify/issues/316
Skip to content
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

Jump to bottom

Message argument checking is inconsistent between functions #316

Open
prashantv opened this issue Jun 22, 2016 · 6 comments
Open

Message argument checking is inconsistent between functions #316

prashantv opened this issue Jun 22, 2016 · 6 comments
Labels
good first issue help wanted

Comments

@prashantv
Copy link

@prashantv prashantv commented Jun 22, 2016

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
Copy link
Member

@ernesto-jimenez ernesto-jimenez commented Jun 22, 2016

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
Copy link
Author

@prashantv prashantv commented Jun 22, 2016

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
Copy link
Member

@ernesto-jimenez ernesto-jimenez commented Sep 24, 2016

@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
Copy link
Author

@prashantv prashantv commented Sep 26, 2016

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

@ernesto-jimenez
Copy link
Member

@ernesto-jimenez ernesto-jimenez commented Sep 26, 2016

awesome, thanks!

@bzon
Copy link

@bzon bzon commented Jun 20, 2018

This seems related to #119.

bzon added a commit to devopsctl/gitlabctl that referenced this issue Jun 20, 2018
@bzon
replace testify with standard lib to resolve unknown panic errors
5838c68 
References:
* stretchr/testify#316
* stretchr/testify#119
bzon added a commit to devopsctl/gitlabctl that referenced this issue Jun 20, 2018
@bzon
replace testify with standard lib to resolve unknown panic errors
60a6c6a 
References:
* stretchr/testify#316
* stretchr/testify#119
bzon added a commit to devopsctl/gitlabctl that referenced this issue Jun 20, 2018
@bzon
replace testify with standard lib to resolve unknown panic errors
56c3224 
References:
* stretchr/testify#316
* stretchr/testify#119
bzon added a commit to devopsctl/gitlabctl that referenced this issue Jun 20, 2018
@bzon
replace testify with standard lib to resolve unknown panic errors
948f7e6 
References:
* stretchr/testify#316
* stretchr/testify#119
bzon added a commit to devopsctl/gitlabctl that referenced this issue Jun 20, 2018
@bzon
replace testify with standard lib to resolve unknown panic errors
7d4ba3e 
References:
* stretchr/testify#316
* stretchr/testify#119
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue help wanted
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ernesto-jimenez @prashantv @bzon