[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#10418: [PATCH] {master} tap/perl: handle missing or non-executable s
From: |
Stefano Lattarini |
Subject: |
bug#10418: [PATCH] {master} tap/perl: handle missing or non-executable scripts better |
Date: |
Thu, 02 Feb 2012 20:22:48 +0100 |
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: \"address@hidden"\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