Re: [lmi] Patches for stderr output

[Vadim Zeitlin]

On Thu, 04 Dec 2014 04:43:17 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2014-11-23 23:14Z, Vadim Zeitlin wrote:
GC> [...]
GC> >  The first patch redirects
GC> > the output to stderr and disables the time stamps. It also changes the 
test
GC> > behaviour: it now closes the main window and exits the application after
GC> > finishing running all the tests, making the test fully non-interactive.
GC> 
GC> Committed 20141204T0421Z, revision 6042.
GC> 
GC> I would slightly have preferred to write to stdout, but I find no
GC> wxLogStdout in the wx documentation. Is this intentional?

 Yes in the sense that wxLogStderr can be actually used to write to stdout
by just passing the latter as "FILE*" argument to the ctor of the former,
i.e. normally the following trivial patch should have resulted in your
preferred behaviour:

---------------------------------- >8 --------------------------------------
diff --git a/main_wx_test.cpp b/main_wx_test.cpp
index ce27173..6beb59a 100644
--- a/main_wx_test.cpp
+++ b/main_wx_test.cpp
@@ -538,10 +538,10 @@ bool SkeletonTest::OnInit()
     // stamps in the logs to avoid spurious differences due to them.
     wxLog::DisableTimestamp();

-    // Log everything to stderr, both to avoid interacting with the user (who
+    // Log everything to stdout, both to avoid interacting with the user (who
     // might not even be present) and to allow redirecting the test output to a
     // file which may subsequently be compared with the previous test runs.
-    delete wxLog::SetActiveTarget(new wxLogStderr);
+    delete wxLog::SetActiveTarget(new wxLogStderr(stdout));

     if(!Skeleton::OnInit())
         {
---------------------------------- >8 --------------------------------------

Except that testing it showed that it didn't and the output was still sent
to stderr even after applying it. I admit I should have soon it sooner, but
it took me close to an hour to understand what was going on -- especially
because, strangely enough, I couldn't reproduce the problem under the
debugger. As you have probably already realized, this is due to the changes
of r6007 which added setting of wxLogStderr in the base Skeleton class
itself unless running under the debugger. So the really working patch needs
to move this class after Skeleton::OnInit():

---------------------------------- >8 --------------------------------------
diff --git a/main_wx_test.cpp b/main_wx_test.cpp
index ce27173..9645731 100644
--- a/main_wx_test.cpp
+++ b/main_wx_test.cpp
@@ -538,16 +538,16 @@ bool SkeletonTest::OnInit()
     // stamps in the logs to avoid spurious differences due to them.
     wxLog::DisableTimestamp();

-    // Log everything to stderr, both to avoid interacting with the user (who
-    // might not even be present) and to allow redirecting the test output to a
-    // file which may subsequently be compared with the previous test runs.
-    delete wxLog::SetActiveTarget(new wxLogStderr);
-
     if(!Skeleton::OnInit())
         {
         return false;
         }

+    // Log everything to stdout, both to avoid interacting with the user (who
+    // might not even be present) and to allow redirecting the test output to a
+    // file which may subsequently be compared with the previous test runs.
+    delete wxLog::SetActiveTarget(new wxLogStderr(stdout));
+
     // Run the tests at idle time, when the main loop is running, in order to
     // do it in as realistic conditions as possible.
     CallAfter(&SkeletonTest::RunTheTests);
---------------------------------- >8 --------------------------------------

 This works but while rereading this code and the thread where we discussed
it I have a nasty feeling that I actually completely lost the point in that
thread and that the change of r6007 was actually wrong and I'll follow up
in that thread to voice these concerns.

 In the meanwhile, to finish with this one: I don't actually think writing
everything to stdout is correct neither. It's arguably more correct than
writing everything to stderr, but the ideal behaviour would seem to be to
write the test progress messages to stdout while sending the errors to
stderr. This can't be done with any standard class such as wxLogStderr but
it's not difficult to define a custom log target which would do it. Do you
think it's worth doing this or is the trivial patch above enough?


GC> All lines on stderr begin "[TEST] " except for one:
GC> 
GC> [TEST] benchmark_census: started
GC> Run case for census_1: 423ms elapsed (expected -2.53%)
GC> [TEST] benchmark_census: ERROR (Assertion 'std::fabs(diff_in_percents) < 
10' ...
GC> 
GC> Is that intentional?

 I wouldn't go as far as saying that this is intentional, but I am not
actually sure how should this behave. Do we want to have "[TEST] " prefix
for all messages produced by the testing code? So far it's only used for
the generic messages such as "starting test", "success", "failure" and so
on. If we do, it would be better to add it automatically instead of doing
it manually as right now. And this would be simpler to do by using a custom
log target, so if we want to have one for selectively sending messages to
stdout or stderr depending on their severity anyhow, I'd rather fix this
once we have it.

 So my outstanding questions here are:

1. Do we want to send [only] messages about test errors to stderr?
2. For which messages, if any, do we want to have "[TEST] " prefix?


 Thanks in advance,
VZ