Fix compairsion of struct containing time.Time.

[leoleoasd]

Summary

#979 fixed comparison of time.Time, but struct containing time.Time still can't be compared correctly because still, we will use reflect.DeepEqual.
This PR is trying to fix it.

Related issues

Closes #984

[leoleoasd]

I have a workaround here, but it only works with "struct containing time.Time", not with "struct containing struct containing time.Time".

func ObjectsAreEqual(expected, actual interface{}) bool {
	if expected == nil || actual == nil {
		return expected == actual
	}

	switch expected.(type) {
	case time.Time:
		if tAct, ok := actual.(time.Time); ok {
			return expected.(time.Time).Equal(tAct)
		}
		return false
	case []byte:
		if act, ok := actual.([]byte); ok {
			exp := expected.([]byte)
			if exp == nil || act == nil {
				return exp == nil && act == nil
			}
			return bytes.Equal(exp, act)
		}
		return false
	}
	v1 := reflect.ValueOf(expected)
	v2 := reflect.ValueOf(actual)
	if v1.Kind() == reflect.Struct {
		if !v1.IsValid() || !v2.IsValid() {
			return v1.IsValid() == v2.IsValid()
		}
		if v1.Type() != v2.Type() {
			return false
		}
		// Cause we won't call ObjectsAreEqual again
		// So params like the "visited" param of reflect.deepValueEqual is not required.
		for i, n := 0, v1.NumField(); i < n; i++ {
			timeV1, ok1 := v1.Field(i).Interface().(time.Time)
			timeV2, ok2 := v2.Field(i).Interface().(time.Time)
			if ok1 && ok2 {
				if !timeV1.Equal(timeV2) {
					return false
				}
			} else {
				if !reflect.DeepEqual(v1.Field(i).Interface(), v2.Field(i).Interface()) {
					return false
				}
			}
		}
		return true
	}
	return reflect.DeepEqual(expected, actual)
}

If we call ObjectsAreEqual again on each field, we should deal with recursive types that the visited parameter of reflect.deepValueEqual are used for, thus we need a new param of this function.

[boyan-soubachov]

I see. I'll revert the merged commit then to give us more room to work out a better approach.

[leoleoasd]

It occurs to me that the building the visited map requires an unexported field of reflect.Value.

[boyan-soubachov]

I've reverted the previous merge. No rush, feel free to scope out a better solution should you wish :)

[leoleoasd]

@boyan-soubachov Added a workaround to deal with non-recursive types. I think we can do nothing about recursive types, cause we can't look into the unexported field of reflect.Value.

Something weird is that when I'm running TestObjectsAreEqual_StructContainingTime only, it will succeed, but if I run all tests together, it will fail.

Also, is it normal for the tests of the "suite" package kept failing?

[leoleoasd]

And......
The tests running from travis-ci succeed.....
I'm using a mac, my go version is go version go1.14.4 darwin/amd64.

go test ./assert/... kept failing, but running only the TestObjectsAreEqual_StructContainingTime test from Goland IDE kept succeed. I've tried multiple times with the same result.

Update: running all tests from go version go1.14.6 linux/amd64 succeed.
Update: this is an issue of valueMaybeRecursive, and my local time is 2am now. I've come up with a fix and I'll write them tomorrow.

[leoleoasd]

@boyan-soubachov ready for review.

[boyan-soubachov]

Hmm. I'm not sure the complexity and safety (the use of unsafe pointers) is worth the benefits. We'll be converting to use go-cmp in testify v2 so for a quick-fix this seems a bit too much.

Thank you for your time and effort but I think it's best we hold off on this. Thoughts?

[leoleoasd]

Hmm. I'm not sure the complexity and safety (the use of unsafe pointers) is worth the benefits.

The unsafe pointers are only used to prevent dead-recurse. I'm using it exactly the same way which reflect.DeepEqual uses it, so it should be safe.

Thank you for your time and effort but I think it's best we hold off on this. Thoughts?

For most cases(and in my case), support for "struct containing time.Time" is fairly enough. So, maybe this fix is acceptable?
Of course, it needs some modification to support un-addressable variables.

[brackendawson]

For the same reasons in #1536 we cannot deviate from reflect.DeepEqual on time.Time objects in v1.