assert.EqualExportedValues: fix handling of arrays

[zrbecker]

Summary

In EqualExportedValues, testify treats an Array as a Slice, however, the reflect library throws an error if the type passed to reflect.MakeSlice is not a Slice kind. This issue was identified in #1404

The result is a very simple test such as

func TestEqualExportedValuesWithArray(t *testing.T) {
	type S struct{ Values [2]int }
	require.EqualExportedValues(t, S{Values: [2]int{1, 2}}, S{Values: [2]int{1, 2}})
}

results in a panic

panic: reflect.MakeSlice of non-slice type

This PR adjusts the offending switch statement in copyExportedFields to handle arrays separately, and construct the array using reflect.New(reflect.ArrayOf(len, type)).Elem() instead of reflect.MakeSlice.

Changes

• Changed copyExportedFields (used by EqualExportedValues) to handle Array type separately from Slices as they must be constructed differently.

• Added a test case to TestEqualExportedValues to verify an array object can be set on object passed to EqualExportedValues.

Motivation

A user was unable to use EqualExportedValues if they had an exported array type nested somewhere in their struct.

A test like this will now pass

func TestEqualExportedValuesWithArray(t *testing.T) {
	type S struct{ Values [2]int }
	require.EqualExportedValues(t, S{Values: [2]int{1, 2}}, S{Values: [2]int{1, 2}})
}

Related issues

Closes #1404

[zrbecker]

@HaraldNordgren Looks like this was introduce in #1379, I saw this along with the issue you are addressing in #1384.

[HaraldNordgren]

Awesome!

[HaraldNordgren]

I think this needs some more tests. And would it make sense to combine the array and slice paths more since the logic is so similar after instantiating? Something like this:

// copyExportedFields iterates downward through list data structures and creates a copy
// that only contains the exported struct fields.
func copyExportedFieldsList(expectedValue, result reflect.Value) reflect.Value {
	for i := 0; i < expectedValue.Len(); i++ {
		index := expectedValue.Index(i)
		if isNil(index) {
			continue
		}
		unexportedRemoved := copyExportedFields(index.Interface())
		result.Index(i).Set(reflect.ValueOf(unexportedRemoved))
	}
	return result
}

and then

	case reflect.Array:
		result := reflect.New(reflect.ArrayOf(expectedValue.Len(), expectedType.Elem())).Elem()
		result = copyExportedFieldsList(expectedValue, result)
		return result.Interface()

	case reflect.Slice:
		result := reflect.MakeSlice(expectedType, expectedValue.Len(), expectedValue.Len())
		result = copyExportedFieldsList(expectedValue, result)
		return result.Interface()

[HaraldNordgren]

(See my comment above)

[HaraldNordgren]

@dolmen @boyan-soubachov Can you take a look at this? It's a good change that should be merged to unbreak the code.

[dolmen]

The only difference between the reflect.Array and reflect.Slice blocks are how result is allocated.
Please refactor to keep a single switch block and just branch with an if for the construction of result.

[zrbecker]

@dolmen I moved the two cases into a single switch statement to reduce code duplication as requested.

[dolmen]

Thank you!