Anatomy of a bad test
Joakim via Digitalmars-d
digitalmars-d at puremagic.com
Thu Jul 7 01:27:58 PDT 2016
Dan recently found an off-by-one bug in std.conv.toChars from
Phobos, that somehow wasn't surfaced by our tests:
https://issues.dlang.org/show_bug.cgi?id=16192
I looked into why this opSlice test
assert(r[1..2].array == "0");
wasn't triggering it, thought I'd write up what I found and how
this can be avoided.
It turns out that the test was checking the corner case where you
slice up to the length of the array, 2, and only compared the
last character, "0". In that case, opSlice was supposed to
right-shift the input, 16u, zero times and check the last byte,
0. However, the off-by-one bug meant it right-shifted the input
-4 times. Given such an invalid negative count, both ARM and x86
CPUs will just put zero in the result register, _which also
happens to have the same last byte, 0_. Since the slice only
checked the last byte and the result was 0 in either case, the
test passed.
So what went wrong here? As Dan noted in a PR to fix this, the
fundamental mistake was not checking the happy path, ie the
common path with no corner cases or exceptions, as almost all
input would've been wrong because of this bug. This test only
checked a corner case and happened to feed it the exact test
input that coincided with undefined behavior from the right-shift
operator. A fix for toChars was just merged that extensively
tests the happy path:
https://github.com/dlang/phobos/pull/4489
It would also be nice if we could put in runtime checks to warn
about such bugs. Clang has sanitizers, including a runtime check
for negative shifts, that would have caught this, if enabled.
I'll look into adding that to ldc. Johan found another
right-shift bug in Phobos recently, that hadn't been found by the
tests:
https://github.com/dlang/phobos/pull/4509
Who wrote toChars and its tests? I just looked it up yesterday
and was surprised to find Walter wrote it last summer:
https://github.com/dlang/phobos/pull/3477
That PR was checked by Andrei and Dmitry, two really good D
reviewers. I'm pretty sure most everybody else would have missed
it too: after all, opSlice passed the same test three times! And
frankly, this is a small part of Phobos and the PR, though it
would have broken any code that tried to slice the result.
Moral of the story: always check the happy path with your tests.
It's easy to get caught up in all the corner cases, just don't
forget the happy path.
More information about the Digitalmars-d
mailing list