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

[Ralf Wildenhues]

* Stefano Lattarini wrote on Tue, Sep 14, 2010 at 02:15:59PM CEST:
> On Monday 13 September 2010, Ralf Wildenhues wrote:
> > The patch introduces significant extra testsuite overhead.
> How much, quantitatively speaking?

Dunno.  Well, it adds 2*N aclocal, autoconf, and automake invocations
with N = 45, IIUC.  That's something.  On the plus side, it allows more
parallelism.

We need to think about line length issues for our testsuite, though,
due to long $(TESTS).  I will followup separately about this topic.

> 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).

Have you done testing on w32 (MinGW/MSYS or Cygwin) before,
where typical testsuite runs already take a day or so?
The question is how much incremental gain this patch brings.

> > 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.

Hmpf.

> > > --- /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/ :-)

Yes, that's what I meant, my sed script was just shorter.  ;->

> > > +# 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.

I probably wouldn't have written anything if the () was prefixed by
space; I don't mind too much the newlines before { }.

> > > +# 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?

That is so long.  "file" or "name" is so nice and short, yet more
descriptive than "b" and "d" which I inferred you used for directory
and base part of a file name ... ;-)

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

Here's a general rule: in general, all portability issues relative to
Posix are documented in the Autoconf manual; only exceptions need
special mention.  Can you find a good place to stick that rule, either
in some piece of Automake documentation, or on a note near your screen,
so that we don't have to keep repeating it in the code?  That way, you
can shorten this to:

  ## The ":;" works around a bug in Bash 3.2 and earlier.

Thanks.

> > > +## 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.

OK, here's a hint: 50% of all Automake tests require a C compiler,
without being annotated; and as far as I can see, the pending patch
series does not really fix that.  And it doesn't need to, either:
AC_PROG_CC will cause configure to exit 77 if no C compiler is found.
You shouldn't need to annotate required=cc.

Sorry for not having thought of this earlier, would've saved you some
work in the other patch series I guess.

> > 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.

Why?  I'm not sure I follow.

> > 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 ;-)

I take that as you volunteering to handle those bug reports.  Good then.
;-)

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

No that's fine, I was just smiling at this.  I don't think we need the
extra '|| exit 99' though: although it is more correct, and going from
exit to Exit introduced an ever so tiny regression there for when
some user intentionally removed defs or made it unreadable, it *really*
looks like over-engineering to me.

> 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.
>  ---

Sure.

> > > +# 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').

I am *very* reluctant to change *any* author tags that are not mine.
IMVHO, one would at the very least have to ask the corresponding author
for permission.  And honestly, I'm not going to ask anybody.  I'm just
glad there are almost no tags in the tests/ code today, and hoping that
we can agree to not add more.  Again, if you would like to add your name
to this file in that case, I won't prevent you from doing so.

I consider git log information the most valuable meta data carrying
authorship anyway.

> > > +# 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).

Until there's a bug in $enable_xtraces code ...
(please try not to over-engineer things; there are lots of non-testsuite
bugs also still waiting to be fixed)

> > > +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'?).

I really dislike arms races.  Messages sticking out more clearly than
others are an arms race, especially since the above message often will
have very little value.  Please just use the normal GCS approach of
prefixing the program name; that way, you can grep for what you are
interested in *at the time* you are looking at a trace, not at what you
thought users would be interested in when writing the code.

But see below, at the very end.

> > 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,

Buggy code is not a reason to add more of the same.

> and both of them are
> anyway redirected to log files by the testsuite driver.

Not if you run the test interactively.  Also, the testsuite driver might
evolve.

> Anyway, since
> the change was very easy to do, and the resulting behaviour theoretically
> more correct, I did the redirections to stderr.

Thanks.

> > 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...

Yeah, maybe I am; take it as trying to get you used to GCS style for non
testsuite code.  (I probably wouldn't get so overly nitpicky if there
weren't other things to fix also.)

But see below.

> 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?

See my reasoning above; and below.

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

Well, as I wrote, I really am not sure.  I haven't checked.  If
anywhere, I'd search on Sven Mascheck's pages.  But see below.

> > 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.

Which is a good sign that maybe the approach should be reconsidered.
The code should be getting simpler and more maintainable, not the
other way round.

> Here is the new much simplified code:

Looks better, thanks, but I still wonder why you don't just do something
like the following, and get rid of all eval and globbing and other
complications:

ht=...
first=:
# loop over pairs: verbose name of characters, then characters.
for chars in \
    space ' ' htab "$ht" ...
do
  if $first; then
    first=false
    name=$chars
    continue
  else
    first=:
  fi
  create test instspc-$name.test using $chars ...
done

That way, the code could become a lot simpler, the tests shorter, you
wouldn't need to use 'set +x' any more, plus a lot of other nits above
would just go away by itself, and a lot of the info messages could be
omitted.  :-)

Thanks,
Ralf