[libtest-perl-critic-perl] lintian FTBFS with new version

[gregoa]

We have the following bug reported to the Debian package of
Test-Perl-Critic (https://bugs.debian.org/795716):

It doesn't seem to be a bug in the packaging, so you may want to take
a look. Thanks!

(Summarising the relevant part from the bug log)

------8<-----------8<-----------8<-----------8<-----------8<-----

> > Package: libtest-perl-critic-perl
> > Version: 1.03-1
> > Severity: serious
> > control: affects -1 lintian
> > 
> > Sid version FTBFS whereas testing work fine see
> > https://jenkins.debian.net/job/lintian-tests_sid/

The symptom is:

"""

> $ perl -Ilib  t/scripts/01-critic/docs-examples.t
> 1..1
>     # Subtest: Critic all code in doc/examples/checks
>     ok - Test::Perl::Critic for "doc/examples/checks/my-vendor/another-check.pm"
>     ok - Test::Perl::Critic for "doc/examples/checks/my-vendor/some-check.pm"
>     1..2
> not ok 1 - No tests run for subtest "Critic all code in doc/examples/checks"
> # Failed test 'No tests run for subtest "Critic all code in doc/examples/checks"'
> # at t/scripts/01-critic/docs-examples.t line 47.
> # Looks like you failed 1 test of 1.
> 
> """

We run the all_critic_ok in a subtest like so:

"""
            # all_critic_ok emits its own plan, so run it in a subtest
            # so we can just count it as "one" test.
            subtest "Critic all code in $arg" => sub {
                all_critic_ok($arg);
            };
"""

This seems to be broken by upstream, which is now running all the
"critic_ok" tests in a subprocess (via mce_grep).  This means that
all_critic_ok in total do 0 tests!

------8<-----------8<-----------8<-----------8<-----------8<-----

Thanks for considering,
gregor herrmann,
Debian Perl Group

[xtaran]

Since I just ran into this issue with another project again, here's the Test::Critic::Perl-relevant part of my analysis on the culprit as posted in the mentioned Debian bug report back then:

I'm actually not sure if the bug is really in Test::Perl::Critic. We actually [have several] options where the bug could be:

• [Skipping solutions in affected packages]
• libtest-simple-perl: Test::More's subtest, because it doesn't seem to be thread-safe.
• libmce-perl: MCE::Grep's mce_grep seems to have no option to enforce the using of plain grep upon request.
• libtest-perl-critic-perl, because Test::Perl::Critic uses mce_grep unconditionally (replacing it with plain grep solves the issue, too.) and its fiddling with Test::Builder's internals ignores $builder->{Test_Results} which is used by subtest.

So a working and tested patch is the following:

diff --git a/lib/Test/Perl/Critic.pm b/lib/Test/Perl/Critic.pm
index 2fdf32b..021e0f7 100644
--- a/lib/Test/Perl/Critic.pm
+++ b/lib/Test/Perl/Critic.pm
@@ -101,7 +101,7 @@ sub all_critic_ok {
     # workers. So we disable the T::B sanity checks at the end of its life.
     $TEST->no_ending(1);
 
-    my $okays = mce_grep { critic_ok($_) } @files;
+    my $okays = grep { critic_ok($_) } @files;
     my $pass = $okays == @files;
 
     # To make Test::Harness happy, we must emit a test plan and a sensible exit

I was tempted to apply that patch [in Debian] and upload it […] but this removes all gain which was added by using MCE::Grep, so that's likely not the right solution. [This] is something upstream must fix — if it is a bug in Test::Perl::Critic at all.

[petdance]

Can someone please describe the problem? I'm seeing all these code examples, but I don't understand what you're seeing.

What is the behavior you're seeing and what are you expecting it to be doing instead?

[xtaran]

Embedding all_critic_ok(…); inside a Test::More 'subtest "test name", sub { …; }' always fails despite the tests by all_critic_ok(…); were ok and that work around should have helped against all_critic_ok() calling its own done_testing() or equivalent. (And it didn't do that in the past.)

I might come up with a full working example later, if the above explanation doesn't suffice.

[edmundadjei]

I suspect this is corrected by the fix to #9

I tested it with and without MCE and both permutations work

Test file below:

---

use Test::More;
use Test::Perl::Critic;

my @libs = qw!/home/eadjei/lib_1/ /home/eadjei/cvl/lib_2/!;

for my $lib( @libs ){

subtest "Critic all code in $lib" => sub {
    all_critic_ok($lib);
    done_testing;
};

}
done_testing;

[gregoa]

On Wed, 17 Jan 2018 11:33:40 -0800, Ed wrote: I suspect this is corrected by the fix to #9 I tested it with and without MCE and both permutations work

Great. Lokking forward to the next Test::Perl::Critic release with this fix. Cheers, gregor

…

-- .''`. https://info.comodo.priv.at -- Debian Developer https://www.debian.org : :' : OpenPGP fingerprint D1E1 316E 93A7 60A8 104D 85FA BB3A 6801 8649 AA06 `. `' Member VIBE!AT & SPI Inc. -- Supporter Free Software Foundation Europe `- NP: Townes Van Zandt: For the Sake of the Song