Improve list/map support for ObjectsAreEqual #1058
Conversation
Function `ObjectsAreEqual` now compares two objects of type array/slice/map by iterating over each element. When two objects contain the same value but have different types, the function will now return `true`.
dolmen
added
pkg-assert
|
Hi @harryzcy, Thanks for your fix. Do you have any idea when this fix will be released? |
No, I don't know when maintainers will review them. |
There was a problem hiding this comment.
This is a similar change to #1586 and I think it can be a welcome one.
Currently the doc for EqualValues says:
EqualValues asserts that two objects are equal or convertible to the larger type and equal.
That would need to change because this behavior is quite different. Probably a second sentence describing the behavior for lists and maps. Though I've often been hesitant to have a function with different behavior based on the value of its arguments. Would it be better to have a new assertion for this? Genuine question as I'm undecided.
Apologies for the delay in this review. I advise you read my last comment first.
| expectedValue := reflect.ValueOf(expected) | ||
| actualValue := reflect.ValueOf(actual) |
There was a problem hiding this comment.
These reflect calls aren't needed until after the length checking, can we move them down.
| @@ -174,6 +174,17 @@ func ObjectsAreEqualValues(expected, actual interface{}) bool { | |||
|
|
|||
| expectedType := expectedValue.Type() | |||
| actualType := actualValue.Type() | |||
|
|
|||
| if expectedType.Kind() == actualType.Kind() { | |||
There was a problem hiding this comment.
I think in the spirit of EqualValues, array and slice should also pass:
assert.EqualValues(t, []interface{}{1}, [1]interface{}{int64(1)})But this comparison prevents it.
| @@ -202,6 +213,68 @@ func isNumericType(t reflect.Type) bool { | |||
| return t.Kind() >= reflect.Int && t.Kind() <= reflect.Complex128 | |||
| } | |||
|
|
|||
| // listsAreEqualValues gets whether two lists(arrays, slices) are equal, or if their | |||
There was a problem hiding this comment.
It doesn't get if lists are equal because lists aren't comparable.
| // listsAreEqualValues gets whether two lists(arrays, slices) are equal, or if their | |
| // listsAreEqualValues returns true if the expected and actual lists (arrays or slices) are the same length, and each index in both lists is convertible to the larger type and equal. |
| return true | ||
| } | ||
|
|
||
| // mapsAreEqualValues gets whether two maps are equal, or if their |
There was a problem hiding this comment.
As above:
| // mapsAreEqualValues gets whether two maps are equal, or if their | |
| // mapsAreEqualValues returns true if all the keys in the expected and actual maps are convertible to the larger type and equal. |
| // Key types should be convertible | ||
| expectedKeyType := expectedValue.Type().Key() | ||
| actualKeyType := actualValue.Type().Key() | ||
| if !expectedKeyType.ConvertibleTo(actualKeyType) { |
There was a problem hiding this comment.
Is it wise to attempt the EqualValues algorithm on they keys as well?
I notice this fails. I'm guessing interfaces aren't convertible.
assert.EqualValues(t, map[any]any{1: 1}, map[any]any{int64(1): 1}) // falseMore concerning is this case:
i := uint32(math.MaxUint32)
i++
a := map[uint32]int{
1: 1,
i: 2,
}
b := map[uint64]int{
1: 1,
math.MaxUint32 + 1: 2,
}
assert.EqualValues(t, a, b) // false
assert.EqualValues(t, b, a) // trueThe same comparison in reverse has the opposite outcome. Unless I'm mistaken you can't solve this, the key must always be a direct comparison.
Summary
Function
ObjectsAreEqualnow compares two objects of type array/slice/map by iterating over each element. When two objects contain the same value but have different types, the function will now returntrue.Changes
ObjectsAreEqualValuesinassertion.gois changed.listsAreEqualValuesandmapsAreEqualValuesare added.Motivation
Currently,
assert.EqualValueswould fail when two objects are of type array/slice/map and contain values of different types.But,
assert.EqualValuesshould always compare objects by value. So the above code should idealy pass.Related issues
Closes #1057