Re: Autotest: enable colored test results.

[Eric Blake]

On 06/13/2010 12:50 AM, Ralf Wildenhues wrote:
> The logical next step for Autotest to be on par with Automake's
> parallel-test.
> 
> Unlike Automake, the testsuite author does not have to do anything for
> the user to be able to use color; AT_COLOR_TESTS only changes the
> default to yes.  We could easily let it accept an optional argument, if
> you think it is useful.

I'm debating whether:

AT_INIT([testsuite])
AT_COLOR_TESTS

vs.

AT_INIT([testsuite], [color])

looks nicer.  But keeping it as a separate macro for now seems okay.

Did you make sure that AT_COLOR_TESTS can be specified either side of
AT_INIT, or is there a fixed invocation order that authors must be aware of?

>  
> address@hidden --color
> address@hidden address@hidden@r{|address@hidden|address@hidden
> +Enable colored test results.  Test results are colored if the argument
> address@hidden is given, or if standard output is connected to a terminal
> +and either the macro @code{AT_COLOR_TESTS} is used or the argument
> address@hidden is not given.

Personally, I like tri-state color options: --color=no or --color=never
to disable, --color=yes or --color=always to enable, and --color=auto or
the simpler --color to depend on tty status.  Are you making 'yes' a
synonym for 'always' or for 'auto'?  Should we support other common
synonyms?

AT_COLOR_TESTS was used by the package maintainer, not by the end-user
running the testsuite.  So it seems like this --help message could be
conditionalized on whether AT_color was defined by the package
maintainer, to accurately reflect the default state of this option for
this particular testsuite.  That is, if the packager omits
AT_COLOR_TESTS, this would roughly list 'output is colored only if this
option is given, and either the argument is always, or stdout is tty and
argument is auto'; if the packager includes AT_COLOR_TESTS, this would
list 'output is colored if argument is always, or if stdout is a tty and
this option is not used with argument no'.

>  
> +    --color )
> +     at_color=yes
> +     ;;

Given my above tri-state thoughts, this would be at_color=auto.

> +    --color=* )
> +     at_color=$at_optarg
> +     ;;

This would be where we normalize never=>no, yes=>always...

> +
>      --debug | -d )
>       at_debug_p=:
>       ;;
> @@ -663,6 +672,17 @@ else
>    # Sort the tests, removing duplicates.
>    at_groups=`AS_ECHO(["$at_groups"]) | tr ' ' "$as_nl" | sort -nu`
>  fi
> +
> +if test x"$at_color" = xalways \
> +   || { test x"$at_color" = xyes && test -t 1; }; then

...and test against auto instead of yes.

> +  at_red='@<:@0;31m'
> +  at_grn='@<:@0;32m'
> +  at_lgn='@<:@1;32m'
> +  at_blu='@<:@1;34m'
> +  at_std='@<:@m'

I don't know how to easily detect ascii vs. ebcdic ESC (which is a
different encoding); \e and \E escapes are common, but non-portable.

But I _do_ think that we should use printf '\033' rather than embed a
raw ascii ESC into the file, if we are happy with tying ourselves to
ascii ESC.

> +else
> +  at_red= at_grn= at_lgn= at_blu= at_std=
> +fi
>  m4_divert_pop([PARSE_ARGS_END])dnl
>  m4_divert_push([HELP])dnl
>  
> @@ -704,6 +724,8 @@ dnl extra quoting prevents emacs whitespace mode from 
> putting tabs in output
>  Execution tuning:
>    -C, --directory=DIR
>  [                 change to directory DIR before starting]
> +      --color[[=yes|no|always]]
> +[                 enable colored test results on terminal, or always]

And here, you could list (default auto) if AT_COLOR_TESTS, and (default
no) otherwise.

> +++ b/tests/autotest.at
> @@ -1402,6 +1402,91 @@ AT_CHECK([test -s sigpipe-stamp || test ! -f 
> micro-suite.dir/7/micro-suite.log],
...
> +red='@<:@0;31m'
> +grn='@<:@0;32m'
> +lgn='@<:@1;32m'
> +blu='@<:@1;34m'
> +std='@<:@m'

Again, raw ESC are awkward, and I'd rather see printf(1) used.  I don't
think we will trip any portability problems by using printf(1), even on
systems where it is not a shell builtin (thinking particularly of
Solaris /bin/sh, which lacks the builtin and where /bin/printf can dump
core on long strings).

But overall, I like the idea.  Let's see another revision with these
comments taken into account (either with updated code, or with a
rebuttal why changing code based on a particular comment doesn't make
sense after all) before pushing anything.

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature