Re: [PATCH 1/7] Tests defs: various reorderings, some improvements.
* Stefano Lattarini wrote on Thu, Nov 11, 2010 at 12:37:23AM CET: > Well, the patch is "almost" just code reordering, with the very minor > exceptions that: > > 1. in the older version, some error messages could be printed before > `$me' was defined, so they couldn't use it; now `$me' is defined > early, so those error messages has been amended to include it; > > 2. in the older version, a test might be skipped from within code > in `tests/defs' *after* the exit trap was installed, so a call > to "Exit 77" was in order; now the skips only happens before the > installation of the exit trap, so "exit 77" is enough; > > 3. some comments has been slighty adjusted and extended, because the > act of reordering the code suggested them to me. > > Anyway, none of these additional "minor" changes is really required to > make the new `tests/defs.in' work correctly, so I agree that they should > have been done in follow-up patches. I've respin and split my patch to > do so (see the attached four patches). Thank for respinning. The patches are ok then, but I think you should take care that the newly-introduced `pwd` is replaced by the variable you later introduce for this (in the later patch, of course). Thanks, Ralf
Re: [PATCH 7/7] Tests defs: move static definitions in a new file `minidefs'.
* Stefano Lattarini wrote on Thu, Nov 11, 2010 at 12:16:27AM CET: > On Wednesday 10 November 2010, Ralf Wildenhues wrote: > > * Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:29:27PM CET: > > > Tests defs: move static definitions in a new file `minidefs'. [...] > > > * tests/defs: ... this new file ... > > > * tests/minidefs.in: ... and this new file. > > > > You need to mention the remaining changes here; e.g., that defs sources > > the other file now. > Is this OK? > > * tests/defs.in: Removed, its contents split among ... > * tests/defs-static.in: ... this new file ... > * tests/defs: ... and this new file, including the former. Sure, if that really is all that was changed. Thanks, Ralf
Re: [PATCH 5/5] Tests defs: improve messages for skipped tests.
On Wednesday 10 November 2010, Ralf Wildenhues wrote: > * Stefano Lattarini wrote on Wed, Sep 08, 2010 at 08:57:19PM CEST: > > * tests/defs.in: Give meaningful messages about the reasons of a > > test skip; this is especially useful as this file is run without > > verbose xtraces on. Related reorderings in the code and new > > comments. > > What happened to this patch in the new, rebased patch series? Nothing; I just left it out, because my main concern in the new patch series was to arrive quickly at the separation of defs/defs-static which I need to write the patch optimizing the `instspc*.test' tests. > Was it already subsumed in some other series? No; I'm going to rebase it, and repropose it as a stand-alone patch soonish (after the present patch series is done with). Regards, Stefano
Re: [PATCH 1/7] Tests defs: various reorderings, some improvements.
On Wednesday 10 November 2010, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:00:47PM CET:
> > This is basically just a reordering patch, which should organize the
> > code in `tests/defs.in' in a clearer and more rational way.
> >
> > * tests/defs.in: Reordered various snippets of code in a clearer
> > way. Improved a couple of error messages, by reporting the test
> > name in them. Some comments added.
>
> I cannot review this patch. Either just reordering, or just other
> changes, but not both in one patch please.
Well, the patch is "almost" just code reordering, with the very minor
exceptions that:
1. in the older version, some error messages could be printed before
`$me' was defined, so they couldn't use it; now `$me' is defined
early, so those error messages has been amended to include it;
2. in the older version, a test might be skipped from within code
in `tests/defs' *after* the exit trap was installed, so a call
to "Exit 77" was in order; now the skips only happens before the
installation of the exit trap, so "exit 77" is enough;
3. some comments has been slighty adjusted and extended, because the
act of reordering the code suggested them to me.
Anyway, none of these additional "minor" changes is really required to
make the new `tests/defs.in' work correctly, so I agree that they should
have been done in follow-up patches. I've respin and split my patch to
do so (see the attached four patches).
Thanks and sorry for the noise,
Stefano
From d402d06c01d3f065ce7d2c0d05885d1873dac410 Mon Sep 17 00:00:00 2001
From: Stefano Lattarini
Date: Wed, 2 Jun 2010 22:25:25 +0200
Subject: [PATCH 1/4] Tests defs: various reorderings.
* tests/defs.in: Reordered various snippets of code in a
clearer way.
---
ChangeLog |6 +
tests/defs.in | 347 -
2 files changed, 199 insertions(+), 154 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 657ea37..8fd8bad 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2010-11-10 Stefano Lattarini
+
+ Tests defs: various reorderings.
+ * tests/defs.in: Reordered various snippets of code in a
+ clearer way.
+
2010-11-06 Stefano Lattarini
New tests on obsoleted usages of automake/autoconf macros (such
diff --git a/tests/defs.in b/tests/defs.in
index fe67b0f..57a9cde 100644
--- a/tests/defs.in
+++ b/tests/defs.in
@@ -20,6 +20,11 @@
# Defines for Automake testing environment.
# Tom Tromey
+
+## - ##
+## Shell and environment sanitizing (plus some early initializations). ##
+## - ##
+
# Absolutely necessary variable(s).
testsrcdir='@abs_srcdir@'
top_testsrcdir='@abs_top_srcdir@'
@@ -44,26 +49,20 @@ else
case `(set -o) 2>/dev/null` in *posix*) set -o posix;; esac
fi
-# Ensure we are running from the right directory.
-test -f ./defs || {
- echo "defs: not found in current directory" 1>&2
- exit 1
-}
-
-# Ensure $testsrcdir is set correctly.
-test -f "$testsrcdir/defs.in" || {
- echo "$testsrcdir/defs.in not found, check \$testsrcdir" 1>&2
- exit 1
-}
+echo "=== Running test $0"
me=`echo "$0" | sed -e 's,.*[\\/],,;s/\.test$//'`
+
+## --- ##
+## Initialization: AC_SUBST'ed and/or environment variables. ##
+## --- ##
+
APIVERSION='@APIVERSION@'
PATH_SEPARATOR='@PATH_SEPARATOR@'
# Make sure we override the user shell.
-SHELL='@SHELL@'
-export SHELL
+SHELL='@SHELL@'; export SHELL
# User can override various tools used.
test -z "$PERL" && PERL='@PERL@'
test -z "$MAKE" && MAKE=make
@@ -75,8 +74,7 @@ test -z "$MISSING" && MISSING=$top_testsrcdir/lib/missing
# (Tests for which this is inappropriate should use -Wno-error.)
test -z "$ACLOCAL" && ACLOCAL="aclocal-$APIVERSION -Werror"
# Extra flags to pass to aclocal before all other flags added by this script.
-ACLOCAL_TESTSUITE_FLAGS=
-export ACLOCAL_TESTSUITE_FLAGS
+ACLOCAL_TESTSUITE_FLAGS=''; export ACLOCAL_TESTSUITE_FLAGS
# See how Automake should be run. We put --foreign as the default
# strictness to avoid having to create lots and lots of files. A test
@@ -86,11 +84,130 @@ export ACLOCAL_TESTSUITE_FLAGS
# should use -Wnone or/and -Wno-error
test -z "$AUTOMAKE" && AUTOMAKE="automake-$APIVERSION --foreign -Werror -Wall"
-PATH="`pwd`$PATH_SEPARATOR$PATH"
+# POSIX no longer requires 'egrep' and 'fgrep',
+# but some hosts lack 'grep -E' and 'grep -F'.
+EGREP='@EGREP@'
+FGREP='@FGREP@'
+
+# The amount we should wait after modifying files depends on the platform.
+# For instance, Windows '95, '98 and ME have 2-second granularity
+# and can be up to 3 seconds in the future w.r.t. the system clock.
+sleep='sleep @MODIFICATION_DELAY@'
+
+# An old timestamp that can be given to a file, in "touch
Re: [PATCH 7/7] Tests defs: move static definitions in a new file `minidefs'.
On Wednesday 10 November 2010, Ralf Wildenhues wrote: > * Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:29:27PM CET: > > Tests defs: move static definitions in a new file `minidefs'. > > minidefs is not a good name, True, but I couldn't think of a better one right away. Luckily, you could ;-) > as it is bound to not stay "mini". > How about defs-static or defs-init? defs-static is good IMO. I've settled for it. > (And yes, I'm shying away from renaming defs, that's just too big and churny > a change for such a slight misnomer.) Yes, it would require modifying *all* the test scripts :-/ And I dare not to think about other possible merge problems ... > > The new file is designed to be idempotent w.r.t. mutiple inclusions, > > s/designed/meant/, no? OK. > I don't see a design document. ;-> > > > and this will help us to cope better with e.g. generated tests and > > tests with complex setups, scaffoldings or indirections. > > What is scaffoldings? I think the first line of the summary text was > completely sufficient. OK. Removed those lines. > > * tests/defs.in: Removed, it's contents split among ... > > its *blush* Fixed. > > * tests/defs: ... this new file ... > > * tests/minidefs.in: ... and this new file. > > You need to mention the remaining changes here; e.g., that defs sources > the other file now. Is this OK? * tests/defs.in: Removed, its contents split among ... * tests/defs-static.in: ... this new file ... * tests/defs: ... and this new file, including the former. > > * configure.ac (AC_CONFIG_FILES): Remove `tests/defs', add > > `tests/minidefs'. > > * configure.ac (AC_CONFIG_LINKS): Add `tests/defs'. > > * tests/Makefile.am ($(parallel_tests)): Update. > > ($(instspc_tests)): Likewise. > > * tests/.gitignore: Update. > > OK with those changes. > > Thanks, > Ralf > Thanks, Stefano
Re: [PATCH 6/7] Tests defs: new AC_SUBST'ed variable $top_testsbuilddir.
On Wednesday 10 November 2010, Ralf Wildenhues wrote: > * Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:21:09PM CET: > > * tests/defs.in ($top_testsbuilddir): New variable, initialized > > with the value AC_SUBST'ed from @abs_top_build...@. Mostly for > > completeness and consistency with $testsrcdir and $top_testsrcdir. > > Let's add this only when we need it. Do we? Not at the moment, but I thought it would be nice to have it anyway, if only for consistency/simmetry reasons. But I'm OK with removing it, too, if you insist. Do you? Thanks, Stefano
Re: [PATCH 5/7] Tests defs: $testsbuilddir is now AC_SUBST'ed.
On Wednesday 10 November 2010, Ralf Wildenhues wrote: > * Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:19:55PM CET: > > * tests/defs.in ($testsbuilddir): Initialize "statically" > > with the value AC_SUBST'ed from @abs_builddir@, rather than > > "dinamically" with the value returned by `pwd`. > > dynamically > > You could also just write instead: > > * tests/defs.in ($testsbuilddir): Substitute from @abs_build...@. Better than my uselessly convoluted description. I've settled for your simpler and clearer wording. Thanks, Stefano
Re: [PATCH 5/5] Tests defs: improve messages for skipped tests.
* Stefano Lattarini wrote on Wed, Sep 08, 2010 at 08:57:19PM CEST: > * tests/defs.in: Give meaningful messages about the reasons of a > test skip; this is especially useful as this file is run without > verbose xtraces on. Related reorderings in the code and new > comments. What happened to this patch in the new, rebased patch series? Was it already subsumed in some other series? Thanks, Ralf > --- a/tests/defs.in > +++ b/tests/defs.in > @@ -261,6 +261,7 @@ do >export GCJ >echo "$me: running $GCJ --version" >( $GCJ --version ) || exit 77 > + echo "$me: running $GCJ -v" >( $GCJ -v ) || exit 77 >;; > g++) > @@ -297,11 +298,16 @@ do >(echo foo >> $priv_check_temp) >/dev/null 2>&1 >overwrite_status=$? >rm -f $priv_check_temp > - test $overwrite_status = 0 && exit 77 > + if test $overwrite_status = 0; then > +echo "$me: this test shouldn't be run as root" > +exit 77 > + fi >;; > perl-threads) > - # Skip with Devel::Cover: it cannot cope with threads. > - test "$WANT_NO_THREADS" = yes && exit 77 > + if test x"$WANT_NO_THREADS" = x"yes"; then > +echo "$me: skip with Devel::Cover: it cannot cope with threads." > +exit 77 > + fi >;; > python) ># Python doesn't support --version, it has -V > @@ -317,7 +323,10 @@ do >(: > $ro_dir_temp/probe) >/dev/null 2>/dev/null >create_status=$? >rm -rf $ro_dir_temp > - test $create_status = 0 && exit 77 > + if test $create_status = 0; then > +echo "$me: support of read-only directories is required" > +exit 77 > + fi >;; > rst2html) ># Try the variants that are tried in check.am. > @@ -326,6 +335,7 @@ do >echo "$me: running $r2h --version" >$r2h --version && break 2 > done > +echo "$me: no proper rst2html program found" > exit 77 >done >;; > @@ -333,13 +343,16 @@ do ># DejaGnu's runtest program. We rely on being able to specify ># the program on the runtest command-line. This requires ># DejaGnu 1.4.3 or later. > - echo "$me: running runtest --version" > + echo "$me: running runtest SOMEPROGRAM=someprogram --version" >(runtest SOMEPROGRAM=someprogram --version) || exit 77 >;; > tex) ># No all versions of Tex support `--version', so we use ># a configure check. > - test -n "@TEX@" || exit 77 > + if test x'@TEX@' = x; then > +echo "$me: TeX is required, but it wasn't found by configure" > +exit 77 > + fi >;; > texi2dvi-o) ># Texi2dvi supports `-o' since Texinfo 4.1. > @@ -354,6 +367,37 @@ do >esac > done > > +# Using just `$testbuilddir' for the check here is ok, since the > +# further temporary subdirectory where the test will be run is > +# ensured not to contain any whitespace character. > +case $testbuilddir in > + *\ *|*\*) > +case " $required " in > + *' libtool '* | *' libtoolize '* ) > +echo "$me: libtool/libtoolized cannot cope correctly" > +echo "$me: with spaces in the build tree." > +exit 77 > +;; > +esac > +;; > +esac > + > +# This test is necessary, although Automake's configure script bails out > +# when $srcdir contains spaces. This is because $testsrcdir is in not > +# configure-time $srcdir, but is instead configure-time $abs_srcdir, and > +# that is allowed to contain spaces. > +case $testsrcdir in > + *\ * |*\ *) > +case " $required " in > + *' libtool '* | *' libtoolize '* | *' gettext '* ) > +echo "$me: our testsuite setup cannot cope correctly with spaces" > +echo "$me: in the source tree for libtool/gettext tests." > +exit 77 > +;; > + esac > + ;; > +esac > + > # We might need extra macros, e.g., from Libtool or Gettext. > # Find them on the system. > # Use `-I $top_testsrcdir/m4' in addition to `--acdir=$top_testsrcdir/m4', > @@ -384,16 +428,20 @@ case " $required " in >fi > done > case " $required " in > - *' libtool '* | *' libtoolize '* ) test $libtool_found = yes || exit > 77;; > - *' gettext '* ) test $gettext_found = yes || exit 77;; > -esac > -# Libtool cannot cope with spaces in the build tree. Our testsuite setup > -# cannot cope with spaces in the source tree name for Libtool and gettext > -# tests. Using just `$testbuilddir' for the check here is ok, since the > -# furter temporary subdirectory where the test will be run is ensured not > -# to contain any space. > -case $testsrcdir,$testbuilddir in > - *\ * | *\ *) exit 77;; > + *' libtool '*|*' libtoolize '*) > +if test x"$libtool_found" != x"yes"; then > + echo "$me: libtool/libtoolize is required, but libtool.m4 wasn't" > + echo "$me: found in direct
Re: [PATCH 7/7] Tests defs: move static definitions in a new file `minidefs'.
* Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:29:27PM CET: > Tests defs: move static definitions in a new file `minidefs'. minidefs is not a good name, as it is bound to not stay "mini". How about defs-static or defs-init? (And yes, I'm shying away from renaming defs, that's just too big and churny a change for such a slight misnomer.) > The new file is designed to be idempotent w.r.t. mutiple inclusions, s/designed/meant/, no? I don't see a design document. ;-> > and this will help us to cope better with e.g. generated tests and > tests with complex setups, scaffoldings or indirections. What is scaffoldings? I think the first line of the summary text was completely sufficient. > * tests/defs.in: Removed, it's contents split among ... its > * tests/defs: ... this new file ... > * tests/minidefs.in: ... and this new file. You need to mention the remaining changes here; e.g., that defs sources the other file now. > * configure.ac (AC_CONFIG_FILES): Remove `tests/defs', add > `tests/minidefs'. > * configure.ac (AC_CONFIG_LINKS): Add `tests/defs'. > * tests/Makefile.am ($(parallel_tests)): Update. > ($(instspc_tests)): Likewise. > * tests/.gitignore: Update. OK with those changes. Thanks, Ralf
Re: [PATCH 1/7] Tests defs: various reorderings, some improvements.
* Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:00:47PM CET: > This is basically just a reordering patch, which should organize the > code in `tests/defs.in' in a clearer and more rational way. > > * tests/defs.in: Reordered various snippets of code in a clearer > way. Improved a couple of error messages, by reporting the test > name in them. Some comments added. I cannot review this patch. Either just reordering, or just other changes, but not both in one patch please. Thanks, Ralf
Re: [PATCH 6/7] Tests defs: new AC_SUBST'ed variable $top_testsbuilddir.
* Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:21:09PM CET: > * tests/defs.in ($top_testsbuilddir): New variable, initialized > with the value AC_SUBST'ed from @abs_top_build...@. Mostly for > completeness and consistency with $testsrcdir and $top_testsrcdir. Let's add this only when we need it. Do we? Thanks, Ralf
Re: [PATCH 5/7] Tests defs: $testsbuilddir is now AC_SUBST'ed.
* Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:19:55PM CET: > * tests/defs.in ($testsbuilddir): Initialize "statically" > with the value AC_SUBST'ed from @abs_builddir@, rather than > "dinamically" with the value returned by `pwd`. dynamically You could also just write instead: * tests/defs.in ($testsbuilddir): Substitute from @abs_build...@. > Add sanity check on $testsbuilddir, similar to those on > $testsrcdir and $top_testsrcdir. OK. Thanks, Ralf
Re: [PATCH 3/7] Tests defs: rename $curdir -> $testbuilddir
* Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:17:48PM CET: > * tests/defs.in: Rename $curdir to $testbuildir, for clarity and > consistency with $testsrcdir and $top_testsrcdir. OK. Thanks, Ralf
Re: [PATCH 4/7] Tests defs: do not print message "Running test $0" anymore.
* Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:18:53PM CET: > * tests/defs.in: Printing the message "=== Running test $0" at > the beginning of each tests made sense when Automake used the old > test-driver, which sent all the output directly to stdout/stderr. > Now that the parallel test-driver is used, which saves output of > each test in its correspoding log file, that old message is just > useless noise. OK. It's not completely noise, as it still served as a guide to the eye when the ./defs preparation code ended and the actual test code started (but you broke that in an earlier patch in this patch series), when the test was run with 'sh -x ./test'. Thanks, Ralf
Re: [PATCH 2/7] Tests defs: prefer "$curdir" over "`pwd`".
* Stefano Lattarini wrote on Sun, Nov 07, 2010 at 04:16:53PM CET: > * tests/defs.in: We already save the value of `pwd` in $curdir > early in the file, so there no need to recalculate it later, when > the current working directory is not changed. OK. Thanks, Ralf
Re: More problems with `make -n' in automake-generated rules.
* Ralf Wildenhues wrote on Mon, Nov 01, 2010 at 10:18:55PM CET: > Fix and document rules to not touch the tree with `make -n'. > > * doc/automake.texi (Multiple Outputs): Document the problem of > modifications during dry-run execution, propose solution. > * NEWS: Update. > * automake.in (lang_vala_finish_target): Split recipe so the > stamp file is not removed with GNU `make -n'. > (lang_yacc_target_hook): Separate removal of parser output file > and header remaking. > * lib/am/lisp.am ($(am__ELCFILES)): Determine whether -n was > passed to make, take care not to remove any files in that case. > * lib/am/remake-hdr.am (%CONFIG_H%): Separate removal of > %STAMP% file from induced remaking of config header. > * tests/autohdrdry.test, tests/lispdry.test, tests/yaccdry.test: > New tests. > * tests/Makefile.am (TESTS): Update. This test needed a fixup, as yaccdry requires nonempty $YACC. Found by the NixOS Hydra, kudos to them! http://hydra.nixos.org/build/741504 The test could probably also be written without requiring bison (make -t or so), but I'd prefer to have the setup be realistic, that's more important than having coverage on every system out there. Pushed to maint. Thanks, Ralf Fix yaccdry.test failure: require bison. * tests/yaccdry.test: Require bison. Found by NixOS Hydra. diff --git a/tests/yaccdry.test b/tests/yaccdry.test index d11d3fe..d2e7632 100755 --- a/tests/yaccdry.test +++ b/tests/yaccdry.test @@ -16,6 +16,7 @@ # Removal recovery rules for headers should not remove files with `make -n'. +required=bison . ./defs || Exit 1 set -e
