Re: [PATCH] Overhauled and modularized tests in `instspc.test'.

[Stefano Lattarini]

On Monday 13 September 2010, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> > 
> > OK for master?
> 
> This patch ought to be tested with AIX sh, Tru64 ksh, and IRIX.
> When you're through addressing the nits below, and still don't have
> access, ping me and I'll test it.
Thanks for the offer!  In fact, it might take a while for to me get
access to those systems... so I might end up loading you with more
work :-(
> I'm sorry to have you do another round on this one.
Don't worry, that was to be expected, considered the potentially
disruptive nature of the patch.
 
> The patch introduces significant extra testsuite overhead.
How much, quantitatively speaking?  While writing the patch, I was
focused on correctness (which has been tricky enough to get right,
BTW), so I haven't noticed performance problems (since they've not
been dramatic IMHO).

> Would it be possible to unify build and install tests pairwise?
Not easily, since the "weird chars" causing expected failures for install
tests are different from those causing expected failures for build tests.
Besides, I have to say that I'd like to waste some machine cycles rather
than complicating this already complex patch even more.  And we can always
(try to) optimize later, if a real need arise.

> A big thanks for this one,
A big thanks for the review.

> Ralf
> 
> > From bc70edd3627fc20785211ad06c9fe386ebecccff Mon Sep 17 00:00:00
> > 2001 From: Stefano Lattarini <address@hidden>
> > Date: Sun, 6 Jun 2010 18:38:27 +0200
> > Subject: [PATCH] Overhauled and modularized tests in
> > `instspc.test'.
> > 
> > The test `instspc.test' was way too big and fragile.  Its running
> > time was very long.  It also produced a log that was nearly
> > unreadable due to its lenght, making it very difficult to find 
> length
Fixed.

> > out the reason of failures. 
> s/of/for/
Fixed
> (sorry for being so nitpicky)
Sorry?  Keep it up the nitpicking, you're in fact helping me to improve
my english! (long read ahead, mind you ;-)
 
> > ($(instspc_tests:.test=.log)): New rule, registers the dependency
> > of all `instspc*.test' tests from the `instspc.sh' script.
> s/from/on/
Fixed.

> > --- a/tests/Makefile.am
> > +++ b/tests/Makefile.am
> > @@ -16,6 +16,9 @@
> > 
> >  # You should have received a copy of the GNU General Public
> >  License # along with this program.  If not, see
> >  <http://www.gnu.org/licenses/>.
> > 
> > +MAINTAINERCLEANFILES =             ## updated later
> > +EXTRA_DIST = ChangeLog-old         ## likewise
> 
> Please avoid comments in variable settings.  I know that '##' are
> exceptional in that automake removes them, but here, not only would
> a comment on a line before that be more readable and less
> confusing, just dropping them would be most readable, and not less
> clear.
OK, no big deal for me.  Comments removed.

> > +
> > +include $(srcdir)/instspc-tests.am
> > +
> > +$(srcdir)/instspc-tests.am: gen-instspc-tests Makefile.am
> > +   $(AM_V_GEN)(cd $(srcdir) && $(SHELL) ./gen-instspc-tests) >$@
> 
> This cd needs to be $(am__cd);
Fixed.
> the one in the parallel_tests.am rule would need it too (separate
> patch for the latter is preapproved).
Done and pushed (to master).

> >  TESTS = \
> >  aclibobj.test \
> > 
> > @@ -411,7 +430,6 @@ instman2.test \
> > 
> >  instmany.test \
> >  instmany-mans.test \
> >  instmany-python.test \
> > -instspc.test \
> >  interp.test \
> >  interp2.test \
> >  java.test \
> > 
> > @@ -813,9 +831,11 @@ yaccpp.test \
> > 
> >  yaccvpath.test \
> >  yflags.test \
> >  yflags2.test \
> > 
> > -$(parallel_tests)
> > +$(parallel_tests) \
> > +$(instspc_tests)
> 
> There is no need to insert them at the end, you can just put them
> where instspc.test was before.  The parallel tests were only added
> there because there is no canonical alphabetic place for them.
Agreed.

> > --- /dev/null
> > +++ b/tests/gen-instspc-tests
> > 
> > +# Generate a Makefile fragment which defines:
> > +#  1. Rules to generate tests that we can building from, and/or install
> > +#     to, directories having in their name shell/make metacharacters,
> > +#     control characters, white spaces, autoconf quadrigraphs, or other
> > +#     suspicious/problematic tokens.
> > +#  2. The macro $(instspc_tests), containing the list of the tests
> > +#     generated by the makefile fragment.
> > +#  3. The macro $(xfail_instspc_test), containing the list of those
> 
> Typo?  s/test/&s/  ?
No, typo s/xfail_instspc_test/instspc_xfail_tests/ :-)

> 
> > +#     generated tests expected to fail.
> > +
> > +# Original report from James Amundson about file names with spaces.
> > +# Other characters added by Paul Eggert.
> > +
> > +set -e # abort on unhandled errors
> 
> Does this really need a comment?
Definitely not.  I can't fathom how I ended up adding it.  Removed.

> > +# Defined to help avoiding headaches with multiple escaping into
> > +# backquotes, below.
> > +escape_dollar() { sed 's/\$/$$/g'; }
> 
> GNU style is space before (), newlines before { and }
Even for one-liners ;-) ?  Anyway, fixed, I saw no reason not to.

> more instances below
Fixed those too.

> > +
> > +cat <<'END'
> > +## Generated by gen-instspc-tests.  DO NOT EDIT!
> > +instspc_tests =
> > +instspc_xfail_tests =
> > +END
> > +
> > +count=0
> > +# Some of these contain symbolic reperesentations of problematic
> representations
Fixed.

> > +# characters, which could easily confuse make (e.g. `#', `$' or
> > +# newline).  They will be properly expanded by instspc.sh.
> > +for weird_chars in \
> 
> "weird" is colloquial, and also not very fitting IMVHO.  Please use
> just $file or so, like the original code did.
But they are not file names, they are... well, if not weird, at least
"problematic" chars.  The later usages in instspc.sh confirm this IMO.
What about using "problematic_chars" or "problematic_string" instead?
> I guess I simply don't understand why this code deviates so much
> from the original.
 
> > +  '!' '"' '${sh}' '${dl}' '%' '&' '${sq}' '(' ')' '*' '+' ','
> > '-' \ +  ':' ';' '<' '=' '>' '?' '@' '[' '\' ']' '^' '`' '{' '|'
> > '}' '~' \ +  '${bs}' '${cr}' '${ff}' '${ht}' '${lf}' '${sp}' \
> > +  '@<:@' '@:>@' '@S|@' '@%:@' '@&t@' \
> > +  'a${sp}b' 'a${sp}${sp}b' 'a${lf}b' '...' 'a:'
> > +do
> > +  count=`expr $count + 1`
> > +  # Simply naming the tests using incremental numbers seems to
> > +  # be the best policy.
> 
> Would've been nice to have names reflecting the characters (maybe
> an ascii translation), but I understand that this might be too
> much to ask for.
At least for this patch.  That doesn't mean that a proper follow-up
will not be able to take care of that... someday ;-).
 
> > +  tst=instspc$count
> > +  # We have to escape `$' in makefiles.
> > +  case "$weird_chars" in
> > +    *\$*) weird_escaped=`printf '%s' "$weird_chars" | escape_dollar`;;
No no no!  Unportable to non-GNU sed!  I'll use this instead:
 *\$*) weird_escaped=`printf '%s\n' "$weird_chars" | escape_dollar`;;

> > +    *) weird_escaped=$weird_chars;;
> > +  esac
> > +  echo # separate sections for different tests
> 
> Do we really have to comment this?
No, but it doesn't hurt either IMHO.  Anyway, removed, since this is
a bike-shedding issue.

> > +  echo "$tst-build.test $tst-install.test: instspc-tests.am"
> > +  cat <<'END'
> > +   $(AM_V_at)rm -f $@ address@hidden
> > +## The apparently useless ":;" works around a bug of Bash 3.2 and earlier.
> 
> s/apparently useless //
> s/of/in/
Makes sense.   This is what I've replaced the above formulation with:
 ## The ":;" works around a bug in Bash 3.2 and earlier.  See section
 ## "Limitations of Shell Builtins" in the Autoconf manual for more info.
OK?

