Re: [lmi] wx_test cleanup

[Greg Chicares]

On 2014-11-02 01:31Z, Vadim Zeitlin wrote:
> On Wed, 29 Oct 2014 21:23:52 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2014-10-29 15:01Z, Vadim Zeitlin wrote:
> GC> > On Wed, 29 Oct 2014 12:05:15 +0000 Greg Chicares <address@hidden> wrote:
> GC> > 
> GC> > GC> I anticipate running 'wx_test' frequently--perhaps before every
> GC> > GC> commit--which makes the following side effects inconvenient; can
> GC> > GC> they be suppressed?
> GC> > 
> GC> >  They can, of course. I'm just not sure when should this be done:
> GC> > 
> GC> > (a) Always?
> GC> > (b) Only if all tests pass?
> GC> > (c) If some specific command line option is [not] given?
> GC> 
> GC> Or:
> GC>   (d) If the test that created the file passes.
> 
>  Yes, this is another possibility which I thought was similar enough to (b)
> to not warrant including it, but I'm, to spoil the answer to your question
> below, perfectly fine with it.

Okay. I'll spoil my answer to your question below by saying I
still favor (d), even over (e).

> GC> Is (d) actually feasible, at least most of the time?
> 
>  It definitely is, but I do have a few questions about the exact behaviour
> we want:
> 
> 1. What to do with {New,Calc}Ill{Full,Summary}.txt files created by the
>    "calculation_summary" text? This is related to one of the questions in
>    my email from 2014-09-08T11:07:20Z

Here, I'll have to defer to Wendy: I don't know the motivation for
particular tests.

> 2. Some PDF viewers, notably the default/official one, lock the PDF file
>    while it's opened in them, meaning that deleting the PDF file created
>    in "pdf_illustration" test case will usually fail

This is 'LMI_WX_TEST_CASE(pdf_illustration)' in 'wx_test_pdf_create.cpp'.
I'm not sure I understand the rationale for this test, but it seems to
simulate "File | Print preview" for a new '.ill' and then test only this
single condition:

    // Finally check for the expected output file existence.
    LMI_ASSERT(fs::exists(pdf_path));

IOW, it opens the system's PDF viewer, but actually tests only that the
output file exists. The relevant code in lmi is:

    if(emission & mce_emit_pdf_file)
        {
        write_ledger_as_pdf(ledger, filepath);
        }
...
    if(emission & mce_emit_pdf_to_viewer)
        {
        std::string pdf_out_file = write_ledger_as_pdf(ledger, filepath);
        file_command()(pdf_out_file, "open");
        }

and this test can fail only if the write_ledger_as_pdf() part fails.
Therefore, it would seem more economical to add menu and toolbar
commands to write a PDF file without invoking the PDF viewer, which
takes some time to load and then must be closed manually--and then
test that new command instead.

As for file_command()(...), that could be tested separately, if
necessary, with a command that doesn't have the side effect of
opening an external GUI program.

So I think this leads to a new and better question: should we offer
menu and toolbar commands to write a PDF file? Isn't that something
that a program such as lmi would normally provide? The toolbar button
is gnome's 'stock_save-pdf.png', which suggests that some gnome apps
do this. We'd need to change the toolbar text to make it suitable for
both '.cns' and '.ill', e.g.:

-         <tooltip>Print case to disk</tooltip>
+         <tooltip>Write PDF to disk</tooltip>
          <bitmap stock_id="save-pdf"/>
-         <longhelp>Run and print all cells to disk instead of 
printer</longhelp>
+         <longhelp>Run and write PDF to disk (entire case for a 
census)</longhelp>

I think the corresponding menu command really does belong on the
"Census" menu for '.cns' (because it requires calculating every value,
which is a potentially time-consuming operation specific to a census),
so the corresponding '.ill' command belongs on the "Illustration" menu.
(An argument could be made for putting it on the "File" menu, but I
don't think it should be there for '.ill' yet elsewhere for '.cns'.)
This may be a valuable enhancement in its own right; and if we do it,
then would we be happy enough to test that in 'wx_test' rather than
"File | Print preview"?

At any rate, thanks for the [snipped] ideas discussing how we could
force the PDF viewer to close, but I agree that we don't want to
consider going to such heroic lengths.

> 3. Implementation-wise, tracking all files created by the test is
>    difficult. I looked a little but couldn't find any simple solution to
>    collect all the files created by the process, even from the process
>    itself (it's possible to hook Windows CreateFile() syscall, but this is
>    hardly simple).

Not worth doing at the OS level.

>    We could take a snapshot of the directories where the output files are
>    created and delete all new files appearing there after running the
>    tests. This seems appealing but automatically deleting files is always
>    dangerous, so I'm not sure if we really want to do this.

I agree: let's not go there. Instead...

> 4. Continuing with the implementation questions, if we decide to not do
>    anything "smart" like automatically deleting all the new files and just
>    handle them on a one-by-one basis,

Yes, that's the right way.

>    it would be very nice if I could
>    override wxDocManager::MakeNewDocumentName() in the testing code as I
>    could handle almost all new files creation in a single place then.

I'm perfectly okay with something like this:

-- 8< --
Index: docmanager_ex.cpp
===================================================================
--- docmanager_ex.cpp   (revision 6018)
+++ docmanager_ex.cpp   (working copy)
@@ -200,6 +200,11 @@
     delete printout;
 }

