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

Issues implementing a "Not" comparison #147

Open
cpuguy83 opened this issue Mar 20, 2019 · 12 comments
Open

Issues implementing a "Not" comparison #147

cpuguy83 opened this issue Mar 20, 2019 · 12 comments

Comments

@cpuguy83
Copy link
Contributor

Basically, I tried implementing a generic Not comparison like assert.Assert(t, Not(cmp.Equal(foo, bar)).

The interface here seems a bit limiting as I do not have access to variables, formatting or anything from my Not comparison.
It seems right now I would have to implement a NotEqual comparison in order to gain access variables and the like.

I'm wondering if some additional interface could help here? Maybe something that a comparison could optionally implement to make it easier to wrap?

@dnephin
Copy link
Member

dnephin commented Mar 20, 2019

https://godoc.org/gotest.tools/assert has some examples of how Not can be done with assert.Assert().

Since you are comparing against a fixed value, and the failure message will contain the literal source code used in the assertion, you should be able to use !foo or != value and have a failure print the literal.

If the expected value is a variable then I guess the literal is less useful. Is that the case?

@rliebz
Copy link

rliebz commented Apr 23, 2019

Negating a bool is trivial, but a cmp.Comparison is not.

As an example of a use case I had, if I had a string myText that should contain "foo" but should not contain "bar":

assert.Check(t, cmp.Contains(myText, "foo"))
assert.Check(t, !strings.Contains(myText, "bar"))

It's workable, but the error messaging on the Comparison functions tend to be better for these situations.

ijc pushed a commit to ijc/docker-app that referenced this issue May 1, 2019
The use of `!strings.Contains()` in one assertion, rather than
`!cmp.Contains()` is due to gotestyourself/gotest.tools#147

Signed-off-by: Ian Campbell <ijc@docker.com>
ijc pushed a commit to ijc/docker-app that referenced this issue May 7, 2019
The use of `!strings.Contains()` in one assertion, rather than
`!cmp.Contains()` is due to gotestyourself/gotest.tools#147

Signed-off-by: Ian Campbell <ijc@docker.com>
ijc pushed a commit to ijc/docker-app that referenced this issue May 13, 2019
The use of `!strings.Contains()` in one assertion, rather than
`!cmp.Contains()` is due to gotestyourself/gotest.tools#147

Signed-off-by: Ian Campbell <ijc@docker.com>
ijc pushed a commit to ijc/docker-app that referenced this issue May 13, 2019
The use of `!strings.Contains()` in one assertion, rather than
`!cmp.Contains()` is due to gotestyourself/gotest.tools#147

Signed-off-by: Ian Campbell <ijc@docker.com>
@tiborvass
Copy link

tiborvass commented Aug 26, 2019

FWIW I implemented something like it here: https://github.com/tiborvass/docker/blob/c03078863fca6f37fe852b36c19d48e2ac025f44/integration-cli/checker/checker.go#L38-L50 which required the creation of another type whose Compare name I don't like because it conflicts with cmp.Comparison but oh well.

@glenjamin
Copy link

i recently tried to do this with the following code, it worked well when it passed, and produced an unhelpful message when it failed.

func not(c cmp.Comparison) cmp.Comparison {
	return func() cmp.Result {
		return InvertedResult{c()}
	}
}

type InvertedResult struct {
	cmp.Result
}

func (r InvertedResult) Success() bool {
	return !r.Result.Success()
}

If the FailureMessage interfaces were public, it might be possible to wrap generically enough to prefix the messages with NOT - but I didn't spend a lot of time on this

@dnephin
Copy link
Member

dnephin commented May 1, 2020

func (r InvertedResult) FailureMessage(args []ast.Expr) string {
	if f, ok := r.Result.(interface{
                FailureMessage() string
	}); ok {
            return "NOT " + f.FailureMessage()
        }

	if f, ok := r.Result.(interface{
                FailureMessage(args []ast.Expr) string
	}); ok {
            return "NOT " + f.FailureMessage(args)
        }        
	return "failed, but I do not know why"
}

I think something along those lines should work.

@dnephin
Copy link
Member

dnephin commented May 1, 2020

assert.Check(t, !strings.Contains(myText, "bar"))

It's workable, but the error messaging on the Comparison functions tend to be better for these situations.

Sorry for not replying to this comment!

I agree. In cases where the default message is not great I will add more context at the end:

assert.Assert(t, !strings.Contains(myText, "bar"), "got: %v", myText)
doer, ok := something.(Doer)
assert.Assert(t, ok, "got: %T", something)

Some examples of this in the intro docs would probably be a good addition.

It's a bit more typing, but hopefully negative assertions are relatively rare.

I think there are even advantages to this approach over of a Comparison. It encourages adding additional context to assertions, and keeps the interface small.

@glenjamin
Copy link

I think something along those lines should work.

Yep, that makes sense - thanks.

@eloo
Copy link

eloo commented Jan 25, 2021

Hi,

i stumbled over this issue while trying to create something like a "NotDeepEqual" comparision.
But sadly it looks like such a thing is not supported.
So how should a check if two structs are not equal?

Thanks

@dnephin
Copy link
Member

dnephin commented Jan 30, 2021

Hi @eloo , could you share more about your use case? What is the test trying to verify?

The reason I ask is because NotDeepEqual is a very weak check. It doesn't tell you much about the expected behaviour. Generally it is possible to write a test in a different way that gives you much better confidence by avoiding the use of Not comparisons.

@eloo
Copy link

eloo commented Jan 31, 2021

Hi @dnephin
my use case is that i want to try a replace function with random structs.
And with a NotDeepEqual its pretty easy to check if the struct has been replaced or not.

@RangelReale
Copy link

Got this same problem while using is.Contains, I want "not contains", but there is no way of inverting the check.
I'm converting from this testify code:

    require.NotContains(t, fields, "tag_id")

@dkrieger
Copy link

now that generics exist, could this be revisited? lacking a not modifier is a bizarre limitation, understandable based on language limitations and lib design decisions, but if it's possible to do generically now, I'd submit that there's no good reason not to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants