Re: [PATCH 8/5] tap: support colorization of testsuite progress output

[Stefano Lattarini]

On Monday 18 July 2011, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Mon, Jul 18, 2011 at 10:30:56AM CEST:
> > * lib/tap-driver (%COLORS): New variable (definition extracted
> > from `lib/am/check.am:$(am__tty_colors)', with some obvious
> > adjustments.
> > (report): Adjust to colorize console output when required,
> > using ...
> > (decorate_result): ... this new function.
> > (colored): New function, used by the one above.
> > * tests/tap-summary.test: Also run the checks when `color-tests'
> > is in use.
> > * tests/Makefile.am (XFAIL_TESTS): Remove `tap-color.test'.
> 
> OK once dependent changes are in, and with nits below addressed.
> 
> Thanks,
> Ralf
> 
> > --- a/lib/tap-driver
> > +++ b/lib/tap-driver
> 
> > +# Keep this in sync with `lib/am/check.am:$(am__tty_colors)'.
> > +my %COLOR = (
> > +  red => '[0;31m',
> 
> I'm sure perl has a way to encode ESC without a literal ESC.
>
Good observation, I should use "\e" instead of a literal esc.
Consider that done.

> > +  grn => '[0;32m',
> > +  lgn => '[1;32m',
> > +  blu => '[1;34m',
> > +  mgn => '[0;35m',
> > +  brg => '[1m',
> > +  std => '[m',
> > +);
> 
> > @@ -211,17 +222,39 @@ sub stringify_test_result ($)
> 
> > +sub decorate_result ($)
> > +{
> > +  return $_[0] unless $cfg{"color-tests"};
> > +  # Best way to simulate a 'switch' construct here.
> 
> Please don't, that only obfuscates the code.  automake.in uses long
> if ... else lists.  If you don't like that,
>
I don't, at least not when using the GNU Coding Standards (as too much
vertical space gets wasted IMO).

> use a map.
>
How would that be clearer?  Honest question.

Oh, or myabe you were suggesting to use an hash?

> > +  for (@_)
> > +    {
> > +      $_ eq "ERROR" and return colored ('mgn', $_);
> > +      $_ eq "PASS"  and return colored ('grn', $_);
> > +      $_ eq "XPASS" and return colored ('red', $_);
> > +      $_ eq "FAIL"  and return colored ('red', $_);
> > +      $_ eq "XFAIL" and return colored ('lgn', $_);
> > +      $_ eq "SKIP"  and return colored ('blu', $_);
> > +      return $_; # Don't colorize unknown stuff.
> > +    }
> > +}
> > +
> >  sub report ($;$)
> >  {
> >    my ($msg, $result, $explanation) = (undef, @_);
> >    if ($result =~ /^(?:X?(?:PASS|FAIL)|SKIP|ERROR)/)
> >      {
> > -      $msg = "$result: $test_script_name";
> > +      $msg = ": $test_script_name";
> >        add_test_result $result;
> >      }
> >    elsif ($result eq "#")
> >      {
> > -      $msg = "# $test_script_name:";
> > +      $msg = " $test_script_name:";
> >      }
> >    else
> >      {
> > @@ -229,10 +262,11 @@ sub report ($;$)
> >      }
> >    $msg .= " $explanation" if defined $explanation;
> >    $msg .= "\n";
> > -  print OLDOUT $msg;
> > +  # Output on console might be colorized.
> > +  print OLDOUT decorate_result ($result) . $msg;
> >    # Log the result in the log file too, to help debugging (this is
> >    # especially true when said result is a TAP error or "Bail out!").
> > -  print $msg;
> > +  print $result . $msg;
> >  }
> >  
> >  sub testuite_error ($)
> [...]
> 

Regards,
  Stefano