Re: [PATCH 2/5] Tests defs: new subroutine `skip' for test skipping.

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Mon, Nov 15, 2010 at 06:23:21PM CET:
> Tests defs: new subroutine `skip' for test skipping.

I don't like having this patch alone.  If we switch to functions, then
we should so wholesale, and at once, and fully.  Also, there is
precedent in {coreutils,gnulib}/tests/init.sh which has more features
than this implementation and I think we should follow (and/or improve
upon if that is not sufficient):

  # Print warnings (e.g., about skipped and failed tests) to this file number.
  # Override by defining to say, 9, in init.cfg, and putting say,
  # "export ...ENVVAR_SETTINGS...; exec 9>&2; $(SHELL)" in the definition
  # of TESTS_ENVIRONMENT in your tests/Makefile.am file.
  # This is useful when using automake's parallel tests mode, to print
  # the reason for skip/failure to console, rather than to the .log files.
  : ${stderr_fileno_=2}

  warn_() { echo "$@" 1>&$stderr_fileno_; }
  fail_() { warn_ "$ME_: failed test: $@"; Exit 1; }
  skip_() { warn_ "$ME_: skipped test: $@"; Exit 77; }
  framework_failure_() { warn_ "$ME_: set-up failure: $@"; Exit 99; }


Generally, when we change something, it is a good idea to avoid NIH.
(That doesn't mean we have to change to remove existing NIH, but avoid
new.  The point being that stuff that is new is untested and broken.)

That said, I found minor issues in the proposed patch which I'm listing
so you take measures to avoid them when redoing.  This kind of patch
should really be written with a script, not manually.

> * tests/defs.in (skip): New subroutine, causing the running
> test script to be SKIP'd, possibly with a meaningful message.
> Use it throughout.

This file is ...

> * tests/REAMDE: Updated.
> * tests/color.test: Use the new `skip' subroutine.
> * tests/color2.test: Likewise.
> * tests/compile2.test: Likewise.
> * tests/defs.in: Likewise.

... listed twice.

> * tests/dejagnu7.test: Likewise.
> * tests/distlinks.test: Likewise.
> * tests/distlinksbrk.test: Likewise.
> * tests/fn99.test: Likewise.
> * tests/fn99subdir.test: Likewise.
> * tests/forcemiss2.test: Likewise.
> * tests/fort5.test: Likewise.
> * tests/gettext3.test: Likewise.
> * tests/install2.test: Likewise.
> * tests/instfail-info.test: Likewise.
> * tests/instfail-java.test: Likewise.
> * tests/instfail-libtool.test: Likewise.
> * tests/instfail.test: Likewise.
> * tests/instmany-mans.test: Likewise.
> * tests/instmany-python.test: Likewise.
> * tests/instmany.test: Likewise.
> * tests/instsh3.test: Likewise.
> * tests/makej2.test: Likewise.
> * tests/mkinst3.test: Likewise.
> * tests/parallel-tests3.test: Likewise.
> * tests/subobj9.test: Likewise.
> * tests/symlink2.test: Likewise.
> * tests/tar.test: Likewise.
> * tests/tar2.test: Likewise.
> * tests/txinfo26.test: Likewise.
> * tests/vala2.test: Likewise.
> * tests/vala3.test: Likewise.
> * tests/vala5.test: Likewise.
> * tests/instdir-texi.test: Likewise, plus some other minor
> improvements.

Please refrain from doing other changes that don't belong in the same
patch.

> * tests/txinfo21.test: Likewise.

[...]

> --- a/tests/instsh3.test
> +++ b/tests/instsh3.test

> @@ -18,10 +18,11 @@
>  
>  required=non-root
>  . ./defs || Exit 1
> +
>  set -e

unrelated change

>  # Solaris /usr/ucb/touch does not accept -t.
> -touch -t $old_timestamp foo || Exit 77
> +touch -t $old_timestamp foo || "touch utility doesn't accept '-t' option"

missing skip

>  ./install-sh -d d1
>