Re: [PATCH] Modernize, improve and/or extend tests `colon*.test.

[Stefano Lattarini]

At Thursday 17 June 2010, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Sat, Jun 12, 2010 at 11:48:15PM CEST:
> > Another patch tweaking scripts in the testsuite.
> 
> Thanks, most of this is uncontroversial, but a couple of things I
>  don't understand:
> > --- a/tests/colon3.test
> > +++ b/tests/colon3.test
> > [CUT]
> > -
> > -# Makefile should depend on two.in.
> > -echo Grep2
> > -grep '^Makefile:.* \$(srcdir)/two.in' zardoz.in
> > -# Likewise three.in.
> > -echo Grep3
> > -grep '^Makefile:.* \$(srcdir)/three.in' zardoz.in
> > +$PERL -ne '
> > +  s/\bzardoz\.(in|am)\b/zrdz.$1/g;
> > +  print if /zardoz/;
> > +' <zardoz.in >out
> > +test ! -s out || { cat out; Exit 1; }
> 
> Your changes seem to have valued efficiency so far.  But calling
> perl is bound to be more expensive than a couple of greps.
Yes, but it's blazingly fast if compared with an aclocal/automake 
call.

In fact:

 $ time for i in 1 2 3 4; do sh ./colon3.test >/dev/null 2>&1; done
 real    0m9.366s
 user    0m6.656s
 sys     0m1.952s
 $ sed 's/^\$AUTOMAKE$/&; exit/' colon3.test >foo.test
 $ time for i in 1 2 3 4; do sh ./foo.test >/dev/null 2>&1; done
 real    0m8.975s
 user    0m6.660s
 sys     0m1.888s

And with colon3.test modified by the attached amended patch:

 $ time for i in 1 2 3 4; do sh ./colon3.test >/dev/null 2>&1; done
 real    0m9.302s
 user    0m6.656s
 sys     0m1.924s
 $ sed 's/^\$AUTOMAKE$/&; exit/' colon3.test >foo.test
 $ time for i in 1 2 3 4; do sh ./foo.test >/dev/null 2>&1; done
 real    0m8.831s
 user    0m6.608s
 sys     0m1.904s

> The change looks good to me, how come you went this way though?
> Just curious.
Because writing that perl script was waaay simpler than cooking up a 
sed script or a sed/grep pipeline doing the same thing portably.  
And even if that perl script isn't much efficient, it is very simple to
understand IMO.
Maybe you can think of a *simple* way to do that which doesn't involve 
perl?  Just curious (no pun intended :-)

> 
> > +# Makefile should depend on two.in and three.in.
> > +grep '^Makefile:.* \$(srcdir)/two\.in' zardoz.in
> > +grep '^Makefile:.* \$(srcdir)/three\.in' zardoz.in
> 
> FYI, such greps can break in the future, if the dependency lines
>  happen to be line-wrapped.  Yes, this has been there before.
Thanks for pointing this out.  I modified the test so that the perl 
script takes care of line wrapping too.

> 
> > --- a/tests/colon5.test
> > +++ b/tests/colon5.test
> > [CUT]
> >
> > +cat > Makefile.am <<'END'
> > +.PHONY: test
> > +test:
> > +   case ' $(DIST_COMMON) ' in \
> > +     *' $(srcdir)/Makefile.dep '*) exit 0;; \
> 
> Hmm.  The variable might just as validly list Makefile.dep, without
> preceding `$(srcdir)/'.  That is really necessary only if the file
>  is listed both as a prerequisite and as a target somewhere in the
>  makefile.
But why should we relax this test if it's working correctly?  After 
all, Automake is expected to have full control in the generation of 
the autotools-related rebuild rules, right?  And we can relax the test 
in the future if the necessity arises.
For the moment, I haven't changed this fragment.  Obviously, you are
free to amend it yourself if you still think it's too strict.

> > +
> > +sed '/@SET_MAKE@/d' <Makefile.in >Makefile.sed
> > +$MAKE -f Makefile.sed SHELL=$SHELL test
> 
> I see this idiom is used a couple of times in the testsuite.  How
>  come?
NOTE: the following is just my opinion...  Absolutely no warranty :-)
>  Because running ./configure is a bit more expensive than sed?
Yes, in part (and sometimes it is indeed *much* more expensive).  And 
also because, if one wants to run just a small subset of the Makefile's
rules, or to check just a small subset of its variables, he doesn't 
need all the prerequisites ./configure would look for (erroring out if 
it fails to find any of them).  For example, the older versions of
'ansi.test' seemed to use the "sed hack" to do this.

