Re: [FYI] {testsuite-work} tests: use `$SHELL' to run the shell scripts from `lib/'

[Stefano Lattarini]

On Monday 06 June 2011, Peter Rosin wrote:
>
> [SNIP]
>

> Den 2011-06-05 12:57 skrev Stefano Lattarini:
>
> > (BTW, this would be another good reason to resurrect you patch on
> > AM_PROG_AR; our disagreement there ended up with me acknowledging
> > that you were 90% right, so it shouldn't be too difficul to reach
> > consensus now).
> 
> IIRC, the discussion ended when we got tired of quibbling over how
> the warning message should be worded and in what circumstances it
> should appear, mostly the latter. We then waited for someone else<tm>
> to just decide for us, but that never happened.
>
I suspect that this Someone Else (TM) might prefer to read a thread with
(say) three or four messages showing mostly agreement, rather than an
old thread were we quibbled endlessly about the color of the shed ;-)

> >> Now, the ar-lib script is perhaps not the best example, but the same
> >> holds for the compile script since not all projects do AM_PROG_CC_C_O.
> >> Your patch changes the rules for the scripts,
> >>
> > Not exactly true; it only changes the rules for the tests on these scripts.
> > Yes, this implies that now the usage we care *more* about for these scripts
> > is when they are run with configure-selected $SHELL; but that doesn't mean
> > we don't care about keeping the them runnable with /bin/sh anymore.
> 
> You're right, not exactly true, but implicitly so. I was extrapolating
> and exaggerating a teeny-weeny bit in order to drive home the point that
> we should be testing the endorsed usage patterns; by changing the tests
> those endorsed usage patterns also (implicitly) change.
>
Agreed.

> >> and the reason is a dubious argument to increase testsuite coverage,
> >> see more below.
> >>
> >> We both think the testsuite should execute the script as they are executed
> >> when they are used. And I think that it should only involve $SHELL if there
> >> is a guarantee that $SHELL will always be used, otherwise there will be
> >> inconsistencies.
> >>
> >> It should also be safe to copy-paste things to the command line and
> >> re-run commands (i.e. $SHELL has to be expanded when displayed, I don't
> >> know if this is already the case).
> >>
> > Yes, that should be the case when `xtrace' is in effect:
> >   $ sh -c 'SHELL=ksh; set -x; :; $SHELL -c ":"'
> >   + :
> >   + ksh -c :
> 
> Oh, I meant that it should be drop-dead easy to copy-paste from the
> normal build output when you want to re-run some (failing) part of
> the build process (with tweaked arguments), without worrying about
> if $SHELL is set in the environment. Again, that may very well already
> be the case.
>
In the tests I've touched, yes, since they just use $SHELL directly to
run the checked script; and for what concerns these checked scripts,
doing:
  $ export SHELL=/bin/bash; $SHELL ar-lib ...
or:
  $ /bin/bash ar-lib ...
should be perfectly equivalent.

>
> [SNIP]
>

> >> I.e. the gain is that *you* can do lab style tests more easily, but the
> >> loss is less testsuite coverage in the wild. It just has to be better to
> >> fix the lab instead.
> >>
> > OK, you've made good points as usual.
> > 
> > I now think I should revert this version of the patch, and repropose it so
> > that the affected tests will run the scripts once with /bin/sh and once with
> > $SHELL (skipping these latter checks if $SHELL happens to be /bin/sh).  This
> > should cater both for the "lab" and the "wild".
> > 
> > Oh, and we'll need a couple of new requirements `shell-not-bin-sh' and
> > `xsi-bin-sh' in 'tests/defs' (and the `xsi-shell' requirement could be
> > adjusted to check $SHELL instead of the shell that is running the test
> > script itself).  These changes could be proposed in preliminary patches.
> 
> Re xsi-shell adjustment, isn't $SHELL always running the test scripts?
>
Yes, unless the user forces the use of a different shell (you know, always
give the user enough rope to hang himself).  On the other hand, $SHELL
cannot currently be overridden in the test scripts, since `tests/defs'
defines it unconditionally (and letting it being overridden from the
environment would be a royally bad idea, since it's probably not uncommon
for users to have, say, SHELL=/bin/tcsh).

> *time passes*
> 
> Ahhh, you are thinking about the case when test scripts are executed
> standalone,
>
No, because the test scripts, even when executed standalone, takes care of
re-executing themselves with $SHELL.  See commit `v1.11-874-g1321be7'.

> so scratch that.
>
> *time passes*
> 
> Hmmm, $SHELL is potentially different when running tests standalone and
> when running them from make check,
>
No, it's not.  See above.

> leading to different test results.  Is that acceptable?
>
Hardly so; but luckily, it doesn't happen in the present setup :-)

> > Would this scenario be more acceptable for you?  FWIW, I think it would
> > be an improvement both over the previous situation and my previous patch.
> 
> Yes, that would be fine. I thought about that myself, but didn't mention
> it because I'm not too fond of "sister tests"; code duplication makes me
> suspicious.
>
I plan to avoid code duplication as follows: the test code should be
moved in a new *.sh file, so that the sister tests could then just run
by sourcing that file after setting one or two variables that will
configure its behaviour (for a reference, similarly to what is currently
done in master with the auto-generated `instpc*.test' tests).

> Maybe the affected tests could somehow re-exec themselves
> (only if /bin/sh != $SHELL) with some knob to trigger the sibling behavior
> (I don't know if that is reasonable, just throwing out an idea). That way
> there would be less chance of future errors when updating the tests.
>
I think my idea above is even simpler, and should address your (justified)
concerns about code duplication.  Anyway, I'll let you judge my idea when
I have a patch to back it up.

>
> [SNIP]
>
> Cheers,
> Peter
> 

I'll go ahead and revert the patch then, and prepare new ones on the
lines sketched above.

Regards,
  Stefano