Re: [lmi] [PATCH] Error reporting

[Vadim Zeitlin]

On Fri, 05 Dec 2014 20:47:32 +0000 Greg Chicares <address@hidden> wrote:

GC> We accidentally made a change that prevented wx errors from being
GC> displayed. I'd like to add an automated test that would flag that
GC> problem in case we accidentally make a similar mistake in the future.

 I definitely agree that it's important to have a test for this, but
this did require some more extensive changes, let me explain why:

 The problem is that we want to check for wxLogWarning() or wxLogError()
dialogs appearance, but these dialogs are currently disabled during the
test execution because we redirect all logs to stdout. Initially I thought
that I'd just check that the warning/error message was logged, at wxLog
level, but I quickly realised that such test would be useless: even with
the buggy change, the message was still logged, the problem was exactly
that it was not shown to the user in spite of being logged correctly.

 So the real solution is to stop redirecting the logs in the test program
to stdout. And this, in turn, means that the tests can't use wxLogMessage()
for output any more, so I had to replace all its occurrences with either
wxPrintf() (which is a safe pseudo-vararg template function and so doesn't
suffer from std::printf() type safety problems) or wxPuts(). Hence the
first patch does just this and while it doesn't change anything on its own,
it's required by the two other ones.


GC> On 2014-12-05 19:21Z, Vadim Zeitlin wrote:
GC>
GC> >  Patch returning to the old behaviour and just making the debug messages
GC> > visible is simple enough: we just need to define our own log target that
GC> > sends debug log messages to stderr (I assume here it is really more
GC> > appropriate than stdout, isn't it?) while handling the rest of them in the
GC> > usual way.
GC> 
GC> Okay, please proceed with that; and yes, here we really do want stderr.

 The second attached patch does just this. As you can see, the patch is
actually rather short (and could be made even shorter if you don't want to
have the list of all wxLOG_XXX levels in the switch, which I put there
just because I believe this makes the code more clear/self-documented), but
its description is relatively long because I also changed when the log
messages are redirected: instead of doing it under all platforms only when
not running under a debugger, it is now done only under MSW, but always. I
hope to have explained why the new behaviour is correct in the commit
message, but please let me know if I was unsuccessful.


 Finally, the third patch adds a new unit test checking that wxLog messages
are shown to the user. I chose to trigger the warning message given by the
doc/view framework when it can't determine the type of the file being
opened as this is exactly the error message that was (wrongly) not given
for the files specified on the command line, as mentioned in
http://lists.nongnu.org/archive/html/lmi/2015-03/msg00022.html, so it's a
realistic example. I verified that the test failed without the second patch
above and passes now.


 This is not quite the end of the story however: while all the patches
above work as expected in my testing, I've also tried testing the test
suite behaviour in presence of the unexpected wxLog errors (i.e. I've just
inserted something resulting in an error in another test). The tests did
fail, but not in the most helpful way because the wxLog dialog was only
shown after the execution of all tests and not while running the test which
really logged the error. So the fourth patch explicitly flushes the log
after finishing with each test, to ensure that the correct test is "blamed"
for the unexpected log messages. Unfortunately testing it found a bug in
wxWidgets: wxLogGui::Flush() was not exception safe and if showing the
dialog threw an exception (which it can never do in normal use but can in
our tests), logs remained disabled. So while this fourth patch doesn't do
any harm with the current wxWidgets version, neither does it really help
and for everything to work correctly you also need to get this fix:

https://github.com/wxWidgets/wxWidgets/commit/bcacb4467e2735f5fb2ac2bf30f7e06dd7ab7d64

I don't think it's necessary to upgrade your version of wxWidgets
immediately, as I said, the patch above does no harm and everything does
work correctly with the current tests. But the wx commit above will help
with the errors clarity if we ever have unexpected wxLog messages when you
upgrade to it.


 It would, OTOH, be great if you could please apply the patches attached to
this message, especially the first two of them as the second one is really
extensive and might conflict with the future patches to the tests.

 Thanks in advance!
VZ

0001-Use-wxPrintf-instead-of-wxLogMessage-for-the-test-su.patch
Description: Text document

0002-Don-t-redirect-all-wxLog-messages-to-stderr-just-the.patch
Description: Text document

0003-Add-a-unit-test-verifying-that-wxLog-errors-are-show.patch
Description: Text document

0004-Flush-wxLog-after-every-test-execution-in-the-test-s.patch
Description: Text document