Skip to content

Commit

Permalink
Fix Subset/NotSubset when map is missing keys from the subset (#1261)
Browse files Browse the repository at this point in the history 
`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.
  • Loading branch information
@danielwhite
danielwhite authored Feb 25, 2023
1 parent 0ab3ce1 commit f36bfe3
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 68 deletions.
68 changes: 29 additions & 39 deletions assert/assertions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
}
}

Expand All @@ -877,43 +872,38 @@ 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
}
}

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...)
Expand Down
72 changes: 43 additions & 29 deletions assert/assertions_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,69 +672,83 @@ 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",
"b": "y",
}, 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",
"b": "y",
}, 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)
}
}
})
Expand Down

0 comments on commit f36bfe3

Please sign in to comment.