Test utils/ #32
Test utils/ #32
Conversation
| {"ab_c", []string{"a", "b", "c"}, 2, [][]string{{"a", "b"}, {"c"}}}, | ||
| {"abc", []string{"a", "b", "c"}, 3, [][]string{{"a", "b", "c"}}}, | ||
| {"abc4", []string{"a", "b", "c"}, 4, [][]string{{"a", "b", "c"}}}, | ||
| } |
There was a problem hiding this comment.
As the count parameter is a signed integer, try negative numbers 😏
I would suggest making the function documentation more obvious and stating that count <= 0 will either panic or be rewritten to 1. A count of 0 returns an infinite loop emitting empty string arrays until eternity and a negative count results in a slice bounds out of range access.
{"abc0-to-infinity-and-beyond", []string{"a", "b", "c"}, 0, [][]string{{}, {}, {}}},
{"abc-1-crash", []string{"a", "b", "c"}, -1, nil},=== RUN TestBatchSliceOfStrings/abc0-to-infinity-and-beyond
utils_test.go:51:
Error Trace: /home/REDACTED/git/github.com/Icinga/icinga-go-library/utils/utils_test.go:51
Error: Should be false
Test: TestBatchSliceOfStrings/abc0-to-infinity-and-beyond
Messages: receiving from channel should not return anything, received []
=== RUN TestBatchSliceOfStrings/abc-1-crash
panic: runtime error: slice bounds out of range [:-1]
There was a problem hiding this comment.
Where I just ref an issue/PR (like you #32 (comment)), I prefix it with * and a space, so I show its title and status. E.g.:
There was a problem hiding this comment.
After this got merged, please include it in the tests, e.g., by adding another shouldPanic field to the table test struct.
| {"empty_bytes", []byte(nil)}, | ||
| {"space_string", " "}, | ||
| {"space_bytes", []byte(" ")}, | ||
| } |
There was a problem hiding this comment.
The Checksum function panics for other input types. Please test those as well.
Al2Klimov
force-pushed
the
test-utils
branch
from
45590ff
to
22e11dd
Compare
Al2Klimov
force-pushed
the
test-utils
branch
from
22e11dd
to
1a80bdc
Compare
There was a problem hiding this comment.
There are still two open review comments from my previous review: #32 (comment) and #32 (comment). Please address those and feel free to re-request another review again.
| for _, i := range []int{0, -1, -2, -30} { | ||
| t.Run(fmt.Sprint(i), func(t *testing.T) { | ||
| require.Panics(t, func() { BatchSliceOfStrings(context.Background(), nil, i) }) |
There was a problem hiding this comment.
Here, I assert panic on negative numbers.
| for _, st := range unsupported { | ||
| t.Run(st.name, func(t *testing.T) { | ||
| require.Panics(t, func() { Checksum(st.input) }) |
There was a problem hiding this comment.
And here, I assert panic on unsupported types.
No description provided.