> > +## See section "Limitations of Shell Builtins" in the Autoconf manual.
> > +   $(AM_V_GEN) :; { \
> > +     action=`echo $@ | sed -e 's/\.test$$//' -e s/^.*-//;`; \
> > +     echo '#!/bin/sh'; \
> > +     echo '# DO NOT EDIT! GENERATED AUTOMATICALLY!'; \
> > +     echo 'required=gcc # FIXME: any C compiler should be ok!'; \
> 
> I don't understand the FIXME.  Either any compiler is ok, in which
> case we could remove the line, or it isn't, in which case there is
> nothing to fix.
Well, we currently don't have a way to require a generic C compiler in
out tests (patches pending).  The "FIXME" comment helps us to remember
that once this limitation is lifted, the script *might* be adjusted
accordingly.
 
> In this case, when you remove the required line, chances are
> nonzero that we'll get spurious extra failures due to some
> compilers having crappily-programmed shell wrapper drivers or so.
We'd see many other similar test failures anyway.
> The question is whether we want to see those issues as test
> failures or not.  I'm not sure.
Well, these failures would inform us of a real problem which would affect
also the rest of testuite, so I'd prefer to see them.  In the worst case,
we can adjust the (still theoretical-only) C-compiler-requiring code in
the test driver to skip tests when such braindead compiler wrappers are
encountered.  Or we could just tell the user to fix their damn wrappers ;-)
 
> > +     echo '. ./defs || Exit 99 || exit 99'; \
> ;-)
A similar tweaking (for all tests) is planned in my ongoing "Testsuite
initialization refactoring".  Do you want me to drop it in this patch
and re-insert it when the refactoring branch is merged?

> > +     echo action=$$action; \
> > +END
> > +  # leading tab here
> > +  echo "     echo weird_chars=\\''$weird_escaped'\\'; \\"
> > +  cat <<'END'
> > +     echo '. "$$testsrcdir/instspc.sh"'; \
> > +   } > address@hidden
> > +   $(AM_V_at)chmod a+rx address@hidden && mv -f address@hidden $@
> > +END
> > +  echo "instspc_tests += $tst-build.test $tst-install.test"
> > +  # Some of the above "weird" file names cannot be used as a build
> 
> See above.

What about these two comments instead?
 ---
  # Some of the above "problematic" characters cannot be used in the
  # name of a build directory on a POSIX host.  This list should be
  # empty, but is not due to limitations in Autoconf, Automake, Make,
  # M4, or the shell.
 ---
  # Similarly, some of the above "problematic" characters cannot be
  # used in the name of an install directory on a POSIX host.  Ideally,
  # this list also should be empty.
 ---


> > --- /dev/null
> > +++ b/tests/instspc.sh
> > 
> > +# Check that building from, or installing to, directories with shell
> > +# metacharacters succeed.
> > +
> > +# This script is expected to be sourced by specific, individual tests,
> > +# which, before its inclusion, should properly define the variable
Oops, s/variable/&s/ here.  Fixed.
> > +# `$weird_chars' and `$action' (this last one to either "build" or
> > +# "install").
> > +
> > +# Original report from James Amundson about file names with spaces.
> > +# Other characters added by Paul Eggert.
> > +
> > +# Transition from `instspc.test' to `instspc.sh' done
> > +# by Stefano Lattarini.
> 
> I was kinda hoping to avoid territoriality in Automake.  Fine if
> you insist,
I don't, but I'd prefer an "all-or-nothing" approach: either no author
name, or all author names (on a file-by-file basis, at least).  And it
seemed rude to me to just remove the names of the previous contributors.
OTOH...

> but I suggest reading the following for a discussion
> of the topic:
>  <http://producingoss.com/en/producingoss.html#territoriality>
"In any case, credit information can already be obtained from the
 version control logs and other out-of-band mechanisms like mailing
 list archives, so no information is lost by banning it from the
 source files themselves".
