Skip to content

Commit f36bfe3

Browse files
danielwhitedanielwhite
authored
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.
1 parent 0ab3ce1 commit f36bfe3

File tree

2 files changed

+72
-68
lines changed

2 files changed

+72
-68
lines changed

‎assert/assertions.go

+29-39
Original file line numberDiff line numberDiff line change
@@ -816,49 +816,44 @@ func Subset(t TestingT, list, subset interface{}, msgAndArgs ...interface{}) (ok
816816
return true // we consider nil to be equal to the nil set
817817
}
818818

819-
defer func() {
820-
if e := recover(); e != nil {
821-
ok = false
822-
}
823-
}()
824-
825819
listKind := reflect.TypeOf(list).Kind()
826-
subsetKind := reflect.TypeOf(subset).Kind()
827-
828820
if listKind != reflect.Array && listKind != reflect.Slice && listKind != reflect.Map {
829821
return Fail(t, fmt.Sprintf("%q has an unsupported type %s", list, listKind), msgAndArgs...)
830822
}
831823

824+
subsetKind := reflect.TypeOf(subset).Kind()
832825
if subsetKind != reflect.Array && subsetKind != reflect.Slice && listKind != reflect.Map {
833826
return Fail(t, fmt.Sprintf("%q has an unsupported type %s", subset, subsetKind), msgAndArgs...)
834827
}
835828

