Re: [boost] Re: Review: Boost Test Library

From: Beman Dawes (bdawes_at_[hidden])
Date: 2001-02-09 15:57:02

At 07:21 PM 2/9/2001 +0000, williamkempf_at_[hidden] wrote:

 >Documentation:
 >
 >test_lib_design.htm has the text "and an design oriented design".
 >Honestly, I'm not sure I know what's being said in this sentence.  My
 >guess is that you accidently used the word "design" twice, and the
 >first word was meant to be something else.  Regardless it would be "a
 >design" not "an design".

Supposed to read: "an object-oriented design"

Fixed.

 >test_tools.htm has the text "human inspection of output to detect
 >error messages is to time consuming and unreliable" under Rationale.
 >Need to change "to" to "too".

Fixed.

 >Implementation:
 >
 >cpp_main.cpp uses a form of conditional include in which an error is
 >generated if the file is included twice.  This "feels" wrong to me.
 >It suggests that you're attempting to prevent violations of ODR, but
 >in fact it will only prevent multiple inclusions in the same
 >translation unit.  I think general usage patterns will insure that
 >the ODR is not violated and that there's no need for artificially
 >trying to protect the programmer from themselves, especially when
 >it's only a partial solution.

I see your point.  The code is there because the file is sometimes included 
as a header, and it is traditional to protect headers from double 
inclusion.  But of course it isn't a header in the normal sense, so what 
you say makes sense.  I think about it a bit more.

 >Also, since catch_exceptions takes a function object as the first
 >parameter the use of a function pointer and two global variables
 >doesn't feel right.  I'd suggest using a true function object
 >constructed with the argc and argv parameters instead.  I know that
 >this is a personal preference kind of thing, but usage of global
 >variables for this sort of thing seems very questionable.

They actually aren't globals; they are in the unnamed namespace and thus 
safe to use AFAIK.  But none-the-less, I had exactly the same thought about 
using a true function object.  My reasoning went something like "why bother 
since it doesn't actually buy you anything."  But I may reconsider in light 
of your comment.

 >In catch_exceptions.hpp I wonder about the usefulness of capturing
 >every exception type in the std::exception heirarchy.  If it's
 >beneficial to have the exact type reported then it seems that there
 >needs to be a way to specify types that will be caught.  Otherwise
 >libraries that extend this heirarchy with their own exception types
 >will be reported as generic exception types while the standard
 >exception types that extend the heirarchy are more precise.  I'd be
 >in favor of just catching std::exception types instead.

I don't agree.  I think we should be as explicit as possible.  Furthermore, 
as soon as I get a chance, my plan was to further extend the types caught 
to cover platforms like Windows where it is possible to turn OS exceptions 
into C++ exceptions with some platform dependent code.

 >In test_tools.hpp I'd suggest adding two more macros:
 >BOOST_TEST_FAILURE and BOOST_TEST_CRITICAL_FAILURE.  These would take
 >string descriptions instead of boolean test results for reporting
 >errors that are hard or even impossible to capture in boolean
 >conditions.  For example, if we expect a call to return an exception
 >when can write test code like this:
 >
 >try
 >{
 >   foo();
 >   BOOST_TEST_FAILURE("foo() did not generate a bar() exception");
 >}
 >catch (bar&)
 >{
 >// ok
 >}

I like it!  I also like your use of "CRITICAL" to identify a non-resumable 
error. (Assuming that is what you meant!).  I'm a bit nervous over the name 
BOOST_TEST_ASSERT because I'm afraid readers will think it is just a 
half-baked wrapper.

Maybe the names should be:

    BOOST_TEST_VERIFY
    BOOST_TEST_CRITICAL_VERIFY
    BOOST_TEST_FAILURE
    BOOST_TEST_CRITICAL_FAILURE

or

    BOOST_TEST_VERIFY
    BOOST_TEST_VERIFY_CRITICAL
    BOOST_TEST_FAILURE
    BOOST_TEST_FAILURE_CRITICAL

Comments?

 >That's all that I caught on my initial review.  Over all I believe
 >the submission to be more than appropriate for acceptance.

Thanks!

 >I'm anxiously awaiting the unit test stuff.

Me too.  I hope someone will look at a generic programming approach in 
addition to the more traditional object-oriented approach.

--Beman