... I agree with this.  What about getting rid of all "author tags"
in the Automake tree?  This should obviously be done with a different
patch, and discussed in a different thread, so let's drop this topic
FTM (in the meantime, I removed the addition of my name to credits in
`instspc.sh').

>  (the whole book is very interesting BTW).
Yes it is.  Thanks for recommending it.

> > +# This is mostly the same input as nobase.test, but we do not use
> > +# libtool libraries, because Libtool does not preserve space in
> > +# file names (Issue observed with ltmain.sh (GNU libtool) 1.5a (1.1323
> > +# 2003/11/10 21:06:47))
> 
> Yeah, that reference to old code is not really relevant.  libtool
> does not preserve spaces, not even today.
OK, removed.
 
> > +set -e
> > +
> > +# This file will be sourced by many tests, so avoid cluttering up
> > +# the verbose logs too much.
> > +set +x
> 
> I understand, and agree, with the sentiment.  Problem however is
> that this will make it harder to debug your shell script, because
>   $ sh -x instspc-$foo.test
> will be useless.  Not sure how to avoid that.
Probably by analizying `$-' in tests/defs, and define proper variables
$disable_xtraces and $enable_xtraces.  But that's for a follow-up patch
IMHO (good material for the branch on `tests/defs.in' refactoring, BTW).

> > +echo "INFO: $me: shell traces disabled"
> 
> This weird "INFO: " prefix doesn't look like normal GCS-like
> messages.
This prefix is there so that the info messages stick out more clearly
among the tons of verbose logs. IMO we might think about a refactoring
of this later (maybe a new function in `tests/defs.in'?).

> Status messages on stderr?  At least for error messages.
Not a big deal in this case, since the rest of the testsuite too doesn't
discriminate very well between stdout and stderr, and both of them are
anyway redirected to log files by the testsuite driver.  Anyway, since
the change was very easy to do, and the resulting behaviour theoretically
more correct, I did the redirections to stderr.
 
> > +echo "INFO: $me: action=$action"
> > +echo "INFO: $me: weird_chars='$weird_chars'"
> > +
> > +# Sanity check 1.
> > +# NOTE: Don't use `test -z' here since Solaris 10 /bin/ksh seems
> > +# to have problems grokking `test -z ")"'.
> > +if test x"$weird_chars" = x; then
> > +  echo "$me: FATAL: \$weird_chars unset or empty"
> > +  Exit 99
> > +fi
> > +
> > +# Sanity check 2.
> > +case $action in
> > +  "") echo "$me: FATAL: \$action unset or empty"; Exit 99;;
> 
> inconsistent "$me: " prefixing order, see INFO: above.
True.

> FWIW, I'd just drop INFO: and FATAL:, it is not used anywhere
> else in the testsuite, and things should be clear from the exit
> status.  If you really want such a prefix, GCC uses 'warning: '
> and 'error: ' and I think 'notice: '.
> 
> More information on this topic in "info standards Errors".
Aren't we being a little to paranoid about standard format of messages,
considering how the logs from our testsuite look like anyway?  After
all, our tests are not reusable, flexible programs to be used by other
scripts someday...

For the moment, I've kept the "INFO:" prefixes, and dropped the "FATAL:"
almost-prefixes (as a fatal failure sticks out quite enough by itself).
Are you ok with that?

> > +  build|install) ;;
> > +  *) echo "$me: FATAL: invalid \$action: '$action'"; Exit 99;;
> > +esac
> > +
> > +# Some control characters that are white space.
> > +bs='' # back space
> > +cr='?' # carriage return
> > +ff='' # form feed
> > +ht='       ' # horizontal tab
> > +lf='
> > +'  # line feed (aka newline)
> 
> comment alignment?
Done.

> 
> > +sp=' ' # space
> > +
> > +# Some other character that might be problematic in makefiles.
> > +dl=\$ # dollar
> > +sq=\' # single quote
> > +sh=\# # sharp
> > +
> > +# Now we might have to perform some substitutions in the given
> > "weird +# file name".  This is necessary, because some "unusual"
> > characters +# can easily wreak havoc in makefiles (see the
> > comments in the script +# `gen-instspc-tests' for more
> > information).  Since we are at it, we +# perform some more
> > sanity checks.
> > +case $weird_chars in
> > +  *\#*|*"$lf"*|*'${lf}${lf}')
> 
> The second pattern can just be *$lf* here.
Now a moot point, considering the below.

