Re: bug#10418: [PATCH] {master} tap/perl: handle missing or non-executable scripts better

[Jim Meyering]

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.

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

> -# (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/

> +  # 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.