Re: [PATCH 2/3] {testsuite-work} tests: can use also $SHELL to check shell scripts from `lib/'

[Stefano Lattarini]

On Tuesday 07 June 2011, Peter Rosin wrote:
> Nothing more to say on 1/3.
>

> Den 2011-06-07 12:29 skrev Stefano Lattarini:
> >>> index cf8d6cb..c7e8a0e 100755
> >>> --- a/tests/compile4.test
> >>> +++ b/tests/compile4.test
> >>> @@ -20,6 +20,8 @@
> >>>  required='cl'
> >>>  . ./defs || Exit 1
> >>>  
> >>> +get_shell_script compile
> >>> +
> >>
> >> This test no longer checks if $AUTOMAKE -a copies over compile, as
> >> that is done manually now. I assume this aspect of $AUTOMAKE -a is
> >> tested elsewhere. Or is it?
> >>
> > Yes, in 'subobj.test'.
> 
> The same argument could be made about the other instances where the
> script is brought in explicitly. Seems like a bit of a fluke that
> subobj.test covered the compile script.
>
Agreed.  I now think we should have a centralized test where to check
for files installed with `--add-missing', not to risk reduced coverage
anymore.  Patch coming up soon ...

> >>> diff --git a/tests/defs b/tests/defs
> >>> index 37b5baa..1d50b1d 100644
> >>> --- a/tests/defs
> >>> +++ b/tests/defs
> >>> @@ -283,6 +283,23 @@ unindent ()
> >>>  }
> >>>  sed_unindent_prog="" # Avoid interferences from the environment.
> >>>  
> >>> +# get_shell_script SCRIPT-NAME
> >>> +# -----------------------------
> >>> +# Fetch an Automake-provided test script from the `lib/' directory into
> >>> +# the current directory, and, if the `$test_prefer_config_shell' variable
> >>> +# is set to "yes", modify its shebang line to use $SHELL instead of
> >>> +# /bin/sh.
> >>> +get_shell_script ()
> >>> +{
> >>> +  if test x"$test_prefer_config_shell" = x"yes"; then
> >>> +    sed "1s|#!.*|#! $SHELL|" "$top_testsrcdir/lib/$1" > "$1"
> >>> +    chmod a+x "$1"
> >>> +  else
> >>> +    cp "$top_testsrcdir/lib/$1" .
> >>> +  fi
> >>> +  sed 10q "$1" # For debugging.
> >>> +}
> >>> +
> >>
> >> I'm not too fond of rewriting the scripts. Wouldn't it be better
> >> to execute them as they would be executed from Makefiles instead,
> >> and tweak $SHELL for the case where the shebang is desired to
> >> take effect?
> >>
> >> I.e.
> >> if "$test_prefer_config_shell" = yes; then
> >>   TEST_SHELL=$SHELL
> >> else
> >>   TEST_SHELL=
> >> endif
> >>
> >> and then
> >>  
> >> $TEST_SHELL ./compile ...
> >>
> >> or something?
> >>
> > The point is that tweaking the shebang line in the scripts helps to
> > ensure that they are always run with $SHELL *unless explicitly told
> > otherwise*, while with your idiom they are run with /bin/sh *unless
> > $SHELL is used explicitly*.  The former scenario is safer for what
> > concerns coverage.
> > 
> > Also, by tweaking the test scripts, we reduce the amount of tweaking
> > necessary to have all script executions take place with $SHELL; see
> > how the diffs in this patch are much smaller and easier to follow
> > than those in my original patch?  I think this is a real advantage.
> > 
> > Does these motivations sway your judgement a bit?
> 
> A bit, and I'm not confident enough to "override" your arguments.
> 
> >> Doing it that way would also not remove the $AUTOMAKE -a tests.
> >>
> > Luckily, this is a non-issue, as such a check is already covered
> > by 'subobj.test'.
> 
> Yes, for compile, but what about missing, mkinstalldirs, etc?
>
See above (and BTW, thanks for bringing up this issue, which I would
have easily missed).

> Cheers,
> Peter
> 

Regards,
  Stefano