Use go-cmp instead of reflect.DeepEqual #155

hslatman · 2021-03-24T23:15:20Z

As discussed in #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 · 2021-03-24T23:15:25Z

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 · 2021-03-24T23:16:49Z

@googlebot I consent.

coveralls · 2021-03-24T23:16:57Z

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

butuzov · 2021-03-27T20:04:21Z

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

hslatman · 2021-03-27T21:51:38Z

@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 · 2021-03-28T02:41:15Z

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

Maybe we %s placeholders can be used instead of %v in all places now?

butuzov · 2021-03-28T02:44:42Z

gotests_test.go

@@ -918,7 +919,7 @@ func TestGenerateTests(t *testing.T) {
 				return
 			}
 			if got := string(gts[0].Output); got != tt.want {
-				t.Errorf("%q. GenerateTests(%v) = \n%v, want \n%v", tt.name, tt.args.srcPath, got, tt.want)
+				t.Errorf("%q. GenerateTests(%v) = diff=%v", tt.name, tt.args.srcPath, cmp.Diff(got, tt.want))


Would it be possible to create maybe a custom reporter so we can get got/want with a lines of the diff instead -/+ ?

also %v now can be %s

The custom reporter approach certainly looks interesting. So far I just updated @mekegi's code, but I could give integrating the custom reporter a try.

So your goal would be to print the output of the custom reporter instead of the default cmp.Diff(x, y), right? It seems doable for this specific line of code, i.e. in the gotests_test.go it should be fairly trivial to add.

Do you mean to also use the custom reporter for the generated tests? And by extension, making that configurable by the user of gotests too? Or am I way off and did you mean to just put it in gotests_test.go?

Taking the example DiffReporter and changing the lines to this:

var r reporter.DiffReporter if got := string(gts[0].Output); !cmp.Equal(got, tt.want, cmp.Reporter(&r)) { t.Errorf("%q. GenerateTests(%v) = diff=%s", tt.name, tt.args.srcPath, r.String()) outputResult(t, tmp, tt.name, gts[0].Output) } // basic Report() function, slightly altered from the example func (r *DiffReporter) Report(rs cmp.Result) { if !rs.Equal() { vx, vy := r.path.Last().Values() r.diffs = append(r.diffs, fmt.Sprintf("\n\nGot:\n\n%+v\n\nWant:\n\n%+v\n", vx, vy)) } }

Results in the following output:

gotests_test.go:924: "Receiver is indirect imported struct". GenerateTests(testdata/test037.go) = diff= Got: package testdata import "testing" func Test_someIndirectImportedStruct_Foo037(t *testing.T) { tests := []struct { name string smtg *someIndirectImportedStruct }{ // TODO: Add test cases. } for _, tt := range tests { tt.smtg.Foo037() } } Want: package testdata import "testing" func Test_someIndirectImportedStruct_Foo037(t *testing.T) { tests := []struct { name string smtg *someIndirectImportedStruct }{ // TODO: Add test cases. } for _, tt := range tests { smtg := &someIndirectImportedStruct{} smtg.Foo037() } } gotests_test.go:958: /var/folders/m1/h8xf_67j5fqccwr1277f3_zc0000gn/T/gotests_test230212921/receiver_is_indirect_imported_struct.go

This doesn't seem really useful, but it may be possible to customize it down to the actual lines that are different.

If I understand you correctly, you want to show just got/want and the corresponding lines, correct? I find the -/+ quite easy to use because the differences are closer to each other, so I'm not sure if we should drop that, unless you want it to become configurable for a user of gotests?

I was talking only about generated tests and you are right without granular line detection it does not seem really useful.

+1 to keeping the -/+. Without them it's very difficult to know what the delta is.

I've only scratched the surface of what may be possible using the custom reporter. It may very well be possible to print a compact version of the -/+ delta, but that'll likely require becoming a bit more acquainted with the go-cmp reporting code. Could be something for the future, perhaps?

butuzov · 2021-03-28T02:51:43Z

gotests.go

+var (
+	cmpImport = &models.Import{
+		Name: "",
+		Path: `"github.com/google/go-cmp/cmp"`,
+	}
+)


minor: is there any other data it should be grouped in the future?

Suggested change

var (

cmpImport = &models.Import{

Name: "",

Path: `"github.com/google/go-cmp/cmp"`,

}

)

var cmpImport = &models.Import{

Name: "",

Path: `"github.com/google/go-cmp/cmp"`,

}

butuzov

really minor nit

butuzov · 2021-03-30T02:35:45Z

ping @cweill

cweill · 2021-04-01T02:41:18Z

.github/workflows/go.yml

@@ -35,6 +35,9 @@ jobs:
    - name: Get Dependencies (github.com/golang.org/x/tools/imports)
      run: go get -v ./...

+    - name: Get go-cmp
+      run: go get github.com/google/go-cmp/cmp


Why do we need to manually fetch this package? If a user forgets to do this will gotests not work for them? IMO go get -v ./... should cover installing this dependency.

Without this explicit call the tests will fail in GitHub Actions for Go 1.9 - 1.12:

Run go test -v ./... go test -v ./... shell: /usr/bin/bash -e {0} env: GOPATH: /home/runner/work/gotests/gotests GO111MODULE: auto GOROOT: /opt/hostedtoolcache/go/1.12.17/x64 # github.com/cweill/gotests FAIL github.com/cweill/gotests [setup failed] gotests_test.go:16:2: cannot find package "github.com/google/go-cmp/cmp" in any of: /opt/hostedtoolcache/go/1.12.17/x64/src/github.com/google/go-cmp/cmp (from $GOROOT) /home/runner/work/gotests/gotests/src/github.com/google/go-cmp/cmp (from $GOPATH) FAIL github.com/cweill/gotests/gotests [setup failed]

Other solutions to make this work may exist. The GO111MODULE env var could be set to on, but that'll likely only work for Go 1.11 and up. To me this seemed the easiest solution to make it work across versions without having to make the CI configuration more complex.

I tried the go-cmp version of gotests locally on a sample project. It resulted in a _test.go file with github.com/google/go-cmp/cmp in the import section. Running the tests for the first time results in go-cmp being inserted into go.mod. The go.mod of gotests itself does contain go-cmp, so that should work correctly too.

My concern is for who don't have go-cmp installed, will this change break them unless they manually install the package? Is there any way we can install it automatically when they install gotests?

cweill · 2021-04-01T02:43:45Z

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

mekegi · 2021-04-01T12:03:12Z

@googlebot I consent

googlebot · 2021-04-01T12:03:35Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

hslatman · 2021-04-01T18:20:46Z

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 · 2021-04-01T19:33:23Z

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 #99 (comment)). 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.

cweill · 2021-04-01T19:37:43Z

.github/workflows/go.yml

@@ -35,6 +35,9 @@ jobs:
    - name: Get Dependencies (github.com/golang.org/x/tools/imports)
      run: go get -v ./...

+    - name: Get go-cmp
+      run: go get github.com/google/go-cmp/cmp


My concern is for who don't have go-cmp installed, will this change break them unless they manually install the package? Is there any way we can install it automatically when they install gotests?

mrsufgi · 2022-03-09T16:32:13Z

Any updates on this?

hslatman · 2022-03-10T11:29:14Z

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.

When the `--use_go_cmp` flag is provided, equality checks will use `cmp.Equal` (https://pkg.go.dev/github.com/google/go-cmp/cmp) instead of `reflect.DeepEqual`. Differences between two objects will be reported using `cmp.Diff`. The tests that used `reflect.DeepEqual` have been duplicated, so that all behavior that was tested is now also tested when using `google/go-cmp`.

hslatman · 2022-03-12T23:31:21Z

@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 · 2022-03-13T10:43:36Z

@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 #168.

edgio-mwodrich · 2024-03-06T18:19:02Z

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.

mekegi and others added 3 commits May 13, 2019 19:00

use go-cmp instead of reflect.DeepEqual

cec4af4

Merge branch 'develop' into go-cmp

66d408d

Fix tests

7c6e3ac

googlebot added the cla: no label Mar 24, 2021

Merge branch 'develop' into go-cmp

0e65e4f

butuzov reviewed Mar 28, 2021

View reviewed changes

Fix PR comments

0a209c7

cweill approved these changes Apr 1, 2021

View reviewed changes

googlebot added cla: yes Contributor signed Google CLA and removed cla: no labels Apr 1, 2021

cweill requested changes Apr 1, 2021

View reviewed changes

hslatman added 3 commits March 12, 2022 15:59

Merge branch 'develop' into go-cmp

5603b29

Fix missing named flag in README.md

2ecd7a0

Fix typo in reflect.DeepEqual

2156f80

Remove old Go version (temporarily)

3ad4a92

hslatman requested a review from cweill March 13, 2022 13:55

Swathi-L mentioned this pull request Apr 14, 2022

Add option to generate cmp.Diff usage vs reflect.DeepEqual #130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use go-cmp instead of reflect.DeepEqual #155

Use go-cmp instead of reflect.DeepEqual #155

hslatman commented Mar 24, 2021

googlebot commented Mar 24, 2021

hslatman commented Mar 24, 2021

coveralls commented Mar 24, 2021 •

edited

Loading

butuzov commented Mar 27, 2021

hslatman commented Mar 27, 2021

butuzov commented Mar 28, 2021 •

edited

Loading

butuzov left a comment

butuzov Mar 28, 2021 •

edited

Loading

hslatman Mar 29, 2021

hslatman Mar 29, 2021 •

edited

Loading

butuzov Mar 30, 2021

cweill Apr 1, 2021

hslatman Apr 1, 2021 •

edited

Loading

butuzov Mar 28, 2021

butuzov left a comment

butuzov commented Mar 30, 2021

cweill Apr 1, 2021

hslatman Apr 1, 2021

cweill Apr 1, 2021

cweill commented Apr 1, 2021

mekegi commented Apr 1, 2021

googlebot commented Apr 1, 2021

hslatman commented Apr 1, 2021 •

edited

Loading

cweill commented Apr 1, 2021 •

edited

Loading

cweill Apr 1, 2021

mrsufgi commented Mar 9, 2022

hslatman commented Mar 10, 2022

hslatman commented Mar 12, 2022 •

edited

Loading

hslatman commented Mar 13, 2022 •

edited

Loading

edgio-mwodrich commented Mar 6, 2024

Use go-cmp instead of reflect.DeepEqual #155

Are you sure you want to change the base?

Use go-cmp instead of reflect.DeepEqual #155

Conversation

hslatman commented Mar 24, 2021

googlebot commented Mar 24, 2021

hslatman commented Mar 24, 2021

coveralls commented Mar 24, 2021 • edited Loading

butuzov commented Mar 27, 2021

hslatman commented Mar 27, 2021

butuzov commented Mar 28, 2021 • edited Loading

butuzov left a comment

Choose a reason for hiding this comment

butuzov Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hslatman Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hslatman Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butuzov left a comment

Choose a reason for hiding this comment

butuzov commented Mar 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cweill commented Apr 1, 2021

mekegi commented Apr 1, 2021

googlebot commented Apr 1, 2021

hslatman commented Apr 1, 2021 • edited Loading

cweill commented Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

mrsufgi commented Mar 9, 2022

hslatman commented Mar 10, 2022

hslatman commented Mar 12, 2022 • edited Loading

hslatman commented Mar 13, 2022 • edited Loading

edgio-mwodrich commented Mar 6, 2024

coveralls commented Mar 24, 2021 •

edited

Loading

butuzov commented Mar 28, 2021 •

edited

Loading

butuzov Mar 28, 2021 •

edited

Loading

hslatman Mar 29, 2021 •

edited

Loading

hslatman Apr 1, 2021 •

edited

Loading

hslatman commented Apr 1, 2021 •

edited

Loading

cweill commented Apr 1, 2021 •

edited

Loading

hslatman commented Mar 12, 2022 •

edited

Loading

hslatman commented Mar 13, 2022 •

edited

Loading