Re: [PATCH] {test-protocols} parallel-tests: fix bug in Automake's own testsuite

[Stefano Lattarini]

On Thursday 23 June 2011, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Thu, Jun 23, 2011 at 09:52:55AM CEST:
> > On Wednesday 22 June 2011, Ralf Wildenhues wrote:
> > > Also, have you thought about a quoting mechanism so that test authors do
> > > not have to think about breaking the test driver mechanism accidentally?
> > > That would seem more robust to me.
> > >
> > I don't understand.  A quoting mechanism for own our testsuite is already
> > offered by the new `dump_log_files' function (and the new maintainer
> > checks try to unsure that this function is used when required).  OTOH,
> > the authors of third-party test drivers *must* allow the direct writing
> > of `:test-result:' directives in the driver output -- this is the only
> > way to declare test results that are understood and accounted for by the
> > new Automake test harness.
> 
> So, we are doing inband signaling.
>
Yes.

> But Automake's own testsuite is not a TAP or a subunit one.  Thus it
> does not need to do inband-signaling.  Right?
>
Strictly speaking, it doesn't have to.  But the point of my patches so
far is exactly to provide a minimal in-band signaling support that can
be used by *all* test drivers that implements our published APIs -- so
that the logs produced by those drivers can be parsed by the generic
'parallel-tests' harness when creating the final testsuite report.

> The reason I'm harping on this is that, if all existing users of
> Automake test drivers are updated to use the same kind of inband
> signaling, then they all are liable to similar bad signals.  That
> is a potential source for regression.
>
Correct; that's possible, even if very unlikely.  BTW, I had proposed
to use of `:am-test-result:' instead of `:test-results:' results exactly
to make such "spurious" signals less likely (but they'd be still possible
of course, so that's not a great solution).

Another possibility would be to make the RST field used to declare
in-band test results configurable (at either Automake or make time);
so that the users could protect themselves from spurious test results
by defining that field to a value they know will never be emitted in
the stdout/stderr of their tests.  But that's for a follow up patch
IMO (and it would also require support from the test drivers).

> What do TAP and subunit do to protect against this?
>
Basically nothing more than we do.  For example, TAP offers support
for "diagnostic output" by guaranteeing that lines beginning with `#'
will not be taken into consideration by TAP parsers; quoting from
<http://search.cpan.org/~petdance/Test-Harness/lib/Test/Harness/TAP.pod>

  Diagnostics
  -----------

   Additional information may be put into the testing output on separate
   lines.  Diagnostic lines should begin with a #, which the harness must
   ignore, at least as far as analyzing the test results. The harness is
   free, however, to display the diagnostics.  Typically diagnostics are
   used to provide information about the environment in which test file is
   running, or to delineate a group of tests.
     ...
     ok 18 - Closed database connection
     # End of database section.
     # This starts the network part of the test.
     # Daemon started on port 2112
     ok 19 - Opened socket
     ...
     ok 47 - Closed socket
     # End of network tests

  Anything else
  -------------

   Any output line that is not a plan, a test line or a diagnostic is
   incorrect.  How a harness handles the incorrect line is undefined.
   Test::Harness silently ignores incorrect lines, but will become
   more stringent in the future.


We do even better in this regard (sorta): we say that any line that do
*not* begin with the `:test-result:' string will not be taken into
consideration by the testsuite-outcome reporting code.

> Quote normal log output?
>
Basically yes.  Or, less safely, expoit the fact that most lines are
anyway ignored by the protocol parsers.

> Like email, prepend '> ' to lines?  Please explain.
>

> > > Also, most of your uses of "useless" are actually superfluous: either
> > > you go on to explain why something is not actually useless after all
> > > (in which case there seems no point in calling it useless in the first
> > > place), or it is useless, in which case why have it?
> > >
> > What about "extra quotes" instead?  I'll use that if you don't object.
> 
> Well, just explain the reason /for/ something.  For example:
> 
> +# FIXME: the useless quoting below is to please maintainer-check.
> +env VERBOSE'='yes $MAKE -e check > stdout && { cat stdout; Exit 1; }
> 
> You can just write:
>   # Quote for maintainer-check (FIXME).
> 
> (I'd even leave out the FIXME, but I can see why you'd like that.)
> It has all information your comment gave.
>
Yes, that's better.  Will fix.

> > > No time for more detailed review in the short span allowed for feedback.
> > > Why BTW?
> > >
> > Because without this patch I'm experiencing spurious failures in the
> > testsuite.  OK, I can visually scan the output of "make check" and
> > see that there is no unexpected "foo.test: FAIL" line in there, and
> > thus conclude that the failure is really only spurious.  But this is
> > cumbersome to say the least, and surely error-prone.
> 
> > > You can always git rebase later, and it's not like this is
> > > security relevant
> > >
> > This is true.
> > 
> > > or needs to be rolled out this week.
> > >
> > I disagree: Ggven what I said above, I'd rather apply the patch today
> > (I'll wait until this evening or tomorrow morning though, to give you
> > enough time to answer).
> 
> None of your reasons make sense to me.  Of course, you should fix this
> in your tree in order to not see spurious errors any more.  But there is
> absolutely no reason to rush with a push, or not to go back and update
> the patch after I review it in three days.  Please give a reason against
> *that*.
>
The reason boils down to the fact that it would be more expedient *for me*
to have this patch in the main repository soon.  I also thought that the
patch was pretty uncontroversial (and fixlets/improvements could go in a
follow-up patch), but I see this is not the case.  Since at the moment I'm
writing/fixing the documentation for the new "custom test drivers" feature,
the existing spurious failures are not a problem right now, so I can hold
on for a couple of days.

Regards,
  Stefano