Re: [lmi] wx_test cleanup

[Greg Chicares]

On 2014-11-06 01:23Z, Vadim Zeitlin wrote:
> On Sun, 02 Nov 2014 15:03:41 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2014-11-02 01:31Z, Vadim Zeitlin wrote:
[...]
> GC> > 1. What to do with {New,Calc}Ill{Full,Summary}.txt files created by the
> GC> >    "calculation_summary" text? This is related to one of the questions 
> in
> GC> >    my email from 2014-09-08T11:07:20Z
> GC> 
> GC> Here, I'll have to defer to Wendy: I don't know the motivation for
> GC> particular tests.
> 
>  For now I'm leaving these files, it's a bit annoying to have them, but
> they can be added to .svnignore (or .gitignore in my case) and I just
> really have no idea what should really be done here.

We'll need to resolve this question in the USA.

> GC> > 2. Some PDF viewers, notably the default/official one, lock the PDF file
> GC> >    while it's opened in them, meaning that deleting the PDF file created
> GC> >    in "pdf_illustration" test case will usually fail
> GC> 
> GC> This is 'LMI_WX_TEST_CASE(pdf_illustration)' in 'wx_test_pdf_create.cpp'.
> GC> I'm not sure I understand the rationale for this test, but it seems to
> GC> simulate "File | Print preview" for a new '.ill' and then test only this
> GC> single condition:
> GC> 
> GC>     // Finally check for the expected output file existence.
> GC>     LMI_ASSERT(fs::exists(pdf_path));
> GC> 
> GC> IOW, it opens the system's PDF viewer, but actually tests only that the
> GC> output file exists.
> 
>  It also indirectly tests for the absence of errors when launching the PDF
> viewer (if any dialogs would appear, the test would fail), which, I think,
> can be useful. It would, of course, be even better if it could check if the
> PDF viewer (or at least something launched by "Print preview")

That's all true; but I'm questioning whether the benefit of this test
exceeds its cost, which is a question we'll discuss on these shores.

We rarely change the code that prints (or previews) PDF files. If we
do change it, then we will certainly test our changes manually before
release. I suppose it's likelier that the wx mime-type handler would
change; that's the only value I see in automating this test.

The cost is really just the nuisance of closing the PDF viewer. But
that is a real nuisance. To me, it just seems dubious that an
automated test requires manual cleanup.

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

BTW, that command is useful in its own right and was added yesterday.

>  I'm not sure about this... The GUI test is supposed to check that the user
> actions have the expected effects and it seems to me that previewing the
> PDF is a more useful action than checking that it could be created. It also
> has the advantage that the person running the test can look at the document
> opened in the PDF viewer after the test end and verify that, at least at a
> glance, it looks correct (e.g. it's not empty).

Then it's not really an automated test. That's a philosophical issue
that we'll need to discuss in the USA.

>  Of course, I could be completely wrong about all this and maybe LMI users
> never preview the PDFs and this test wasn't designed to specifically check
> for this. But this is the impression I had.

I'm not really sure either. I'll have to ask here.

> GC> > it would be very nice if I could override
> GC> > wxDocManager::MakeNewDocumentName() in the testing code as I could
> GC> > handle almost all new files creation in a single place then.
[...]
> To make this more precise, I suggest the following change:

Committed 20141107T1205Z, revision 6031.

Am I correct in thinking that wx won't delete the document-manager object,
so that it must be deleted in OnExit()? If not, then it would be good to
simplify OnExit().

Just BTW, I preferred CreateDocManager() to DoCreateDocManager() for the
reason given in this 2006 message that was off the mailing list:

| Is there a reason for the 'Do' prefix here:
|     void DoCopyLedgerValues();
| ? Usually when I see that I expect to see something like
|
|   class Base
|   {
|     void Foo();
|     virtual void DoFoo() = 0;
|   };
|
| where derived classes only implement DoFoo(). Some code I wrote
| years ago doesn't follow that convention, but it seems to be a
| best practice--e.g.,
|
| http://groups.google.com/group/comp.lang.c++.moderated/msg/f834bd9d73053566
| | There is a de facto naming convention that
| | prefixes the private virtual function with a "do"