Hello William, thanks for the patches. Here's a pretty nitty review, I hope you don't mind.
* William Pursell wrote on Sat, Oct 11, 2008 at 05:27:25AM CEST: > Ralf Wildenhues wrote: >>> + if test "$$all" -eq 1; then \ >>> + tests="test"; \ >>> + All=""; \ >>> + else \ >>> + tests="tests"; \ >>> + All="All"; \ >> >> How about making that "All " ... > > I had considered that, but thought the > extra indentation on " 1 test passed" was > okay, and more acceptable than $$All$$all > in the shell code, which is IMO a bit > ugly. But I'm certainly willing to defer > to your opinion on this point. Well, ugliness in the code is less of a problem than in the output. The code is *very* ugly in parts anyway. * William Pursell wrote on Sat, Oct 11, 2008 at 08:34:43AM CEST: > > Test for fence post case in the banner. > > Treating test counts of one as a special case in > the banner (eg printing "1 test passed" instead of > "All 5 tests passed") requires checks for all > cases in which pass, fail, or skipped counts is 1. > This test does not actually validate the text of > the banner, but just ensures that things work as > expected. Hmm. The working of things is already checked by some of the other tests. I'd prefer to have a test that actually ensures that there are no wrong singular or plural forms. So I added a few grep patterns for sentences that are wrong, and bingo, found that we had forgotten the expected failures/passes. I renamed your test to check10, as the check* tests all test the testsuite interface. (This helps consistency.) > --- /dev/null > +++ b/tests/banner.test > @@ -0,0 +1,57 @@ > +#! /bin/sh > +# Copyright (C) 1999, 2001, 2002 Free Software Foundation, Inc. This file should have copyright year 2008 only, and the older ones added only if it takes much from another test. > +for name in pass fail skip; do > + cat > $name-test <<- EOF I'm not sure whether there are old shells that don't understand "<<-", but I prefer we avoid the question by unrolling this loop. Also, and you certainly can't know this, there are a couple of maintainer-check tests in the toplevel Makefile.am which don't cope with this. > +cat > configure.in <<- EOF > + AC_INIT([GNU Automake banner test], [0], [dev/null]) > + AM_INIT_AUTOMAKE([1.10a]) > + AC_CONFIG_FILES([Makefile]) > + AC_OUTPUT > +EOF The defs.in script already prvoides a skeleton configure.in script which just needs completion here. > +echo TESTS = fail-test skip-test pass-test > Makefile.am > +$ACLOCAL || Exit 1 I've moved to just using 'set -e' and drop all the '|| Exit 1' in new tests; the errexit support is reasonably ok on most systems now. This is what I arrived at, relative to your first patch. I then committed these changes combined, to master and branch-1-10: <http://git.savannah.gnu.org/gitweb/?p=automake.git;a=commitdiff;h=8f126edc167f035c6f9b9fd7607e7dfa77f33e60> Thanks again, Ralf * tests/check10.test: New test. * tests/Makefile.am: Update. diff --git a/lib/am/check.am b/lib/am/check.am index f8c1436..fe9c0c6 100644 --- a/lib/am/check.am +++ b/lib/am/check.am @@ -91,19 +91,21 @@ check-TESTS: $(TESTS) All=""; \ else \ tests="tests"; \ - All="All"; \ + All="All "; \ fi; \ if test "$$failed" -eq 0; then \ if test "$$xfail" -eq 0; then \ - banner="$$All $$all $$tests passed"; \ + banner="$$All$$all $$tests passed"; \ else \ - banner="$$All $$all $$tests behaved as expected ($$xfail expected failures)"; \ + if test "$$xfail" -eq 1; then s=; else s=s; fi; \ + banner="$$All$$all $$tests behaved as expected ($$xfail expected failure$$s)"; \ fi; \ else \ if test "$$xpass" -eq 0; then \ banner="$$failed of $$all $$tests failed"; \ else \ - banner="$$failed of $$all $$tests did not behave as expected ($$xpass unexpected passes)"; \ + if test "$$xpass" -eq 1; then es=; else es=es; fi; \ + banner="$$failed of $$all $$tests did not behave as expected ($$xpass unexpected pass$$es)"; \ fi; \ fi; \ ## DASHES should contain the largest line of the banner. diff --git a/tests/Makefile.am b/tests/Makefile.am index 7f9ff9f..cc95743 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -96,6 +96,7 @@ check6.test \ check7.test \ check8.test \ check9.test \ +check10.test \ checkall.test \ clean.test \ clean2.test \ diff --git a/tests/check10.test b/tests/check10.test new file mode 100755 index 0000000..21e013d --- /dev/null +++ b/tests/check10.test @@ -0,0 +1,87 @@ +#! /bin/sh +# Copyright (C) 2008 Free Software Foundation, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3, or (at your option) +# any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# 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 singular and plural in test summaries. + +. ./defs || Exit 1 + +set -e + +cat >> configure.in << 'END' +AC_OUTPUT +END + +cat > Makefile.am << 'END' +TESTS = fail pass skip xfail xpass fail2 pass2 skip2 xfail2 xpass2 +XFAIL_TESTS = xfail xpass xfail2 xpass2 +END + +cat >>pass <<'END' +#! /bin/sh +exit 0 +END +cat >>fail <<'END' +#! /bin/sh +exit 1 +END +cat >>skip <<'END' +#! /bin/sh +exit 77 +END +chmod a+x pass fail skip +cp pass pass2 +cp pass xpass +cp xpass xpass2 +cp fail xfail +cp fail fail2 +cp xfail xfail2 +cp skip skip2 + +$ACLOCAL +$AUTOCONF +$AUTOMAKE -a + +unset TESTS || : + +./configure +( + # Do not check for failure in this subshell + set +e + env TESTS=pass $MAKE -e check + env TESTS=fail $MAKE -e check + env TESTS=skip $MAKE -e check + env TESTS=xfail $MAKE -e check + env TESTS=xpass $MAKE -e check + env TESTS="pass pass2" $MAKE -e check + env TESTS="fail fail2" $MAKE -e check + env TESTS="skip skip2" $MAKE -e check + env TESTS="xfail xfail2" $MAKE -e check + env TESTS="xpass xpass2" $MAKE -e check + env TESTS='pass skip xfail' $MAKE -e check + $MAKE check +) >stdout +cat stdout + +grep '1 [tT]ests' stdout && Exit 1 +grep '^[^1]* [tT]est ' stdout && Exit 1 +grep '1 .* were ' stdout && Exit 1 +grep '^[^1]* was' stdout && Exit 1 +grep 'All 1 ' stdout && Exit 1 +grep '^ .*[tT]est' stdout && Exit 1 +$EGREP '1 (un)?expected (failures|passes)' stdout && Exit 1 +$EGREP '[^1] (un)?expected (failure|pass)\)' stdout && Exit 1 + +:
