Re: [PATCH] {maint} Improve, extend and tweak tests on Texinfo support.

[Ralf Wildenhues]

Hi Stefano,

sorry for the delay.

* Stefano Lattarini wrote on Tue, Jan 04, 2011 at 03:41:45PM CET:
> On Monday 03 January 2011, Ralf Wildenhues wrote:
> > * Stefano Lattarini wrote on Mon, Jan 03, 2011 at 02:38:48PM CET:
> > > Subject: [PATCH] Improve, extend and tweak tests on Texinfo support.
[...]

> > OK with nits addressed.
> >
> I have a couple of questions below; I'll wait to push until they've
> been addressed.

> > > --- /dev/null
> > > +++ b/tests/comments-in-var-definition.test
> > 
> > How about s/definition/defn/?  That is still unique, easily understood,
> > and a lot shorter.
> >
> Fine with me (even if I still don't understand this bias against longer
> test names ;-)

The longer the names, and the more the tests, the earlier we will exceed
the command line length limit in our 'check' rules (important to fix on
all systems it happens) and our 'distdir' rule (important at least for
the maintainer's machine).  So, support for more than one parallel-tests
testsuite per Makefile.am is needed soonish.  Besides, while I agree
that the 8+3 names are often lacking descriptiveness, I also don't like
typing too much.  For example, I'm not sure why we named the
'posixsubst*.test' files that way; there is little specifically posixy
about these substitution rules.

> > > +grep '^am__TEXINFO_TEX_DIR *=.*[/ ]tex *$' Makefile.in
> > > +$EGREP 'am__TEXINFO_TEX_DIR.*=.*(comment|#)' Makefile.in && Exit 1
> > 
> > These two lines access internal details that could change.  Acceptable
> > if it must be that way but better if we can do without.
> >
> I added those lines to avoid reducing coverage in the code I moved
> from `txinfo22.test' -- which indeed had a check:
>   test -d "$(am__TEXINFO_TEX_DIR)"
> in its Makefile.am.

Oh, ok, I didn't realize we were already using internal details before.

> Hhmm... but maybe it would be simpler & safer to just add back that check
> (and new similar ones) in the Makefile.am of `comments-in-var-defn.test'? 
> I think so; here is what I'll squash in if there are no objections:

Fine as well, but I don't see how it makes much of a difference.
am__TEXINFO_TEX_DIR is still an internal detail.

> --- a/tests/comments-in-var-defn.test
> +++ b/tests/comments-in-var-defn.test
> @@ -21,11 +21,20 @@
>  
>  set -e
>  
> +cat >> configure.in <<'END'
> +AC_OUTPUT
> +END
> +
>  # Use a slash in the comment, because automake takes the dirname
>  # of TEXINFO_TEX to compute $(am__TEXINFO_TEX_DIR).
>  cat > Makefile.am << 'END'
>  TEXINFO_TEX = tex/texinfo.tex    # some comment w/ a slash
>  info_TEXINFOS = main.texi
> +.PHONY: test
> +test:
> +       test tex/texinfo.tex = $(TEXINFO_TEX)
> +       test -d '$(am__TEXINFO_TEX_DIR)'
> +       case '$(am__TEXINFO_TEX_DIR)' in tex|./tex) :;; *) exit 1;; esac
>  END
>  
>  cat > main.texi << 'END'
> @@ -41,7 +50,9 @@ $AUTOMAKE
>  
>  grep TEX Makefile.in # for debugging
>  grep '^TEXINFO_TEX *= *tex/texinfo\.tex  *# some comment w/ a slash *$' 
> Makefile.in
> -grep '^am__TEXINFO_TEX_DIR *=.*[/ ]tex *$' Makefile.in
> -$EGREP 'am__TEXINFO_TEX_DIR.*=.*(comment|#)' Makefile.in && Exit 1
> +
> +$AUTOCONF
> +./configure
> +$MAKE test

> > > --- /dev/null
> > > +++ b/tests/vtexi3.test

