assert.Equal randomly fails testing maps

[phemmer]

assert.Equal (and other functions) will randomly fail testing equality of maps with the same values but different type.

For example:

package main

import (
    "fmt"
    "github.com/stretchr/testify/assert"
    "testing"
)

func TestExactly(t *testing.T) {
    map1 := map[string]interface{}{"foo": 1234, "bar": int16(4321)}
    map2 := map[string]interface{}{"foo": 1234, "bar": int32(4321)}

    for i := 0; i < 3; i++ {
        if assert.Equal(t, map1, map2) {
            fmt.Printf("Everything is ok\n")
        } else {
            fmt.Printf("Everything is NOT OK\n")
        }
    }
}

Results in:

=== RUN TestExactly
Everything is NOT OK
Everything is ok
Everything is NOT OK
--- FAIL: TestExactly (0.00s)
    Location:   main_test.go:14
    Error:      Not equal: map[string]interface {}{"foo":1234, "bar":4321} (expected)
                    != map[string]interface {}{"foo":1234, "bar":4321} (actual)

    Location:   main_test.go:14
    Error:      Not equal: map[string]interface {}{"foo":1234, "bar":4321} (expected)
                    != map[string]interface {}{"bar":4321, "foo":1234} (actual)

FAIL
exit status 1
FAIL    _/tmp/test6 0.012s

The issue is because the type on bar is different between the maps (int16 in one, int32 in the other).
ObjectsAreEqual uses reflect.DeepEqual which properly detects this, but then it continues to check equality of a fmt.Sprintf("%#v",...) on the values, and this randomly fails depending on the order it prints the map values in.

The solution seems simple: remove the fmt.Sprintf() check. But I wasn't sure why that was in there to begin with.

[benma]

I agree, it sounds like a bad idea to compare string representations. This is not robust enough.

Why not simply return reflect.DeepEqual(expected, actual) here?

[OttoAllmendinger]

👍 remove fmt.Sprintf() check. The comparison makes it difficult to reason about the (Not)Equal methods.

For example, the %#v verb checks if the the GoStringer interface is implemented. Considering two values as equal because their string representation is equal is unexpected behavior in my opinion.

[benma]

@tylerb, @matryer, any decision on this? Can we remove the fmt.Sprintf("%#v",...) check?

[tylerstillwater]

I'm fine with this, though I am not sure if there are tests that rely on this behavior. Care to submit a PR with it removed and tests passing?

[cryptix]

I am not sure if there are tests that rely on this behavior.

Aren't such tests broken by design since the iteration over the keys is intentionally random?

[tylerstillwater]

I am not sure either. That portion was written ages ago when we were just learning about the best way to compare objects. I'll give it a look.

The tests may be broken by design, yes.

[tylerstillwater]

It seems the only test that depends on this is a test comparing functions. See https://github.com/stretchr/testify/blob/master/assert/assertions_test.go#L119

@cryptix @benma @OttoAllmendinger @phemmer this will change behavior when it comes to comparing functions. I am fine with this change myself, as I never do that.

Thoughts?

[phemmer]

We could add an explicit check to see if the 2 items being compared are functions, and if so manually check them for equality (instead of deferring to reflect.DeepEqual()). However this would only work if the arguments are functions, and not if they're nested in an array or map. I can't think of any (simple) way to reliably test in that case.

[tylerstillwater]

A simple

if reflect.ValueOf(expected) == reflect.ValueOf(actual) {
    return true
}

made that test pass. I'm comfortable with that.

[evanphx]

@tylerb I didn't think that comparing reflected Values with == made sense. The godocs indicate that as I recall. You're just testing how reflect produces an internal value struct.

[tylerstillwater]

You're right. From this post on SO it seems this is probably something we shouldn't even try to do. I won't miss it myself.

[evanphx]

Yeah, I'd just return the value from DeepEqual and call it a day. Side note: removing the sprintf comparison broke a whole shit-ton of tests in my suite because the types of numerics is always checked now. My tests are the reason for the breakage, so no biggy.

But I wonder if maybe, because a literal numeric is boxed into an interface{} as an int or float32, maybe the code should, if the expected value is either of those, try to check if the values are equal by casting both values to int64 and float64 respectively and then comparing. That would cleanup code using literals numerics. Thoughts?

[evanphx]

At the very least, when reporting the 2 values are not equal, the types of the values needs to shown too, to avoid output like this:

    Error:      Not equal: 12 (expected)
                    != 12 (actual)

[tylerstillwater]

@evanphx Right. This is something that could be added, certainly. For example, in my other testing framework, is (I don't use testify anymore), I do:

aValue := reflect.ValueOf(a)
bValue := reflect.ValueOf(b)
// Convert types and compare
if bValue.Type().ConvertibleTo(aValue.Type()) {
    return reflect.DeepEqual(a, bValue.Convert(aValue.Type()).Interface())
}

to handle these cases. This could be added to address your issue. Thoughts?

[evanphx]

Yeah, that would definitely work.

[tylerstillwater]

It looks like the ObjectsAreEqualValues function already implements this. It isn't used by the standard assertion methods though. I seem to recall there was an issue/debate a bit ago about this automatic conversion approach and that function was added to address it.

[deiu]

Why has this issue been marked as closed? I'm still getting errors when comparing int to int64. Also, @evanphx is right. Errors like Not equal: 523020 (expected) != 523020 (actual) are really annoying. :-)

[tylerstillwater]

I would recommend opening a new issue for this so it can be discussed and a proper solution implemented.

On Thu, Apr 16, 2015 at 4:43 PM, Andrei notifications@github.com wrote:

Why has this issue been marked as closed? I'm still getting errors when comparing int to int64. Also, @evanphx is right. Errors like Not equal: 523020 (expected) != 523020 (actual) are really annoying. :-)

Reply to this email directly or view it on GitHub:
#143 (comment)

[james-maguire]

Am I correct in my understanding that this was never resolved?

[MarcoSantarossa]

This is still happening in the latest library version. The tests pass in macOS but fails in linux. Can someone have a look on this please ?

[ButcherWithSmile]

The presence of fmt.Sprintf() check after reflect.DeepEqual() in testing libraries like assert.Equal() is meant to provide additional context in case of failures. It's used to generate a human-readable representation of the values being compared, so that developers can easily see what's different when the comparison fails.

In your example, the issue arises due to the random order in which the map values are printed by fmt.Sprintf(). Since maps in Go do not have a guaranteed order for iteration, the order of key-value pairs can vary, resulting in different string representations even when the actual values are equivalent.

If you're encountering issues with such random failures, one approach could be to explicitly convert the maps to a consistent format before comparison. For instance, you could convert the maps to JSON strings and then compare those strings. This ensures a consistent representation for comparison, regardless of the internal order of the map's elements.

Here's how you might modify your test function:

import (
    "encoding/json"
    // ... other imports
)

func TestExactly(t *testing.T) {
    map1 := map[string]interface{}{"foo": 1234, "bar": int16(4321)}
    map2 := map[string]interface{}{"foo": 1234, "bar": int32(4321)}

    jsonMap1, _ := json.Marshal(map1)
    jsonMap2, _ := json.Marshal(map2)

    if assert.Equal(t, string(jsonMap1), string(jsonMap2)) {
        fmt.Printf("Everything is ok\n")
    } else {
        fmt.Printf("Everything is NOT OK\n")
    }
}

This modification ensures that the order of map elements doesn't affect the comparison, and you'll get consistent results even if the map representations are different due to type variations.