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

Subject: Re: [boost] Call for Review: Boost.Test documentation rewrite
From: Richard (legalize+jeeves_at_[hidden])
Date: 2014-01-06 14:07:22

[Please do not mail me a copy of your followup]

Thank you Mathieu for your detailed feedback.

Mathieu Champlon <m.champlon_at_[hidden]> spake the secret code
<52CA700F.5020104_at_[hidden]> thusly:

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

I think when the code gets deployed to the web site, you end up seeing
a browser-ified version of the source code.  For instance, the
existing links to source files appear this way on the web site, but I
cannot find any indiciation that the quickbook source for other
libraries, or the existing non-quicbook documentation for boost.test,
does anything special to provide this.

>* 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 ?

So if I might paraphrase -- you're saying to display best practices
first and make a note of where the setup/tear down code would appear?

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

Yes, the trunk version of boost.test has a specific option for listing
the tests called --list_content and not executing them.  I'm not a fan
of the name of this argument, but it is present on trunk and not on
release.  When I originally wrote this documentation it was based on
trunk but I rebased it on release in order to get it moving forward.

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

I'll reword this to say that the tests are executed as well as test
names shown.

>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 was using this as a way to introduce test suites :-).

>I personally rarely use test suites because I usually use a naming 
>convention like the one you introduced in a previous tutorial.

If you prefer that style of test organization, was the explanation in
this tutorial good enough for you to see how to use that style?

>* 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 ?

I can reword this for more clarification.

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

Good idea.  More linking never hurts.

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

It is native size.  Were you thinking that providing a smaller,
filtered version of the screen shot with the image being a link to the
native size version?

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

Ah.  Good catch.

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

This is why I talk about acceptance testing being an integral part of
any plan for automated testing.  Unit tests alone are not enough
because they exercise pieces of the system in isolation; you still
need some sort of automated tests that bring all the pieces together
and exercise them as a whole.

>Any reason for the "extern" when declaring text_files ?

It is a function with external linkage.  I don't want to declare it as
a local function, but one that is provided externally.

Do you feel that this is a leftover "C"-ism?

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

That may simply be a matter of style -- I picked this up from "Modern
C++ Programming with Test-Driven Development" where he advises that
using named constants for arbitrary inputs helps make it clear that
the actual value used in the test isn't important so much as how that
value appears in inputs and expected outputs.

Regarding the casing of the identifier, I was falling back on
constants being all caps, but I'll consult the boost style guidelines
and adjust per their recommendations.

>* 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 ?

It is, but mocking objects is too important to be left unmentioned.
Since Turtle is designed to work directly with Boost.Test I wanted to
give it a mention.

>* guide/testing__main_.html
>
>Again, any reason for the "extern" ? I'm just thinking it might confuse 
>beginners.

Same as above.

>* 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 ?

OK, I'll expand this example to be more complete when viewed
standalone.

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

OK.  I'll revise the examples to avoid using namespaces in order to
make it more obvious where functions originate.

>* reference/assertion.html
>
>There is a typo in the note e.g. "be defiend" instead of "be defined".

Good catch.  I will run a spell checker on everything to make sure I
find any other typos like this.

>* reference/assertion/*
>
>None of the pages provide links to the source code files, is it on purpose ?

I thought the example snippets as they stood would be sufficient.  I
can certainly link to the example source file.

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

The floating-point comparison is the one area where I still need to
write something real instead of just a placeholder.  This is the last
area that I have yet to finish.

>* reference/assertion/boost_level_equal.html
>
>In example_equal_custom_compare there's an extra space in the first 
>initialization which might be confusing.

Which space are you referring to?

The space between the definition of the string s and the first assertion?

>Also I don't get how the comment is relevant given that operator== uses 
>std::string to make the comparison ?

The standard library provides operator== and operator<< for
std::string, so BOOST_REQUIRE_EQUAL can be used without requiring you
to define these.

For your own custom type you'll have to define them.

>* reference/assertion/_boost____level___equal_collections_.html
>
>I believe "static" to be deprecated at a namespace level ?

Static linkage to a function means that it is only visible within that
single compilation unit.  However, since generate_list is the system
under test, this may be a little confusing.  I could separate it into
another compilation unit and give it a head, but then the whole
example becomes much more complicated being spread across three source
files, a new static library and an executable.  For the purposes of
making a small illustrative example I felt that was overkill.

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

There is a feature request from 4 years ago(!) for exactly this:
<https://svn.boost.org/trac/boost/ticket/3871>

I recently tried to read the Boost.Range documentation and I confess
that I didn't exaclty find it accessible at a quick read.  In this
documentation I wanted it to be as standalone as possible and not drag
the reader unnecessarily into other libraries.  In this case I think
that the vast majority of C++ programmers are not going to have a
problem with container.begin() or container.end() whereas many may not
have seen boost::begin(container) and boost::end(container).

Is the motivation for this suggestion to guide C++ programmers more
towards C++11 style usage where std::begin() and std::end() are
available?  I can see how their use for fixed length arrays is
preferable over the old "sizeof(array)/sizeof(array[0])" business, but
how exactly is it preferable for containers where we have begin() and
end() member functions?

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

See above.  They are given static linkage to intentionally limit the
accessibility of their names.

>* reference/assertion/_boost____level___small_.html
>
>Maybe explain the unit of the tolerance for completeness and coherence 
>with BOOST_level_CLOSE* ?

This is part of the remaining area that needs to be done.
-- 
"The Direct3D Graphics Pipeline" free book <http://tinyurl.com/d3d-pipeline>
     The Computer Graphics Museum <http://computergraphicsmuseum.org>
         The Terminals Wiki <http://terminals.classiccmp.org>
  Legalize Adulthood! (my blog) <http://legalizeadulthood.wordpress.com>