Re: Formal review: New Boost.Test Library

From: rogeeff (rogeeff_at_[hidden])
Date: 2001-12-06 12:15:31

--- In boost_at_y..., "Fernando Cacciola" <fcacciola_at_g...> wrote:
> Here are some comments on the new Test Library:
> 
> ) There is a problem with the macros like
> 
>   BOOST_TEST_SUITE_MESSAGE_END << end();
> 
>   What is 'end()'? Depends on the context in which the macro is 
*used*, not
> defined.
>   Therefore, this and every other identifier that appears in a 
macro MUST be
> fully qualified.

OK.

> 
> ) In "test_tools.hpp", there are macros (i.e: BOOST_CHECKPOINT) 
which appear
> *before* the declaration of  the objects they use. To me, this is 
confusing,
> I recommend moving those macros to the bottom.

I disagree. Macros are the public interface fot Test Tools and should 
be on top of file.

> 
> ) I don't like the names of the macros 'BOOST_WARN_MESSAGE',
> 'BOOST_CHECK_MESSAGE', 'BOOST_REQUIRE_MESSAGE', they don't do what 
they are
> sematically implying: That is, 'check message' implies: 'check *the*
> message, not 'check *this* and eventually show a message'. I 
suggest....
> something else :)  [just couldn't come up with something]

I hope it would be clear enouph, gived their non _MESSAGE versions. 
Anyway I did not find better names.

> 
> ) Also, I think the the 'message' in 'BOOST_CHECK_MESSAGE' (etc...) 
should
> also contain the predicate, as in 'BOOST_CHECK'.  In other words, 
the
> MESSAGE should be *added* to the output.
> This is useful, because instead of:
> 
>    BOOST_CHECK_MESSAGE(a!=0,"A isn't zero. Enter a different 
number")
> 
> you would write:
> 
>    BOOST_CHECK_MESSAGE(a!=0,"Enter a different number")

I disagree. The purpose of _MESSAGE is t oprovide a readable failure 
description instead of posibly cryptic predicate dump. If you do need 
to add the predicate you can always do. Even Better you can write 
like this
   BOOST_CHECK_MESSAGE(a!=0,"Invalid input: " << a << " != 0" );

So current version allows everything.

> 
> ) I *strongly* disagree with having floating point and collection 
equlity
> tools as part of the test library. They are orthogonal to the unit 
test
> framework. 

Thay are not a part of Unit Test Framework. Thay are part of Test 
Tools.

> There is an unbounded set of predicates that can be tested and
> they shouln't be embebbed in the test library.

For example? And why not

There several tools left over to be included as an extentions: 
BOOST_CHECK_NOT_EQUAL, BOOST_CHECK_LESS, BOOST_CHECK_LESS_EQUAL

> I would agree to provide, in the proper place, a floating point 
comparator
> and a sequence matcher, but they should be outside the test suite.

What if you need to check equality of to collections? Or result of 
you function under test if floating-point value, so you need to check 
how close it is t oexpected result? I think it's a natural part of 
Test Tools component.

> 
> ) Just a question:
> 
> Why does wrapstrstream::operator << returns a 'const' reference?  
This
> requires 'buf' to be mutable.

Instances of wrapstrstream in most cases created inline and passed by 
const references. So all methods of class wrapstrstream should be 
const. And buf should be mutable. We could make operator<< to return 
non-const value. But it is not good to return non-const reference 
from const method.

> 
> ) "test_tools.hpp" should include <except>, since it is deriving a 
class
> from std::exception.

OK

> 
> ) I couldn't understand the role of 'output_test_stream'. I'd like 
to see
> more 'in-header' documentation.

I found couple day ago That I am missing output_test_stream in 
documentation. I will include it with next update.

> 
> Well, that's it for today, more to come later.
> 
> Regards,
> 
> Fernando Cacciola
> Sierra s.r.l.
> fcacciola_at_g...
> www.gosierra.com

Thank you,

Gennadiy.