www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Anatomy of a bad test

reply Joakim <dlang joakim.fea.st> writes:
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.
Jul 07 2016
next sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 07/07/2016 04:27 AM, Joakim wrote:

 Dan recently found an off-by-one bug in std.conv.toChars from Phobos,
 that somehow wasn't surfaced by our tests:


Nice! Worth a blog entry? -- Andrei
Jul 07 2016
parent Joakim <dlang joakim.fea.st> writes:
On Thursday, 7 July 2016 at 13:48:56 UTC, Andrei Alexandrescu 
wrote:

 On 07/07/2016 04:27 AM, Joakim wrote:

 Dan recently found an off-by-one bug in std.conv.toChars from 
 Phobos,
 that somehow wasn't surfaced by our tests:


 Nice! Worth a blog entry? -- Andrei


Maybe once we do something different about our process, whether 
adding sanitizers or adding coverage checks to github PRs, seems 
too short right now.

On Thursday, 7 July 2016 at 14:04:30 UTC, Seb wrote:

 On Thursday, 7 July 2016 at 08:27:58 UTC, Joakim wrote:

 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.


 Isn't the moral that we should start to check  code coverage 
 for PRs and alert reviewers if the code coverage decreased or 
 new, untested lines are added?


While that's a good idea, it wouldn't have caught this bug, as 
the corner case test did exercise all of the opSlice function 
that had the off-by-one bug.  It just did so with test input that 
happened to pass anyway.
Jul 07 2016
prev sibling parent Seb <seb wilzba.ch> writes:
On Thursday, 7 July 2016 at 08:27:58 UTC, Joakim wrote:

 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.


Isn't the moral that we should start to check  code coverage for 
PRs and alert reviewers if the code coverage decreased or new, 
untested lines are added?
Jul 07 2016