Comparison of reflect.Value types about go-testdeep

[maxatome]

Hi,
So you want reflect.Value be handled specifically so that only their content should be compared, and not the reflect.Value itself.

a, b := reflect.ValueOf(1234), reflect.ValueOf(1234)
td.Cmp(t, a, a)    // should succeed like now, but...
td.Cmp(t, a, b)    // should succeed, but...
td.Cmp(t, a, 1234) // should fail

why not.

[maxatome]

Or provide a pluggable mechanism (like we do with anchors) to allow a custom behavior for any type?

[stewi1014]

Yes, that's exactly what I mean.

I'm just wanting to compare the values inside. I'm agnostic to comparing the reflect.Value itself. Either way works for my use case, but it's possible someone may want to check that two reflect.Values are themselves the same. Perhaps they have a function returning a reflect.Value and they want to check that it's always addressable. Allowing custom behaviour for specific types could be quite powerful. However, as long as you're happy with this behaviour being in go-testdeep then I think custom behaviour for different types may be overkill for this issue, up to you :)

AFAIK if both reflect.Values contain the same type and have the same setability and addressability then they do have the same flag and typ fields, but I don't think this is something that can be relied upon.

CanAddr(), CanInterface() and CanSet() could be compared in addition to the contained value. This would assert that the reflect.Value will behave the same way, and future-proofs against changes to the reflect package, which AFAIK will likely be coming in go 2.0.

[maxatome]

Could you tell me if the branch https://github.com/maxatome/go-testdeep/tree/hooks fills your needs?

func TestHooks(tt *testing.T) {
	got, expected := reflect.ValueOf(1234), reflect.ValueOf(1234)
	t := td.NewT(tt)
	t.AddCmpHook(func(got, expected reflect.Value) bool {
		return td.EqDeeply(got.Interface(), expected.Interface()) // or whatever test you want
	})
	t.Cmp(got, expected) // now succeeds
}

It is a quick proof of concept, it needs some docs + tests, but this feature could be available as is. What do you think about it?

AddCmpHook method only accepts a function matching the signature func (T, T) bool (it calls this one behind the scene).

[stewi1014]

Wow, that was fast! I've imported it and am running some tests with it and it's working great, this definitely works. This library is revolutionising my testing :P

I did find something rather interesting. I noticed that CmpFalse in the test was failing. It took a while, but I figured out why. If I passed the number as a global variable to reflect.ValueOf it worked. Turns out the compiler is optimising the '1234' in got, expected := reflect.ValueOf(1234), reflect.ValueOf(1234) to the same address in memory on my machine! Of course this means all fields in got and expected are equal with the normal comparison.

e.g.

func TestHooks(tt *testing.T) {
	got, expected := reflect.ValueOf(1234), reflect.ValueOf(1234)

	// get ptr field in got
	gotElemPtr := unsafe.Pointer((*(*[2]uintptr)(unsafe.Pointer(&got)))[1])

	// get ptr field in expected
	expectedElemPtr := unsafe.Pointer((*(*[2]uintptr)(unsafe.Pointer(&expected)))[1])

	// Same address!!
	fmt.Printf("%p %p\n", gotElemPtr, expectedElemPtr)

	// Just to sanity check it is actually the correct pointer.
	fmt.Println(*(*int)(gotElemPtr), *(*int)(expectedElemPtr))

	ttt := test.NewTestingTB(tt.Name())
	t := td.NewT(ttt)

	// Not the same reflect.Value or not-comparable
	td.CmpFalse(tt, func() bool {
		// A panic occurs when -tags safe:
		//   dark.GetInterface() does not handle private unsafe.Pointer kind
		defer func() { recover() }() //nolint: errcheck
		return t.Cmp(got, expected)
	}())

	// Add a hook to compare what is inside the reflect.Value
	t.AddCmpHook(func(got, expected reflect.Value) bool {
		return td.EqDeeply(got.Interface(), expected.Interface())
	})
	td.CmpTrue(tt, t.Cmp(got, expected))
}

I'm not sure how the compiler handles this, although it seems to work for me.

n := 1234
got, expected := reflect.ValueOf(n), reflect.ValueOf(1234)

but I do know the compiler is guaranteed to not optimise global variables away irrelevant of their usage. It also appears as though this is non-deterministic, as clearing caches and trying again sometimes results in the test passing.

[maxatome]

Yes I experienced the same behavior with master branch of go during my tests. I will remove this CmpFalse test as nothing can guarantee it always succeeds (or fails) whatever the go version is used. Anyway, thanks for the explanations, when I saw that only master failed, I went to bed without digging any further...

We have to keep in mind that reflect.Value have not to be compared deeply, that is why AddCmpHook is useful :)

Note that the same applies for reflect.Type, it should be:

t.AddCmpHook(func(got, expected reflect.Type) bool {
	return got == expected
})

I'll clean the code, write docs and some tests, and come back with a PR.

Do not forget to advertise go-testdeep! :D

[stewi1014]

Do not forget to advertise go-testdeep! :D

I will certainly be mentioning go-testdeep in the README for my encoding library :)

Thanks a lot, and let me know if I can help!

[maxatome]

Could you check #108 please?

I added doc and tests, accepted signatures for a hook are now func (T, T) bool and func (T, T) error, and T cannot be an interface.

Do not hesitate to comment if you see a mistake, it's late here...