> > +    echo "$me: invalid \$weird_chars: '$weird_chars'" >&2
> 
> Are you guarding against broken shells here, that mismatch
> patterns?
Nope, I am guarding against unexpected values generated by
gen-instspc-tests (say `${tab}' instead of '${ht}' -- this,
or something similar, already happened in practice).  This
guard helped me to catch various bugs in previous versions
of the patch, and will proably help us to avoid new ones.
> I don't understand what the failure scenario is for this.
See above.
 
> > +    Exit 99
> > +    ;;
> > + 
> > '${bs}'|'${cr}'|'${ff}'|'${ht}'|'${lf}'|'${sp}'|'${dl}'|'${sq}'|
> > '${sh}') +    eval "weird_chars=$weird_chars" || Exit 99
> 
> How can this eval go wrong?  Oh well, guess being extra safe is not
> a problem.
This seems especially true after having been forced to rewrite the patch
four or five times due to ever-subtler bugs ;-)

> > +    if test -z "$weird_chars"; then
> > +      echo "$me: \"weird\" char '$weird_chars' evaluated to empty" >&2
> > +      Exit 99
> > +    fi
This too is a guard againts "internal errors", in the sense of errors
introduced by the developer himself.

> > +    ;;
> > +  *'${sp}'*|*'${lf}'*)
> > +    #
> > +    # Whow, this is tricky!
> > +    #
> > +    # NOTE 1: The `subst_spaces' function helps us avoiding headaches
> > +    #         with backslashes into backquotes.
> > +    #
> > +    # NOTE 2: The use of `#' and newline as a characters "impossible in
> > +    #         input" is OK, because $weird_chars should never contain
> > +    #         newline or a `#' character (the first match in the current
> > +    #         "case" statement takes care of this check). 
> > +    #
> > +    # NOTE 3: We must add a trailing newline to the expression printed by
> > +    #         printf, to remain portable to all sed implementations.
> > +    #
> > +    # NOTE 4: We must be careful not to let the command suctitution strip
> > +    #         significant trailing newlines.  This is not difficult, as
> > +    #         we can be positive that there is at most one such trailing
> > +    #         newline (the first match in the current "case" statement
> > +    #         takes care of this check).
> > +    #
> > +    subst_spaces() {
> > +      printf '%s\n' "$*" | sed -e 's/\${sp}/ /g' -e 's/\${lf}/#/g' \
> > +        | tr -d "$lf" | tr "#" "$lf"
> 
> I'm not so sure $* inside double-quotes is portable to old shells.
Really ?!?  Ouch...

> But see below.
> 
> > +    }
> > +    new_weird_chars=`subst_spaces "$weird_chars"` || Exit 99
> > +    case $weird_chars in
> > +      *'${lf}')
> > +        # Add back trailing newline stripped by command substitution.
> > +        weird_chars=${new_weird_chars}${lf};;
> > +      *)
> > +        weird_chars=$new_weird_chars;;
> > +    esac
> 
> Is this tested on MSYS?
Nope, no Windows installation here.
 
> > +    unset new_weird_chars
> 
> I don't get this whole section.  Why not just
>       \${sp}) weird_chards=" " ;;
>       \${lf}) weird_chards="$lf" ;;
> and so on?  That would allow you to get rid of the big NOTEs above,
> subst_spaces, and all code regarding new_weird_chars, right?
Absolutely right.  In my defense, I can say that the complex code above
was quite simple in the beginning, then it grew and mutated to avoid all
the various bugs and limitation continuously cropping up along the road...
until it became the present mess.

Here is the new much simplified code:

case $weird_chars in
  '${bs}'|'${cr}'|'${ff}'|'${ht}'|'${lf}'|'${sp}'|'${dl}'|'${sq}'|'${sh}')
    eval "weird_chars=$weird_chars" || Exit 99
    if test x"$weird_chars" = x; then
      echo "$me: \"weird\" char '$weird_chars' evaluated to empty" >&2
      Exit 99
    fi
    ;;
  'a${sp}b'|'a${sp}${sp}b'|'a${lf}b')
    eval "weird_chars=$weird_chars" || Exit 99
    if test x"$weird_chars" = x"ab"; then
      echo "$me: bad expansion for \"weird\" chars '$weird_chars'" >&2
      Exit 99
    fi
    ;;
  *\$*)
    echo "$me: invalid \$weird_chars: '$weird_chars'" >&2
    Exit 99
    ;;
esac

Our internal checks ensure that this code is robust w.r.t. future additions
to the list of weird characters.

> > +
> > +echo "INFO: $me: munged x\"\$weird_chars\"x: x${weird_chars}x"
> > +echo "INFO: $me: enabling shell traces"
> > +set -x
> > +
> > +# Skip if this system doesn't support this characters in file names.
> 
> Either "this character" or "these characters".
"these characters".  Fixed.
 
> > +# The created directory will also be used below, so don't remove.
> Useless comment.
Removed.
 
> > +mkdir "./$weird_chars" || Exit 77
> > +
> > +mkdir sub1  # Will be used below.
> Ditto.
Ditto :-)

Thanks for the review,
    Stefano