Skip to content

fix: keep nil map values in EqualExportedValues - #1972

Open
januththedev wants to merge 1 commit into
stretchr:masterfrom
januththedev:fix/equal-exported-values-nil-map
Open

januththedev wants to merge 1 commit into
stretchr:masterfrom
januththedev:fix/equal-exported-values-nil-map

Conversation

@januththedev

Copy link
Copy Markdown

Summary

EqualExportedValues reported structurally different maps as equal when a map value was untyped nil.

In copyExportedFields, the reflect.Map case did:

unexportedRemoved := copyExportedFields(index.Interface())
result.SetMapIndex(k, reflect.ValueOf(unexportedRemoved))

For a map[string]interface{} holding a nil value, index.Interface() is an untyped nil, so copyExportedFields returns untyped nil. reflect.ValueOf(nil) is the zero reflect.Value, and per the reflect contract SetMapIndex treats a zero Value as delete this key. The entry therefore vanished from the cleaned copy before the comparison ever ran.

The consequence is the worst kind for an assertion library: the assertion passes when it should fail, silently turning a real test failure green.

Reproduction

assert.EqualExportedValues(map[string]interface{}{"a": nil}, map[string]interface{}{})  // reported true

with an empty failure message, while the non-nil control map[string]interface{}{"a": 1} against map[string]interface{}{} correctly reports false — isolating the defect to nil values specifically.

The change

unexportedRemoved := copyExportedFields(index.Interface())
if unexportedRemoved == nil {
    // reflect.ValueOf(nil) is the zero Value, which
    // SetMapIndex interprets as "delete this key". Store the
    // element type's zero value instead so nil map values are
    // preserved and the entry keeps taking part in the
    // comparison.
    result.SetMapIndex(k, reflect.Zero(expectedType.Elem()))
    continue
}

Typed nils such as map[string]*int{"a": nil} are unaffected — they compare != nil as an interface and still take the original path. Only genuinely untyped nils, which previously disappeared, take the new one. The Struct case already handled nil correctly by leaving the zero value in place, so this makes the two cases consistent.

Tests

  • Two of the four new table entries fail before the change with Expected EqualExportedValues to be false, but was true, and all pass after.
  • Failure output is now informative rather than silent.
  • go test -count=1 ./assert/ ./require/ → 593 and 116 tests, 0 failures.
  • go build ./... and go vet ./... clean; edited files are gofmt-clean.

Note on the 12 failures in ./suite/: those are pre-existing panic/recover test failures on this Windows checkout. I confirmed they reproduce identically with my changes stashed on a clean tree, and they are in a different package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant