Re: [PATCH] Improve and extend tests `pluseq*.test' (on `+=' support).

[Ralf Wildenhues]

Hello Stefano,

* Stefano Lattarini wrote on Tue, Dec 07, 2010 at 11:42:35AM CET:
> Subject: [PATCH] Improve and extend tests `pluseq*.test' (on `+=' support).
> 
> * tests/pluseq.test: Enable `errexit' shell flag.  Make grepping
> of generated Makefile.in stricter.
> * tests/pluseq2.test: Also run autoconf, run ./configure with
> different values of conditionals, and do deeper tests by running
> `make' properly.
> * tests/pluseq3.test: Likewise.  Also, relax grepping of generated
> Makefile.in w.r.t. whitespaces, to avoid depending too much on
> automake internals.
> * tests/pluseq4.test: Improve testcase description.  Make grepping
> of the generated Makefile.in slightly stricter.  Extend the test a
> bit.
> * tests/pluseq6.test: Update testcase description.  Make grepping
> of the generated Makefile.in slightly stricter.  Remove useless
> AC_SUBST from configure.in and useless variable definition from
> Makefile.am.  Avoid unnecessary use of a temporary variable.
> Remove "threatening" comment.
> * tests/pluseq-header-vars.test: New test, taking over the older
> role of pluseq6.test.
> * tests/pluseq7.test: Make grepping of automake error messages
> stricter.  Remove "threatening" comment.
> * tests/pluseq8.test: Run autoconf, ./configure and make rather
> than resorting to an overly complex grepping of Makefile.in.
> Add comments telling to keep it in sync with ...
> * tests/pluseq12.test: ... this new test, similar to pluseq8.test,
> but using leading tabs in continuation lines.
> * tests/pluseq10.test: Prefer running tests from extra rules
> in Makfile.am, rather then grepping `make' output.
> * tests/pluseq-comment.test: New test on `+=' and comments.
> * tests/pluseq-comment-bslash.test: Likewise, but xfailing.
> * tests/Makefile.am (TESTS, XFAIL_TESTS): Update.

This patch is not ok.  The reasons can be read in the comments inline.

Sorry,
Ralf

> --- /dev/null
> +++ b/tests/pluseq-comment-bslash.test
> @@ -0,0 +1,52 @@

> +# Test for `+=' and comments, when line continuations *in comments*
> +# are involved.
> +# This test is currently failing; it's not even clear if "fixing"
> +# automake to make it pass would be worthwhile.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >>configure.in << 'END'
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am << 'END'
> +VAR = foo
> +VAR += quux # \
> +zardoz
> +VAR += bar
> +
> +.PHONY: test
> +test:
> +     test x'$(VAR)' = x'foo quux bar'

This doesn't seem right.  The first += line above is what I'd call
invoking undefined behavior, and I don't really want to have to define
it.  Thus I prefer this test to be omitted from the patch.

The problem is that we do not want to disallow things like
  var = -D"foo=\"#3\"' -D'bar="#4"'  ... \
       ...

and that this works with some or most makes, and we cannot know if the
author wanted to be fully portable or not.

> +END
> +
> +$ACLOCAL
> +$AUTOMAKE
> +
> +grep '^VAR *=.*#' Makefile.in && Exit 1
> +grep '^VAR *=.*zardoz' Makefile.in && Exit 1
> +
> +$AUTOCONF
> +
> +./configure
> +$MAKE test
> +
> +:

> --- /dev/null
> +++ b/tests/pluseq-comment.test
> @@ -0,0 +1,49 @@
> +
> +# Test for += and comments.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat >>configure.in << 'END'
> +AC_OUTPUT
> +END
> +
> +cat > Makefile.am << 'END'
> +VAR = foo # foo-bad
> +VAR += bar # bar-bad
> +VAR += baz1 \
> +baz2 # baz-bad
> +VAR += quux

Same thing with this test as with the previous one.  Please omit the
test.

> +.PHONY: test
> +test:
> +     test x'$(VAR)' = x'foo bar baz1 baz2 quux'
> +END
> +
> +$ACLOCAL
> +$AUTOMAKE
> +
> +grep '^VAR *=.*-bad' Makefile.in && Exit 1
> +
> +$AUTOCONF
> +
> +./configure
> +$MAKE test
> +
> +:

> --- /dev/null
> +++ b/tests/pluseq-header-vars.test

> +# Test that `+=' works with standard header-vars.
> +# Please update this test if definition of `pkgdatadir' is modified or
> +# removed from `lib/am/header-vars.am'.
> +
> +. ./defs || Exit 1
> +
> +set -e
> +
> +cat > Makefile.am << 'END'
> +pkgdatadir += foobar
> +pkgdatadir += \
> +  zardoz

Hrmph.  The test is ok, but adding to a variable that is supposed to
name a directory seems something that we might want to warn about in
the future: it's a pretty clear semantic error by the user.

How about using a less problematic special variable, say, EXTRA_DIST?

> +END
> +
> +$ACLOCAL
> +$AUTOMAKE
> +
> +grep '^pkgdatadir *= *\$(datadir)/@PACKAGE@ foobar zardoz *$' Makefile.in
> +test `grep '^pkgdatadir *=' Makefile.in | wc -l` -eq 1
> +
> +:

> --- a/tests/pluseq10.test
> +++ b/tests/pluseq10.test
> @@ -40,9 +40,16 @@ foo += b0.h \
>    b1.h
>  endif
>  
> -.PHONY: print
> -print:
> -     @echo BEG: $(foo) :END
> +.PHONY: test1 test2
> +test1:
> +     ## The value of $(foo) shouldn't contain any backslash character.
> +     case '$(foo)' in *\\*) exit 1;; *) exit 0;; esac

Do you only want to ensure here that automake doesn't introduce/keep
backslashes, or also that make flattens them upon expansion?  The latter
requires that the variable definition still keeps backslash-newline
runs, which are usually added by automake with long-enough lines.

> +test2:
> +     ## Take care of possible extra whitespaces introduced by automake
> +     ## when conditionals are involved.  These extra spaces must
> +     ## beconsidered an implementation detail, and shouldn't cause
> +     ## spurious testsuite failure.
> +     test x"`echo $(foo)`" = x'0.h a0.h a1.h a2.h a3.h'
>  END
>  
>  $ACLOCAL
> @@ -50,8 +57,6 @@ $AUTOCONF
>  $AUTOMAKE
>  
>  ./configure
> -$MAKE print >stdout || { cat stdout; Exit 1; }
> -cat stdout
> -$FGREP 'BEG: 0.h a0.h a1.h a2.h a3.h :END' stdout
> +$MAKE test1 test2
>  
>  :


> --- a/tests/pluseq3.test
> +++ b/tests/pluseq3.test
> @@ -15,14 +15,15 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -# Another `+=' test with conditionals.
> +# Test `+=' with conditionals and line wrapping.
>  
>  . ./defs || Exit 1
>  
>  set -e
>  
>  cat >> configure.in << 'END'
> -AM_CONDITIONAL([CHECK], [true])
> +AM_CONDITIONAL([CHECK], [test x"$cond_check" = x"yes"])
> +AC_OUTPUT
>  END
>  
>  cat > Makefile.am << 'END'
> @@ -40,14 +41,29 @@ else
>  data_DATA += dog
>  endif
>  
> +.PHONY: test_CHECK_TRUE test_CHECK_FALSE
> +test_CHECK_TRUE:
> +     test x'$(data_DATA)' = 
> x'zarrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr doz'

I think this can fail with some make implementations, for expansion
details reasons.  I'm not sure though, and haven't tried it.

> +test_CHECK_FALSE:
> +     test x'$(data_DATA)' = x'dog'
> +
>  END
>  
>  $ACLOCAL
>  $AUTOMAKE
>  
> -grep 'address@hidden@data_DATA = 
> zarrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr \\$' 
> Makefile.in
> -grep 'address@hidden@        doz$' Makefile.in
> +# Weaker grepping checks, for backward-compatibility.  Might need to
> +# be adapted if automake insternals are changed.
> +grep 'address@hidden@data_DATA *= 
> *zarrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrrr \\$' 
> Makefile.in
> +grep 'address@hidden@[       ][      ]*doz$' Makefile.in
> +grep 'address@hidden@data_DATA *= *dog$' Makefile.in
> +
> +$AUTOCONF
> +
> +./configure cond_check=yes
> +$MAKE test_CHECK_TRUE
>  
> -grep 'address@hidden@data_DATA = dog$' Makefile.in
> +./configure cond_check=no
> +$MAKE test_CHECK_FALSE
>  
>  :

> --- a/tests/pluseq4.test
> +++ b/tests/pluseq4.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/>.
>  
> -# Yet another `+=' test.
> +# Check that we can extend AC_SUBST'd variables using `+='.