836-
subsetValue := reflect.ValueOf(subset)
837829
if subsetKind == reflect.Map && listKind == reflect.Map {
838-
listValue := reflect.ValueOf(list)
839-
subsetKeys := subsetValue.MapKeys()
830+
subsetMap := reflect.ValueOf(subset)
831+
actualMap := reflect.ValueOf(list)
840832

841-
for i := 0; i < len(subsetKeys); i++ {
842-
subsetKey := subsetKeys[i]
843-
subsetElement := subsetValue.MapIndex(subsetKey).Interface()
844-
listElement := listValue.MapIndex(subsetKey).Interface()
833+
for _, k := range subsetMap.MapKeys() {
834+
ev := subsetMap.MapIndex(k)
835+
av := actualMap.MapIndex(k)
845836

846-
if !ObjectsAreEqual(subsetElement, listElement) {
847-
return Fail(t, fmt.Sprintf("\"%s\" does not contain \"%s\"", list, subsetElement), msgAndArgs...)
837+
if !av.IsValid() {
838+
return Fail(t, fmt.Sprintf("%#v does not contain %#v", list, subset), msgAndArgs...)
839+
}
840+
if !ObjectsAreEqual(ev.Interface(), av.Interface()) {
841+
return Fail(t, fmt.Sprintf("%#v does not contain %#v", list, subset), msgAndArgs...)
848842
}
849843
}
850844

851845
return true
852846
}
853847

854-
for i := 0; i < subsetValue.Len(); i++ {
855-
element := subsetValue.Index(i).Interface()
848+
subsetList := reflect.ValueOf(subset)
849+
for i := 0; i < subsetList.Len(); i++ {
850+
element := subsetList.Index(i).Interface()
856851
ok, found := containsElement(list, element)
857852
if !ok {
858-
return Fail(t, fmt.Sprintf("\"%s\" could not be applied builtin len()", list), msgAndArgs...)
853+
return Fail(t, fmt.Sprintf("%#v could not be applied builtin len()", list), msgAndArgs...)
859854
}
860855
if !found {
861-
return Fail(t, fmt.Sprintf("\"%s\" does not contain \"%s\"", list, element), msgAndArgs...)
856+
return Fail(t, fmt.Sprintf("%#v does not contain %#v", list, element), msgAndArgs...)
862857
}
863858
}
864859

@@ -877,43 +872,38 @@ func NotSubset(t TestingT, list, subset interface{}, msgAndArgs ...interface{})
877872
return Fail(t, "nil is the empty set which is a subset of every set", msgAndArgs...)
878873
}
879874

880-
defer func() {
881-
if e := recover(); e != nil {
882-
ok = false
883-
}
884-
}()
885-
886875
listKind := reflect.TypeOf(list).Kind()
887-
subsetKind := reflect.TypeOf(subset).Kind()
888-
889876
if listKind != reflect.Array && listKind != reflect.Slice && listKind != reflect.Map {
890877
return Fail(t, fmt.Sprintf("%q has an unsupported type %s", list, listKind), msgAndArgs...)
891878
}
892879

880+
subsetKind := reflect.TypeOf(subset).Kind()
893881
if subsetKind != reflect.Array && subsetKind != reflect.Slice && listKind != reflect.Map {
894882
return Fail(t, fmt.Sprintf("%q has an unsupported type %s", subset, subsetKind), msgAndArgs...)
895883
}
896884

897-
subsetValue := reflect.ValueOf(subset)
898885
if subsetKind == reflect.Map && listKind == reflect.Map {
899-
listValue := reflect.ValueOf(list)
900-
subsetKeys := subsetValue.MapKeys()
886+
subsetMap := reflect.ValueOf(subset)
887+
actualMap := reflect.ValueOf(list)
901888

902-
for i := 0; i < len(subsetKeys); i++ {
903-
subsetKey := subsetKeys[i]
904-
subsetElement := subsetValue.MapIndex(subsetKey).Interface()
905-
listElement := listValue.MapIndex(subsetKey).Interface()
889+
for _, k := range subsetMap.MapKeys() {
890+
ev := subsetMap.MapIndex(k)
891+
av := actualMap.MapIndex(k)
906892

907-
if !ObjectsAreEqual(subsetElement, listElement) {
893+
if !av.IsValid() {
894+
return true
895+
}
896+
if !ObjectsAreEqual(ev.Interface(), av.Interface()) {
908897
return true
909898
}
910899
}
911900

912901
return Fail(t, fmt.Sprintf("%q is a subset of %q", subset, list), msgAndArgs...)
913902
}
914903

915-
for i := 0; i < subsetValue.Len(); i++ {
916-
element := subsetValue.Index(i).Interface()
904+
subsetList := reflect.ValueOf(subset)
905+
for i := 0; i < subsetList.Len(); i++ {
906+
element := subsetList.Index(i).Interface()
917907
ok, found := containsElement(list, element)
918908
if !ok {
919909
return Fail(t, fmt.Sprintf("\"%s\" could not be applied builtin len()", list), msgAndArgs...)

‎assert/assertions_test.go

+43-29
Original file line numberDiff line numberDiff line change
@@ -672,69 +672,83 @@ func TestContainsNotContainsOnNilValue(t *testing.T) {
672672
}
673673

674674
func TestSubsetNotSubset(t *testing.T) {
675-
676-
// MTestCase adds a custom message to the case
677675
cases := []struct {
678-
expected interface{}
679-
actual interface{}
680-
result bool
681-
message string
676+
list interface{}
677+
subset interface{}
678+
result bool
679+
message string
682680
}{
683681
// cases that are expected to contain
684-
{[]int{1, 2, 3}, nil, true, "given subset is nil"},
685-
{[]int{1, 2, 3}, []int{}, true, "any set contains the nil set"},
686-
{[]int{1, 2, 3}, []int{1, 2}, true, "[1, 2, 3] contains [1, 2]"},
687-
{[]int{1, 2, 3}, []int{1, 2, 3}, true, "[1, 2, 3] contains [1, 2, 3"},
688-
{[]string{"hello", "world"}, []string{"hello"}, true, "[\"hello\", \"world\"] contains [\"hello\"]"},
682+
{[]int{1, 2, 3}, nil, true, `nil is the empty set which is a subset of every set`},
683+
{[]int{1, 2, 3}, []int{}, true, `[] is a subset of ['\x01' '\x02' '\x03']`},
684+
{[]int{1, 2, 3}, []int{1, 2}, true, `['\x01' '\x02'] is a subset of ['\x01' '\x02' '\x03']`},
685+
{[]int{1, 2, 3}, []int{1, 2, 3}, true, `['\x01' '\x02' '\x03'] is a subset of ['\x01' '\x02' '\x03']`},
686+
{[]string{"hello", "world"}, []string{"hello"}, true, `["hello"] is a subset of ["hello" "world"]`},
689687
{map[string]string{
690688
"a": "x",
691689
"c": "z",
692690
"b": "y",
693691
}, map[string]string{
694692
"a": "x",
695693
"b": "y",
696-
}, true, `{ "a": "x", "b": "y", "c": "z"} contains { "a": "x", "b": "y"}`},
694+
}, true, `map["a":"x" "b":"y"] is a subset of map["a":"x" "b":"y" "c":"z"]`},
697695

698696
// cases that are expected not to contain
699-
{[]string{"hello", "world"}, []string{"hello", "testify"}, false, "[\"hello\", \"world\"] does not contain [\"hello\", \"testify\"]"},
700-
{[]int{1, 2, 3}, []int{4, 5}, false, "[1, 2, 3] does not contain [4, 5"},
701-
{[]int{1, 2, 3}, []int{1, 5}, false, "[1, 2, 3] does not contain [1, 5]"},
697+
{[]string{"hello", "world"}, []string{"hello", "testify"}, false, `[]string{"hello", "world"} does not contain "testify"`},
698+
{[]int{1, 2, 3}, []int{4, 5}, false, `[]int{1, 2, 3} does not contain 4`},
699+
{[]int{1, 2, 3}, []int{1, 5}, false, `[]int{1, 2, 3} does not contain 5`},
702700
{map[string]string{
703701
"a": "x",
704702
"c": "z",
705703
"b": "y",
706704
}, map[string]string{
707705
"a": "x",
708706
"b": "z",
709-
}, false, `{ "a": "x", "b": "y", "c": "z"} does not contain { "a": "x", "b": "z"}`},
707+
}, false, `map[string]string{"a":"x", "b":"y", "c":"z"} does not contain map[string]string{"a":"x", "b":"z"}`},
708+
{map[string]string{
709+
"a": "x",
710+
"b": "y",
711+
}, map[string]string{
712+
"a": "x",
713+
"b": "y",
714+
"c": "z",
715+
}, false, `map[string]string{"a":"x", "b":"y"} does not contain map[string]string{"a":"x", "b":"y", "c":"z"}`},
710716
}
711717

712718
for _, c := range cases {
713719
t.Run("SubSet: "+c.message, func(t *testing.T) {
714720

715-
mockT := new(testing.T)
716-
res := Subset(mockT, c.expected, c.actual)
721+
mockT := new(mockTestingT)
722+
res := Subset(mockT, c.list, c.subset)
717723

718724
if res != c.result {
719-
if res {
720-
t.Errorf("Subset should return true: %s", c.message)
721-
} else {
722-
t.Errorf("Subset should return false: %s", c.message)
725+
t.Errorf("Subset should return %t: %s", c.result, c.message)
726+
}
727+
if !c.result {
728+
expectedFail := c.message
729+
actualFail := mockT.errorString()
730+
if !strings.Contains(actualFail, expectedFail) {
731+
t.Log(actualFail)
732+
t.Errorf("Subset failure should contain %q but was %q", expectedFail, actualFail)
723733
}
724734
}
725735
})
726736
}
727737
for _, c := range cases {
728738
t.Run("NotSubSet: "+c.message, func(t *testing.T) {
729-
mockT := new(testing.T)
730-
res := NotSubset(mockT, c.expected, c.actual)
739+
mockT := new(mockTestingT)
740+
res := NotSubset(mockT, c.list, c.subset)
731741

732742
// NotSubset should match the inverse of Subset. If it doesn't, something is wrong
733-
if res == Subset(mockT, c.expected, c.actual) {
734-
if res {
735-
t.Errorf("NotSubset should return true: %s", c.message)
736-
} else {
737-
t.Errorf("NotSubset should return false: %s", c.message)
743+
if res == Subset(mockT, c.list, c.subset) {
744+
t.Errorf("NotSubset should return %t: %s", !c.result, c.message)
745+
}
746+
if c.result {
747+
expectedFail := c.message
748+
actualFail := mockT.errorString()
749+
if !strings.Contains(actualFail, expectedFail) {
750+
t.Log(actualFail)
751+
t.Errorf("NotSubset failure should contain %q but was %q", expectedFail, actualFail)
738752
}
739753
}
740754
})

0 commit comments

Comments
 (0)