Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Automate squizlabs/php_codesniffer #32785

Merged
merged 2 commits into from
Sep 28, 2018
Merged

Automate squizlabs/php_codesniffer #32785

merged 2 commits into from
Sep 28, 2018

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Sep 20, 2018 •
edited

Description

  • implement phpcs to enforce the existing acceptance test code style rules
  • do it as an extra step of the existing test-php-style make target
  • fix style of code

Related Issue

Motivation and Context

See issue.

How Has This Been Tested?

Local run:

make test-php-style

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@phil-davis phil-davis added this to the development milestone Sep 20, 2018
@phil-davis phil-davis self-assigned this Sep 20, 2018
@codecov
Copy link

codecov bot commented Sep 20, 2018 •
edited

Codecov Report

Merging #32785 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #32785   +/-   ##
=========================================
  Coverage     64.14%   64.14%           
  Complexity    18721    18721           
=========================================
  Files          1184     1184           
  Lines         70453    70453           
  Branches       1270     1270           
=========================================
  Hits          45195    45195           
  Misses        24888    24888           
  Partials        370      370
Flag Coverage Δ Complexity Δ
#javascript 52.89% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.42% <ø> (ø) 18721 <ø> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db3df6c...528ad27. Read the comment docs.

@phil-davis phil-davis mentioned this pull request Sep 24, 2018
Closed
@phil-davis phil-davis force-pushed the test-acceptance-style branch 3 times, most recently from 5ce05fb to ee210ad Compare September 27, 2018 04:02
@phil-davis phil-davis mentioned this pull request Sep 27, 2018
Merged
@phil-davis phil-davis force-pushed the test-acceptance-style branch 2 times, most recently from e1b01c7 to 329c878 Compare September 27, 2018 08:33
@PVince81
Copy link
Contributor

did you guys consider simply adding it to the existing make test-php-style rule ?

@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 27, 2018 •
edited

@PVince81 good idea! Then developers will get the checks "for free".

I disabled the line length checks. Those give lots of warnings because the acceptance tests have many long lines due to the length of some Gherkin step text. The warnings make a lot of noise in the the output, we mostly ignored them anyway, and it would really annoy developers to get loads of crap output when doing make test-php-style

Anyway, this is not any sort of "final project standard". It is what we do in the QA team at the moment, and can easily be changed if the project decides to adopt some project-wide coding standard.

@phil-davis
Copy link
Contributor Author

@PVince81 please review again.
And merge if you like it - needs the override of no Travis result.

PVince81
PVince81 reviewed Sep 27, 2018
View reviewed changes
Makefile
@@ -196,6 +196,7 @@ test-php-lint: $(composer_dev_deps)
.PHONY: test-php-style
test-php-style: $(composer_dev_deps)
$(composer_deps)/bin/php-cs-fixer fix -v --diff --diff-format udiff --dry-run --allow-risky yes
$(composer_deps)/bin/phpcs --runtime-set ignore_warnings_on_exit --standard=phpcs.xml tests/acceptance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, so we won't be using the same rules for QA code ?

can you make a separate ticket to consolidate said rules so we only use a single tool ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand your question.
php-cs-fixer implements some "basic" rules across the whole code-base, including acceptance test code. That is already in place.

phpcs has even more code style rules than php-cs-fixer.

During discussions about code style, PHPdoc block etc standards some time ago, the QA team implemented some phpcs rules to get started, and applied it to the tests/acceptance/... folders. This is an "interim" standard while we await the "whole project" deciding on a project-wide standard. We have been manually running phpcs locally on PRs to see that we meet our standard, but of course we often forget!

So automating it here will force us (and anyone who touches tests/acceptance/.../*.php) to remember.

If the whole project some day decides to adopt a standard that is inclusive of these rules, then easy - the acceptance test code already meets the rules here. If the project adopts some standard that is not the same/similar, then we reformat doc blocks or whatever (like the rest of the project will have to do anyway).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm ok, so if phpcs itself has more rules than phpcsfixer we could eclipse phpcsfixer then... and many IDEs support loading phpcs rules but not ones from phpcsfixer...

@phil-davis
Copy link
Contributor Author

phil-davis commented Sep 27, 2018 •
edited

@PVince81 this will need a merge override, because it is expecting a Travis result.

@phil-davis phil-davis mentioned this pull request Sep 28, 2018
Merged
@phil-davis
Copy link
Contributor Author

Backport stable10 #32900

@PVince81 PVince81 merged commit 2349089 into master Sep 28, 2018
@PVince81 PVince81 deleted the test-acceptance-style branch September 28, 2018 07:08
@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@PVince81 PVince81 PVince81 left review comments

@individual-it individual-it Awaiting requested review from individual-it

@patrickjahns patrickjahns Awaiting requested review from patrickjahns

@paurakhsharma paurakhsharma Awaiting requested review from paurakhsharma

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

Assignees

@phil-davis phil-davis

Labels
3 - To Review dev:acceptance-tests QA:team
Projects
None yet
Milestone
development
Development

Successfully merging this pull request may close these issues.

Add automated phpcs checks in drone for acceptance test code
2 participants
@phil-davis @PVince81