cc @fabriziopandini @killianmuldoon @ykakarap

Along the same lines, In our tests I would recommend switching to BeComparableTo from Equal when comparing maps, objects and other relevant types. In case of failure Equal just prints the 2 objects and we have to manually identify the differences. BeComparableTo produces a much more human readable output with a clearer diff.

BeComparableTo internally uses go-cmp.

Good point, we have a separate issue for that one: #8095

/triage accepted

/assign

Took a closer look at go-cmp. For me it seems like a good option:

• README sounds good to me: https://github.com/google/go-cmp
• godoc sounds good to me: https://github.com/google/go-cmp/blob/master/cmp/compare.go#L93
• Gomega is using it in the BeComparableTo matcher
• We already have a dependency to go-cmp through Gomega and because we use it directly in tests

If someone wants to consider other alternatives, now would be the time to bring them up :)

I'm +1 for replacing it in tests, but I'm not sure about doing so in non-test code.

From the go-doc:

// It is intended to only be used in tests, as performance is not a goal and
// it may panic if it cannot compare the values. Its propensity towards
// panicking means that its unsuitable for production environments where a
// spurious panic may be fatal.

That means that possibly we should wrap our equality checks in a util package and maybe use go-cmp underneath. If we do this we can catch panics and return them as errors. WDYT?

Sounds reasonable. gomega also catches panics from go-cmp in BeComparableTo.

I don't think performance will be an issue with the things we compare (but I don't have data)

Yeah - I'm not very worried about performance. I also think we shouldn't have things that can't be compared, given we're almost always going to be comparing the same types, but catching the panics and turning them into errors is still a useful backstop.

/reopen

This issue covers replacing the reflect DeepEqual in more places across the codebase than #8266

@killianmuldoon: Reopened this issue.

Unlike reflect.DeepEqual, unexported fields are not compared by default; they result in panics unless suppressed by using an Ignore option (see cmpopts.IgnoreUnexported) or explicitly compared using the Exporter option.

This is probably something that we keep an eye one when moving to cmp.Diff and BeComparableTo. There might be some places in our tests were we inherently compare the unexported fields (I don't imagine there will be a lot as we mostly just compare API types and all those fields are exported) and we should be careful when switching over.

Definitely more an issue for tests, in prod code we should keep it in mind but I would expect it not be an issue as you said.

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

/priority important-longterm

/assign @sbueringer
To re-assess