Introduce PHP coding-standards thresholds #15911

moorscode · 2020-08-21T10:55:51Z

Context

We are working towards using the latest coding standards configuration for a while now. As we are still using PHPCS 0.4.3 we are introducing problems for 2.0.0+ every day because we are unaware that we do so. The checks are simply not being run.
This PR introduces a way to cap the errors and warnings thresholds and allows us to use the latest configuration to test against.

Problem to solve:
We only have branch-checks on Travis, which would mean that we will break trunk if new warnings are introduced.
Enabling trunk builds will require more resources to be used on Travis, but will make this process more reliable.

Summary

This PR can be summarized in the following changelog entry:

N/A

Relevant technical choices:

Removed config-yoastcs as this is being done automatically by the newer YoastCS version
Removed maybe-restore-composer.lock as this is no longer needed
Added thresholds for CS errors and warnings to the composer configuration; this seemed the most logical place to manage them
Added the thresholds check to the composer cs menu

Test instructions

This PR can be tested by following these steps:

Use composer install to install the latest packages
Use composer check-cs-thresholds to verify the current code is at or below the thresholds
See Travis running the check-branch-cs for the Pull Request instead of doing a full thresholds check

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

I have written documentation for this change.

Quality assurance

I have tested this code to the best of my abilities
~~I have added unittests to verify the code works as intended~~

Bump PHPCS to ~2.0.0 to use the latest and greatest. Introduces code that captures the PHPCS results output to have an opinion on the number of errors and warnings that are expected. Updates the PHPCS configuration by removing exclusions that are marked to be irrelevant for the bumped version of YoastCS. Fixes spaces vs tabs in PHPCS configuration.

Use thresholds on non-pull requests (push, tags) Use check-branch-cs for pull-requests, this will fail on any introduced problems.

This allows Travis to fail on errors.

In line with the phpcs configuration additions.

Mention * Thanks for the inspiration from https://github.com/OXID-eSales/coding_standards_wrapper as the source of inspiration for the code used. The package was not dynamic enough for us to be able to use it practically.

moorscode · 2020-08-21T13:12:50Z

Herre and Irene revisited the issue about thresholds yesterday - squizlabs/PHP_CodeSniffer#2094 and noted that there was a solution being created to extract the numbers from the results of PHPCS.

This PR implements a similar approach in a slightly different implementation. We think this should help avoid introducing new warnings and errors for the 2.0.0+ branch of YoastCS; which currently are just too easy to accidentally do.

@jrfnl I'm sure this change warrants some more configuration for PHPCS. Could you take a look?

moorscode · 2020-08-21T13:21:45Z

@herregroen / @jrfnl could you think along with the stated problem in the description of the issue?

moorscode · 2020-08-21T13:23:04Z

We could do a threshold check on PRs, it takes around 16 seconds on my machine - so probably 60 seconds on Travis..

jrfnl

Hi @moorscode, thanks for the ping.

I've left some comments and questions inline.

Would you like me to add some commits updating the ruleset to what it should be for YoastCS 2.x to this PR ? We would need to adjust the threshold numbers after the ruleset update.

I'd also like to suggest I do a clean-up run before this gets merged, so the threshold numbers are as low as can be at this time and don't include low-hanging fruit.

Problem to solve:
We only have branch-checks on Travis, which would mean that we will break trunk if new warnings are introduced.
Enabling trunk builds will require more resources to be used on Travis, but will make this process more reliable.

PHPCS 3.x has the caching feature, if we enable that and store the cache file between builds, running a full CS check on branches/trunk should be faster. Would that help ?

We could do a threshold check on PRs, it takes around 16 seconds on my machine - so probably 60 seconds on Travis..

AFAICS, the threshold check basically adds a second CS run to the build and a full one at that, that seems counter to the intentions of minimizing the time taken ?

If we enable caching and do a full run, including threshold check, is there still a need for a trunk/branch toggle ?

On a single run, multiple reports can be generated, so that is not a problem --report=full, summary,source, however, I can imagine that the branch specific run makes it easier to find the specific issues introduced by a PR as only the issues in the changed file will be shown ?

Could the scripts be adjusted to allow for only outputting the "full" reports for the changed files ?

composer.json

config/composer/actions.php

.travis.yml

composer.json

jrfnl · 2020-08-22T03:59:37Z

composer.json

 		"cs": [
 			"Yoast\\WP\\SEO\\Composer\\Actions::check_coding_standards"
 		],
