Re: [boost] Call for Review: Boost.Test documentation rewrite

Subject: Re: [boost] Call for Review: Boost.Test documentation rewrite
From: Mathieu Champlon (m.champlon_at_[hidden])
Date: 2014-01-06 03:57:51

Hi,

As others have said, this is really a major and much needed improvement 
over the existing documentation.
I especially like the little tricks here and there (heck I've been using 
Boost.Test for years and I learned a few !) as well as the design 
suggestions ("Testing Protected or Private Members") and the big picture 
overviews ("Test Case Design and Maintenance").

Here are a few comments, feel free to discard some, most or all of them. :)

* source code links

When clicking on a source code link (any of the "Example source code" of 
the tutorials section for instance) my browser (firefox) downloads the 
file and attempts to open it using Visual Studio.
I would rather be able to display the source code as html for a quick 
look instead.

* tutorials/testing_with_fixtures.html

The first example has a constructor and destructor for the fixture. 
While it's explained a little further that the default ones are fine 
here (and it should be obvious to a seasoned C++ developer), I fear that 
beginners who skim through the documentation will simply copy-paste this 
into their fixtures cluttering their code needlessly. Ideally having it 
the other way around (start with a simple fixture without 
constructor/destructor, then complicating things further) would be 
better, but maybe just moving the "In most cases, the setup will be a 
little more involved than just creating an instance variable and there 
will be some work done in the constructor of the fixture. We could 
simply use the default constructor and destructor of our fixture in this 
case. We've written the constructor and destructor explicitly to 
emphasize that this is where setup and tear down occur. " part into a 
"Note" right after the code would prove enough ?

* tutorials/running_selected_tests.html

I find "We can list the available tests in a test executable with 
--report_level set to detailed" a bit misleading as it doesn't just list 
the tests but still primarily executes them.
Maybe rephrase it to something along the line of "Executed tests can be 
listed with --report_level set to detailed when running a test executable" ?

Is there any reason why you didn't mention using 
--run_test=hello_world_* first in order to filter on the test case names 
instead of jumping into test suites ?
I personally rarely use test suites because I usually use a naming 
convention like the one you introduced in a previous tutorial.

* guide/compilation/minimal_header.html

In the "Meaning" column I fear that for instance "the test fails and 
test execution terminates" may leave a (perhaps non English native) 
reader wondering whether it aborts the current test or the test 
application ?

* guide/test_case_design.html

Maybe add a link to http://en.wikipedia.org/wiki/Test-driven_development 
to "(...), using test-driven development" ?

The Visual Studio configuration snapshot is a bit big in my opinion :)

Under "Goal: Repeatable Test" a bullet point list doesn't display properly.

* guide/testing_file_io.html

Just for the record I found writing tests with files to be one of the 
recurring pitfalls where manipulating actual files does indeed help, 
because when abstracting the file system things like re-entrance and 
file locking can be easily remain untested.

Any reason for the "extern" when declaring text_files ?

I believe the static for defining ARBITRARY_DIRECTORY_NAME and all isn't 
needed. Also the upper case names are usually reserved for macros (in 
Boost).

* guide/mocking_collaborators.html

Being the author of turtle I certainly appreciate the spotlight here, 
but isn't this part a bit out of Boost.Test documentation scope ?

* guide/testing__main_.html

Again, any reason for the "extern" ? I'm just thinking it might confuse 
beginners.

* reference/test_case/boost_test_case_template.html

The fact that 'signed_types' is defined elsewhere is a bit odd 
especially since this page comes first when browsing the reference pages 
using the "next" button.
Also 'unsigned_types' doesn't show up anywhere as it's simply not included.
Maybe move both to the BOOST_TEST_CASE_TEMPLATE page or add them to both 
relevant pages ?

* reference/test_suite/boost_test_suite.html

manual_registration.cpp starts with a "using namespace 
boost::unit_test;" which doesn't appear in the documentation making it 
difficult to understand where the functions come from.
Note that the same issue appears in 
guide/manually_registering_test_cases_and_suites.html which includes the 
same code.

* reference/assertion.html

There is a typo in the note e.g. "be defiend" instead of "be defined".

* reference/assertion/*

None of the pages provide links to the source code files, is it on purpose ?

* reference/assertion/_boost____level___close_.html and 
reference/assertion/_boost____level___close_fraction_.html

Both have the exact same description and example code. An explanation on 
how they differ would certainly prove useful as this is probably one of 
the most asked question about Boost.Test on the mailing lists.
Also a note about when to use BOOST_level_SMALL instead would be nice.

* reference/assertion/boost_level_equal.html

In example_equal_custom_compare there's an extra space in the first 
initialization which might be confusing.
Also I don't get how the comment is relevant given that operator== uses 
std::string to make the comparison ?

* reference/assertion/_boost____level___equal_collections_.html

I believe "static" to be deprecated at a namespace level ?

I would personally rather use
     BOOST_REQUIRE_EQUAL_COLLECTIONS(
         boost::begin( expected ), boost::end( expected ),
         boost::begin( actual ), boost::end( actual ));
which has the clear advantage to cover all the cases, but I understand 
the need for more "raw" examples.

* reference/assertion/boost_level_exception.html, 
reference/assertion/boost_level_message.html and 
reference/assertion/_boost____level___predicate_.html

The "static" aren't really needed, I believe.

* reference/assertion/_boost____level___small_.html

Maybe explain the unit of the tolerance for completeness and coherence 
with BOOST_level_CLOSE* ?

Thanks for doing this !

MAT.