verify.Values should compare private fields like reflect.DeepEqual

[magiconair]

The following test case shows that reflect.DeepEqual can compare private fields whereas verify.Values cannot. Tested with Go 1.8

type A struct {
	x bool
}

func TestA(t *testing.T) {
	got, want := &A{true}, &A{false}
	fmt.Println("DeepEqual:", reflect.DeepEqual(got, want))
	verify.Values(t, "", got, want)
}

This prints

=== RUN   TestA
DeepEqual: false
--- FAIL: TestA (0.00s)
	values.go:22: verification for  at fsm_test.go:19:
		/%!s(<nil>): Can't read private fields
FAIL
exit status 1

[pascaldekloe]

Deep equals uses unsafe and the functionality is not exported in the reflection package.
https://golang.org/src/reflect/deepequal.go#L132
https://golang.org/src/reflect/value.go#L922

Of course verify.Values could use reflect.DeepEqual for unexported fields yet that also introduces NaN != NaN.

The only way I see to do this right is a values equivalent from @awalterschulze his goredrive.

[awalterschulze]

This should be easy to do with goderive and much harder with reflect and unsafe.
The tradeoff is the extra generated code.
This tradeoff is great when you gain speed and readability of code, but I don't know if its always worth it in the case of tests.
I am on the fence here.

[magiconair]

verify.Values is used for testing. Why should I generate and maintain another method just for this purpose? It is not relevant whether the implementation is pure. It only matters whether it works.

[magiconair]

On second thought the Equal method could also be generated in the test code but that would limit the application to tests of structs that happen within the same package.

Oh, relying on generating the Equal method would prevent verifying structs that are outside of the package or which are vendored in...

[awalterschulze]

vendored in structs should not be exposed. Or can you give an example, maybe I am not understanding the problem.

[magiconair]

If I have to generate an Equal method then I can do this only for the structs within the same package iff I want to keep the equal method in the test code. Everything that is outside the package you're testing would have to have a public Equal method which is exported. This can work for your own code but becomes iffy for vendored in code.

As for vendored structs that shouldn't be exposed: How about http.Client and url.URL? How would I add an Equal method if I have a URL field in my struct, for example?

[awalterschulze]

Aha makes sense. Good point. I'll have to think about that.

[sandymcp]

Well you could just take the bloody minded (aka engineering) approach:

if got, want := stuff, test.stuff; !reflect.DeepEquals(got,want) {
      t.Fatalf("..Got %+v\n.Want %+v", got, want)
      verify.Values(t, "", got, want)
}

Then your test is 100% safe and you will probably get some info from the verify, if not just have to dig a bit deeper.

[pascaldekloe]

1. Use unsafe. Sounds like a ton of work and I don't feel like spending it.

2. Accept NaN != NaN and use reflect.DeepEquals where needed.

3. Keep this limitation.

[magiconair]

@sandymcp The point is not whether or not I can work around it. Of course, I can and it isn't much work.

I think the main argument is that the results from verify.Values and reflect.DeepEqual differ and that shouldn't be. Also, it isn't obvious that they do. The reason is an implementation detail no matter how complex it is.

Since you use verify.Values in tests as a more convenient drop-in replacement for reflect.DeepEqual they should produce the same results. Otherwise, you can not trust it.

[pascaldekloe]

The common usecase is testing indeed. That verify.Values tests the content to be present rather than honoring what IEEE stated to be equal makes sense to me. How else are you going to verify a float to be NaN?
Anyway, it's not that much of a problem and it seems like the least amount of work.

[awalterschulze]

So there are two issues here:

1. NaN, I think goderive is just going to go with whatever go does here when a.(float64) == b.(float64) is done on two NaNs. But I agree with pascal, this is not ideal behavior for tests.
2. Accessing private fields in different packages, like the standard library. An ideal verify values should be able to do this or know that it can't and have a fallback strategy to reflect.DeepEqual, but the conflict in logic with NaN makes it a little wonky. Also as someone who likes using unsafe, I still would not recommend maintaining unsafe code, especially since I find the unsafe usage in reflect.DeepEqual harder to read than most unsafe code. As for goderive I am still unsure. I can fallback to reflect.DeepEqual in the case of private fields and generate local functions in the case of public fields, but in some cases I would prefer a compile error. Unfortunately I can't see a way around the fallback strategy for the standard library and thus vendored in packages. But for local packages I would prefer a compile error. So still thinking, and I will cross that bridge when I get an issue report on goderive from an actual user, at least I have a backup plan.

[awalterschulze]

goderive can now derive equal for structs in other packages as well as private fields. So I think its now comparable to reflect.DeepEqual

func deriveEqualextra_PrivateFieldAndNoEqualMethod(this, that extra.PrivateFieldAndNoEqualMethod) bool {
	thisv := reflect.Indirect(reflect.ValueOf(&this))
	thatv := reflect.Indirect(reflect.ValueOf(&that))
	return *(*int64)(unsafe.Pointer(thisv.FieldByName("number").UnsafeAddr())) == *(*int64)(unsafe.Pointer(thatv.FieldByName("number").UnsafeAddr())) &&
		deriveEqualSliceOfint64(*(*[]int64)(unsafe.Pointer(thisv.FieldByName("numbers").UnsafeAddr())), *(*[]int64)(unsafe.Pointer(thatv.FieldByName("numbers").UnsafeAddr()))) &&
		((*(**int64)(unsafe.Pointer(thisv.FieldByName("ptr").UnsafeAddr())) == nil && *(**int64)(unsafe.Pointer(thatv.FieldByName("ptr").UnsafeAddr())) == nil) || (*(**int64)(unsafe.Pointer(thisv.FieldByName("ptr").UnsafeAddr())) != nil && *(**int64)(unsafe.Pointer(thatv.FieldByName("ptr").UnsafeAddr())) != nil && **(**int64)(unsafe.Pointer(thisv.FieldByName("ptr").UnsafeAddr())) == **(**int64)(unsafe.Pointer(thatv.FieldByName("ptr").UnsafeAddr())))) &&
		deriveEqualSliceOfPtrToint64(*(*[]*int64)(unsafe.Pointer(thisv.FieldByName("numberpts").UnsafeAddr())), *(*[]*int64)(unsafe.Pointer(thatv.FieldByName("numberpts").UnsafeAddr()))) &&
		deriveEqualPtrToextra_StructWithoutEqualMethod(*(**extra.StructWithoutEqualMethod)(unsafe.Pointer(thisv.FieldByName("strct").UnsafeAddr())), *(**extra.StructWithoutEqualMethod)(unsafe.Pointer(thatv.FieldByName("strct").UnsafeAddr())))
}

[awalterschulze]

Writing a modification of equal function to rather turn an error with a description of the diff should now be an easy modification of the equal generator, but I have other functions with a higher priority for now.

[pascaldekloe]

Just found this newcomer: https://godoc.org/github.com/google/go-cmp/cmp

[awalterschulze]

I have already added it as an issue :)
awalterschulze/goderive#15

[awalterschulze]

Sjoerd told me about it. There was a talk at GopherCon.