Re: Review: Boost Test Library

From: williamkempf_at_[hidden]
Date: 2001-02-09 14:21:55

--- In boost_at_y..., Jens Maurer <Jens.Maurer_at_g...> wrote:
> 
> The review for Beman Dawes' proposal of a Boost Test Library
> has been scheduled for next week, i.e.
> 
>   12 Feb 2001 to 18 Feb 2001
> 
> The library is available at
>   http://groups.yahoo.com/group/boost/files/test_lib.zip
> 
> Please send your comments to the boost mailing list, or to the
> review manager for issues you do not want to post to the list
> at large.
> 
> Please make sure that your review comments include an overall
> statement whether the library should be accepted or not. For
> additional information on the review process, see
> http://www.boost.org/more/formal_review_process.htm

Sorry to respond before formal review is supposed to start, but I'm 
going to be using the test_lib stuff during my final polishing of the 
BTL, which I'm hip deep in.  I've got time to comment now, but may 
not next week.

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".

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".

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.

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.

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.

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
}

That's all that I caught on my initial review.  Over all I believe 
the submission to be more than appropriate for acceptance.  I'm 
anxiously awaiting the unit test stuff.

Bill Kempf