Improve default failed equality match output to more clearly show diffs

adamvictorclever commented 2 years ago

Requested feature Sometimes I'll generate a mock and EXPECT some calls. In cases where the test fails because the mock was called with a different input, it can be very hard to tell what the difference is. This is especially apparent in cases where some fields are pointers.

I've tried to set up an example in Go Playground to show what I mean: https://go.dev/play/p/pVxqpZpqiwz

Basically, I have a struct that looks like

type Obo struct {
    a *string
    b *bool
    c []*Obo
}

and an interface I generate a mock for that looks like

type TestThing interface {
    DoThing(obo Obo)
}

In my TestWithMock case, I expect a call with a certain Obo and provide a call with a slightly different one, causing a test failure. The only difference between the two provided is that the first one has an a value set to &"apple" and the second set to &"banana".

Here's the output I see when the test fails:

=== RUN   TestWithMock
    prog.go:25: Unexpected call to *main.MockTestThing.DoThing([{0xc0000a6f50 0xc0000c2998 [0xc0000a9440]}]) at /tmp/sandbox1909100993/prog.go:25 because: 
        expected call at /tmp/sandbox1909100993/prog.go:24 doesn't match the argument at index 0.
        Got: {0xc0000a6f50 0xc0000c2998 [0xc0000a9440]} (main.Obo)
        Want: is equal to {0xc0000a6ef0 0xc0000c2988 [0xc0000a93b0]} (main.Obo)
    controller.go:137: missing call(s) to *main.MockTestThing.DoThing(is equal to {0xc0000a6ef0 0xc0000c2988 [0xc0000a93b0]} (main.Obo)) /tmp/sandbox1909100993/prog.go:24
    controller.go:137: aborting test due to missing call(s)
--- FAIL: TestWithMock (0.00s)

In reality, the diff is quite small, just a single field with a different value. However, the output from the failure provides basically no information on this mismatch. Every field appears different because the only thing that it displays is the pointer, not the value. This makes the issue much more difficult to debug!

Compare this to the output of TestWithAssert, a similar case that directly compares the two Obo objects using assert.Equal from the github.com/stretchr/testify/assert package.

