Re: [lmi] wx_test cleanup

[Greg Chicares]

On 2014-10-29 15:01Z, Vadim Zeitlin wrote:
> On Wed, 29 Oct 2014 12:05:15 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> I anticipate running 'wx_test' frequently--perhaps before every
> GC> commit--which makes the following side effects inconvenient; can
> GC> they be suppressed?
> 
>  They can, of course. I'm just not sure when should this be done:
> 
> (a) Always?
> (b) Only if all tests pass?
> (c) If some specific command line option is [not] given?

Or:
  (d) If the test that created the file passes.
Of course, that doesn't work for files that are used by many tests
(if there are any).

>  I don't think (a) is a good idea because I think these files could be
> useful for diagnosing the problem if the test fails.

Exactly. That's why I use (d) in this snippet from 'input_test.cpp':

    bool okay = files_are_identical("eraseme0.xml", "eraseme1.xml");
    BOOST_TEST(okay);
    // Leave the files for analysis if they didn't match.
    if(okay)
        {
        BOOST_TEST(0 == std::remove("eraseme0.xml"));
        BOOST_TEST(0 == std::remove("eraseme1.xml"));
        }

> In principle, (b)
> should be fine, but perhaps [sometimes?] the person running the tests would
> need to have a look at the outputs to verify something not checked
> automatically?

I can't imagine why that wouldn't indicate an opportunity for
further automation. It just seems to violate the prime directive
of testing automation.

Well, if there's some output file that summarizes the results
of all tests, that would be a special case: we'd want to
preserve it.

> In this case (c) seems like the best choice.
> 
> GC> (1) It creates at least eighteen files in /opt/lmi/bin where I
> GC> run it. That's how the program normally behaves, so that's how
> GC> the tests should work: it'd probably be a bad idea to test in
> GC> a different directory. Therefore, wouldn't it be best to delete
> GC> them when the test is finished?
> 
>  Here the only problem is when to do it, i.e. which of (a), (b) and (c)
> above do you prefer.

Is (d) actually feasible, at least most of the time?
If so, can I persuade you that it's better than the others?

> GC> (2) Apparently 'configurable_settings.xml' is modified.
> 
>  This is unexpected, I don't think we intentionally modify it anywhere.
> Could you please tell me what exactly is changed if you already know it?

-  <calculation_summary_columns>PolicyYear AttainedAge Outlay CSVNet_Current 
AcctVal_Current CSVNet_Guaranteed AcctVal_Guaranteed EOYDeathBft_Current 
EOYDeathBft_Guaranteed NetWD NewCashLoan LoanIntAccrued_Current 
</calculation_summary_columns>
+  <calculation_summary_columns/>
-  <use_builtin_calculation_summary>0</use_builtin_calculation_summary>
+  <use_builtin_calculation_summary>1</use_builtin_calculation_summary>

In 'wx_test_calculation_summary.cpp', use_builtin_calculation_summary()
opens the dialog:
    ui.Char('f', wxMOD_CONTROL);    // "File|Preferences"
but I can't figure out how it's closed: I don't see a simulated action
of "OK" or "Cancel".

Is "OK" triggered by ExpectMvcDialog::OnInvoked() here? If so, then
that's what causes 'configurable_settings.xml' to change.

> GC> Would it make sense for 'wx_test' to rename it at the start, then
> GC> rename it back at the end?
> 
>  I'll do this if the modifications should be done in the first place.

I don't understand this test's implementation thoroughly, but
I suspect that the best way to test the dialog is to simulate
"OK" and verify the outcome...so yes, I think the config file
really should be modified by the test...and therefore, it
would be good, once the test has finished, to restore the file
to its former state.

It might be a good idea to make a backup copy of this file at
the beginning of the test, in case the test fails.

I don't think we need any heroic measures to preserve this
file if the test fails abnormally.

> GC> (3) File history also changes. Is there any way to save the
> GC> original values before the test, and restore them afterward?
> 
>  We could do it like this, but a simpler solution would be to just avoid
> saving the history into wxConfig when testing. This can be done entirely
> outside of the normal program code by clearing the history in
> SkeletonTest::OnExit() before calling the base class version, as the call
> to FileHistorySave() in it then wouldn't have any effect.

Oh--it wasn't obvious to me at first, but I guess it's a FIFO,
so adding nothing to it leaves it unchanged. (Well...maybe
Least Recently Used First Out, but there's no acronym for that.)

>  It's not as robust as saving wxConfig contents before starting the test
> and restoring it when it ends, but as long as FileHistorySave() is only
> called from Skeleton::OnExit(), it works just as well and is much simpler.
> Do you have any objections to doing it like this?

No objection.

It's saved only once:
  /lmi/src/lmi[0]$grep FileHistorySave *.?pp
  skeleton.cpp:        doc_manager_->FileHistorySave(*config_);
  /lmi/src/lmi[0]$
and I have no wish to change that.

And we don't care about supreme robustness. Losing my file history
on rare occasions is an inconvenience, not a tragedy.