EqualExportedValues: Support nested structs and time.Time fields

[feidtmb]

Summary

Updates the new EqualExportedValues function to support comparing nested struct fields and time.Time values. The function explicitly no longer supports recursive data structures (see this since removed test case for example).

Changes

#1309 introduced EqualExportedValues. I was looking to add support for comparing structs containing time.Time fields, and this seemed like an ideal function to adjust for that since:

1. The explicit comparison of only exported values suggests the caller doesn't want a typical deep equality check, so comparing times using time.Time.Equal is likely more in line with the desired behavior.
2. EqualExportedValues is not yet in a tagged release, and so this behavior change should not affect many real users.
3. Past PRs have tried to add time.Time equality support to functions like assert.Equal, but that task was found to be more complicated (for example, see Fix compairsion of struct containing time.Time.  #985). Adding this support to only EqualExportedValues is more easily achieved.

Motivation

There is currently no good method for comparing structs that contain time.Time fields that don't have matching timezones or monotonic clock readings.

Example Usage:

type Q struct {
	TS *time.Time
}
type R struct {
	Nested Q
}
type S struct {
	Nested R
	CreatedAt time.Time
}

t1 := time.Date(2023, time.April, 7, 12, 56, 32, 0, time.UTC)
t2 := time.Date(2023, time.April, 7, 7, 56, 32, 0, time.FixedZone("UTC-5", -5*60*60))

s1 := S{Nested: R{Nested: Q{TS: &t1}}, CreatedAt: t1}
s2 := S{Nested: R{Nested: Q{TS: &t2}}, CreatedAt: t2}

ObjectsExportedFieldsAreEqual(S1, S2) => true

Related issues

Potentially closes #984, assuming we're okay with requiring use of EqualExportedValues to compare structs with time.Time fields and don't expect assert.Equal to handle that.

[feidtmb]

CC @mchlp since this proposes changes to your new functions from #1309.

[feidtmb]

Of course right after I undraft this I realize recursive structs need to be handled...

Looking into that and will undraft again when ready.

[HaraldNordgren]

Hi @feidtmb, I was also looking into the issues of the recursive structs (my entry point was the issues with pointers and slices). Here is a fix I proposed, if you want to take a look: #1379

If you think it makes sense, I we could move the time.Time logic in there too another case in the switch logic 🤗

[mchlp]

I think EqualExportedValues is the right place to put the time comparisons with time.Time.Equal. However, maybe EqualExportedValues would no longer be an accurate name for the function since we're doing more than just comparing the exported values. Maybe EqualPublicInterface?

[feidtmb]

@HaraldNordgren I left a review on your PR. It looks good to me, thanks for tackling the recursive structures problem!

I've removed the expectation for this PR to handle recursive data structures, and will leave it as a draft for now with the assumption that your PR will be merged instead. Once that's done I'll either close this or adjust it to handle only time.Time equality like you mentioned.

[feidtmb]

@HaraldNordgren Looking over your PR again, while I do think your approach is more complete than this one since it handles maps and arrays, I don't think it's handling recursive data structures. (See this test case.)

Just wanted to call that out since I was mistaken in my comment above. (I left a similar comment on your PR.) Still happy to proceed with your solution though.

[HaraldNordgren]

Hi @feidtmb!

In a real scenario in don’t see a case where there are objects referencing themselves. I don’t think I have ever seen them in production code. This I why I wouldn't want to add a comment explaining the lack of this. But maybe I’m wrong?

Regarding a possible solution to it: One level of recursion should be simple to solve. But if we have multiple levels of data before the structure refers back to top level, then it becomes difficult to solve, where we have to keep track of all previous levels. Probably not impossible, but much more complex to write.

If we want to do it, maybe all of this is better suited for a follow-up PR. What do you think? 🤗