+wxString DocManagerEx::MakeNewDocumentName()
+{
+    return wxDocManager::MakeNewDocumentName();
+}
+
 // Use a popup menu, instead of wxGetSingleChoiceData with strings
 // that are not generally appropriate. Our users don't understand
 // "Select a document template", they'd rather not have to hit
Index: docmanager_ex.hpp
===================================================================
--- docmanager_ex.hpp   (revision 6018)
+++ docmanager_ex.hpp   (working copy)
@@ -66,6 +66,7 @@
     // deprecated unless it's wanted on other platforms?

     // wxDocManager overrides.
+    virtual wxString       MakeNewDocumentName();
     virtual wxDocTemplate* SelectDocumentType
         (wxDocTemplate** templates
         ,int             noTemplates
-- >8 --

And I think that's all you need. It doesn't have to be changed
from private to protected, because you'll have no need to call
DocManagerEx::MakeNewDocumentName(): you can just call the wx
base class's version directly. Should I commit this change?

Oh, and at the same time, could you take a look at the comments
marked "WX !!" in these two files, and let me know what you
think about them? It's been many years since I wrote them, and
maybe some are no longer relevant.

[...here we're discussing option (d) given above...]
> GC> If so, can I persuade you that it's better than the others?
> 
>  I think it is better than the options I listed, but all this thinking
> about the monitor process for the test made me wonder if there could be a
> much simpler (and hence faster to implement) solution, let me call it
> 
> (e) Create a shell script that would run the test itself and then delete
>     all the files known to be created by it.
> 
> This is as trivial as it gets and, of course, it can't deal with the
> individual tests failures. And this script would need to be updated
> whenever a new file starts being created by the test. But it's dead easy to
> implement and the advantage is that such script could always be ran just to
> clean up the files, e.g. after you finish examining them after a test
> failure, while all the other solutions would require you to do it manually
> or fix the test and rerun it.
> 
>  So I have a suspicion that in practice this could be a very simple "good
> enough" solution to the problem at hand. What do you think?

I still favor (d) because encapsulation makes it tidy:
  create a file
  test it
  delete it
Every test is self-contained: it cleans up after itself. OTOH,
(e) would move the "delete it" step into a different source file,
increasing the cost of maintenance and risk of error; and we might
of course forget to run the script.

[... testing 'configurable_settings.xml' ...]
> GC> I suspect that the best way to test the dialog is to simulate
> GC> "OK" and verify the outcome...so yes, I think the config file
> GC> really should be modified by the test...and therefore, it
> GC> would be good, once the test has finished, to restore the file
> GC> to its former state.
> 
>  Do you think it would be better to do it by simulating the UI actions
> needed to do it or...
> 
> GC> It might be a good idea to make a backup copy of this file at
> GC> the beginning of the test, in case the test fails.
> 
> ... by just preserving a backup copy of the file?

I think it's better to preserve and restore a backup copy, with
the original last-modified date and permissions. This file is
necessary for the correct operation of the system, and it may
embody customizations that I'd want to preserve, so we should
take care to preserve it. If the test fails catastrophically,
we could find, e.g., 'configurable_settings.xml.preserved' and
rename it.

> I.e. would there be any
> benefit in testing that by performing the correct actions in the dialog we
> can prevent the file from being modified?

I don't see any benefit, and such a test will fail whenever we
change the source code, as we did only six weeks ago, here:
  http://svn.savannah.nongnu.org/viewvc?view=rev&root=lmi&revision=5940
Let's also remember, as that changeset shows, that we have a
CLI unit test for this class already. I haven't taken the time
to compare the CLI and GUI tests; we needn't take aggressive
steps to eliminate all overlap, but obviously we don't need
to introduce gratuitous redundancy. And anything that we can
test just as well in the CLI unit test, should be tested there:
CLI tests are lightweight, smaller, faster, and easier to read.

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

That would be most excellent. I kludged this locally because it
is so convenient to keep file history working, but I'd like to
get rid of that kludge.