Re: Formal review: New Boost.Test Library

From: rogeeff (rogeeff_at_[hidden])
Date: 2001-12-06 12:58:39

--- In boost_at_y..., "Fernando Cacciola" <fcacciola_at_g...> wrote:
> 
> > > ) 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.
> >
> How about:  BOOST_CHECK_msg()..
> Are macros names usually required to be ALL upercase?

What is the difference from 'sematically implying' standpoing and 
macro do need to be uppercase.

> > > ) 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 understand your point that the predicate *could* be too cryptic.
> However, notice that this way you are forced either to add the 
predicate
> string yourself or to miss it from the output.

And you force the predicate *always* to be presend. Message should be 
clear enouph. In your case it could be: "Enter a positive number"
I choose this way cause it more flexible (though may be less 
convinient in some rare cases).

> > ) 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.
> > > 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.
> >
> See my other post...

Be also aware that any separate library will introduce a coupling 
with Boost.Test Tools, cause Test Tools will need to use it 
internally. So it should be al least compilable everywhere.

> 
> > >
> > > ) 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.
> OK.
> 
> > So all methods of class wrapstrstream should be const.
> Why? Do you mean that inside the function that took
> 
>  wrapstrstream  const&
> 
> you need to use operator << ?

Yes. And str() is const either and used internally.

> 
> Then why should it be taken as 'const&' instead of '&'?
cause 'wrapstrstream in most cases created inline'

> 
> Fernando Cacciola
> Sierra s.r.l.
> fcacciola_at_g...
> www.gosierra.com

Gennadiy.