Re: Fixing plural case in Banner

[Ralf Wildenhues]

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
+
+: