www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - std.unittests vote tally

reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
Vote has closed last night at 23:59:59:99, but I accepted Lars' late vote.

Thanks Jonathan for responding to comments and suggestions, and for a 
very dedicated attitude throughout.

YES votes mean wholesale acceptance of the library. NO means either 
partial acceptance or no acceptance at all.

We have eight NOs and even YESs. (In fairness I have changed my vote 
after Don committed to improve assert(), but forgot to submit it.)

NO:

SHOO (arguments: unittest code should be easy to read without prior 
knowledge)
Don (arguments: assertPred is harder to read than assert, "don't use if 
you don't like" doesn't apply to Phobos, Phobos becomes difficult to 
read if we continue adopting clever functions, something that has any 
appearance of being complicated needs VERY strong justification. Voted 
yes for assertThrown and assertNotThrown. Asked for bugzilla enhancement 
requests to have assert obviate assertPred)
Michel Fortin
Brad Roberts
David Nadlinger (yes for assertThrown, 50/50 for assertNotThrown and 
collectExceptionMsg, no for assertPred)
spir (yes to assertThrown, abstain for assertNotThrown and 
collectExceptionMsg)
Jim (reiterates that at best assert should be improved)
Lars T. Kyllingstad (on the fence with assertPred)

YES:

Jens Mueller
bearophile
Andrej Mitrovic
Nick Sabalausky
Andrei Alexandrescu (contingent on reducing the size of examples)
Masahiro Nakagawa (with a few notes)
Andrew Wiley

Reviewer Manager's decision:
============================

We have had an unexpected development: we can change assert() to obviate 
assertPred(), and Don all but promised he'll look into it. This means if 
we accept the library as it is, we'll look at a function on the brink of 
deprecation for the sake of a short-term benefit. Perhaps this is not 
the best course of action.

So let's not put assertPred() for now in Phobos, though Jonathan is to 
be commended for his work which is leading to a vast improvement to a 
core facility.

assertThrown seems to be liked by a vast majority - please add to 
std.exception at your earliest convenience.

assertNotThrown and collectExceptionMsg are on the fence and it's 
unclear whether some "NO" voters want them as isolated functions. Let us 
take a one-week vote for each. I will create one thread for each.


Thanks to everyone for participating, and special thanks to Jonathan!

Andrei
Feb 08 2011
next sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday 08 February 2011 07:27:55 Andrei Alexandrescu wrote:
 Vote has closed last night at 23:59:59:99, but I accepted Lars' late vote.
 
 Thanks Jonathan for responding to comments and suggestions, and for a
 very dedicated attitude throughout.
 
 YES votes mean wholesale acceptance of the library. NO means either
 partial acceptance or no acceptance at all.
 
 We have eight NOs and even YESs. (In fairness I have changed my vote
 after Don committed to improve assert(), but forgot to submit it.)
 
 NO:
 
 SHOO (arguments: unittest code should be easy to read without prior
 knowledge)
 Don (arguments: assertPred is harder to read than assert, "don't use if
 you don't like" doesn't apply to Phobos, Phobos becomes difficult to
 read if we continue adopting clever functions, something that has any
 appearance of being complicated needs VERY strong justification. Voted
 yes for assertThrown and assertNotThrown. Asked for bugzilla enhancement
 requests to have assert obviate assertPred)
 Michel Fortin
 Brad Roberts
 David Nadlinger (yes for assertThrown, 50/50 for assertNotThrown and
 collectExceptionMsg, no for assertPred)
 spir (yes to assertThrown, abstain for assertNotThrown and
 collectExceptionMsg)
 Jim (reiterates that at best assert should be improved)
 Lars T. Kyllingstad (on the fence with assertPred)
 
 YES:
 
 Jens Mueller
 bearophile
 Andrej Mitrovic
 Nick Sabalausky
 Andrei Alexandrescu (contingent on reducing the size of examples)
 Masahiro Nakagawa (with a few notes)
 Andrew Wiley
 
 Reviewer Manager's decision:
 ============================
 
 We have had an unexpected development: we can change assert() to obviate
 assertPred(), and Don all but promised he'll look into it. This means if
 we accept the library as it is, we'll look at a function on the brink of
 deprecation for the sake of a short-term benefit. Perhaps this is not
 the best course of action.
 
 So let's not put assertPred() for now in Phobos, though Jonathan is to
 be commended for his work which is leading to a vast improvement to a
 core facility.
 
 assertThrown seems to be liked by a vast majority - please add to
 std.exception at your earliest convenience.
 
 assertNotThrown and collectExceptionMsg are on the fence and it's
 unclear whether some "NO" voters want them as isolated functions. Let us
 take a one-week vote for each. I will create one thread for each.
 
 
 Thanks to everyone for participating, and special thanks to Jonathan!

Enhancement request for assert: http://d.puremagic.com/issues/show_bug.cgi?id=5547 Okay. I'll look at doing another proposal which has the functionality of assertPred which doesn't make sense to add to assert, though I'll probably wait until the voting for assertNotThrown and collectExceptionMsg is done. I would point out, however, that it would be rather silly to include assertThrown and not assertNotThrown. Good unit tests should test _both_ that a function succeeds as it's supposed to _and_ that it fails as it's supposed to. So, I would hope that people vote in favor of assertNotThrown. collectExceptionMsg isn't as critical, but it really does make it easy to test that exception messages are correct, since if you use collectException, you have to worry about checking for null before you can check the message. With collectExceptionMsg, it can be a an easy one-liner to check exception messages. Without it, you end up taking several lines, because you have to save and check the exception for null before you can check its message. I'll wait for the vote on assertNotThrown and collectExceptionMsg to be completed before putting assertThrown in Phobos. Then it can just all be taken care of at once. - Jonathan M Davis
Feb 08 2011
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/8/11 10:54 AM, Jonathan M Davis wrote:
 Enhancement request for assert:
 http://d.puremagic.com/issues/show_bug.cgi?id=5547

Thanks!
 Okay. I'll look at doing another proposal which has the functionality of
 assertPred which doesn't make sense to add to assert, though I'll probably wait
 until the voting for assertNotThrown and collectExceptionMsg is done.

 I would point out, however, that it would be rather silly to include
 assertThrown and not assertNotThrown. Good unit tests should test _both_ that a
 function succeeds as it's supposed to _and_ that it fails as it's supposed to.
 So, I would hope that people vote in favor of assertNotThrown.

I think many people would emulate assertNotThrown by simply calling the function and... well if it throws then the unittest fails.
 collectExceptionMsg isn't as critical, but it really does make it easy to test
 that exception messages are correct, since if you use collectException, you
have
 to worry about checking for null before you can check the message. With
 collectExceptionMsg, it can be a an easy one-liner to check exception messages.
 Without it, you end up taking several lines, because you have to save and check
 the exception for null before you can check its message.

 I'll wait for the vote on assertNotThrown and collectExceptionMsg to be
 completed before putting assertThrown in Phobos. Then it can just all be taken
 care of at once.

Sounds great. Thanks! Andrei
Feb 08 2011
next sibling parent Michel Fortin <michel.fortin michelf.com> writes:
On 2011-02-08 12:26:23 -0500, Andrej Mitrovic 
<andrej.mitrovich gmail.com> said:

 So in the most basic form assertThrown is used to check that our
 functions throw on bad (user) input,
 and assertNotThrown is used to check that our functions work with valid input?

Most functions are said to work whey they have the desired effect, not when they do not throw. To make sure a function works, you should call it and then check the return value and/or whatever the side effects should be. -- Michel Fortin michel.fortin michelf.com http://michelf.com/
Feb 08 2011
prev sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 2/8/11 12:20 PM, Jonathan M Davis wrote:
 On Tuesday, February 08, 2011 08:36:27 Andrei Alexandrescu wrote:
 On 2/8/11 10:54 AM, Jonathan M Davis wrote:
 Enhancement request for assert:
 http://d.puremagic.com/issues/show_bug.cgi?id=5547

