Problem/Motivation
Part of #1299710: [meta] Automate the coding-standards part of patch review.
We want the test runner to be able to run phpcs to check coding standards for projects. This could allow for halting the test run based on CS errors.
Proposed resolution
It's not really the job of the test runner to figure out what version of coder to use, or what rules apply, so core should allow us to run phpcs and capture the results.
This fits with the general CS strategy of core, which changes the set of sniffs which should be fixed as CS errors are fixed.
So that means that for the runner to work we need to modify it with the following:
- Add a phpcs task plugin. (It'll be a near-copy of DrupalCI\Plugin\BuildTask\BuildStep\CodebaseValidate\PhpLint.)
- The phpcs task will run phpcs within core/, or within the root directory of a contrib project.
- Add some sanity-checking on core version to make sure we're not trying to run phpcs against an earlier version with no phpcs. (D7 or D8 from before phpcs is a --dev requirement.)
- Modify the test run definitions to include this new plugin.
- Look at a configuration variable to determine whether to fail the test run based on failed CS.
- Store the result file in the artifacts directory.
Remaining tasks
Update d.o infrastructure to use this information usefully.
Update d.o UI to allow for failing the test run on CS errors.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#44 | 42-44-interdiff.txt | 1.67 KB | alexpott |
#44 | 1266444.44.patch | 2.02 KB | alexpott |
#42 | 1266444.42.patch | 1.18 KB | alexpott |
#30 | interdiff.txt | 9.22 KB | Mile23 |
#30 | 1266444_30.patch | 14.15 KB | Mile23 |
Comments
Comment #1
cweagansThis is postponed because we can't turn this on until core is 100% compliant or all the tests will start failing.
Then again, perhaps that'd be a good way to get a LOT of people working on code style cleanup! =P
Comment #2
basic CreditAttribution: basic commentedWe are working on cleaning up Drupal.org-related issue queues. Many issues have been open for years with no resolution because there were no volunteers interested in taking on the task or staff time available to complete the work.
We are going through old issues and marking them as Closed (won't fix). If you feel your issue has been closed in error, please do comment on the issue and let us know. If we know something is critical, we may reconsider the closure of a feature request or bug report.
Thank you for your understanding while we work to clean up the Drupal.org queues.
Comment #3
YesCT CreditAttribution: YesCT commentedrelated #1299710: [meta] Automate the coding-standards part of patch review
Comment #5
YesCT CreditAttribution: YesCT commentedThe current plan is to only put one coding standard in place for automatic testing, one that core already passes for. And start the system like that.
Then, we can introduce one new standard at a time, along with an issue to get core passing for just that one additional standard.
Comment #6
isntall CreditAttribution: isntall at Drupal Association commentedComment #7
isntall CreditAttribution: isntall at Drupal Association commentedComment #8
jthorson CreditAttribution: jthorson commentedThis would be implemented through a new 'CodeSniffer' job type.
Instructions on what this job type would need to perform are located at https://www.drupal.org/node/1419988.
I anticipate this would leverage the PHP containers instead of the web containers, as I do not believe CodeSniffer have any apache dependencies.
Comment #9
mradcliffeAdded a related issue from PIFR that will need to be taken into account in the CodeSniffer job type.
Comment #10
pfrenssenSetting back to active as per #2571965: [meta] Fix PHP coding standards in core. This has been cleared to work on, but any changes can only be put in action after the release of 8.0.x-RC1.
To work around the chicken-and-egg problem that the tests would fail unless all coding standards are fixed, we are going to maintain a blacklist of coding standards checks. Initially we will blacklist all coding standards, so that effectively no tests will run. We will then fix coding standards one by one, each time removing the fixed standard from the blacklist. These standards can then be tested automatically from that point onwards. The blacklist is being added in #2573377: Add our own phpcs.xml file.
Comment #11
pfrenssenHere's an initial implementation of a 'php_codesniffer' command. It downloads the Coder module using git, installs PHP CodeSniffer by running
composer install
in the Coder module folder, and runs the coding standards checks based on aphpcs.xml.dist
which is expected to be present in thecore/
folder (this will be added in #2573377: Add our own phpcs.xml file).It is currently using the latest 8.x-2.x dev release of Coder, but we should probably "pin" a recent commit to avoid sudden failures if new sniffs are added to the Coder module.
Comment #12
attiks CreditAttribution: attiks at Attiks commentedPatch looks good and works, but what else is needed to move this forward?
Comment #13
pfrenssenIt needs to be RTBC'd :)
Comment #14
attiks CreditAttribution: attiks at Attiks commentedMarking as RTBC, if this gets deployed to the real test bot, I think we should add coder to the docker image to save time and pin it to a certain version.
Comment #15
pfrenssenRaising priority, now that the release candidate is out we can start working on coding standards fixes, but this needs to get in first. This is now blocking the 30+ issues from #2571965: [meta] Fix PHP coding standards in core.
Comment #17
jthorson CreditAttribution: jthorson commentedAdded to branch 1266444-add-coder-support
Ready for merge, and no risk (completely different job type).
Comment #18
elachlan CreditAttribution: elachlan commentedComment #19
elachlan CreditAttribution: elachlan commentedComment #20
elachlan CreditAttribution: elachlan commentedComment #21
MixologicComment #22
pfrenssenIs there anything holding this up? The whole coding standards initiative has lost its momentum after waiting 6 weeks for this issue to be committed :(
Comment #23
pfrenssenThis probably needs another look in the meanwhile.
Comment #24
basic CreditAttribution: basic at Drupal Association commentedUnfortunately there is a lot of work that still needs to happen. We are not just waiting on this to be committed in order to enable coding standards.
DrupalCI needs a larger refactor in order to incorporate a change like this, and while we can commit this as is, DrupalCI would not actually be able to use it correctly because it is a different job type.
Each Job in drupalci currently assumes an entire process from start to finish - environment creation (containers), Codebase setup (git clones and patches), running the Job, and finally post processing the Job results in to a standardized format for jenkins to consume, so that Drupal.org can access to the results from the Jenkins API.
Additionally, the AWS management portion of the DrupalCI Job process spins up EC2 instances in response to individual Jobs, and we cannot afford to spin up double the number of bots in order to make this happen.
What we need is to refactor DrupalCI to be able to execute multiple "tasks" as part of a "job". This way we can have simpletest testing, plus all of the other coding standards checks, plus any additional tasks that we come up with all run as part of the *same* Job, without spinning up a new bot, rebuilding/reconstructing the container environment or recreating the codebase.
Before we do the refactor needed for splitting Jobs into smaller "tasks", the priority is to add functional/unit testing to DrupalCI. We need a better way to verify that we're not introducing regressions. We need to do some pretty serious restructuring of the codebase to make that happen, primarily we need to add a dependency injection container to have proper mocks and real unit tests alongside some functional tests.
Once testing is in place, we can begin the refactor DrupalCI to allow for tasks inside of a jobs knowing we haven't introduced big regressions. This patch can be refactored as a task in the DrupalCI Job which outputs checkstyle xml format, Jenkins can be configured to parse that xml, and Drupal.org integration can be built to parse that information from the Jenkins API and display the results on Drupal.org.
Comment #25
YesCT CreditAttribution: YesCT commentedComment #26
xjm@basic, so how can we move this issue forward?
Comment #27
alexpottMight as well use PHP7 it'll be quicker and therefore cost less
Not needed with 2.5.0 version of PHPCS
Is spinning up a new bot expensive - I thought it was mostly about the time a bot is running - these bots will be short lived.
Comment #28
Mile23Codesniffer is properly a separate job, since that gives us a way to specify that it run last or whatever.
As per the discussion over here: #1299710-50: [meta] Automate the coding-standards part of patch review we should have a way to specify individual files, based on what files have changed in the repo.
Comment #29
Mile23Something more like a review:
We want to add
drupal/coder
andsquizlab/php_codesniffer
to ourcomposer.json
file, so we have an executable in the testbot rather than relying on downloading the module somewhere.That leads to:
This path should point to the phpcs binary we'd have installed by composer.
Comment #30
Mile23Here's an appropriate change to
composer.json
, based on #11. This doesn't make any other code changes. Runcomposer update
after you apply this.I'd do more code-y work on this but the vagrant box is a pile.
Comment #31
alexpottCan't we do something simpler and more cost affective? At the moment we php lint all the changed files as part of every test. Can't we just add a step to run phpcs with the drupal coding standards and core's current config against the changed files. This is what I'm doing on every commit I make with my precommit hook... https://github.com/alexpott/d8githooks/blob/master/pre-commit
At least if we do this we don't introduce regressions for things we've already fixed.
Comment #32
Mile23Addressing blockers for this issue: #2654650: [meta] Jobs for linting and coding standards
Comment #33
hass CreditAttribution: hass commentedWe already had coder_love sh** enabled in past. This implemented non-core rules. Additionally coder module *always* has fails detections. I never seen it working reliable.
I'm wondering how you'd like to solve this.
Comment #34
pfrenssen@hass how long has it been since you used Coder? It's very reliable. I run it multiple times per day on a very large distribution that has around 10000 PHP and JS files across hundreds of custom modules and features, written by 70+ developers of varying skill levels. We have not a single false positive.
If you encounter a false positive then please file an issue so it can be fixed.
Comment #35
alexpott@hass another point is that we're only enabling rules in phpcs.xml.dist when we have core passing. Therefore all you have to do is remove on of the disabled rules and get core to pass in a patch... and hey presto we'll then get no regressions.
Comment #36
hass CreditAttribution: hass commented@pfrenssen: I'm not aware that only one of my fails positive issues I have filed against coder has been fixed in the last 5 years. All my cases should still be open. I think my last test was two months ago when i checked all my D8 projects with coder "minor" mode.
Comment #37
hass CreditAttribution: hass commentedJust one example. Full of bullsh** http://pareview.sh/pareview/httpgitdrupalorgprojectlinkcheckergit-7x-1x
Comment #38
jthorson CreditAttribution: jthorson commentedI think there's a bit of confusion here ... PAReview != Coder.
PAReview was created to automate reviews of Project Applications long before Coder was refactored to use CodeSniffer; and has it's own set of rules and custom scripts.
Comment #39
hass CreditAttribution: hass commentedSorry, #539984: Only invalid errors reported on linkchecker 2.x should have been linked and #1896084: The control statement should be on a separate line from the control conditional and #1012706: hook_user() false positive detection of module internal functions and #1012704: Critical warning for file_scan_directory, but nomask is not used and #314422: Database schema validation / reserved name validation
5-6 years open and unfixed.
Aside this pareview is linked in coder project home as the coder online version...
Comment #40
jthorson CreditAttribution: jthorson commented@hass: In any case, your problem appears to be with Coder, and not with DrupalCI ... this issue is about enabling Coder type reviews in DrupalCI, but we shouldn't derail the effort by bringing baggage which is specific to Coder module into scope of this particular discussion.
Comment #41
xjmIn a meeting with the core committers and DA infrastructure staff today, we learned that it will take a lot of resources or a long time to actually implement this on DrupalCI. A workaround was proposed for the short-term: #2663086: [meta] Determine whether run-tests.sh can trigger and report phantomjs and phpcs results
Comment #42
alexpottSo we could make phpcs run as part of the simpletest job and only on the files that have changed after applying a patch. The patch here fails as expected when applying https://www.drupal.org/files/issues/phpcs.test.patch and running some tests. We can probably optimise the installation of phpcs and Drupal's coding standards but pulling them down each time at least allows improvements and perhaps we can get rid of the
sleep 5
to regain the lost seconds :)The next will be to work on how we communicate this back to Jenkins etc...
Comment #43
alexpottComment #44
alexpottPatch attached generates a checkstyle report and saves it in
/var/www/html/artifacts/phpcs-checkstyle.xml
. Afaics that should mean that this will get shipped back to Jenkins and Jenkins has a plugin to read checkstyle files...Here's an example of a checkstyle file when it fails:
If there are no files changed (a branch test) then there is no code style test and therefore no file.
If the patch has no errors detactable by phpcs then you get an empty report:
Comment #45
MixologicComment #46
MixologicComment #47
Mile23Want this?
Then we need this: #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core
Which is waiting on this: #2745355: Use "composer install --no-dev" to create tagged core packages
Comment #48
Mile23Comment #49
Mile23Comment #51
Mile23This issue already has branch
1266444-add-coder-support
but it's from before the big refactor.Adding branch
1266444-phpcs-coder
which adds some non-spec behavior just to get it working.What it does:
simpletest_sniff
test definition which omits therun-tests.sh
part of testing.Codebase::getModifiedFilesForNewPath($path)
which lets you substitute the environment path for the build path.composer_phpcs
plugin which always uses composer to require coder and phpcs. This replaces thecomposer
plugin in the test definition flow during build. This is a temporary measure and eventually we’ll just check if the phpcs executable is present in the validate plugin after #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/corecore_phpcs
plugin which occurs just after PHP linting. This needs to be split into a generic runner and core’s specific needs.Run it and see. Set up a local testbot and do this:
echo $?
then shows you that the test failed because of poor CS by outputting 1. Remove theDCI_CS_SniffFailsTest
env variable it will pass despite poor CS. There’s also aDCI_CS_WarningFailsSniff
which promotes CS warnings to failures.Things to think about:
Both linting and sniffing occur in a step called
validate_codebase
.validate_codebase
happens after the containerized environments have been set up, which means that they currently have to wait for things like the database to start up. This is necessary because the PHP environment is required for both linting, which is tied to a specific PHP version, and also for composer to work out dependencies based on platform. But it's not the greatest because we have to spin up those resources which aren't needed to fail the test run at that point. So splitting the db environment out from the web environment might be a follow-up.This branch doesn't handle any special reporting. D.O will need to use the report somehow.
Comment #53
Mile23Teensy bit of refactoring so we have a generic phpcs plugin. Renamed plugins to be more generic and understandable.
Checks whether phpcs exists before proceeding in the validation plugin. This still gets a false positive because we're using the
composer_force_coder
plugin during codebase build.Comment #55
Mile23So what I did in 25158b3 is:
Removed the special-case core coder sniff plugin. Everything it did is now configurable to do in the
phpcs
plugin.Added a couple tests. Needs more, especially once #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core happens.
phpcs
can configure:Comment #58
MixologicOkay, made a bunch of tweaks to support phplinting, which ended up on this branch because I was lazy, but also had some shared concerns.
So, some changes I still think need to happen that we've talked about in IRC:
1. I think I've changed my mind on the "modified files" being the determiner for whether or not to sniff everything, as the modified files are only sniffing php files, so we wouldnt want to sniff all php if the patch only introduced javascript changes, for example. In other words, lets keep the config option and the way it works here in the branch.
2. I'd still like to remove the 'composer force coder' plugin and just have that be an additional composer.coder step with the 'options' configured. There may be an issue with installing coder for contrib modules however, since we add packages.drupal.org/8 to require the contrib module, and there exists, currently, a bug with coder at the composer facade. I think though that requiring the 8.2.8 version alleviates that due to it falling back on packagist, but its something we should make sure works , like with a test or something.
3. If we add phpcs to the default use cases (simpletest/simpletest7), we can have it run on jenkins for now, and produce output there until such time as we come up with a UI to show it on d.o.
4. We should also either find an issue, or open one with core that asks "how shall we proceed when we find codesniff failures?" Should a failed phpcs check fail the build and return "whoops" immediately? or should we go ahead and run tests.. I suppose that doesnt even really need to be worked out in advance.
I may go ahead and merge this into dev, since it doesnt really affect anything to have the extra plugins yet, even if they arent totally 'done'
Comment #59
Mile23The only reason that plugin exists is because core doesn't currently require coder/phpcs. Once it does, we should only check whether it's present during the phpcs plugin step. #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core
That is: We shouldn't even add an options step to composer, and only add the phpcs plugin. It will check the codebase and only run if the tool is present in core's vendor/bin/.
Not all contrib has a phpcs.xml file or would even want a CS report. Examples does... :-)
+1.
Full-codebase sniffs of core can take a long time, however, and might not be wanted, so let's not sniff if there are no changes worth sniffing.
Hang on... Let's get rid of the force-coder plugin and ensure that the policies are right.
Comment #60
Mile23Comment #62
Mile23Removed composer_force_coder, improved docs for fixture/demo build definitions, added policy test for phpcs to not kill a build job if it fails and will only sniff changed files, minor tweaks
Comment #63
Mixologicsweet. am approving of all of the things.
Im even more inclined to merge this in, unless you've got more planned (still assigned to you)
Comment #64
Mile23Still needs contrib stuff, such as changing the config to point to module/projectname and also tests.
Comment #65
Mile23So we merged the work here in order to fix the git added files issue here: #2597778: Install composer dependencies for contrib projects before running tests
This issue accomplished the goal of allowing a way to fail the test run based on phpcs status, for Drupal core.
Still waiting on core to catch up... :-) #2744463: Add phpcs, coder 8.2.8 as --dev requirements for drupal/core
Follow-up to add in contrib support and follow-through on other bugs in #2841472: DrupalCI should return failed for poor code style Part II - php