For the record, if I run the colon5.test in its present "sed hack"
form, I get:
  $ time for i in {1..10}; do ./colon5.test; done
  real    0m24.281s
  user    0m16.965s
  sys     0m4.604s
while if I modify it to run also autoconf and configure:
  $ time for i in {1..10}; do ./colon5.test; done
  real    0m39.141s
  user    0m23.913s
  sys     0m9.525s 

Regards,
    Stefano

From 34d0f933468b10233ba14819028bddea62d9743e Mon Sep 17 00:00:00 2001
From: Stefano Lattarini <address@hidden>
Date: Fri, 18 Jun 2010 12:12:54 +0200
Subject: [PATCH] Modernize, improve and/or extend tests `colon*.test.

* tests/colon.test: Rely on the `configure.in' stub created by
`./defs', rather than writing one from scratch.  Do not create
a useless dummy file.
* tests/colon4.test: Enable the `errexit' shell flag, and
related changes.  Also, rely on the `configure.in' stub created
by `./defs', rather than writing one from scratch.
* tests/colon7.test: Enable `errexit' shell flag, and related
changes.  Also, improve the generated `configure.in' file.
* tests/colon2.test: Likewise.  Also, add some new checks.
* tests/colon5.test: Improve the generated `configure.in' file.
Add new, much deeper checks.
* tests/colon6.test: Likewise.
* tests/colon3.test: Improve the generated `configure.in' file.
Prefer perl over pipelined grep.  Made stricter.  Other changes,
cosmetic and not.
---
 ChangeLog         |   19 +++++++++++++++++++
 tests/colon.test  |    8 +++-----
 tests/colon2.test |   26 ++++++++++++++++----------
 tests/colon3.test |   44 ++++++++++++++++++++++++--------------------
 tests/colon4.test |   16 +++++++++-------
 tests/colon5.test |   27 +++++++++++++++++++++------
 tests/colon6.test |   27 ++++++++++++++++++++++-----
 tests/colon7.test |   15 +++++++++------
 8 files changed, 123 insertions(+), 59 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c4e69a5..2e3a8d9 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
+2010-06-18  Stefano Lattarini  <address@hidden>
+
+       Modernize, improve and/or extend tests `colon*.test.
+       * tests/colon.test: Rely on the `configure.in' stub created by
+       `./defs', rather than writing one from scratch.  Do not create
+       a useless dummy file.
+       * tests/colon4.test: Enable the `errexit' shell flag, and
+       related changes.  Also, rely on the `configure.in' stub created
+       by `./defs', rather than writing one from scratch.
+       * tests/colon7.test: Enable `errexit' shell flag, and related
+       changes.  Also, improve the generated `configure.in' file.
+       * tests/colon2.test: Likewise.  Also, add some new checks.
+       * tests/colon5.test: Improve the generated `configure.in' file.
+       Add new, much deeper checks.
+       * tests/colon6.test: Likewise.
+       * tests/colon3.test: Improve the generated `configure.in' file.
+       Prefer perl over pipelined grep.  Made stricter.  Other changes,
+       cosmetic and not.
+
 2010-06-12  Ralf Wildenhues  <address@hidden>
 
        Remove a couple of unneeded conditionals from tests.
diff --git a/tests/colon.test b/tests/colon.test
index da7a9da..269e6b8 100755
--- a/tests/colon.test
+++ b/tests/colon.test
@@ -22,15 +22,13 @@
 
 set -e
 
-cat > configure.in << 'END'
-AC_INIT
-AM_INIT_AUTOMAKE(nonesuch, nonesuch)
-AC_OUTPUT(Makefile foo.h:foo.hin)
+cat >> configure.in <<END
+AC_CONFIG_FILES([foo.h:foo.hin])
+AC_OUTPUT
 END
 
 : > Makefile.am
 : > foo.hin
-: > stamp-h.in
 
 $ACLOCAL
 $AUTOMAKE
diff --git a/tests/colon2.test b/tests/colon2.test
index ebb2bfe..a52dfa8 100755
--- a/tests/colon2.test
+++ b/tests/colon2.test
@@ -1,5 +1,6 @@
 #! /bin/sh
-# Copyright (C) 1996, 2000, 2001, 2002  Free Software Foundation, Inc.
+# Copyright (C) 1996, 2000, 2001, 2002, 2010 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
@@ -18,18 +19,23 @@
 
 . ./defs || Exit 1
 
