[meta] Automate the coding-standards part of patch review

sub

Better sub-task nesting.

+1 subscribe! :D

Hm. Possible related/duplicate issue? #1266442: [meta] Implement automated code style checks for core

Hm. I think I would say PIFR, since if all goes well, Drupal core will just be one of many projects that gets to have this capability.

sub... scribing.

Missed a link.

Updating PIFT coder status

The attached image shows:

• Testing status tab on d.o project pages
  • ( #1228462: Provide branch testing status and retest options on Drupal.org project pages )
• Testing of a contrib project with dependencies
  • ( #1241704: Deploy Project Dependency, Project and PIFT on d.o )
• "Advisory" Coder Reviews, not failing the project
  • ( #830838: Allow configurable "Advisory" reviews )
• Coder working nicely *with Project Dependency*
  • ( #1296790: Project_Dependency breaks Coder tests )
• Ability to test any release for any full project (but not sandboxes)

Assuming these make it into the next stable releases of PIFR/PIFT, this should satisfy steps 1 and 2 of the above proposal.

Patch reviews welcome!

Recent PIFT Additions

Oh holy hell I LOVE IT.

That's awesome!

Oh wow.

Great to see this finally getting used.

No, we shouldn't need to go that crazy (though a D8 version of Coder is still extremely valuable). The way Coder works is you define a set of rules (in Coder these all found in the /includes folder) to go through and implement hook_reviews() to describe how to apply them. So just as we have separate .inc files for 4.7 -> 5 and 5 -> 6 upgrade paths, we just need separate include files for each version of the coding standards (plus I guess a "global" one for everything that they'd inherit from).

Currently it defaults to:

array('comment', 'sql', 'i18n', 'style', 'security', 'coder_tough_love')

which can be overridden using coder.reviews property. We should probably remove coder_touch_love unless that has already been done in the new branch. This code was developed for Examiner which wanted coder_tough_love so that is why it originally got in there.

Considering how many false positives we will already get coder_tough_love will most likely just add more.

At the moment coder runs in worker site which means it has to run the version of coder compatible with pifr version (aka Drupal 6). It does not install the Drupal that it is reviewing and I still think that is the best idea because we don't want coder reviews to depend on the stability of Drupal core (aka during 8.x-dev).

Using coder 7.x would require porting pifr to 7.x (or ReviewDriven which uses Drupal 7).

FYI: I created a site to do some basic testing of project applications (sandbox), for the moment it checks coding standards, branch names, README and LICENSE.

Site: http://projectapp.h001.attiks.com/
Code: http://drupal.org/sandbox/attiks/1319432

Functionality in #9 is now part of PIFT/PIFR 6.x-2.7, released last weekend. Will be available once d.o/qa.d.o are upgraded to the new version.

Functionality in #9 now deployed on d.o ... all that remains is i) some minor configuration changes to enable advisory coder reviews, and ii) a blog post or other community communication outlining what the strange new 'coder' messages in test results mean, and how to go about getting the detailed results from qa.d.o (as finding the coder results is a little less than intuitive at the moment!).

Deploying advisory coder on d.o is currently stalled, dependant on PIFR tasks #1358940: Remove Coder_Tough_Love dependency from PIFR_Coder and #1358942: Install coder on qa.drupal.org, and activate the PIFR_Coder module.

Shouldn't take long ... but the 'Coder Tough Love' item would require that we roll a new release. Alternatively, we could install Coder Tough Love on qa.d.o to satisfy the dependency for now, and then remove it once the new release is created.

Coder reviews are live on branch tests ... we'll look at removing the CTL dependency in a future release.

I opened #1361508: [META] Tracking issue for Coder Advisory Review test issues to track all advisory review issues for false positives and missing coverage, since there are two projects giving us assertions.

Morbus has also added me as a co-maintainer for CTL, so in the middle term, rather than removing that dependency, we can deploy fixes for that module.

Additional status updates

I added a patch to remove CTL from PIFR if there is anyone that can review it. #1358940-4: Remove Coder_Tough_Love dependency from PIFR_Coder

Now that #1358940: Remove Coder_Tough_Love dependency from PIFR_Coder and #1358942: Install coder on qa.drupal.org, and activate the PIFR_Coder module are fixed is there anything else that needs to be done for this?

If there is agreement that we are ready to test patches for coding style (i.e. run them through coder), this is as simple as checking a configuration checkbox.

Kinda exciting to see this finally about to happen.

I say we go ahead and try it. It will basically be a note for people submitting patches but it will provide useful feedback and it is the best way to refine the output from it. I see no negatives to turning it on and letting users know what coder thinks of their patches. Worst case scenario we turn it back off if it's causing problems.

I'm not sure I agree with #28. At first I was going to say "Yes, absolutely, go for it!" but then I realized that until we make core compliant, the reviews for patches do not actually add information among the noise of core's fails. I'd hate for some poor novice to spend hours fixing out-of-scope problems in a module because they happened to find the advisory review tab. We might want to wait until the cleanup sprint is done (or at least in swing).

Did this die? Sure seems this might help alleviate a lot of frustration when trying to post core patches that need "tough love".... especially with all the D8 work being done. Granted I know it's probably not up to D8 coding standards, nor does anyone really have time for this.

IMO this should be the very first thing to go into D9. If it means we have to do some standards cleanups in core before we can commit anything new to core, then so be it.

The amount of time wasted on humans reviewing large patches for trailing commas, whitespaces, indentation, etc... is really huge - way more time saved over the course of the development of a major release of drupal than the amount of work implementing this would take.

This was disabled on qa.d.o due to the maintainers of Coder moving Coder 6.x into unsupported status, and thus we can not keep it updated for any false positives that might appear during testing. The evolution path is moving to the Drupal Codesniffer version of Coder (7.x), which can be used on 6.x sites as a drush plugin ... But the code conversion hasn't happened for this yet.

Do you know if there's an issue open for what you're describing in #32?

#33: Not much to see, but the issue is at #1904614: Switch Coder operation over to leverage the 'drush' plugin and 7.x-2.x ruleset.

Right, so at the least, this meta obviously needs an issue summary update as that issue isn't mentioned anywhere in the related/blocking issues list.

Updated issue summary.

at symfonycon hearing about ubot.io https://twitter.com/fabpot/status/411450578934693888 reminded me of this issue.

https://www.drupal.org/contribute/core/beta-changes
and
#2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?
mentions that coding standard fixes are now postponed until 8.1.x (and/or until we have an automated detection in place).

And, I think this issue might be postponed on Drupal.org CI 2.0 being deployed.

I dont think there is an infrastructure issue to postpone this on... or an issue in https://www.drupal.org/project/issues/drupalci?categories=All
but
https://groups.drupal.org/drupal-org-testing-infrastructure
is probably the place to watch for that being ready.

Updating title, QA.Drupal.org will be replaced with results.drupalci.org.

Just a random longterm idea, provide a tool which automatically fixes your CS errors.
The symfony project has implemented that idea as part of your bot, see http://fabbot.io/report/symfony/symfony/14619/a1ff5c22024006258cf7bc7a79... for example.
IMHO this would be a really great thing, given the reduce of barrier for people. Note, they just look for changed code, not all existing one.

The page shows much more interesting ideas, like automatic ensuring that docs are there, fixing common typos as well as ensuring some issue metadata is there.

@dawehner: that tool already exists as phpcbf command in the Coder project. We would just have to integrate it.

this would be a great timesaver :)

Coming here from #2490370: Fix the node module coding standards issues and then #1533254: Make node module pass Coder Review (thanks @dawehner) I also believe it would significantly increase core's velocity.

This is at least major. I can't quite quantify how much real, honest-to-goodness human brain time this would save per week, but it's hugely significant.

@Webchick, I have updated the child issues for the javascript automated review.

They need some love from the CI team.

Especially #2600626: [PP-1] Ensure availability of node/npm in the testrunners.

ps. I am pretty excited about the improvements coming to CI, its going to make a huge difference. The modules that I work on tend to have lots of people post patches and it will save so much time!

Updated related issues in issue summary.

Just to keep everyone up to date. We are doing some re-factoring of the container in #2609560: Base DCI containers off official containers.

This lays down the foundations for the JS/CSS/YAML Linting steps as well as #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary.

Any help would be greatly appreciated!

If i read this correctly part of the work is blocked on getting core code clean first, and only then enable code style checking for patching.
Could we do some first step to enable code style checking only on the changed lines in the patch? This could allow getting code clean, and getting new code clean happen in parallel. At least no new code style issues would be committed.
(BTW Civicrm works this way, they check code style on the changed lines, but not on full core (since it's not following code style fully)).

I think #50 is a good idea - not sure if we have any existing support for that though.

The other thing that might be possible is checking only certain rules (that core passes on) so that it doesn't regress, then gradually adding more as core gets cleaned up.

Technically i'm not really sure how #50 would work. And i'm not really able to help get this in our CI infrastructure.
Civicrm works with Github PR's, and their script sort of merges the PR with a no-commit, and then check the listed files.
Although they also use the same tools for the code style analysis.
Their 'linting' script is public at https://github.com/civicrm/civicrm-buildkit/blob/master/bin/civilint perhaps this can help more.

I don't think the re-factoring in #2609560: Base DCI containers off official containers will be done unless some more resources are pointed in its direction. Without it, the automated code reviews won't be added.

How do we get more resources pointed towards it?

If I've interpreted #52 correctly, it implies that each file is being checked in its entirety against the coding standards, rather than just the modified portions?

That would mean that pre-existing coding standards violations can be flagged when testing entirely-unrelated patches?

I understand that the intent is to have the entire core passing these tests before making those failures actually reject the patches, so core is mostly protected from undesirable consequences here; but I can see this playing real havoc in contrib land.

As it's been stated that restricting the testing to the changed lines only is not going to happen, may I suggest a two-pass approach whereby the files are checked for coding standards violations before the patch is applied as well as after, and patches are not set back to "Needs work" in the case when the original files were also not passing?

(edit: Although they could perhaps be set to "Needs work" if there are more violations post-patch than there were pre-patch? I haven't spent enough time thinking about this to be certain that's definitely safe, but offhand it seems like a reasonable idea.)

Meta for implementation discussion in the testbot project: #2654650: [meta] Jobs for linting and coding standards

Also: There doesn't have to be only one behavior for all testbots.

testbot sort-of has a test definition file format: #2295511: Architecture: DrupalCI Test definition format

This means that each project could define their own criteria for what is a pass and what is not a pass. Don't care about coding standards? Don't add it to your definition file. This is analogous to .travis.yml.

#2575497: DrupalCI should report code style issues - CSS and #2575501: [PP-2] DrupalCI should return failed for poor code style - JS are done.

#2591827: Add YAML linting to core coding standards checks still needs to be done, but we are waiting on a confirmation of the Drupal YAML code style standards.

Currently, HEAD reports about 500 coding standards failures because our CSS does not comply with our .csslintrc. This makes it very difficult to spot other errors in the full branch tests. Core does comply with both the .eslintrc JS rules and the enabled PHP rules, so we should probably disable the CSS checking in order to get to the point where we can actually fail/fix for PHP and JS standards as soon as possible. CSS standards might also regress, but we're not worse off than we have been if they do.

Here's a potential interim fix for csslint whilst HEAD is falling. #2864753: Fix csslint by excluding files and then fixing them - basically only testing the files that pass.

PIFT currently isn’t keeping track of the source of the messages. It would be straightforward to add the tool each message came from, and add filtering for that.

PIFT currently isn’t keeping track of the source of the messages. It would be straightforward to add the tool each message came from, and add filtering for that.

This was one of the primary motivations for the original 'each activity is a separate job/task/insertYourWordHere' approach, and having each job/task return an independent set of results. The intent at that time was to break things apart into smaller, less complex, modular and standalone components; as opposed to adding more and more functionality into a single build (which was viewed as one of the primary architectural flaws we kept bumping up against within PIFR). The original DrupalCI architecture proposal supported individual artifact files and processing logic per tool (which aligns with the proposed enhancement quoted above) as opposed to trying to map results from every job type into a common xml-based results file. The current approach was not only an unfortunate side effect of our 'build simpletest first' approach, but also hard-codes a dependency on Jenkins ... thus breaking the decoupling of testbot -> dispatcher -> presentation logic we were striving for (which reduced risk by more effectively supporting independent drupal.org/back-end changes).

I would still advocate strongly for this segregation of functions when considering this 'per tool filtering' capability, allowing for a significantly reduced level of complexity in the handling of different artifact / result types for each type of testing ... and I view this simplicity within each modular component as a worthwhile trade-off against the increased number of components that would be required as a result.

I meant lightweight filtering, like we have for show all results or failing only results. In the UI, implemented with JS. So if you have a lot of coding standards messages, you can focus on what you want.

(In this case, Jenkins wasn’t helpful for aggregating coding standards messages, PIFT is parsing the XML these tools output.)

The original DrupalCI architecture proposal supported individual artifact files and processing logic per tool (which aligns with the proposed enhancement quoted above) as opposed to trying to map results from every job type into a common xml-based results file.

Pick a recent test build. Look at it on the dispatcher. Examine the 'artifacts' section... Its organized by task name (phpcs, etc), and each task outputs a number of self-defined artifacts. d.o can choose what to do with those artifacts.

Re @xjm #59:

Any task could terminate/fail the build. drupalci currently lacks a way for a project maintainer to say: "I want phpcs to fail the build," though it could be specified in the build definition. For instance:

  assessment:
    validate_codebase:
[..]
      csslint:
        lint_fails_test: false
      phpcs:
        sniff_fails_test: true

If you look at a build definition artifact, you'll see all these parameters filled out to the values that were used during the build. Here's a recent patch build.

Coding standards checks run before running the tests, so if they were to fail the build, then the tests wouldn't run. View that as a bug or a feature. :-)

In #64, Mile23 wrote:

Coding standards checks run before running the tests, so if they were to fail the build, then the tests wouldn't run. View that as a bug or a feature. :-)

Some coding standards requirements are impossible to comply with, so if failing a coding standard requirements would prevent tests from running, that would IMHO be rather counterproductive, and I would classify it as a bug.

As an example, take the max 80 characters per line requirement. It even apply to text, such as README.md. In one of my projects, the README.md contains the following line:

[7]: https://api.drupal.org/api/drupal/includes!mail.inc/function/drupal_mail_system/7.x

It contains a 84 character long URL, pointing to the Drupal API. If I were to break the line at 80 charcters, the URL would no longer work (unless Markdown has some syntax for "soft" line breaks that I am unaware of).

All three coding standards checking tools support a concept of both "errors" and "warnings" whereby each rule can be configured to fall into one of those two categories.

We could make it so that only errors would cause a build to abort, whereas warnings would allow the build to proceed, and provide a report on those warnings. That way developers could configure the tools via .eslintrc.json, phpcs.xml.dist, and .csslintrc if they have known "uncompliables" like gisle pointed out earlier.

Re #65: By project I assume you mean drupal.org project. In that case, you could turn off the 80 chars rule, or exclude .md files. Add a phpcs.xml.dist file to your repo and add the exclusions.

If you look at core's core/phpcs.xml.dist, you'll see that it excludes quite a few rules in order to be passing the sniff while working on improvement.

You can also submit an issue to the Coder issue queue about 80 chars special cases, as well.

Is it now possible to configure contrib projects to utilize automated coding standard checks as well? If not, is there an effort to do so?

Coding standards checks run with all projects, automatically. Click through to a test result page on Drupal.org and look for the heading like “15 coding standards messages”.

@drumm That is great; thanks for the assist! Is it possible to get the tests to show as failed if any of the code style checks fail?

run with all projects

Except themes... #2769599: [PP-1] Allow themes to be testable

@markdorison: AFAIK the testbot itself can be configured to halt on CS errors, but the integration with D.O doesn't give you a way to do that.

Also, if you want to use different CS tool configurations, then just add the config file to the root of your project. Also, for phpcs, you can specify a version of phpcs or coder in your project's composer.json file.

Also, if you want to use different CS tool configurations, then just add the config file to the root of your project. Also, for phpcs, you can specify a version of phpcs or coder in your project's composer.json file.

Is there a place this really interesting information is documented?

@dawehner: DrupalCI has a guide: https://www.drupal.org/drupalorg/docs/drupal-ci

And it also has this issue: #2857994: Documentation needs some love

AFAIK the testbot itself can be configured to halt on CS errors, but the integration with D.O doesn't give you a way to do that.

I think adding such support would be a very valuable improvement.

For projects which are already coding-standards compliant, maintainers may likely find it desirable that any patches with coding standards violations are automatically flagged as failures so that the patch author can fix them without anyone else having to be involved. Anything which reduces the maintainer workload is a boon.

@jweowu Please file an issue in this project about that.

Also, did you know that if you follow the test results link to view on the dispatcher, you'll see an Artifacts directory? And if you explore that you will very likely find a patch that solves all the CS errors that can be auto-solved by phpcs/Coder? There's already an issue to have a direct link for that: #2851916: Expose phpcs patches to comment stream

#2872359: Enable coding-standards non-compliance to be a testbot failure condition on a per-project basis

Is this resolved by the PHPCS and PHPSTAN checks in Gitlab?

Yes I think we can close this now!