Re: [lmi] Error reporting

[Greg Chicares]

On 2014-12-05 19:21Z, Vadim Zeitlin wrote:
> On Fri, 05 Dec 2014 18:51:43 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2014-12-05 17:04Z, Vadim Zeitlin wrote:
> ...
> GC> >  So for me the behaviour pre-r6007, which would have shown the exact 
> error
> GC> > to the user, was preferable (although not ideal because it would 
> actually
> GC> > show two message boxes and just one would be better).
> GC> 
> GC> I had seen it as an improvement. Before r6007, two messages appeared
> GC> in the GUI <...> After r6007, end users see only the first message
> 
>  Yes, this is what motivated my "not ideal" parenthesis above.

At this point, let me interrupt to say that it's acceptable for
two messageboxes to be shown in this case. AFAIK, no invocation
of system_command() has ever failed for an end user. I recognize
that it's not ideal; but we have much bigger fish to fry. Sorry
I didn't make this clear earlier, but I don't want to change
'system_command_wx.cpp' at all if this is its worst problem.

> GC> > GC> Do we need a different test? And is there a way to automate a test 
> for
> GC> > GC> whatever GUI phenomena would be correct?
> GC> > 
> GC> >  Yes, there is, we can check that the expected dialog is shown or, 
> simpler,
> GC> > that the expected error message is logged.
> GC> 
> GC> I just spent a lot more time trying to understand that problem
> GC> than I did writing about it. Could I prevail upon you to write
> GC> an automated test for this? I guess this calls for a new
> GC> 'wx_test_whatever.cpp' files, but that's certainly okay; or
> GC> perhaps it fits nicely into some other file you were already
> GC> going to add--it's your choice.
> 
>  I'm not sure how should this interact (if it should interact at all) with
> the discussion about getting rid of the "Test" menu items completely and
> replacing them with unattended tests. AFAIK no final decision was taken
> about this (please correct me if I missed it somehow), but it seems a bit
> strange to add tests for the commands which may disappear completely very
> soon anyhow. Could you please clarify the plans for the "Test" menu before
> I start doing it?

I'd still like to get rid of it. We could remove its other menuitems
and leave only "Test | Test system command..." if that's necessary for
the GUI test. But isn't there a way we could accomplish our testing
goal without using any menu? First, we need to define the goal...

We accidentally made a change that prevented wx errors from being
displayed. I'd like to add an automated test that would flag that
problem in case we accidentally make a similar mistake in the future.
It suffices to do anything at all that will always trigger a wx error.
You'd be able to devise a better way than I would, but I imagine that
creating a window with negative dimensions might be appropriate, just
as a naive example. I think this is our only goal.

I don't think it's actually useful to use 'wx_test' to test the
system_command() facility. It's used in 'authenticity.cpp' anyway,
in a way that's certain to be called when we run 'wx_test'. It's
also used in 'ledger_xsl.cpp' whenever we write a pdf file, so
'wx_test' necessarily uses it there, too. So I don't think it's
very important to test system_command() per se in 'wx_test'.

Just to be very clear, this is a change in the specifications.
I had asked you to write automated tests for every menuitem in
the "Test" menu; now I've changed my mind and am excluding the
"Test system command..." menuitem from that request. Sorry I
didn't think that through carefully earlier.

> GC> > GC> If your analysis of the behavior is correct, then I suppose I should
> GC> > GC> want to fix it. But that's still hypothetical for me, because I 
> can't
> GC> > GC> see what's wrong.
> GC> > 
> GC> >  Not showing wxWidgets errors to the users is wrong IMNSHO. And it
> GC> > definitely was not done before.
> GC> 
> GC> I will gladly commit a patch that fixes this and a test that proves it.
> 
>  Patch returning to the old behaviour and just making the debug messages
> visible is simple enough: we just need to define our own log target that
> sends debug log messages to stderr (I assume here it is really more
> appropriate than stdout, isn't it?) while handling the rest of them in the
> usual way.

Okay, please proceed with that; and yes, here we really do want stderr.

>  Patch fixing the problem of two message boxes shown in a row if the
> command being executed doesn't exist depends on which solution to this
> problem do you prefer.

Let's not fix that.