Add EqualsWithNaN to testutil
#3
Conversation
I think you mean |
Generally relfect.DeepEqual considers NaN != NaN, but in some specific cases, it might consider NaN == NaN according to an old & long issue thread: golang/go#12025. But yes, generally it considers NaN != NaN. Adding this method to overcome that. Thanks! 🙂 |
merrors/doc.go
Outdated
| // return errs.Err() | ||
| // } | ||
| // | ||
| // func CloseAll(closers []io.Closer) error { |
There was a problem hiding this comment.
You wanted to change that?
There was a problem hiding this comment.
So make lint fails on CI and points to merrors/doc.go being modified. But it passes locally. I then tried to format it via IDE, which changed it this way. Pushed to separate commit to see if this fixes it. I think it is gofmt-ing comments?
The same lint fail is on latest commit to main.
testutil/testutil.go
Outdated
| // EqualsWithNaN fails the test if exp is not equal to act and ensures NaN == NaN. | ||
| // Keep in mind that this implementation cannot check unexported fields in structs on its own | ||
| // and needs setting custom options for it. | ||
| func EqualsWithNaN(tb testing.TB, exp, act interface{}, opts cmp.Options, v ...interface{}) { |
There was a problem hiding this comment.
I think this functionality is a bit too narrow.
I wished we could do Equals(<things>, ...option). Perhaps we could do WithCmpOpts(opts).Equal ? and there define which comparison to using (reflect or go-cmp) and perhaps go cmp opts?
Alternatively EqualsWithOpts - but I think I like that former better. Or even With(opts).Equal is fine too.
There was a problem hiding this comment.
Ack! Will try to modify
There was a problem hiding this comment.
So I updated this to now be testutil.WithCmpOpts(opts).Equals for go-cmp comparison, and tesutil.Equals for reflect.DeepEqual one. I wanted to preserve testutil.Equals as it's super clean and used everywhere right now, whereas the go-cmp one would only be needed in a few specific cases. WDYT? 🙂
If this looks good, I might add some comment explaining this then.
merrors/doc.go
Outdated
| // return errs.Err() | ||
| // } | ||
| // | ||
| // func CloseAll(closers []io.Closer) error { |
testutil/testutil.go
Outdated
| opts cmp.Options | ||
| } | ||
|
|
||
| func WithCmpOpts(opts ...cmp.Option) goCmp { |
There was a problem hiding this comment.
Nice, I think I like this direction, but the details matter.
It's weird that WithCmpOpts feels like those are comparison opts, buts actually those are opts ONLY for specific go-cmp thing. Does it matter if reflect.DeepEqual is used or default opts for go-cmp? - essentially can normal func Equals()... use return WithCmpOpts().Equals..? If not, then perhaps we need something like:
| func WithCmpOpts(opts ...cmp.Option) goCmp { | |
| func WithGoCmp(opts ...cmp.Option) goCmp { |
Also I wonder if it makes sense to do our struct with Options, so we are not blocked when they are changing things or when we want to add extra thing.
I think I would go with sth like WithCmpOpts(opts ... CmpOption) comparer
There was a problem hiding this comment.
So, I agree, that it'd be much cleaner if Equals could switch go-cmp and reflect.DeepEqual based on opts, but go-cmp with default opts and reflect.DeepEqual have some subtle difference (for eg. private struct member comparisons).
I think WithGoCmp might be the cleanest way to do this without breaking testutil.Equals API.
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
Signed-off-by: Saswata Mukherjee <[email protected]>
saswatamcode
force-pushed
the
nan-error
branch
from
2ab8467 to
04e25dc
Compare
| opts cmp.Options | ||
| } | ||
|
|
||
| func WithGoCmp(opts ...cmp.Option) goCmp { |
There was a problem hiding this comment.
Let's also mention in the comment that compatibility guarantee for this function arguments is same as go-cmp, so... zero as it is at version 0.x. It's fine, but let's just mention it in comment
Signed-off-by: Saswata Mukherjee <[email protected]>
This PR adds an
EqualsWithNaNfunction totestutilwhich allows comparing interfaces withNaNvalues (nested or otherwise) by considering NaN == NaN. This is useful for comparing values containing NaN in tests likepromql.Result.It uses
go-cmpto achieve this.reflect.DeepEqualconsiders NaN == NaN for some very specific cases which aren't easy to navigate, so an alternative implementation is necessary here. 🙂The limitation of
EqualsWithNaNis that one would need to pass in special options for unexported struct fields.