Subject: Re: bug#10418: [PATCH] {master} tap/perl: handle missing or non-executable scripts better
From: Stefano Lattarini <[email-masked]>
To: Jim Meyering <[email-masked]>
Cc: [email-masked], [email-masked]
Date: Thu, 02 Feb 2012 20:22:48 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi Jim, thanks for the quick review.

On 02/02/2012 07:58 PM, Jim Meyering wrote:
> Stefano Lattarini wrote:
>> On 02/02/2012 03:24 PM, Stefano Lattarini wrote:
>>> On 02/02/2012 02:57 PM, Stefano Lattarini wrote:
>>>> The attached patch should take care of three of the remaining five
>>>> failures still present on latest Fedora (see automake bug#10418).
>>>> I will push to master shortly if there is no objection.
>>>>
>>> Hmph, this causes several new and more serious failures.  Let's drop
>>> this patch.  I hope I'll be able to come up with a correct solution
>>> in the next days.
>>>
>> OK, turns out the failures in perl up to 5.12 were due to a limitation in
>> the IPC::Open3.  So let's keep the patch (slightly adjusted), but relax
>> the tests a bit when a "defective" IPC::Open3 is detected.  Attached is
>> what I'll push shortly if there is no objection.
> 
> I've just tested this on Fedora 16, and confirm that
> it reduces to two the number of "make check" test failures.
>
Good! I'm thus closing this bug report, since the two remaining failures
in 'depmod.tap' are already reported in bug#10434.

>> Subject: [PATCH] tap/perl: handle missing or non-executable scripts better
>>
>> This change improves how our Perl-based TAP driver handles
>> non-runnable test scripts (meaning they might be not executable,
>> or not readable, or even not exist).  In particular, it makes the
>> driver deterministically display a clear "ERROR" result instead
>> of possibly dying with diagnostic from 'TAP::Parser' internals,
>> and prevents it from displaying spurious "missing TAP plan" errors.
>>
>> Moreover, with this change, some testsuite failures present only
>> with newer perl versions (e.g., 5.14) are fixed.  See automake
>> bug#10418.
>>
>> * tests/tap-bad-prog.tap: When testing the perl implementation of
>> the TAP driver, and when the perl interpreter offers a good-enough
>> 'IPC::Open3::open3' function, expect it not to display spurious
>> "missing TAP plan" diagnostic if the error is actually due to a
>> non-runnable test script.
>> * lib/tap-driver.pl (start): Removed, broken up into ...
>> (setup_io): ... this ...
>> (setup_parser): ... and this, which now tries to catch and report
>> errors in launching the test scripts.
>> (finish): New, used by both 'main' and 'setup_parser'.
>> (main): Adjust.
> ...
> 
>>  # Check that no spurious test results is reported.  This is lower-priority
> 
> One nit in context:
> 
>   s/results/result/
>
Fixed.

>> -# (and in fact the check currently fails.
>> -command_ok_ 'no spurious results' -D TODO -r 'still get "missing plan"' \
>> +# (and in fact the check currently fails for our awk-based driver).
>> +directive=
>> +if test $am_tap_implementation = shell; then
>> +  directive=TODO
>> +else
>> +  # Older versions of IPC::Open3 (e.g., version 1.05 on perl 5.12.4 or
>> +  # version 1.0103 on perl 5.6.2) fails to properly trap errors in exec(2)
> 
> One nit in a new comment:  s/fails/fail/
>
Ouch, amazing how may grammar mistakes I've managed to pile up in the last
few days :-(  Fixed this as well

>> +  # calls in the child process; hence, the TAP driver cannot be properly
>> +  # informed of such error.
>> +  if $PERL -w -e '
>> +    use IPC::Open3 qw/open3/;
>> +    $@ = "";
>> +    eval { open3(*STDIN, *STDOUT, *STDERR, "am--no-such-command") };
>> +    $@ =~ m/\bopen3:.*am--no-such-command/
>> +      or die "Bad \$@ value: \"$@\"\n";
>> +  '; then
>> +    : # OK. IPC::Open3 should be good enough.
>> +  else
>> +    for s in '"missing plan" message' 'results'; do
>> +      skip_ -r "IPC::Open3 not good enough" "no spurious $s"
>> +    done
> 
> Perhaps it's just your preferred style, but the quotes around 'results'
> are unnecessary, so I would remove them.
>
I'd rather leave them, for consistency with the other item
('"missing plan" message').  Hope that's OK with you.

Thanks,
  Stefano