=== RUN   TestWithAssert
    prog.go:29: 
            Error Trace:    prog.go:29
            Error:          Not equal: 
                            expected: main.Obo{a:(*string)(0xc000184000), b:(*bool)(0xc000180008), c:[]*main.Obo{(*main.Obo)(0xc000188000)}}
                            actual  : main.Obo{a:(*string)(0xc000184020), b:(*bool)(0xc000180009), c:[]*main.Obo{(*main.Obo)(0xc000188030)}}

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1,3 +1,3 @@
                             (main.Obo) {
                            - a: (*string)((len=5) "apple"),
                            + a: (*string)((len=6) "banana"),
                              b: (*bool)(true),
            Test:           TestWithAssert
--- FAIL: TestWithAssert (0.00s)

With this output, the nice diff immediately makes it clear what the difference was between the two structs, vastly simplifying my debugging experience!

I believe this is related to https://github.com/golang/mock/issues/265, which was closed. If I understand correctly, the proposed solution would be to use WantFormatter and GotFormatter to generate a nicer output myself. Personally, having never used these features before, it feels like a lot of effort for a simple and common problem! Would I need to use these in every test case I have? I think it would be much more helpful to improve the default output when using an equality matcher.

(Optional) Proposed solution Some potential ideas for how I'd like to see the output improved

When outputting a failure, show the value for pointer fields. For example, something vaguely like
```
Got: {0xc0000a6f50("apple") ...} (main.Obo)
Want: is equal to {0xc0000a6ef0("banana") ...} (main.Obo)
```
This would make it easier to manually inspect and see what the difference is
Output a diff, like the one included by the assert function in my example above. This is even better since it makes it really easy to tell the difference.

palsivertsen commented 2 years ago

I'm also running into this issue a lot. It would be great if the library exposed a way to set a diff function with a signature like type DiffFunc func(a, b interface{}) string. That way we could easily control how the diff is printed.

When a test fails and the got/want output is difficult to read, I like to use the following trick to get a pretty diff. It's not an elegant solution, especially when wrapping several values, but it works for debugging purposes.

example.go

```go package example //go:generate mockgen -destination mocks/$GOFILE -package ${GOPACKAGE}_mocks -source $GOFILE type Foo struct { Bar *string } type Fooer interface { DoSomething(Foo) } ```

example_test.go

```go package example_test import ( "example" example_mocks "example/mocks" "log" "testing" "github.com/golang/mock/gomock" "github.com/kr/pretty" ) func String(s string) *string { return &s } func TestFoo_Standard(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockFooer := example_mocks.NewMockFooer(ctrl) mockFooer.EXPECT(). DoSomething( example.Foo{Bar: String("expected")}, ) mockFooer.DoSomething(example.Foo{ Bar: String("actual"), }) } func TestFoo_Improved(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockFooer := example_mocks.NewMockFooer(ctrl) mockFooer.EXPECT(). DoSomething( &diffWrapper{ v: example.Foo{Bar: String("expected")}, }, ) mockFooer.DoSomething(example.Foo{ Bar: String("actual"), }) } type diffWrapper struct { v interface{} } // Implement the gomock.Matcher interface func (m *diffWrapper) Matches(v interface{}) bool { ok := gomock.Eq(m.v).Matches(v) // wrap the gomock.Eq matcher if !ok { // print the diff log.Printf("This is the diff: %s", pretty.Diff(m.v, v)) // any diff func here } return ok // return the result of the match } // Implement the gomock.Matcher interface func (m *diffWrapper) String() string { return gomock.Eq(m.v).String() // again wrap the gomock.Eq matcher } ```

When running the TestFoo_Improved test I get the diff printed above the test output like so:

2022/02/15 12:19:02 This is the diff: [Bar: "expected" != "actual"]
--- FAIL: TestFoo_Improved (0.00s)
    example_test.go:42: Unexpected call to *example_mocks.MockFooer.DoSomething([{0xc00004a590}]) at /tmp/tmp.Rxm4mU8eW1/example_test.go:42 because: 
        expected call at /tmp/tmp.Rxm4mU8eW1/example_test.go:36 doesn't match the argument at index 0.
        Got: {0xc00004a590} (example.Foo)
        Want: is equal to {0xc00004a530} (example.Foo)
    controller.go:269: missing call(s) to *example_mocks.MockFooer.DoSomething(is equal to {0xc00004a530} (example.Foo)) /tmp/tmp.Rxm4mU8eW1/example_test.go:36
    controller.go:269: aborting test due to missing call(s)
FAIL
exit status 1
FAIL    example 0.002s

alnvdl commented 2 years ago

While not perfect (and quite hackish to be honest), it's possible to have a diff in the output. This works in v1.6.0 and uses google/go-cmp. It includes a diff while preserving the original gomock output style:

type eqMatcher struct {
    want interface{}
}

func EqMatcher(want interface{}) eqMatcher {
    return eqMatcher{
        want: want,
    }
}

func (eq eqMatcher) Matches(got interface{}) bool {
    return gomock.Eq(eq.want).Matches(got)
}

func (eq eqMatcher) Got(got interface{}) string {
    return fmt.Sprintf("%v (%T)\nDiff (-got +want):\n%s", got, got, strings.TrimSpace(cmp.Diff(got, eq.want)))
}

func (eq eqMatcher) String() string {
    return fmt.Sprintf("%v (%T)\n", eq.want, eq.want)
}

And then just use as you would with any other matcher:

m.EXPECT().DoSomething(EqMatcher(...))

Which outputs:

/home/ubuntu/dev/mockoutput/example.go:18: Unexpected call to *example.MockMyInterface.DoSomething([{1 map[abc:def xyz:mno] [0xc000026780]}]) at /home/ubuntu/dev/mockoutput/example.go:18 because: 
    expected call at /home/ubuntu/dev/mockoutput/example_test.go:42 doesn't match the argument at index 0.
    Got: {1 map[abc:def xyz:mno] [0xc000026780]} (defs.Object)
    Diff (-got +want):
    defs.Object{
        ID: 1,
        Mapping: map[string]string{
            "abc": "def",
    -       "xyz": "mno",
        },
        Subs: []*defs.SubObject{
            &{
    -           When:   s"2021-10-12 01:02:03.000000004 +0000 UTC",
    +           When:   s"2022-10-12 01:02:03.000000004 +0000 UTC",
                Name:   "sub1",
                Things: {"a", "b"},
            },
        },
      }
    Want: {1 map[abc:def] [0xc000026740]} (defs.Object)

But I agree, some optional, native form of diffing (perhaps even using go-cmp?) would be better.

palsivertsen commented 2 years ago

Any chance we can rework the Controller to accept a diff function? Something along the lines of:

ctrl := gomock.NewController(t, gomock.WithDiffFunc(cmp.Diff))

The "got" and "want" formatters could be added in the same way.

SpencerC commented 2 years ago

I think cmp.Diff (-want +got) should be the default behavior. One design consideration is that cmp.Diff is most useful when you can give it options like cmp.AllowUnexported and protocmp.Transform.

@palsivertsen's solution by requiring developers to wrap cmp.Diff in an anonymous function that configures it to their liking. Alternatively:

ctrl := gomock.NewController(t, gomock.WithDiffOpts(cmp.AllowUnexported(&Something{}), protocmp.Transform()))

vitaly-zdanevich commented 2 years ago

Looks like DoAndReturn has better diff.

golang / mock

Improve default failed equality match output to more clearly show diffs #616