On Friday 04 February 2011, Ralf Wildenhues wrote:
> Hello Stefano,
>
Hi Ralf, and sorry for the delay.
> you've already applied this patch, but I have a couple of nits anyway:
>
> * Stefano Lattarini wrote on Mon, Jan 24, 2011 at 04:04:30PM CET:
> > Subject: [PATCH] coverage: more tests on simple and parallel test drivers
> >
> > * tests/parallel-tests-subdir.test: New test.
> > * tests/check-exported-srcdir.test: Likewise.
> > * tests/check-tests-in-builddir.test: Likewise.
> > * tests/check-tests_environment.test: Likewise.
> > * tests/Makefile.am (TESTS): Update.
>
> > --- /dev/null
> > +++ b/tests/check-exported-srcdir.test
>
> > +# Check that the "Simple Tests" driver (either with or without the
>
> Since this test source will be used for both drivers, why not
> s/"Simple Tests"/testsuite/
>
OK.
> > +# parallel-tests option enabled) exports the `srcdir' value in the
> > +# environment of the tests. This is documented in the manual.
>
> > +mkdir SrcDir BuildDir
>
> Please let's avoid CamelCase in the future. We've managed almost two
> decades without it, we don't need it now. (Plus, it doesn't fit the
> GCS style.)
>
OK. Should I also change the test to avoid it? (From your formulation
above I'd guess the answer is "no", but better be sure).
> > --- /dev/null
> > +++ b/tests/check-tests-in-builddir.test
>
> > +# Check that the "Simple Tests" driver can find test in the srcdir as
> > +# well as in builddir, and that is prefers those in the builddir.
>
> See above.
>
Fixed.
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +cat >> configure.in << 'END'
> > +AC_OUTPUT
> > +END
> > +
> > +cat > Makefile.am << 'END'
> > +TESTS = foo.test bar.test
> > +EXTRA_DIST = $(TESTS)
> > +END
> > +
> > +cat > foo.test << 'END'
> > +#! /bin/sh
> > +exit ${FOO_EXIT_STATUS-0}
> > +END
> > +chmod a+x foo.test
> > +
> > +$ACLOCAL
> > +$AUTOCONF
> > +$AUTOMAKE -a
> > +
> > +mkdir build
> > +cd build
> > +
> > +../configure
> > +
> > +cat > bar.test << 'END'
> > +#! /bin/sh
> > +exit 0
> > +END
> > +chmod a+x bar.test
> > +
>
> unset FOO_EXIT_STATUS || :
>
I hadn't thought about this because it seemed really unlikely that the
user could have a variable named `FOO_EXIT_STATUS' in his environment.
But some extra safety won't hurt, I guess.
> > +$MAKE check >out 2>&1 || { cat out; Exit1; }
> > +cat out
> > +grep '\.\./foo' out && Exit 1
> > +grep '^PASS: foo.test *$' out
> > +grep '^PASS: bar.test *$' out
> > +
> > +rm -f test-suite.log foo.log bar.log
> > +
> > +FOO_EXIT_STATUS=1 $MAKE check >out 2>&1 && { cat out; Exit1; }
> > +cat out
> > +grep '\.\./foo' out && Exit 1
> > +grep '^FAIL: foo.test *$' out
> > +grep '^PASS: bar.test *$' out
> > +
> > +rm -f test-suite.log foo.log bar.log
> > +
> > +# Check that if the same test is present in srcdir and builddir,
> > +# the one in builddir is preferred.
> > +cp bar.test foo.test
> > +FOO_EXIT_STATUS=1 $MAKE check >out 2>&1 || { cat out; Exit1; }
> > +cat out
> > +grep '^PASS: foo.test *$' out
> > +grep '^PASS: bar.test *$' out
> > +
> > +# The tests in the builddir must be preferred also by "make dist".
> > +FOO_EXIT_STATUS=1 $MAKE distcheck
> > --- /dev/null
> > +++ b/tests/check-tests_environment.test
>
> > +# "Simple Tests" testsuite driver: check TESTS_ENVIRONMENT support.
> > +
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +cat >> configure.in << 'END'
> > +AC_OUTPUT
> > +END
> > +
> > +cat > Makefile.am << 'END'
> > +TESTS = foo.test
> > +EXTRA_DIST = $(TESTS)
> > +END
> > +
> > +cat > foo.test << 'END'
> > +#! /bin/sh
> > +test x"$FOO" = x"ok"
> > +END
> > +chmod a+x foo.test
> > +
> > +$ACLOCAL
> > +$AUTOCONF
> > +$AUTOMAKE -a
> > +
> > +./configure
> > +
> > +FOO=bad TESTS_ENVIRONMENT='FOO=ok' $MAKE check
> > +FOO=ok TESTS_ENVIRONMENT='FOO=bad' $MAKE check && Exit 1
>
> I wonder whether we need to unset TESTS_ENVIRONMENT in tests/defs.
>
That seems a good idea. Will do in a follow-up patch.
> > --- /dev/null
> > +++ b/tests/parallel-tests-subdir.test
>
> > +# Check that the parallel-tests driver creates parent directories for
> > +# the log files when needed.
>
> I think we have a test for this already. Just
> grep 'TESTS.*/' tests/*.test
>
Oops, that's true, it's already checked in `check8.test' (albeit
indirectly, so I missed that). Well, at least no existing test was
checking for a test in a sub-subdir like this one does, so it hasn't
been a completely useless addition after all.
> (I can understand your desire for more descriptive test names, but not
> in easy cases like this one. No matter how descriptive, you will still
> need to search existing tests now and then, simply because the naming
> may not be unique.)
>
> > +parallel_tests=yes
> > +. ./defs || Exit 1
> > +
> > +set -e
> > +
> > +cat >> configure.in << 'END'
> > +AC_OUTPUT
> > +END
> > +
> > +cat > Makefile.am << 'END'
> > +TESTS = dir1/foo.test dir2/dir3/foo.test
> > +TEST_LOG_COMPILER = sh
> > +END
> > +
> > +mkdir dir1 dir2 dir2/dir3
> > +echo : > dir1/foo.test
> > +echo : > dir2/dir3/foo.test
> > +
> > +$ACLOCAL
> > +$AUTOCONF
> > +$AUTOMAKE -a
> > +
> > +mkdir build
> > +cd build
> > +../configure
> > +$MAKE check
> > +find . # For debugging.
> > +test -f test-suite.log
> > +test -f dir1/foo.log
> > +test -f dir2/dir3/foo.log
>
> Thanks,
> Ralf
>
Attached is what I'll push (in 72 hours) if there are no further
objections.
Regards,
Stefano
From 5ed043ad03d36be44500501ec0d69f3ba736911d Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <stefano.lattar...@gmail.com>
Date: Sun, 6 Feb 2011 19:43:22 +0100
Subject: [PATCH] tests: tweak few tests on simple and parallel test drivers
* tests/check-exported-srcdir.test: Improve heading comments.
* tests/check-tests-in-builddir.test: Likewise. Also, unset the
`FOO_EXIT_STATUS' variable, so that any pre-existing value in the
environment won't risk to interfere with the test.
Suggestions by Ralf Wildenhues.
---
ChangeLog | 9 +++++++++
tests/check-exported-srcdir.test | 2 +-
tests/check-tests-in-builddir.test | 4 +++-
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/ChangeLog b/ChangeLog
index 31bdd66..3750915 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2011-02-06 Stefano Lattarini <stefano.lattar...@gmail.com>
+
+ tests: tweak few tests on simple and parallel test drivers
+ * tests/check-exported-srcdir.test: Improve heading comments.
+ * tests/check-tests-in-builddir.test: Likewise. Also, unset the
+ `FOO_EXIT_STATUS' variable, so that any pre-existing value in the
+ environment won't risk to interfere with the test.
+ Suggestions by Ralf Wildenhues.
+
2011-02-01 Stefano Lattarini <stefano.lattar...@gmail.com>
coverage: more tests on simple and parallel test drivers
diff --git a/tests/check-exported-srcdir.test b/tests/check-exported-srcdir.test
index 9209fc8..dc02b22 100755
--- a/tests/check-exported-srcdir.test
+++ b/tests/check-exported-srcdir.test
@@ -14,7 +14,7 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
-# Check that the "Simple Tests" driver (either with or without the
+# Check that the testsuite driver (either with or without the
# parallel-tests option enabled) exports the `srcdir' value in the
# environment of the tests. This is documented in the manual.
diff --git a/tests/check-tests-in-builddir.test b/tests/check-tests-in-builddir.test
index b30999b..2d0e423 100755
--- a/tests/check-tests-in-builddir.test
+++ b/tests/check-tests-in-builddir.test
@@ -14,7 +14,7 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.
-# Check that the "Simple Tests" driver can find test in the srcdir as
+# Check that the testsuite driver can find test in the srcdir as
# well as in builddir, and that is prefers those in the builddir.
. ./defs || Exit 1
@@ -36,6 +36,8 @@ exit ${FOO_EXIT_STATUS-0}
END
chmod a+x foo.test
+unset FOO_EXIT_STATUS || :
+
$ACLOCAL
$AUTOCONF
$AUTOMAKE -a
--
1.7.2.3
