Bug 8002 - Make perlcritic stricter to CPAN testing standards
Summary: Make perlcritic stricter to CPAN testing standards
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: Regression Tests (show other bugs)
Version: SVN Trunk (Latest Devel Version)
Hardware: All All
: P2 enhancement
Target Milestone: 4.0.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-06-01 08:35 UTC by Sidney Markowitz
Modified: 2022-08-16 08:43 UTC (History)
1 user (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sidney Markowitz 2022-06-01 08:35:49 UTC 
The upload of 4.0.0-pre2 as a trial to CPAN is generating many errors on their test machines because they use Perl::Critic with stricter policies than we use in our perlcritic test. For example, they enable Bangs::ProhibitBitwiseOperators warnings which trigger a test failure just for using | or & properly.

CPAN's testing procedure is not something that we can change. What we can do is to go through our code and comment every line that generates a perlcritic warning after confirming that the usage is intended, for example,

  ..._flag | &AI_PASSIVE, ## no critic (Bangs::ProhibitBitwiseOperators)

This issue is for going through the perlcritic warnings that are found by CPAN's testers and fixing any errors revealed or adding a ## no critic comment

It isn't being targeted for the 4.0 milestone because it may be too big a job and does not fix any bugs or provide new functionality. If we do make enough progress with it, it will be good to have so that CPAN test results are not drowned out by the noise from perlcritic.

The first step will be to change the severity threshold in our perlcritic test from 5 to 4, to match CPAN, and in the test disable all policies that reveals in our code.

The next steps will be to go through the code dealing with one policy at a time, such as Bangs::ProhibitBitwiseOperators, then remove the disabling of that policy from our test.
Comment 1 Sidney Markowitz 2022-06-01 08:54:07 UTC 
(In reply to Sidney Markowitz from comment #0)
>> The first step will be to change the severity threshold in our perlcritic
> test from 5 to 4, to match CPAN, and in the test disable all policies that
> reveals in our code.

Correction: CPAN does test at severity level 5. The reason they catch bitwise operator warnings that we don't is that the warnings are in scripts such as spamd which our test doesn't check. That makes this step unnecessary.
Comment 2 Sidney Markowitz 2022-06-01 09:35:24 UTC 
This is more complicated than I thought.

First, CPAN testers are not running perlcritic on their own. The failures are in our own t/percritic.t test

Second, they are not enabling more policies than we are. The failures occur running our own test with the same settings that we have.

The difference is that not all percritic policies are installed when one installs Perl::Critic abd Test::Perl::Critic. I had to run

cpanm Perl::Critic::Policy::Bangs::ProhibitBitwiseOperators

to get the bitwiseoperator warnings to show up in t/perlcritic.t

So to get our test to get the same results as CPAN testing will find, we'll have to install the policy modules that they have on their test machines.

The good news is that we can exclude the policies in t/perlcritic.t the same way as the policies we already exclude and we don't have to sweep through all the code right away.
Comment 3 Sidney Markowitz 2022-06-01 11:24:37 UTC 
To test this properly, install the following CPAN modules:
 
 Perl::Critic::Policy::Bangs::ProhibitBitwiseOperators
 Perl::Critic::Policy::Perlsecret
 Perl::Critic::Policy::Compatibility::ProhibitThreeArgumentOpen
 Perl::Critic::Policy::Lax::ProhibitStringyEval::ExceptForRequire
 Perl::Critic::Policy::ValuesAndExpressions::PreventSQLInjection
 Perl::Critic::Policy::ControlStructures::ProhibitReturnInDoBlock

The only changes required were to exclude more warnings in the perlcritic test. As with the ones that were already excluded, we should someday go through code and check the warnings, adding ## no critic as we go.

trunk % svn ci -m "Bug 8002 - Exclude more PerlCritic policies that are checked on CPAN test machines"
Sending        t/perlcritic.pl
Transmitting file data .done
Committing transaction...
Committed revision 1901489.
Comment 4 Sidney Markowitz 2022-07-29 04:07:10 UTC 
Found another module installed on a CPAN test machine, which also issued a warning about a module that it doesn't install that the test references.

Committed to this closed issue because it has not yet been part of a release.

trunk % svn ci -m "Bug 8002 - Exclude another PerlCritic policy found on a CPAN test machine, add required modules for test" Makefile.PL t/perlcritic.pl 
Sending        Makefile.PL
Sending        t/perlcritic.pl
Transmitting file data ..done
Committing transaction...
Committed revision 1903087.
Comment 5 Sidney Markowitz 2022-08-16 08:43:21 UTC 
Found yet another Perl::Critic policy bundle module installed on a CPAN test machine causing failures. This one is Perl::Critic::OTRS which isn't even published on CPAN anymore. This time I'm excluding the entire set of policies and not adding the module to MANIFEST. perlcritic.t will ignore the policies if the test machine has installed them, but we will never want to remove the exclude.

Again, committed to this closed issue because it has not yet been part of a release.

trunk % svn ci -m "Bug 8002 - Exclude another set of PerlCritic policies found on a CPAN test machine" t/perlcritic.pl
Sending        t/perlcritic.pl
Transmitting file data .done
Committing transaction...
Committed revision 1903454.