From f36bfe3c337aa95c86f04c721acdbafb5ffb1611 Mon Sep 17 00:00:00 2001 From: Daniel White Date: Sat, 25 Feb 2023 23:46:30 +1100 Subject: [PATCH] Fix Subset/NotSubset when map is missing keys from the subset (#1261) `MapIndex` returns a zero `reflect.Value` if the map value does not exist. This now aligns more closely with `InDeltaMapValues` by checking `reflect/Value.IsValid`. The use of `recover()` was hiding this error by setting the result of the test to be "false" despite the test suite passing. This led to flapping tests where they would succeed if the panic occurred on a missing key before comparing key values that didn't match! I've ensured the test suite now asserts on the expected error message and added another example where the subset has keys not found on the map under test. --- assert/assertions.go | 68 ++++++++++++++++-------------------- assert/assertions_test.go | 72 +++++++++++++++++++++++---------------- 2 files changed, 72 insertions(+), 68 deletions(-) diff --git a/assert/assertions.go b/assert/assertions.go index c540bb02d..2924cf3a1 100644 --- a/assert/assertions.go +++ b/assert/assertions.go @@ -816,49 +816,44 @@ func Subset(t TestingT, list, subset interface{}, msgAndArgs ...interface{}) (ok return true // we consider nil to be equal to the nil set } - defer func() { - if e := recover(); e != nil { - ok = false - } - }() - listKind := reflect.TypeOf(list).Kind() - subsetKind := reflect.TypeOf(subset).Kind() - if listKind != reflect.Array && listKind != reflect.Slice && listKind != reflect.Map { return Fail(t, fmt.Sprintf("%q has an unsupported type %s", list, listKind), msgAndArgs...) } + subsetKind := reflect.TypeOf(subset).Kind() if subsetKind != reflect.Array && subsetKind != reflect.Slice && listKind != reflect.Map { return Fail(t, fmt.Sprintf("%q has an unsupported type %s", subset, subsetKind), msgAndArgs...) } - subsetValue := reflect.ValueOf(subset) if subsetKind == reflect.Map && listKind == reflect.Map { - listValue := reflect.ValueOf(list) - subsetKeys := subsetValue.MapKeys() + subsetMap := reflect.ValueOf(subset) + actualMap := reflect.ValueOf(list) - for i := 0; i < len(subsetKeys); i++ { - subsetKey := subsetKeys[i] - subsetElement := subsetValue.MapIndex(subsetKey).Interface() - listElement := listValue.MapIndex(subsetKey).Interface() + for _, k := range subsetMap.MapKeys() { + ev := subsetMap.MapIndex(k) + av := actualMap.MapIndex(k) - if !ObjectsAreEqual(subsetElement, listElement) { - return Fail(t, fmt.Sprintf("\"%s\" does not contain \"%s\"", list, subsetElement), msgAndArgs...) + if !av.IsValid() { + return Fail(t, fmt.Sprintf("%#v does not contain %#v", list, subset), msgAndArgs...) + } + if !ObjectsAreEqual(ev.Interface(), av.Interface()) { + return Fail(t, fmt.Sprintf("%#v does not contain %#v", list, subset), msgAndArgs...) } } return true } - for i := 0; i < subsetValue.Len(); i++ { - element := subsetValue.Index(i).Interface() + subsetList := reflect.ValueOf(subset) + for i := 0; i < subsetList.Len(); i++ { + element := subsetList.Index(i).Interface() ok, found := containsElement(list, element) if !ok { - return Fail(t, fmt.Sprintf("\"%s\" could not be applied builtin len()", list), msgAndArgs...) + return Fail(t, fmt.Sprintf("%#v could not be applied builtin len()", list), msgAndArgs...) } if !found { - return Fail(t, fmt.Sprintf("\"%s\" does not contain \"%s\"", list, element), msgAndArgs...) + return Fail(t, fmt.Sprintf("%#v does not contain %#v", list, element), msgAndArgs...) } } @@ -877,34 +872,28 @@ func NotSubset(t TestingT, list, subset interface{}, msgAndArgs ...interface{}) return Fail(t, "nil is the empty set which is a subset of every set", msgAndArgs...) } - defer func() { - if e := recover(); e != nil { - ok = false - } - }() - listKind := reflect.TypeOf(list).Kind() - subsetKind := reflect.TypeOf(subset).Kind() - if listKind != reflect.Array && listKind != reflect.Slice && listKind != reflect.Map { return Fail(t, fmt.Sprintf("%q has an unsupported type %s", list, listKind), msgAndArgs...) } + subsetKind := reflect.TypeOf(subset).Kind() if subsetKind != reflect.Array && subsetKind != reflect.Slice && listKind != reflect.Map { return Fail(t, fmt.Sprintf("%q has an unsupported type %s", subset, subsetKind), msgAndArgs...) } - subsetValue := reflect.ValueOf(subset) if subsetKind == reflect.Map && listKind == reflect.Map { - listValue := reflect.ValueOf(list) - subsetKeys := subsetValue.MapKeys() + subsetMap := reflect.ValueOf(subset) + actualMap := reflect.ValueOf(list) - for i := 0; i < len(subsetKeys); i++ { - subsetKey := subsetKeys[i] - subsetElement := subsetValue.MapIndex(subsetKey).Interface() - listElement := listValue.MapIndex(subsetKey).Interface() + for _, k := range subsetMap.MapKeys() { + ev := subsetMap.MapIndex(k) + av := actualMap.MapIndex(k) - if !ObjectsAreEqual(subsetElement, listElement) { + if !av.IsValid() { + return true + } + if !ObjectsAreEqual(ev.Interface(), av.Interface()) { return true } } @@ -912,8 +901,9 @@ func NotSubset(t TestingT, list, subset interface{}, msgAndArgs ...interface{}) return Fail(t, fmt.Sprintf("%q is a subset of %q", subset, list), msgAndArgs...) } - for i := 0; i < subsetValue.Len(); i++ { - element := subsetValue.Index(i).Interface() + subsetList := reflect.ValueOf(subset) + for i := 0; i < subsetList.Len(); i++ { + element := subsetList.Index(i).Interface() ok, found := containsElement(list, element) if !ok { return Fail(t, fmt.Sprintf("\"%s\" could not be applied builtin len()", list), msgAndArgs...) diff --git a/assert/assertions_test.go b/assert/assertions_test.go index b5193a582..cae11f81d 100644 --- a/assert/assertions_test.go +++ b/assert/assertions_test.go @@ -672,20 +672,18 @@ func TestContainsNotContainsOnNilValue(t *testing.T) { } func TestSubsetNotSubset(t *testing.T) { - - // MTestCase adds a custom message to the case cases := []struct { - expected interface{} - actual interface{} - result bool - message string + list interface{} + subset interface{} + result bool + message string }{ // cases that are expected to contain - {[]int{1, 2, 3}, nil, true, "given subset is nil"}, - {[]int{1, 2, 3}, []int{}, true, "any set contains the nil set"}, - {[]int{1, 2, 3}, []int{1, 2}, true, "[1, 2, 3] contains [1, 2]"}, - {[]int{1, 2, 3}, []int{1, 2, 3}, true, "[1, 2, 3] contains [1, 2, 3"}, - {[]string{"hello", "world"}, []string{"hello"}, true, "[\"hello\", \"world\"] contains [\"hello\"]"}, + {[]int{1, 2, 3}, nil, true, `nil is the empty set which is a subset of every set`}, + {[]int{1, 2, 3}, []int{}, true, `[] is a subset of ['\x01' '\x02' '\x03']`}, + {[]int{1, 2, 3}, []int{1, 2}, true, `['\x01' '\x02'] is a subset of ['\x01' '\x02' '\x03']`}, + {[]int{1, 2, 3}, []int{1, 2, 3}, true, `['\x01' '\x02' '\x03'] is a subset of ['\x01' '\x02' '\x03']`}, + {[]string{"hello", "world"}, []string{"hello"}, true, `["hello"] is a subset of ["hello" "world"]`}, {map[string]string{ "a": "x", "c": "z", @@ -693,12 +691,12 @@ func TestSubsetNotSubset(t *testing.T) { }, map[string]string{ "a": "x", "b": "y", - }, true, `{ "a": "x", "b": "y", "c": "z"} contains { "a": "x", "b": "y"}`}, + }, true, `map["a":"x" "b":"y"] is a subset of map["a":"x" "b":"y" "c":"z"]`}, // cases that are expected not to contain - {[]string{"hello", "world"}, []string{"hello", "testify"}, false, "[\"hello\", \"world\"] does not contain [\"hello\", \"testify\"]"}, - {[]int{1, 2, 3}, []int{4, 5}, false, "[1, 2, 3] does not contain [4, 5"}, - {[]int{1, 2, 3}, []int{1, 5}, false, "[1, 2, 3] does not contain [1, 5]"}, + {[]string{"hello", "world"}, []string{"hello", "testify"}, false, `[]string{"hello", "world"} does not contain "testify"`}, + {[]int{1, 2, 3}, []int{4, 5}, false, `[]int{1, 2, 3} does not contain 4`}, + {[]int{1, 2, 3}, []int{1, 5}, false, `[]int{1, 2, 3} does not contain 5`}, {map[string]string{ "a": "x", "c": "z", @@ -706,35 +704,51 @@ func TestSubsetNotSubset(t *testing.T) { }, map[string]string{ "a": "x", "b": "z", - }, false, `{ "a": "x", "b": "y", "c": "z"} does not contain { "a": "x", "b": "z"}`}, + }, false, `map[string]string{"a":"x", "b":"y", "c":"z"} does not contain map[string]string{"a":"x", "b":"z"}`}, + {map[string]string{ + "a": "x", + "b": "y", + }, map[string]string{ + "a": "x", + "b": "y", + "c": "z", + }, false, `map[string]string{"a":"x", "b":"y"} does not contain map[string]string{"a":"x", "b":"y", "c":"z"}`}, } for _, c := range cases { t.Run("SubSet: "+c.message, func(t *testing.T) { - mockT := new(testing.T) - res := Subset(mockT, c.expected, c.actual) + mockT := new(mockTestingT) + res := Subset(mockT, c.list, c.subset) if res != c.result { - if res { - t.Errorf("Subset should return true: %s", c.message) - } else { - t.Errorf("Subset should return false: %s", c.message) + t.Errorf("Subset should return %t: %s", c.result, c.message) + } + if !c.result { + expectedFail := c.message + actualFail := mockT.errorString() + if !strings.Contains(actualFail, expectedFail) { + t.Log(actualFail) + t.Errorf("Subset failure should contain %q but was %q", expectedFail, actualFail) } } }) } for _, c := range cases { t.Run("NotSubSet: "+c.message, func(t *testing.T) { - mockT := new(testing.T) - res := NotSubset(mockT, c.expected, c.actual) + mockT := new(mockTestingT) + res := NotSubset(mockT, c.list, c.subset) // NotSubset should match the inverse of Subset. If it doesn't, something is wrong - if res == Subset(mockT, c.expected, c.actual) { - if res { - t.Errorf("NotSubset should return true: %s", c.message) - } else { - t.Errorf("NotSubset should return false: %s", c.message) + if res == Subset(mockT, c.list, c.subset) { + t.Errorf("NotSubset should return %t: %s", !c.result, c.message) + } + if c.result { + expectedFail := c.message + actualFail := mockT.errorString() + if !strings.Contains(actualFail, expectedFail) { + t.Log(actualFail) + t.Errorf("NotSubset failure should contain %q but was %q", expectedFail, actualFail) } } })