Go: Update tests to use cmp.Diff instead of reflect.DeepEqual

[thempatel]

What type of PR is this?

Uncomment one line below and remove others.

Other

What package or component does this PR mostly affect?

language/go

What does this PR do? Why is it needed?

reflect.DeepEqual is a poorer chose when choosing to compare test results. For example, DeepEqual will only compare pointer values using their memory address, rather than the underlying value in the type.

This is a pre-factor to make #1243 easier to review (per review feedback)

Which issues(s) does this PR fix?

Fixes #1099

Other notes for review

[achew22]

I completely agree with the direction of this change, swapping reflect out for cmp, but I think we should be careful to bear in mind Go's Testing Style Guide on this matter (under the heading "Assert Libraries". The "idiomatic go" (I do not like that phrase) way of approaching these things would be to run the cmp.Diff inline with an if statement. Something like

if diff := cmp.Diff(want, got); diff != "" {
  t.Errorf("Path was unequal (-want, +got): %s", diff)
}

I would be very happy to merge a change that used the pattern suggested by the Go team with the cmp package instead of reflect.

[achew22]

Can you help me understand why asserting on all types is the correct thing to do here? What fields do you care about that aren't exported? Are they a thing that actually should be a part of the public API?

[thempatel]

This is to make testing easier on unexported types, cmp.Diff will only compare exported fields by default which is pretty useless if you want to use it to validate your un-exported types in tests.

[thempatel]

I completely agree with the direction of this change, swapping reflect out for cmp, but I think we should be careful to bear in mind Go's Testing Style Guide on this matter (under the heading "Assert Libraries". The "idiomatic go" (I do not like that phrase) way of approaching these things would be to run the cmp.Diff inline with an if statement. Something like

if diff := cmp.Diff(want, got); diff != "" {
  t.Errorf("Path was unequal (-want, +got): %s", diff)
}

I would be very happy to merge a change that used the pattern suggested by the Go team with the cmp package instead of reflect.

I am happy to make the suggested change! For dialogue purposes only: I'm not sure I personally agree with the guidance in that wiki. I've been writing go code professionally for a few years now (not that that's some indication that I've been doing it correctly these past years) and most opinions/idiomatic guidance that go suggests, I normally agree with.

but this either stops the test early (if assert calls t.Fatalf or panic)

I think in most cases, or at least the ones I've predominantly seen, Fatal/Fatalf is the correct choice. Tests are often written procedurally which means that continuing a test after the first error isn't going to test the correct behavior: it's the same reason you return an err the moment you find one, and not all at the end.

If the reason you'd want a test to continue after the first error is so that you can assert on the various values of a returned structure, or multiple values not in a structure, it's almost always better to just assert on the structure itself or put the disparate values in a structure, and use cmp.Diff to tell you want was different. Indeed, this is actually the same guidance in the above TestComments wiki:

If your function returns a struct, don't write test code that performs an individual comparison for each field of the struct.

Go has good support for printing structures

I don't see this as a win if the helpers you write, also just do this for you, like I've done here:

func equals(t *testing.T, expected, actual interface{}, format string, args ...interface{})

Assert libraries make it too easy to write imprecise tests and inevitably end up duplicating features already in the language, like expression evaluation, comparisons, sometimes even more.

I think the irony here is that go-cmp basically breaks the "duplicating features in the language ... like comparisons" and yet the guidance in the same doc is to actually use the library.

I'm all for consistency, and again I am going to make the changes you requested, but I think consistency, inconsistently applied has very little value

[thempatel]

PTAL

[achew22]

Once again, I agree with the primary thrust of this effort, making it easier to debug but disagree with the implementation.

To briefly quote from the Assert Libraries documentation:

Assert libraries make it too easy to write imprecise tests

That is what I think is being violated by this change. It is not clear to me that cmpDiff brings clarity and precision to the tests at hand as evidenced by my comment:

Can you help me understand why asserting on all types is the correct thing to do here? What fields do you care about that aren't exported? Are they a thing that actually should be a part of the public API?

The logic of "why are these fields important to be compared" is completely elided when wrapping comparison in something like cmpDiff. The name doesn't describe that it is comparing all unexported fields and that is why it is unique, nor does it contain some indication that it is specifically targeted at these types in the type signature. It diffs a group of structs but does it in a non-standard way (I think there is a pretty strong case to be made that go-cmp is a part of the go standard library even if not officially yet).

Function composition is one way to solve problems, but there are many others. The go-cmp package is built around the notion of functional options and cmp.Diff takes a repeated Option, which I believe the authors intends to be the preferred mode of extending functionality. cmpDiff uses this functionality via Exporter to achieve that goal.

To quote from the go-cmp documentation on Exporters:

In many cases, a custom Comparer should be used instead that defines equality as a function of the public API of a type rather than the underlying unexported implementation.

For example, the reflect.Type documentation defines equality to be determined by the == operator on the interface (essentially performing a shallow pointer comparison) and most attempts to compare *regexp.Regexp types are interested in only checking that the regular expression strings are equal. Both of these are accomplished using Comparers:

Comparer(func(x, y reflect.Type) bool { return x == y })
Comparer(func(x, y *regexp.Regexp) bool { return x.String() == y.String() })

In other cases, the cmpopts.IgnoreUnexported option can be used to ignore all unexported fields on specified struct types.

My preference would be to heed the advice of go-cmp, and define reusable Equals functions on the various types you're interested in comparing. Something like this:

https://go.dev/play/p/02kN3A7712u

It advances the objective of making explicit that you're, in this case, only going to succeed if you're comparing foo objects, and it provides a useful method for people working in this library to compare equality of foo. In addition it affords the caller the continued ability to pass more options.

If you don't think this is the correct course, instead believing that the correct move is to export all the fields, I would much prefer you used the built in system for achieving such ends, AllowUnexported. The idea behind this is that you're selectively allowing individual types to be exported instead of just letting the comparison engine go to town.

https://go.dev/play/p/z7fpCgve66T

What do you think about these options?

[thempatel]

It advances the objective of making explicit that you're, in this case, only going to succeed if you're comparing foo objects, and it provides a useful method for people working in this library to compare equality of foo. In addition it affords the caller the continued ability to pass more options.

Your example stops the diffing at the level for which you've defined the comparator. Example:

Diff (-want,+got):
  main.foo(
- 	{a: "a", b: "b", c: "c"},
+ 	{a: "1", b: "b", c: "c"},
  )

versus using AllowUnexported or the Exporter

Diff (-want,+got):
  main.foo{
- 	a: "a",
+ 	a: "1",
  	b: "b",
  	c: "c",
  }

Before I continue further, I'm curious to get your take on which output you prefer: 1 or 2? Ultimately I don't have to work in this repository in the day-to-day, so I am less motivated to advocate for QOL improvements that I will not be able to enjoy.

[achew22]

#2 is probably what I'd do, it has less boilerplate/

[thempatel]

@achew22 PTAL

[achew22]

This is perfect! Thank you so much for your contribution!

[thempatel]

sweet, thanks for the review!