555438 - Improve the Bugzilla code base using Perl::Critic

Status: RESOLVED FIXED

(Bugzilla :: Testing Suite, enhancement)

Description

[Gabor Szabo]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.8) Gecko/20100214 Ubuntu/9.10 (karmic) Firefox/3.5.8
Build Identifier: 

Perl::Critic and Test::Perl::Critic have been used for many years for defining policies regarding the code base of Perl projects. The way it could work is to add a Test::Perl::Critic based test script to xt/ along with the configuration file defining the rules you want to use. Then configure the automated smoke testing to run these tests.

I created a set of rules based on what we have in Padre and disabled all the rules that are currently violated in the Bugzilla code base. If you accept the idea of code improvement using Perl::Critic then please add these files to xt/ and check if they indeed pass the tests by running:

RELEASE_TESTING=1 prove xt/

Configure the Tinderbox to run these tests (and install Test::Perl::Critic).

Then start removing the policies that are currently violated and start fixing the issues in the code base.



Reproducible: Always

Comment 1

[Gabor Szabo]

Attachment: xt/critic-core.t

Comment 2

[Gabor Szabo]

Attachment: xt/critic-core.ini

Comment 3

[Max Kanat-Alexander]

  szabgab: It's best to request review from somebody specific, or it's likely to never happen.

Comment 4

[Max Kanat-Alexander]

  I've run some Bugzilla files through Perl::Critic before, and I disagreed with almost all the recommendations. So I'm somewhat skeptical, but it could still be valuable. Like you said, we can disable policies we disagree with.

  BTW, I'd rather just have the tests in t/ and do a skip_all if the environment variable isn't set.

Comment 5

[Frédéric Buclin]

There is a clear consensus amongst core developers against using Perl::Critic. We all agree that if we don't want to follow a rule, we will remove/relax it, making this RFE useless. Thanks for suggesting that, though.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Per IRC discussion with Frédéric Buclin, re-opening this bug. We can make apply only the policies that there is a consensus about, and ignore that ones that we disagree with.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Attachment: 555438_1.patch

This includes the suggestions from Gabor from 6 years ago -- protected under a RELEASE_TESTING=1 flag for now. With that enabled, this test will not pass but I think the errors it brings up should be discussed first (from a historical perspective of Bugzilla aspect as well). I'm very keen to fix the warnings it produces and then "tighten the screws".

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 8711489 [details] [diff] [review]
555438_1.patch

Review of attachment 8711489 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 10

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   39d8526..a0f345b  master -> master