Thanks!
 Okay. I'll look at doing another proposal which has the functionality of
 assertPred which doesn't make sense to add to assert, though I'll
 probably wait until the voting for assertNotThrown and
 collectExceptionMsg is done.

 I would point out, however, that it would be rather silly to include
 assertThrown and not assertNotThrown. Good unit tests should test _both_
 that a function succeeds as it's supposed to _and_ that it fails as it's
 supposed to. So, I would hope that people vote in favor of
 assertNotThrown.


True. But the test is clearer if you're explicitly testing that the function doesn't throw instead of just having a stray function call that isn't tested. For instance. assertNotThrown!DateTimeException(TimeOfDay(23, 59, 59)); is clearer than TimeOfDay(23, 59, 59); In the first case, it's clear that you're testing that the function call does not throw. In the second, it's a function call that seems to do nothing, since its result isn't saved, it takes no references, and it's not part of an assert. Also, the first one results in an AssertError that clearly states that the problem is that the function threw when it wasn't supposed to, whereas in the second, you just get a stray exception which is likely going to be a bit hard to track down - even with a stack trace - because the unit test blocks get named with seemingly random numbers rather than real names and tracking down which unittest block an exception was thrown from is a royal pain (one more reason why we really should have named unit tests). So, I think that assertNotThrown definitely helps with clarity, and it makes it much easier to track down the failure. - Jonathan M Davis

I think I'd write that as assert(!collectException(TimeOfDay(23, 59, 59)); Andrei
Feb 08 2011
prev sibling next sibling parent reply spir <denis.spir gmail.com> writes:
On 02/08/2011 04:54 PM, Jonathan M Davis wrote:
 I would point out, however, that it would be rather silly to include
 assertThrown and not assertNotThrown. Good unit tests should test_both_  that a
 function succeeds as it's supposed to_and_  that it fails as it's supposed to.
 So, I would hope that people vote in favor of assertNotThrown.

I do agree failure cases must be tested (maybe even more) and are very often neglected by programmers in unittests. But in a no-throw case the proper assertion is just a regular assert (at least, in my use of unittests): // dunno the syntax assertThrown ( 1/0, DivisionByZero ); assert ( 1/1 == 1 ); If 1/1 throws DivisionByZero, I get all the information I need. Reason for my question mark about including assertNotThrown. When do I need it? What new does it bring? Sorry, I should have asked/commented earlier on this point (but had too much...). Denis -- _________________ vita es estrany spir.wikidot.com
Feb 08 2011
parent Daniel Gibson <metalcaedes gmail.com> writes:
Am 08.02.2011 18:00, schrieb spir:
 On 02/08/2011 04:54 PM, Jonathan M Davis wrote:
 I would point out, however, that it would be rather silly to include
 assertThrown and not assertNotThrown. Good unit tests should test_both_  that a
 function succeeds as it's supposed to_and_  that it fails as it's supposed to.
 So, I would hope that people vote in favor of assertNotThrown.

I do agree failure cases must be tested (maybe even more) and are very often neglected by programmers in unittests. But in a no-throw case the proper assertion is just a regular assert (at least, in my use of unittests): // dunno the syntax assertThrown ( 1/0, DivisionByZero ); assert ( 1/1 == 1 ); If 1/1 throws DivisionByZero, I get all the information I need. Reason for my question mark about including assertNotThrown. When do I need it? What new does it bring? Sorry, I should have asked/commented earlier on this point (but had too much...). Denis

Maybe it can be nested like assertThrown!Exception( assertNotThrown!MyException( fun(42) ) ); to ensure that fun() doesn't throw a MyException, but does throws another Exception? Cheers, - Daniel
Feb 08 2011
prev sibling next sibling parent reply Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, February 08, 2011 08:36:27 Andrei Alexandrescu wrote:
 On 2/8/11 10:54 AM, Jonathan M Davis wrote:
 Enhancement request for assert:
 http://d.puremagic.com/issues/show_bug.cgi?id=5547

Thanks!
 Okay. I'll look at doing another proposal which has the functionality of
 assertPred which doesn't make sense to add to assert, though I'll
 probably wait until the voting for assertNotThrown and
 collectExceptionMsg is done.
 
 I would point out, however, that it would be rather silly to include
 assertThrown and not assertNotThrown. Good unit tests should test _both_
 that a function succeeds as it's supposed to _and_ that it fails as it's
 supposed to. So, I would hope that people vote in favor of
 assertNotThrown.