Interesting.  I didn't know this was possible at all.  Hmpf, I need to
reconsider my mental model about += and internal variables.

>  . ./defs || Exit 1
>  
> @@ -22,15 +22,19 @@ set -e
>  
>  cat >> configure.in << 'END'
>  AC_PROG_CC
> +AC_SUBST([FOOBAR])
>  END
>  
>  cat > Makefile.am << 'END'
>  bin_PROGRAMS = foo
>  CC += -Dwhatever
> +FOOBAR += zardoz
>  END
>  
>  $ACLOCAL
>  $AUTOMAKE
> -$FGREP '@CC@ -Dwhatever' Makefile.in
> +
> +grep '^CC *= address@hidden@ -Dwhatever *$' Makefile.in
> +grep '^FOOBAR *= address@hidden@ zardoz *$' Makefile.in
>  
>  :

> --- a/tests/pluseq6.test
> +++ b/tests/pluseq6.test
> @@ -14,27 +14,21 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -# Test that `+=' works with standard header-vars.
> +# Test that `+=' works with $(mandir).  This test is mostly kept for
> +# historical reasons, and to be safe w.r.t. backward compatibility.
>  
>  . ./defs || Exit 1
>  
>  set -e
>  
> -cat >> configure.in << 'END'
> -AC_SUBST([ZZZ])
> -END

OK, now we're plainly in oscillation land.  In a prior patch, you added
this code, now you remove it again, see 'git log -p tests/pluseq6.test'.
That is usually a clear indicator for me that either we have no idea
where we want to arrive at in the end, or the code is already good
enough as it is.

Usually, without a good rationale, I take such a sign as reason to shoot
down a patch completely right away.  It shows the signal-to-noise ratio
has gone down the drain.  Please make sure that this doesn't happen.

This is really important, and you should take the time to fully
understand it.  I want to arrive at *maintainable* code, that is code
that doesn't need changes.  Testsuite additions should not ever need
regular changes, once they are in the system, and this particular patch
just helps to reinforce that view.  (Although of course oscillation is
bad in code outside of the testsuite as well.)

> -# If you do this in a real Makefile.am, I will kill you.

Do not remove this comment!  That's one of the few really useful ones.

>  cat > Makefile.am << 'END'
>  mandir += foo
> -zq = zzz
>  END
>  
>  $ACLOCAL
>  $AUTOMAKE
> -$FGREP '@mandir@ foo' Makefile.in
>  
> -num=`grep '^mandir =' Makefile.in | wc -l`
> -test $num -eq 1
> +grep '^mandir *= address@hidden@ foo *$' Makefile.in
> +test `grep '^mandir *=' Makefile.in | wc -l` -eq 1
>  
>  :

> --- a/tests/pluseq7.test
> +++ b/tests/pluseq7.test
> @@ -26,7 +26,6 @@ AC_PROG_CC
>  AC_PROG_RANLIB
>  END
>  
> -# If you do this in a real Makefile.am, I will kill you.

See above.

>  cat > Makefile.am << 'END'
>  lib_LIBRARIES = libq.a
>  libq_a_SOURCES = q.c
> @@ -35,6 +34,6 @@ END
>  
>  $ACLOCAL
>  AUTOMAKE_fails
> -grep 'Makefile.am:3:.*AR' stderr
> +grep '^Makefile\.am:3:.*AR.*must be set.*before.*+=' stderr
>  
>  :

> --- a/tests/pluseq8.test
> +++ b/tests/pluseq8.test
> @@ -15,32 +15,33 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -# Another `+=' test.  From Paul Berrevoets.
> +# Another `+=' test w.r.t. line continuations.  From Paul Berrevoets.
> +# Keep this in sync with sister test `pluseq12.test'.
>  
>  . ./defs || Exit 1
>  
>  set -e
>  
> +cat >>configure.in << 'END'
> +AC_OUTPUT
> +END
> +
>  cat > Makefile.am << 'END'
>  VAR = \
>      one \
>      two
>  VAR += three
> +
> +.PHONY: test
> +test:
> +     test x'$(VAR)' = x'one two three'
>  END
>  
>  $ACLOCAL
> +$AUTOCONF
>  $AUTOMAKE
>  
> -sed -n -e '/^VAR =/ {
> -   :loop
> -    p
> -    n
> -    t clear
> -    :clear
> -    s/\\$/\\/
> -    t loop
> -    p
> -    n
> -   }' Makefile.in | grep three
> +./configure
> +$MAKE
>  
>  :