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.

Sign up for GitHub

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

Improve list/map support for ObjectsAreEqual #1058

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Improve list/map support for ObjectsAreEqual #1058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9e8f918
Improve list/map support for ObjectsAreEqual
harryzcy Mar 1, 2021
8568f4a
Merge branch 'master' into issue1057
harryzcy Sep 9, 2023
70077d0
Merge branch 'master' into issue1057
harryzcy Oct 25, 2023
467f7d5
Merge branch 'master' into issue1057
harryzcy Jan 15, 2024
18d91fc
Merge branch 'master' into issue1057
harryzcy Mar 3, 2024
8372880
Fix issues after merge
harryzcy Mar 3, 2024
92f39b3
Merge branch 'master' into issue1057
harryzcy Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions assert/assertions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,17 @@ func ObjectsAreEqualValues(expected, actual interface{}) bool {

expectedType := expectedValue.Type()
actualType := actualValue.Type()

if expectedType.Kind() == actualType.Kind() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Test when object type is array/slice/map
switch actualType.Kind() {
case reflect.Array, reflect.Slice:
return listsAreEqualValues(expected, actual)
case reflect.Map:
return mapsAreEqualValues(expected, actual)
}
}

if !expectedType.ConvertibleTo(actualType) {
return false
}
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't get if lists are equal because lists aren't comparable.

Suggested change
// 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.

// values are equal.
//
// This function should only be used by ObjectsAreEqualValues.
func listsAreEqualValues(expected, actual interface{}) bool {
expectedValue := reflect.ValueOf(expected)
actualValue := reflect.ValueOf(actual)
Comment on lines +221 to +222
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These reflect calls aren't needed until after the length checking, can we move them down.


// Assure two objects have the same length
expectedLen, expectedOK := getLen(expected)
actualLen, actualOK := getLen(actual)
if !expectedOK || !actualOK {
return false
}
if expectedLen != actualLen {
return false
}

// Iterate over elements and compare
for i := 0; i < expectedLen; i++ {
if !ObjectsAreEqualValues(expectedValue.Index(i).Interface(), actualValue.Index(i).Interface()) {
return false
}
}
return true
}

// mapsAreEqualValues gets whether two maps are equal, or if their
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above:

Suggested change
// 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.

// values are equal.
//
// This function should only be used by ObjectsAreEqualValues.
func mapsAreEqualValues(expected, actual interface{}) bool {
expectedValue := reflect.ValueOf(expected)
actualValue := reflect.ValueOf(actual)

expectedKeys := expectedValue.MapKeys()
actualKeys := actualValue.MapKeys()
if len(expectedKeys) != len(actualKeys) {
return false
}

// Key types should be convertible
expectedKeyType := expectedValue.Type().Key()
actualKeyType := actualValue.Type().Key()
if !expectedKeyType.ConvertibleTo(actualKeyType) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}) // false

More 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) // true

The 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.

return false
}

for _, expectedKey := range expectedKeys {
actualKey := expectedKey.Convert(actualKeyType)
expectedElem := expectedValue.MapIndex(expectedKey)
actualElem := actualValue.MapIndex(actualKey)
if !actualElem.IsValid() { // if key doesn't exist
return false
}
if !ObjectsAreEqualValues(expectedElem.Interface(), actualElem.Interface()) {
return false
}
}
return true
}

/* CallerInfo is necessary because the assert functions use the testing object
internally, causing it to print the file:line of the assert method, rather than where
the problem actually occurred in calling code.*/
Expand Down
4 changes: 4 additions & 0 deletions assert/assertions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ func TestObjectsAreEqual(t *testing.T) {
{123.5, 123.5, true},
{[]byte("Hello World"), []byte("Hello World"), true},
{nil, nil, true},
{[]interface{}{1}, []interface{}{1}, true},

// cases that are expected not to be equal
{map[int]int{5: 10}, map[int]int{10: 20}, false},
Expand Down Expand Up @@ -160,6 +161,9 @@ func TestObjectsAreEqualValues(t *testing.T) {
{3.14, complex128(1e+100 + 1e+100i), false},
{complex128(1e+10 + 1e+10i), complex64(1e+10 + 1e+10i), true},
{complex64(1e+10 + 1e+10i), complex128(1e+10 + 1e+10i), true},
{map[string]interface{}{"1": int32(1)}, map[string]interface{}{"1": int64(1)}, true},
{map[int]interface{}{1: int32(1)}, map[int64]interface{}{1: int64(1)}, true},
{map[interface{}]interface{}{1: int32(1)}, map[interface{}]interface{}{"1": int64(1)}, false},
}

for _, c := range cases {
Expand Down