Re: [PATCH] tests: use the get_shell_script helper function

[Peter Rosin]

Stefano Lattarini skrev 2012-03-01 20:27:
> On 03/01/2012 09:28 AM, Peter Rosin wrote:
>> tests/libobj-basic.test: Use the get_shell_script convenience
>> function instead of doing an inferior local hack.
>> tests/libobj10.test: Likewise.
>> tests/libobj19.test: Likewise.
>> tests/pr401.test: Likewise.
>> tests/pr401b.test: Likewise.
>> tests/pr401c.test: Likewise.
>> ---
>>  tests/libobj-basic.test |    2 +-
>>  tests/libobj10.test     |    2 +-
>>  tests/libobj19.test     |    4 ++--
>>  tests/pr401.test        |    2 +-
>>  tests/pr401b.test       |    2 +-
>>  tests/pr401c.test       |    2 +-
>>  6 files changed, 7 insertions(+), 7 deletions(-)
>>
>> Hi!
>>
>> I happened to notice get_shell_script in some test, and am thus
>> proposing this patch.  Ok for master?
>>
> No, sorry.  The patch is based on a misunderstanding.  The 'get_shell_script'
> is not meant to just fetch an automake-provided script to be tested -- it has
> other implications:
> 
>   * When the variable '$am_test_prefer_config_shell' is set to "yes", the
>     'get_shell_script' function tweaks the fetched script so that it uses
>     the configure-time determined '$SHELL' rather than bare '/bin/sh' in
>     its shebang line.
> 
>   * If a test case uses 'get_shell_script', the 'gen-testsuite-part'
>     script automatically generates a sibling tests for it that defines
>     '$am_test_prefer_config_shell' to "yes", so that the fetched script
>     will be "transparently" tested also with /bin/sh.
> 
> The first effect is useless if the script in only invoked from the
> Makefile rules (since there it should already be run by $(SHELL)), and
> the second effect is actually harmful, since it causes the creation of
> a sibling script which is semantically 100% identical to the original
> one (like our testsuite needs to be slowed even more !).
> 
> But your misunderstanding shows that the 'get_shell_script' name is
> indeed poorly chosen, and confusing.  We should improve it -- maybe
> rename it to 'get_cooked_shell_script'?  And probably, now that I think
> about it, the explanation I've given above about the interaction with
> 'gen-testsuite-part' should be written down explicitly in the comments
> for 'get_shell_script' (and a similar explanation should be written
> for the 'fetch_tap_driver' function).  I will do this in the weekend,
> unless someone wants to win extra kudos and beat me at it.
> 
> (And then, we could also define a new 'get_am_script' function that
> merely fetches the given automake-provided script verbatim from the
> proper directory -- at which point your patch could be resurrected
> with minor tweakings).

Heh, I did notice that get_shell_script modified the shebang, but figured
that wouldn't be so bad since the "required" test also looked at the
$am_test_prefer_config_shell variable.  I didn't realize that the test
was set up to be run twice when get_shell_script was used, however.  What
I did realize that it wasn't certain that my change was good, but I
figured you would point out any badness with it, so I didn't spend much
time contemplating if it was in fact better to use the function of not.
So, I calculated with you stopping the patch if it was bad, and wrote the
commit message as if it was good, fully aware that it might not be.

I'm not sure more words in association with the function would have helped,
as the comment would likely talk about things which I (or most people coming
in from the side) wouldn't fully understand the implications of anyway...

Cheers,
Peter