Ensure deterministic order when diffing maps

[robbiemcmichael]

Summary

Ensure deterministic order when diffing maps to make the diffs readable.

Changes

Enable the SpewKeys setting in spew.ConfigState.

Motivation

When generating the diff for two maps where the map key is not a basic type, the order is non-deterministic which makes the diffs unreadable.

By enabling the SpewKeys setting, the map keys will be spewed to strings for the purpose of sorting them. This results in a deterministic order and makes the diffs readable.

For the test case added in this PR, the old config that has SpewKeys disabled would look like this:

--- Expected
+++ Actual
@@ -1,5 +1,2 @@
 (map[assert.Key]int) (len=4) {
- (assert.Key) {
-  x: (int) 4
- }: (int) 4,
  (assert.Key) {
@@ -12,3 +9,6 @@
   x: (int) 3
- }: (int) 3
+ }: (int) 3,
+ (assert.Key) {
+  x: (int) 4
+ }: (int) 999
 }

After enabling SpewKeys, we get the simpler diff that we would expect:

--- Expected
+++ Actual
@@ -12,3 +12,3 @@
   x: (int) 4
- }: (int) 4
+ }: (int) 999
 }

[ccoVeille]

@robbiemcmichael I feel like what you report is a bug in go-spew. Did you find any issue related to this ordering in map issue?

I worked on #1827 to vendor go-spew, maybe the way would be to fix gospew behavior.

I'm asking because I'm now unsure about the consequence of enabling this setting in term of rendering/ordering.

I analyzed a lot the code of gospew lately, and I'm a bit worried enabling a new config flag could lead us to face yet-to-disvover gospew bug/panic hidden in the darkness

[robbiemcmichael]

@ccoVeille: Looking at the docs for SortKeys setting:

// SortKeys specifies map keys should be sorted before being printed. Use
// this to have a more deterministic, diffable output. Note that only
// native types (bool, int, uint, floats, uintptr and string) and types
// that support the error or Stringer interfaces (if methods are
// enabled) are supported, with other types sorted according to the
// reflect.Value.String() output which guarantees display stability.

It does seem as though it should be using reflect.Value.String() to try to turn the value into a string for sorting, but that seems to be a misunderstanding of what that function does. It only prints a string containing <T Value> where T is the type, so it doesn't vary at all for values of the same type.

I think this could be considered a bug in go-spew, though the fix would need us to turn the value into a string which will probably end up being similar to using the setting I've enabled in this PR. I think the current approach should be safe as we must already be spewing the value to a string for the sake of printing it, so if it panics when trying to turn it into a string for sorting then it should already panic when it turns it into a string for printing.

[robbiemcmichael]

@ccoVeille: Any preference on how to proceed with this? I can't see many other sensible ways to turn the key into a string that can be sorted other than by spewing it to a string, which is exactly what the SpewKeys setting is meant to do. These keys already get spewed to strings when they get printed for the diff, so I don't think this is likely to introduce any new panics.

Here's the code that shows what the SpewKeys setting does:

testify/internal/spew/common.go

Lines 244 to 249 in 65697ce

	if vs.strings == nil && cs.SpewKeys {
	vs.strings = make([]string, len(values))
	for i := range vs.values {
	vs.strings[i] = Sprintf("%#v", vs.values[i].Interface())
	}
	}

[ccoVeille]

Sorry for the delay, Christmas period is uneasy.

Thanks for pointing out the code.

I agree with you it sounds OK. But I see one issue we may have to address.

reflect.Value.Interface() can panic in some condition. I'm unsure if it makes sense to have a map with a struct with unexported method used as a key. But something as awkward as this could fail.

Maybe we could find go-spew issue about this strange use case. But I didn't find some.

Maybe we could take a look at utter the go-spew fork that rewrote almost everything. Especially this

But anyway, the fix is easy I think ™️ 😅: adding a check on CanInterface() before and fallback on reflect.Value.String() (that never panic, but we know to be imperfect) or on fmt.SPrint of the value could be something to do.

Maybe I'm simply wrong and there is no problem to activate SpewKeys

Do you think you could add some tests and then update the "internalized" go-spew lib ?

Thanks

I'm confident the path to achieve is OK.