Flatten Array - Allow tests to PASS for both nil and empty slices.
W8CYE opened this issue · 3 comments
Update flatten_array_test.go so that test cases will pass for both nil and empty slices. Some students have had problems where the test FAILS reporting [] != []:
=== RUN TestFlatten/empty
flatten_array_test.go:12: Flatten([]) = [], want: []
=== RUN TestFlatten/all_values_in_nested_list_are_null
flatten_array_test.go:12: Flatten([<nil> [[[<nil>]]] <nil> <nil> [[<nil> <nil>] <nil>] <nil>]) = [], want: []
--- FAIL: TestFlatten (0.00s)
--- FAIL: TestFlatten/flattens_a_nested_array (0.00s)
--- FAIL: TestFlatten/all_values_in_nested_list_are_null (0.00s)
I propose that we implement the same fix used in Exercise grep : fix test case which incorrectly fails on nil slice, but expect empty slice (PR #2452)
Original
func TestFlatten(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
if actual := Flatten(tc.input); !reflect.DeepEqual(actual, tc.expected) {
t.Errorf("Flatten(%v) = %v, want: %v", tc.input, actual, tc.expected)
}
})
}
}
Proposed
func TestFlatten(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
actual := Flatten(tc.input)
// We do not care whether the result is nil or an empty slice.
if len(tc.expected) == 0 && len(actual) == 0 {
return
}
if !reflect.DeepEqual(actual, tc.expected) {
t.Errorf("Flatten(%v) = %v, want: %v", tc.input, actual, tc.expected)
}
})
}
}
Alternatively we could change the error message to explain that zero length and nil are similar, but not the same.
I am happy to submit the PR based on the feedback to this issue.
I think in the context of this exercise, it makes sense to expect a result, that would actually render as []
when e.g. formatted as JSON (how to return []
from a JSON API is a common question from beginners). I also think it is a good practice for the student if the test fails to think about how to fix this. I totally agree there should be a better error message specifically for this case though. E.g. if it would way "got nil slice, expected empty slice", that would be enough for a student to research their way to a solution I think. Could you adjust the PR in this way (or create a new one)?
I am happy to adjust this further to make it more clear. I took @junedev's idea "got nil slice, expected empty slice" but wanted to keep the same or similar formatting to the existing error messages.
New
=== RUN TestFlatten/empty
flatten_array_test.go:13: Flatten([]) = [] (nil slice), want: [] (empty slice)
Original
=== RUN TestFlatten/empty
flatten_array_test.go:13: Flatten([]) = [], want: []
I considered another apporach of adding the #
verb modifier to differentiate the nil
and empty
slice, but I found the results less clear than the solution above.
t.Errorf("Flatten(%v) = %#v, want: %#v", tc.input, &actual, tc.expected)
=== RUN TestFlatten/empty
flatten_array_test.go:16: Flatten([]) = &[]interface {}(nil), want: []interface {}{}
I had the same thoughts around #v
and came to the same conclusion as you.