> > > +day='([1-9]|1[0-9]|2[0-9]|3[01])'
> > > +month='(January|February|March|April|May|June|July|August|September|October|November|December)'
> > > +year='20[0-9][0-9]' # hopefully automake will be obsolete in 80 years ;-)
> > 
> > I do not agree with the tone of the comment, and as punishment will
> > require that the code also works later than the mentioned date.
> > ;->
> > 
> Eh eh :-)  (because that's a joke, right?)

Not totally.  Why do you "hope" that it will be obsolete?

;-)

> > Also, your comment writing style seems to be degrading away from writing
> > whole sentences (including leading capitalization and final period)
> > again here and below.
> >
> Yes, I tend to do so for short comments, especially in tests.  If you
> would prefer to set a policy mandating that comments are always to be
> capitalized correctly and to consist of whole sentences, please just
> do so (ideally stating that in HACKING and tests/README ;-) and I'll
> follow the new policy as consistently as I can (while I like writing
> "casual-style comments" sometimes, I have no strong feeling on the
> matter).

Me neither, at least not strongly enough to do something about it.
I merely noted it though.

> For the moment, I've amended the comments in this test for proper
> capitalization, punctuation and grammar (see the attached squash-in).

Thanks.

> > > +: > ../foobar.info
> > > +: > ../quux.info
> > > +: > ../zardoz.info
> > 
> > These commands are not guaranteed to portably update the time stamp of
> > the files in question on old systems.
> >
> :-O
> 
> > Hmm, the autoconf.texi blurb on `touch' states that this is no longer
> > a practical issue, but IIRC the policy was still enforced in GCC
> > sources, making me wonder whether there still are broken systems out
> > there ...
> > 
> > Anyway, you can easily avoid the issue by
> >   echo stamp > ...
> >
> I'd prefer to use `touch' if that's ok with you, since it makes the
> purpose of the commands even clearer (and is used in other parts of
> the automake testsuite).  Objections?

Well, that's worse in that it has the same issue on those ancient
systems.  Oh well, maybe I should stop caring about them.

> > > +day=`LC_ALL=C date '+%d'`   || Exit 77
> > > +month=`LC_ALL=C date '+%B'` || Exit 77
> > > +year=`LC_ALL=C date '+%Y'`  || Exit 77
> > 
> > Not all shells propagate exit status from commands substitutions in
> > assignments (see autoconf.texi Assignments).
> Hmpf :-(
> 
> Luckily this issue seems of little pratical concern at least: listed
> affected shells are just ash 0.2 (!) and QNX 4.25 shell.

Did you check Sven's page about set -e?

> > You might want to test for nonempty variable contents here.
> >
> In fact, to be even more reliable in case of broken/non-POSIX `date'
> commands, this is what I'd like to squash in if there are no
> objections:

> +case `LC_ALL=C date '+%u'` in
> +  [1-7]) date_is_posix=:;;
> +      *) date_is_posx=false;;
> +esac
> +$date_is_posix \
> +  && day=`LC_ALL=C date '+%d'` && test -n "$day" \
> +  && month=`LC_ALL=C date '+%B'` && test -n "$month" \
> +  && year=`LC_ALL=C date '+%Y'`&& test -n "$year" \
> +  || { echo "$me: 'date' is not POSIX-complaint enough"; Exit 77; }

compliant.

>  day=`echo "$day" | sed 's/^0//'`

> OK?

Sure.

> > > +# BSD 'grep' works from a pipe, but not a seekable file.
> > > +# GNU or BSD 'grep -a' works on files, but is not portable.
> > > +tst=''
> > > +case `echo "$tst" | grep .` in
> > > +  "$tst") ;;
> > > +  *) echo "$me: grep can't parse nonprinting characters" >&2; Exit 77;;
> > > +esac
> > 
> > This kind of test occurs several times in the test suite.  How about a
> > $required entry grep-nonprinting to factor the code?  Did you ensure
> > that BSD grep skips with the $tst value?
> >
> No, because I have no access to BSD grep ATM.
> 
> Also, BSD grep should *not* skip with the $tst value: that's the reason
> we have the apparently useless uses of cat in Makefile.am -- BSD grep
> can match nonprinting characters when reading from a pipe (but not when
> reading from a seekable file).
> 
> Also, thinking over it again, this test doesn't *really* require a grep
> that can parse nonprinting characters!  It just requires a grep that can
> work on input that is not pure text.  So, what about the following
> squash-in?

OK.

Thanks,
Ralf