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

[Peter Rosin]

Nothing more to say on 1/3.

Den 2011-06-07 12:29 skrev Stefano Lattarini:
> On Tuesday 07 June 2011, Peter Rosin wrote:
>> Den 2011-06-06 21:48 skrev Stefano Lattarini:
>>> * tests/ar-lib.test: If the variable `$test_prefer_config_shell'
>>> is set to "yes", run the script under test with configure-time
>>> determined $SHELL, rather than with /bin/sh.
>>> The `$test_prefer_config_shell' variable defaults to empty, but
>>> can be overridden at runtime by the user, thus allowing more
>>> coverage.
>>> * tests/compile.test: Likewise.
>>> * tests/compile2.test: Likewise.
>>> * tests/compile3.test: Likewise.
>>> * tests/compile4.test: Likewise.
>>> * tests/compile5.test: Likewise.
>>> * tests/compile6.test: Likewise.
>>> * tests/instsh2.test: Likewise.
>>> * tests/instsh3.test: Likewise.
>>> * tests/mkinst3.test: Likewise.
>>> * tests/missing.test: Likewise.
>>> * tests/missing2.test: Likewise.
>>> * tests/missing3.test: Likewise.
>>> * tests/missing5.test: Likewise.
>>> * tests/defs (get_shell_script): New subroutine, factoring out
>>> code common to the tests above.
>>> (xsi-lib-shell): If `$am_prefer_config_shell' is set to "yes",
>>
>> $test_prefer_config_shell
>>
> Fixed.
> 
>>> 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.

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

Cheers,
Peter