exercism/go

Flatten Array - Allow tests to PASS for both nil and empty slices.

W8CYE opened this issue · 3 comments

W8CYE commented

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)?

W8CYE commented

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.