Re: [PATCH 0/3] Enable `errexit' a.k.a. `set -e' shell flag in all test scripts.

[Stefano Lattarini]

At Sunday 04 April 2010, Ralf Wildenhues <address@hidden> 
wrote:
> Hello Stefano,
> * Stefano Lattarini wrote on Wed, Mar 31, 2010 at 01:24:05PM CEST:
> > I'll soon post a couple of patches enabling the `errexit' a.k.a.
> > `set -e' shell flag unconditionally in all test scripts.  This
> > helps making the behaviour of different test scripts globally
> > more consistent, and helps catching potential bugs that could
> > lead to false negatives (which are very bad in a testsuite IMO).
> 
> errexit handling is quite inconsistent between different
>  proprietary shells, with many bugs lurking,
Yes, I know that, both theoretically (from the Autoconf manual) and  
pratically (from the extremely weird bug I found in Heirlooom/Solaris 
Sh, which was causing spurious failures in the `vala4.test' test).  I 
just tought that it was better to be consistent among the test cases, 
and since the majority of them (about 520 on 750) set the `errexit' 
flag, I decided to follow suit in the remaining ones.
>  The Autoconf manual has some anectodes.  These changes
>  would require me to recheck all kinds of such systems and shells
>  for regressions, and it would require in-depth inspection of the
>  testsuite logs for false negatives. Checking Solaris and heirloom
>  sh is not enough, I'm afraid.
> 
> I don't have time to do this ATM, but I cannot accept the patches
> without somebody having done this work.
Agreed: it's better to be conservative when dealing with testsuites.
> On that basis I'm afraid
> I'll have to drop these patches for now.  Sorry you invested all
>  that work,
Don't worry, it wasn't really much work (especially if compared with
the patch series to support Zsh).
>  maybe it would be better to ask next time before you
>  delve into bigger issues.
I will.  But I didn't (and don't) consider this one to be a "big
issue", and it's no big deal if my work on it is "wasted", so don't 
worry.   And BTW, I used this occasion to improve my familiarity with 
the Automake testsuite and to uncover some of bugs (the "for free 
fixes" you speak of below), so the work is not really wasted anyway.
>  I hope that at least the copyright year update was done
>  automatically with the gnulib machinery and not manually.
Yes, the changes in both the third and second patches were done
mostly automatically (with gnulib's script `update-copyright' and a
perl one-liner, respectively).

> More generally, when not thinking about it much, I tend to
>  prioritize patches based upon both "how little work does this
>  cause for me" and "how big is the perceived improvement this patch
>  brings".  When thinking about it, I should prioritize the former
>  less, and the latter maybe too and instead focus on getting back
>  to some FIFO state.  This patch series, I'm afraid, won't score
>  high on either series, except for the "for free" fixes.
> 
> > By the way, the patches does actually fix "for free" some test
> > scripts in which failures could have easily gone undetected; this
> > happended because these tests acted like the `errexit' flag was
> > active, but forgot to ever set it.
> 
> It would be nice to have fixes for these actual issues.  Thanks.
> (Even if I had accepted the original patches, it would have nice to
>  have seen these tests mentioned specifically, at least in cases
>  where you know about them).
If you give me some time, I could peek into all the tests not using
`set -e', and look for possible problems.  I didn't even try to make
a list of such problems when I wrote this patch: it wasn't really
required, as they would have been fixed "for free" by the patch.
I just noticed some problems by chance when reading scripts that
required a more careful editing, and took note of a couple of them
to show the potential usefulness of my patch.

> Another ChangeLog style remark: please don't list all files if you
> really adjust all files.  In that case it is sufficient to say 'All
> tests adjusted.' or so, and only list the non-repetetive changes
>  like for defs.in.
Will do (and I'll surely prefer doing it this way).
> 
> > An outstanding example of this problem is the test `cond46.test',
> > in which the `errexit' flag is off, and which "checks" various
> > expected Automake failures using the idiom:
> >   AUTOMAKE_fails
> >   grep DIAGNOSTIC stderr
> > thus forgetting to call `Exit 1' if the expected diagnostic is
> > not found, i.e. not signaling the error to the test driver.
> > Other examples of this problem can be found e.g. in `confh5.test'
> > and `dejagnu7.test'.
> 
> Yes, I'd like to see separate fixes for these.  Thanks.
Will do this as soon as possible, but it might take some time.  The 
good thing is that these fixes can be done incrementally (one script 
fixed at time).

Finally, I'd like to thank you for the quick review and the detailed 
explanations.  Even a patch rejection can be useful if it's well 
motivated like this one was.

Regards,
     Stefano