+		"check-cs-thresholds": [
+			"@putenv YOASTCS_THRESHOLD_ERRORS=625",
+			"@putenv YOASTCS_THRESHOLD_WARNINGS=546",


How will these numbers be maintained ?

jrfnl · 2020-08-22T04:13:08Z

config/composer/actions.php

+	 *
+	 * @return array ['error_count' = (int), 'warning_count' = (int)] Errors and warnings found by PHPCS.
+	 */
+	private static function extract_cs_statistics( $output ) {


Would it be an idea to add the "Files" and "Sources" into the mix as well ?

PHPCS throws two violation types - errors and warnings, those are reported at the bottom of the summary report and addressed here. That same line also contains a file count.

Additionally, as you know, those errors and warnings each have an "error code" which is, for instance, used inline to ignore issues.

Using the source report, you get to see how many errors/warnings there are for each error code with at the bottom a total and a total for the number of error codes.

That last number should not go up as that means that even when the error/warning counts remain the same, one type of error has just been replaced by another.

To get both reports, use --report=summary,source" (works in PHPCS 3.x, not 2.x)

I think we should start keeping this as simple as possible. Just having the distinction between errors and warnings will give the developers a place to start. If we don't see the thresholds going down over time, we could try to enforce more fine-grained control over what is allowed, but we prefer to see that ownership come from the people instead of enforced logic.

I think it would also mean that we need to keep a long list of errors per error-code somewhere (composer.json as setup currently) which doesn't make that easier to work with or adjust thresholds when problems get fixed.

config/composer/actions.php

Co-authored-by: Juliette <[email protected]>

This has been included in YoastCS since 1.0.0

Added a general exclusion in PHPUnit for the composer config directory.

moorscode · 2020-08-25T09:33:56Z

Would you like me to add some commits updating the ruleset to what it should be for YoastCS 2.x to this PR ? We would need to adjust the threshold numbers after the ruleset update.

That would be much appreciated!

moorscode · 2020-08-25T12:09:33Z

AFAICS, the threshold check basically adds a second CS run to the build and a full one at that, that seems counter to the intentions of minimizing the time taken ?

If we enable caching and do a full run, including threshold check, is there still a need for a trunk/branch toggle ?

My idea was to only check for thresholds on the trunk build and have the branch-changed-files run on the PR. So this is not a full CS build.

Not sure if caching would help in that case, because the branch-changed-files is pretty specific and (mostly) not a big task to run.
If the caching helps on reducing the time needed to do the thresholds run, that would be beneficial indeed.

jrfnl · 2020-08-26T02:49:50Z

That would be much appreciated!

Working on it. As Free would now make a huge jump in one go, I'm cleaning up the commits (which were incremental) in my local branch before adding them here. I need a little time to finish cleaning it all up.

Aside from the ruleset, I think we'll also need to fix a number of CS/ignores + add a commit removing the file docblocks from namespaced files, otherwise the threshold numbers will be artificially high when they don't need to be.

My idea was to only check for thresholds on the trunk build and have the branch-changed-files run on the PR. So this is not a full CS build.

Not sure if caching would help in that case, because the branch-changed-files is pretty specific and (mostly) not a big task to run.
If the caching helps on reducing the time needed to do the thresholds run, that would be beneficial indeed.

As most people don't look at the trunk builds that often and a build failure there would always necessitate a new PR to fix, would it be an idea to run this instead only on PR builds ?

moorscode · 2020-08-26T10:50:19Z

As most people don't look at the trunk builds that often and a build failure there would always necessitate a new PR to fix, would it be an idea to run this instead only on PR builds ?

I've changed the Travis file to execute the thresholds for both trunk and PR builds.
For the PR builds only the changed-files need to be checked.

jdevalk · 2020-08-26T13:20:19Z

Reviewed & tested, this is awesome. I'm going to go ahead and merge. @jrfnl please do a pull to fix whatever you can already fix as soon as possible, but I think adding this now prevents us from introducing new stuff which is great.

moorscode added 3 commits August 21, 2020 12:37

Use threshold configuration in Travis

299411e

Use thresholds on non-pull requests (push, tags) Use check-branch-cs for pull-requests, this will fail on any introduced problems.

Update docs for errors and warnings

ecf32a6

moorscode added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Aug 21, 2020

moorscode added 3 commits August 21, 2020 14:29

Add exi codes to commands

eb58abd

This allows Travis to fail on errors.

Reduce warnings and errors

c7809e9

In line with the phpcs configuration additions.

Add source of inspiration

8b2d18a

Mention * Thanks for the inspiration from https://github.com/OXID-eSales/coding_standards_wrapper as the source of inspiration for the code used. The package was not dynamic enough for us to be able to use it practically.

moorscode marked this pull request as ready for review August 21, 2020 13:09

moorscode requested review from herregroen and jrfnl August 21, 2020 13:09

jrfnl requested changes Aug 22, 2020

View reviewed changes

moorscode and others added 8 commits August 22, 2020 12:37

Apply suggestions from code review

5798dfb

Co-authored-by: Juliette <[email protected]>

Remove phpcompatibility from the composer requirements

ba180b2

This has been included in YoastCS since 1.0.0

Remove code-coverage ignores from methods

c42d049

Added a general exclusion in PHPUnit for the composer config directory.

Add action to update thresholds when the results are lower

d63d094

Use the last (only) entry from the summary

5957a26

Use capitalized name of the filter

8763171

Flatten CS check logic

aa064fe

Flatten the CS checks

cf7d2bd

Use correct bash elseif syntax

2eda6b8

moorscode added 2 commits August 26, 2020 12:46

Perform threshold and CS checks on any run

d4edb70

Revert running check-cs as this would always fail if we need thresholds

307e2d0

jdevalk merged commit f60a8b1 into trunk Aug 26, 2020

jdevalk deleted the introduce-cs-thresholds branch August 26, 2020 13:20

igorschoester added this to the 15.0 milestone Aug 27, 2020

Introduce PHP coding-standards thresholds #15911

Introduce PHP coding-standards thresholds #15911

Uh oh!

Conversation

moorscode commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Summary

Relevant technical choices:

Test instructions

UI changes

Documentation

Quality assurance

Uh oh!

moorscode commented Aug 21, 2020

Uh oh!

moorscode commented Aug 21, 2020

Uh oh!

moorscode commented Aug 21, 2020

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrfnl Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

jrfnl Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

moorscode Aug 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moorscode commented Aug 25, 2020

Uh oh!

moorscode commented Aug 25, 2020

Uh oh!

jrfnl commented Aug 26, 2020

Uh oh!

moorscode commented Aug 26, 2020

Uh oh!

jdevalk commented Aug 26, 2020

Uh oh!

Uh oh!

moorscode commented Aug 21, 2020 •

edited

Loading