True. But the test is clearer if you're explicitly testing that the function doesn't throw instead of just having a stray function call that isn't tested. For instance. assertNotThrown!DateTimeException(TimeOfDay(23, 59, 59)); is clearer than TimeOfDay(23, 59, 59); In the first case, it's clear that you're testing that the function call does not throw. In the second, it's a function call that seems to do nothing, since its result isn't saved, it takes no references, and it's not part of an assert. Also, the first one results in an AssertError that clearly states that the problem is that the function threw when it wasn't supposed to, whereas in the second, you just get a stray exception which is likely going to be a bit hard to track down - even with a stack trace - because the unit test blocks get named with seemingly random numbers rather than real names and tracking down which unittest block an exception was thrown from is a royal pain (one more reason why we really should have named unit tests). So, I think that assertNotThrown definitely helps with clarity, and it makes it much easier to track down the failure. - Jonathan M Davis
Feb 08 2011
parent "Nick Sabalausky" <a a.a> writes:
"Jonathan M Davis" <jmdavisProg gmx.com> wrote in message 
news:mailman.1401.1297185535.4748.digitalmars-d puremagic.com...
 On Tuesday, February 08, 2011 08:36:27 Andrei Alexandrescu wrote:
 On 2/8/11 10:54 AM, Jonathan M Davis wrote:
 Enhancement request for assert:
 http://d.puremagic.com/issues/show_bug.cgi?id=5547

Thanks!
 Okay. I'll look at doing another proposal which has the functionality 
 of
 assertPred which doesn't make sense to add to assert, though I'll
 probably wait until the voting for assertNotThrown and
 collectExceptionMsg is done.

 I would point out, however, that it would be rather silly to include
 assertThrown and not assertNotThrown. Good unit tests should test 
 _both_
 that a function succeeds as it's supposed to _and_ that it fails as 
 it's
 supposed to. So, I would hope that people vote in favor of
 assertNotThrown.


True. But the test is clearer if you're explicitly testing that the function doesn't throw instead of just having a stray function call that isn't tested. For instance. assertNotThrown!DateTimeException(TimeOfDay(23, 59, 59)); is clearer than TimeOfDay(23, 59, 59); In the first case, it's clear that you're testing that the function call does not throw. In the second, it's a function call that seems to do nothing, since its result isn't saved, it takes no references, and it's not part of an assert. Also, the first one results in an AssertError that clearly states that the problem is that the function threw when it wasn't supposed to, whereas in the second, you just get a stray exception which is likely going to be a bit hard to track down - even with a stack trace - because the unit test blocks get named with seemingly random numbers rather than real names and tracking down which unittest block an exception was thrown from is a royal pain (one more reason why we really should have named unit tests). So, I think that assertNotThrown definitely helps with clarity, and it makes it much easier to track down the failure.

This is why I've always felt that assert/assertPred/etc should always check whether an exception was thrown and report it accordingly. That way, the test to make sure the result is correct will *automatically* provide all the benefits of assertNotThrown, but without the developer needing to compulsively "assertNotThrown" every single function they write/test. However, that said, I think assertNotThrown would still be useful for void functions since those don't have a result to assert(). Plus, AIUI, assertNotThrown lets you make sure that a *specific* type of exception isn't thrown for the given arguments, which I can imagine would be useful in certain cases (for instance, if a function had been throwing the wrong type of exception upon bad input and you want to prevent regressions).
Feb 08 2011
prev sibling next sibling parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
So in the most basic form assertThrown is used to check that our
functions throw on bad (user) input,
and assertNotThrown is used to check that our functions work with valid input?

Looks good to me.
Feb 08 2011
prev sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Tuesday, February 08, 2011 09:26:23 Andrej Mitrovic wrote:
 So in the most basic form assertThrown is used to check that our
 functions throw on bad (user) input,
 and assertNotThrown is used to check that our functions work with valid
 input?
 
 Looks good to me.

Yes. - Jonathan M Davis
Feb 08 2011