-cat > configure.in << 'END'
-AC_INIT
-AM_INIT_AUTOMAKE(nonesuch, nonesuch)
-AC_OUTPUT(Makefile:zardoz.in)
+set -e
+
+cat > configure.in <<END
+AC_INIT([$me], [1.0])
+AM_INIT_AUTOMAKE
+AC_CONFIG_FILES([Makefile:zardoz.in])
+AC_OUTPUT
 END
 
-: > zardoz.am
+echo 'dummy:' > zardoz.am
 
-$ACLOCAL || Exit 1
-$AUTOMAKE || Exit 1
+$ACLOCAL
+$AUTOMAKE
 
 # We actually check several things here.
-test -f zardoz.in || Exit 1
+test -f zardoz.in
 grep '^zardoz:' zardoz.in && Exit 1
-Exit 0
+grep '^dummy:' zardoz.in
+
+:
diff --git a/tests/colon3.test b/tests/colon3.test
index 6fed8b9..05b0694 100755
--- a/tests/colon3.test
+++ b/tests/colon3.test
@@ -1,6 +1,6 @@
 #! /bin/sh
-# Copyright (C) 1996, 1998, 2000, 2001, 2002, 2003
-#   Free Software Foundation, Inc.
+# Copyright (C) 1996, 1998, 2000, 2001, 2002, 2003, 2010 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
@@ -22,10 +22,11 @@
 
 set -e
 
-cat > configure.in << 'END'
-AC_INIT
-AM_INIT_AUTOMAKE(nonesuch, nonesuch)
-AC_OUTPUT(Makefile:zardoz.in:two.in:three.in)
+cat > configure.in <<END
+AC_INIT([$me], [1.0])
+AM_INIT_AUTOMAKE
+AC_CONFIG_FILES([Makefile:zardoz.in:two.in:three.in])
+AC_OUTPUT
 END
 
 : > zardoz.am
@@ -35,21 +36,24 @@ END
 $ACLOCAL
 $AUTOMAKE
 
-# We actually check several things here.
 # Automake should have created zardoz.in.
 test -f zardoz.in
 
 # The generated file should refer to zardoz.in and zardoz.am, but
-# never just "zardoz".
-echo Grep1
-grep zardoz zardoz.in | $FGREP -v 'zardoz.in' | $FGREP -v 'zardoz.am' > O || :
-# We cat the output file so we see in when verbose.
-cat O
-test -z "`cat O`"
-
-# Makefile should depend on two.in.
-echo Grep2
-grep '^Makefile:.* \$(srcdir)/two.in' zardoz.in
-# Likewise three.in.
-echo Grep3
-grep '^Makefile:.* \$(srcdir)/three.in' zardoz.in
+# never just "zardoz".  Also, Makefile should depend on two.in and
+# three.in.
+$PERL -npe '
+  # Take line wrapping into account.
+  while(s/\\\n//)
+  {
+    my $x = <>;
+    $x =~ s/^\s*//;
+    $_ .= $x;
+  }
+  s/\bzardoz\.(in|am)\b/zrdz.$1/g;
+' <zardoz.in >out
+$FGREP zardoz out && Exit 1
+grep '^Makefile:.* \$(srcdir)/two\.in' out
+grep '^Makefile:.* \$(srcdir)/three\.in' out
+
+:
diff --git a/tests/colon4.test b/tests/colon4.test
index cec3c86..97a4479 100755
--- a/tests/colon4.test
+++ b/tests/colon4.test
@@ -1,5 +1,6 @@
 #! /bin/sh
-# Copyright (C) 1998, 2000, 2001, 2002  Free Software Foundation, Inc.
+# Copyright (C) 1998, 2000, 2001, 2002, 2010 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
@@ -20,10 +21,11 @@
 
 . ./defs || Exit 1
 
-cat > configure.in << 'END'
-AC_INIT
-AM_INIT_AUTOMAKE(nonesuch, nonesuch)
-AC_OUTPUT(Makefile zardoz:one:two:three)
+set -e
+
+cat >> configure.in <<END
+AC_CONFIG_FILES([zardoz:two:three])
+AC_OUTPUT
 END
 
 : > Makefile.am
@@ -31,8 +33,8 @@ END
 : > two
 : > three
 
-$ACLOCAL || Exit 1
-$AUTOMAKE || Exit 1
+$ACLOCAL
+$AUTOMAKE
 
 # The rule should regenerate the file "zardoz".
 grep '^zardoz:one:two' Makefile.in && Exit 1
diff --git a/tests/colon5.test b/tests/colon5.test
index fe97f99..26f10e4 100755
--- a/tests/colon5.test
+++ b/tests/colon5.test
@@ -21,16 +21,31 @@
 
 set -e
 
-cat > configure.in << 'END'
-AC_INIT
-AM_INIT_AUTOMAKE(nonesuch, nonesuch)
-AC_OUTPUT(Makefile:Makefile.in:Makefile.dep)
+cat > configure.in <<END
+AC_INIT([$me], [1.0])
+AM_INIT_AUTOMAKE
+AC_CONFIG_FILES([Makefile:Makefile.in:Makefile.dep])
+AC_OUTPUT
 END
 
-: > Makefile.am
 : > Makefile.dep
 
+cat > Makefile.am <<'END'
+.PHONY: test
+test:
+       case ' $(DIST_COMMON) ' in \
+         *' $(srcdir)/Makefile.dep '*) exit 0;; \
+         *) exit 1;; \
+       esac
+END
+
 $ACLOCAL
 $AUTOMAKE
 
