Open gregoa opened 9 years ago
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 plaingrep
upon request.- libtest-perl-critic-perl, because Test::Perl::Critic uses
mce_grep
unconditionally (replacing it with plaingrep
solves the issue, too.) and its fiddling with Test::Builder's internals ignores$builder->{Test_Results}
which is used bysubtest
.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.
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?
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.
I suspect this is corrected by the fix to https://github.com/Perl-Critic/Test-Perl-Critic/issues/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;
On Wed, 17 Jan 2018 11:33:40 -0800, Ed wrote:
I suspect this is corrected by the fix to https://github.com/Perl-Critic/Test-Perl-Critic/issues/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
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)
Thanks for considering, gregor herrmann, Debian Perl Group