Re: [lmi] [lmi-commits] master 802eb23 6/6: Refactor for simplicity

[Greg Chicares]

On 2016-12-05 13:00, Vadim Zeitlin wrote:
> On Mon,  5 Dec 2016 06:55:45 +0000 (UTC) Greg Chicares <address@hidden> wrote:
> 
> GC> branch: master
> GC> commit 802eb23c6b66a432a1d706ce61f7bf1d50e50e4d
> GC> Author: Gregory W. Chicares <address@hidden>
> GC> Commit: Gregory W. Chicares <address@hidden>
> GC> 
> GC>     Refactor for simplicity
> GC>     
> GC>     This unit test formerly reported
> GC>       1608 tests succeeded
> GC>     but instead now reports
> GC>       650 tests succeeded
> GC>     because the replaced file comparison called unit-test macros 
> liberally.

Background: when I saw...

-// BOOST !! We could use BOOST_CHECK_EQUAL_COLLECTIONS if we could use the
-// full Boost.Test framework.

...I figured we had already discussed why I chose to fork that...

// The original boost.org test library upon which this derived work is
// based was later replaced by a very different library. That new test
// library has more features but is less transparent; it is not
// strictly compatible with tests written for the original library;
// and, for boost-1.31.0 at least, it didn't work out of the box with
// the latest como compiler. The extra features don't seem to be worth
// the cost.

...so I took the "BOOST" comment as suggesting that a general library
facility would be preferable, and figured that you weren't aware that
lmi already had a function to compare files.

Whenever two unit-test data files differ but they should not, I want
to analyze them with whatever tools work best (that's why I strongly
prefer not to delete them automatically in this case). Perhaps this
way of doing things is just so deeply ingrained that I don't see it
as solipsism to impute the same preference to others; but I was
really surprised to read this:

>  In practice, it was very useful to see the offset of the first difference
> and the bytes value immediately when running this test, this allowed me to
> find one or two bugs that I had in the table binary IO code much more
> quickly than if I had to compare the files manually after running the test.

I guess invoking cmp here would break your flow (so the issue is
not how many seconds it takes to invoke it, because that's tiny).

>  Of course, I can't argue that the new code is simpler, but mostly because
> it does less. And personally I regret the part which is not done any
> longer. Are you really sure it's worth doing this? After all, the old code
> wasn't really complex neither (at least IMHO, it was pretty
> straightforward, just longer) and I don't think simplicity or brevity of
> the unit test should count for as much as that of the code of the main
> program.

I certainly wasn't trying to make your work more cumbersome--I only
intended to merge duplicate code. I'd welcome a patch that restores
the lost functionality while avoiding duplication (because all code,
even in system tests, must be maintained, at a nonzero cost). I see
three ways that might be done:

(1) Add code to files_are_identical() to report the first byte
that differs iff they aren't identical. Or, probably better...

(2) Report the same thing with a new function, called cmp() perhaps.
Or, best of all...

(3) Do something like this:
  if(!files_are_identical(file0, file1))
    {
    std::ostringstream oss;
    oss << "cmp " << file0 << ' ' << file1;
    system_command(oss.str());
    }
That seems ideal. The only problem is that it won't work for me in a
cross-development chroot where system_command() uses CreateProcess()
to invoke an msw-native 'cmp' that I don't have. I'm sure there's a
good solution to that, which would also fix all the remaining unit-
test problems in the chroot:

Running authenticity_test:
  Result of 'md5sum --version':

** uncaught exception: std::runtime_error: Exit code 12345 from command 'md5sum 
 --version'.

Running system_command_test:

** uncaught exception: std::runtime_error: Exit code 12345 from command 'grep 
--quiet abc eraseme'.

If you could figure out a good solution for that, then I'll change the
code as sketched out above, and I'll also fix 'authenticity_test' and
'system_command_test'. And then 'rate_table_test' will automatically
do exactly what you want it to (if I've understood that correctly)
without any extra code. Sound good?