-grep 'Makefile:Makefile.in' Makefile.in
+grep 'Makefile:Makefile\.in' Makefile.in
+grep '^Makefile:.* \$(srcdir)/Makefile\.dep' Makefile.in
+
+sed '/@SET_MAKE@/d' <Makefile.in >Makefile.sed
+$MAKE -f Makefile.sed SHELL=$SHELL test
+
+:
diff --git a/tests/colon6.test b/tests/colon6.test
index 99877c5..4fb738e 100755
--- a/tests/colon6.test
+++ b/tests/colon6.test
@@ -21,16 +21,33 @@
 
 set -e
 
-cat > configure.in << 'END'
-AC_INIT
-AM_INIT_AUTOMAKE(nonesuch, nonesuch)
-AC_OUTPUT(demo/Makefile demo/version.good:demo/version.gin)
+cat > configure.in <<END
+AC_INIT([$me], [1.0])
+AM_INIT_AUTOMAKE
+AC_CONFIG_FILES([demo/Makefile demo/version.good:demo/version.gin])
+AC_OUTPUT
 END
 
 mkdir demo
 
-: > demo/Makefile.am
 : > demo/version.gin
 
+cat > demo/Makefile.am <<'END'
+.PHONY: test
+test:
+       case ' $(DIST_COMMON) ' in \
+         *' $(srcdir)/version.gin '*) exit 0;; \
+         *) exit 1;; \
+       esac
+END
+
 $ACLOCAL
 $AUTOMAKE
+
+$EGREP 'Makefile:.*(demo|version)' demo/Makefile.in && Exit 1
+grep 'version\.good:.*version\.gin' demo/Makefile.in
+
+sed '/@SET_MAKE@/d' <demo/Makefile.in >Makefile.sed
+$MAKE -f Makefile.sed SHELL=$SHELL test
+
+:
diff --git a/tests/colon7.test b/tests/colon7.test
index 4df6a13..9c6f1a6 100755
--- a/tests/colon7.test
+++ b/tests/colon7.test
@@ -1,5 +1,6 @@
 #! /bin/sh
-# Copyright (C) 1998, 2000, 2001, 2002  Free Software Foundation, Inc.
+# Copyright (C) 1998, 2000, 2001, 2002, 2010 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
@@ -19,9 +20,11 @@
 
 . ./defs || Exit 1
 
+set -e
+
 cat > configure.in << 'END'
-AC_INIT
-AM_INIT_AUTOMAKE(nonesuch, nonesuch)
+AC_INIT([colon7], [1.0])
+AM_INIT_AUTOMAKE
 AC_OUTPUT(subdir/bar:subdir/foo \
    Makefile \
    subdir/Makefile
@@ -34,8 +37,8 @@ mkdir subdir
 : > subdir/Makefile.am
 : > subdir/foo
 
-$ACLOCAL || Exit 1
-$AUTOMAKE || Exit 1
+$ACLOCAL
+$AUTOMAKE
 
 # shouldn't have any bar.in
 grep 'bar.in' subdir/Makefile.in && Exit 1
@@ -43,4 +46,4 @@ grep 'bar.in' subdir/Makefile.in && Exit 1
 # DIST_COMMON should have foo, not subdir/foo
 grep 'DIST_COMMON.*subdir/foo' subdir/Makefile.in && Exit 1
 
-Exit 0
+:
-- 
1.6.5