Use go-cmp instead of reflect.DeepEqual

[hslatman]

As discussed in https://github.com/cweill/gotests/pull/99, the branch had to be cleared from merge conflicts before go-cmp support could be integrated. That's what I did in this PR.

[googlebot]

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

[hslatman]

@googlebot I consent.

[coveralls]

Coverage increased (+0.06%) to 95.678% when pulling 0a209c7c7885cab45126d8ee675a76b6bd312fd6 on hslatman:go-cmp into 90387e1ec4df7885052899c78aea13cf842c0143 on cweill:develop.

[butuzov]

Hi @hslatman can you merge develop into your feature? I believe I brought some conflicts to your code.

[hslatman]

@butuzov I've merged develop and regenerated the bindata.

While the tests run successfully on GitHub Actions I have two failing tests locally. Is there something special I need to do to to run the tests locally successfully too?

[butuzov]

I would suggest do not run them with -race or concurrently atm. I have also run tests and non failed.

@cweill can you also check this PR?

[butuzov]

ping @cweill

[cweill]

@mekegi Could you please reply @googlebot I consent. to this PR?

[mekegi]

@googlebot I consent

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[hslatman]

I'm curious to know if the output format is OK for you as it is now. It looks like this now:

        diff=  (*client.Client)(
        -   &{
        -       key:               "key",
        -       httpClient:        &http.Client{Timeout: s"30s"},
        -       baseURL:           s"https://example.com/api",
        -       contentTypeHeader: "application/json",
        -       acceptHeader:      "application/json",
        -   },
        +   nil,
          )

One other thing that I found out about is that having unexported fields in structs will result in panics when running tests if they are not explicitly allowed, as mentioned in the third point in the go-cmp repo:

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 AllowUnexported option.

One would thus have to change the generated !cmp.Equal(got, tt.want) and cmp.Diff(got, tt.want) to something like the below (or one of the other options mentioned above):

if !cmp.Equal(got, tt.want, cmpopts.IgnoreUnexported(StructUnderTest{})) {
    t.Errorf("%q. New() = %v, want %v\ndiff=%s", tt.name, got, tt.want, cmp.Diff(got, tt.want, cmpopts.IgnoreUnexported(StructUnderTest{})))
}

[cweill]

I'm curious to know if the output format is OK for you as it is now. It looks like this now:

Looks good to me.

One other thing that I found out about is that having unexported fields in structs will result in panics when running tests if they are not explicitly allowed, as mentioned in the third point in the go-cmp repo:

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 AllowUnexported option.

One would thus have to change the generated !cmp.Equal(got, tt.want) and cmp.Diff(got, tt.want) to something like the below (or one of the other options mentioned above):

I think we should stick to what's simplest, if that means it can panic, at least users can modify the generated boilerplate code to match what they need.

A bigger thing is, I'm not sure if using go-cmp by the default is the right approach. The Golang philosophy is to use the stdlib for most things, and only third-party packages if necessary. Since go-cmp is a third-party package, I'd suggest flag protecting it behind a flag like --use_go_cmp, and using DeepEqual by default (see https://github.com/cweill/gotests/pull/99#issuecomment-507052600). This way gotests users can opt into go-cmp only if they want it. In a future release we can consider making go-cmp the default.

[mrsufgi]

Any updates on this?

[hslatman]

It's been on the backburner for me for quite a while, but I may be able to give this some attention again sometime soon.

[hslatman]

@cweill: can you start the tests?

Added the -use_go_cmp flag, so usage is now conditional. Reverted old test files to using reflect.DeepEqual again and made copies of relevant tests to use cmp.Equal and cmp.Diff. I've switched around the order of want and got for cmp.Diff, because I think the output is easier to interpret this way.

About having to install github.com/google/go-cmp: the changes in the workflow are necessary because I've also changed usages of reflect.DeepEqual used in the tests to cmp.Equal. @mekegi already created logic for adding the github.com/google/co-cmp import to the header only when it's required. When a user first runs gotests and creates tests with -use_go_cmp enabled, it's sufficient to then run go mod tidy manually once afterwards to ensure go-cmp can be used. It may be possible to automate that, but I don't think gotests is currently aware of the context it's executed in, so that it knows if it has to run go mod tidy or not. I think that the message shown by the Go tooling when running the generated tests for the first time is quite clear also:

go test ./...
# trygotest
t1_test.go:6:2: no required module provides package github.com/google/go-cmp/cmp; to add it:
    go get github.com/google/go-cmp/cmp
FAIL    trygotest [setup failed]
FAIL

Simple example failing test output:

type c struct {
    A int
    B string
}

func t1(b string) c {
    return c{
        A: 1,
        B: b,
    }
}

func Test_t1(t *testing.T) {
    type args struct {
        b string
    }
    tests := []struct {
        name string
        args args
        want c
    }{
        {
            name: "bla",
            args: args{
                b: "d",
            },
            want: c{
                A: 2,
                B: "e",
            },
        },
    }
    for _, tt := range tests {
        if got := t1(tt.args.b); !cmp.Equal(tt.want, got) {
            t.Errorf("%q. t1() = %v, want %v\ndiff=%s", tt.name, got, tt.want, cmp.Diff(tt.want, got))
        }
    }
}

go test ./...
--- FAIL: Test_t1 (0.00s)
    t1_test.go:31: "bla". t1() = {1 d}, want {2 e}
        diff=  main.c{
        -   A: 2,
        +   A: 1,
        -   B: "e",
        +   B: "d",
          }
FAIL
FAIL    trygotest   3.995s
FAIL

[hslatman]

@cweill: I've added another commit that removes some old Go versions from the CI (as well as add 1.17.x), which makes the tests pass again: https://github.com/hslatman/gotests/actions/runs/1976091466.

The error I got is the same as in the issue that @davidhsingyuchen reported in https://github.com/cweill/gotests/issues/168.

[edgio-mwodrich]

Digging into cmp.Diff a little, it runs cmp.Equal internally itself, and will produce string output if and only if cmp.Equal is false (if they ever disagree, it panics), so instead of

if !cmp.Equal(got, tt.want, opts) {
    t.Errorf("Function diff(got, want):\n%s", cmp.Diff(got, tt.want, opts))
}

a simplification for performance and maintainability I've adopted in gotests I've converted to use cmp.Diff is to do something like

if diff := cmp.Diff(got, tt.want, opts); diff != "" {
    t.Errorf("Function diff(got, want):\n%s", diff)
}

to avoid redundant runs of cmp.Equal and having to keep separate cmp.Equal and cmp.Diff invocations in sync, especially when using cmpopts.

Either way, I'd be very excited to use this type of functionality in my use of gotests, and I hope that folks might have time to move it forward.