GH Actions: Consider revising the Coding Standards actions to only scan modified files.

[costdev]

In the build up to the feature freeze, we've seen a number of Pull Requests fail CI on Coding Standards issues that existed within core commits.

​coding-standards.yml runs PHPCS on all core files for each Pull Request.

This ticket is to initiate a discussion on whether or not we are interested in modifying this file to:

• Continue to run PHPCS on all core files for push events (commits) to trunk
• Only run PHPCS on the changed files in Pull Requests.

Some potential benefits:

• Pull Requests don't fail CI because of Coding Standards errors in commits to trunk.
• Committers don't have to manually re-run CI for every Pull Request that failed for this reason.
• Committers don't have to stress that Pull Requests are being held up because of an issue in their commit.
• Contributors don't have to stress that they can't find the problem in their Pull Request.
• Contributors and Committers spend less time asking for/providing assistance or time to look into the cause.
• Contributors know that coding standards issues exist specifically within the files they modified.

Resources:

• ​GH Action - Get All Changed Files (third-party)
• ​Get changed files in GitHub Actions

As much as we've done fine without this change, I think that removing the headaches above would be a worthwhile improvement to the contributor experience.

Interested to hear others' thoughts on this.

[desrosj]

I discussed this a bit in Slack with @costdev the other day. Wanted to share my thoughts that were not included in the description above for transparency.

This feels a bit like a micro-optimization to me. The PHPCS job in the Coding Standards workflow almost always takes less than 2 minutes, and sometimes, even less than one minute. Not to say we shouldn’t make this change, but it’s not going to be a high impact one.

It's preferable to find a way to not require a third party action for this, if possible.

An alternative idea I tossed around previously that we could look (I never investigated fully). Locally when you run phpcs, there’s a cache that makes subsequent runs lightning quick, skipping files that have not been changed. I’ve wondered in the past if we could store that cache between workflows, similar to how the NodeJS and Composer dependencies are cached between workflow runs.

[costdev]

Yes absolutely @desrosj, it's only a micro-optimization in terms of performance. The potential benefits that I see are strictly in terms of contributor and committer experience.

In terms of the idea to use the cache that only runs on changed files, if this stops PRs failing due to coding standards errors in trunk then it's well worth exploring alongside the non-third party resource above!

I agree that it's best to have autonomy where possible and practical, and this would certainly apply.

By running only on modified files for PRs and running on the whole codebase for commits to trunk, this will ensure that coding standards are met in the whole codebase regardless of how it's merged.

This is fortunate as the same can't be said for tests where we need to make sure that all tests pass rather than just the ones added in a PR (because the PR could be testing an existing class/method that other tests in the codebase rely on, so we need to make sure that its code doesn't break those tests).

[johnbillion]

Replying to desrosj:

I’ve wondered in the past if we could store that cache between workflows, similar to how the NodeJS and Composer dependencies are cached between workflow runs.

#49783

[johnbillion]

Replying to costdev:

In the build up to the feature freeze, we've seen a number of Pull Requests fail CI on Coding Standards issues that existed within core commits.

How common a problem is this?

The underlying problem is that commits are being made to core that fail the coding standards checks. I would rather address this root cause to prevent that from happening.

[costdev]

Most recently it happened a number of times leading up to the feature freeze. Given that it's a busy and stressful time, this increased the frequency of the issue and its impact.

In general, I would say that it ranges from a minor inconvenience during quiet periods, to unnecessary additional stress/work for Committers during an already very busy/stressful period.

I totally agree that the underlying issue is that commits are being made that don't meet Coding Standards and that ideally this shouldn't be happening, but mistakes will happen and we should try to mitigate the impact they have in all ways that we can.

As we know, not all sniffs will have autofixes if the issues need human attention. This leaves it down to Coding Standards being part of the commit workflow, which I trust it already is and that Committers do everything they can to make sure they cover everything on the list.

Currently:

• When a Pull Request fails on Coding Standards, this doesn't affect commits to trunk or other Pull Requests.

• When a commit to trunk fails on Coding Standards, this affects everything.

IMO, in order to minimise confusion, Coding Standards errors should only affect where they occur.

By only scanning modified files on Pull Requests for Coding Standards errors, we can let those Pull Requests run through CI, pass and be ready for Review/Commit whenever the Coding Standards are fixed on trunk. This improves the Contributor experience and also keeps things running smoothly.

In addition, especially leading up to a freeze, Committers won't have to manually re-run CI on each of the Pull Requests, answer requests for help or be put under additional stress by people panicking and possibly getting frustrated if the issues are happening frequently.

Although the last two paragraphs are re-iterating the ticket description - Sorry!

When it comes to negatives, one would be:

1. A Committer makes a commit with Coding Standards errors.
2. A Contributor fetches from trunk, creates a branch and submits a Pull Request. The Pull Request will fail on the same Coding Standards issue.

The solution: Rebase with trunk (which we should all be doing before submitting a Pull Request anyway).

---

Aside from this ticket's initial proposal, another is that every single commit to trunk is submitted as a Pull Request on GitHub (or SVN equivalent, if there is one... I don't know SVN) to make sure that CI is forced. If that's possible/agreeable, I believe that would also work. However, it may be a controversial approach vs a change to CI.

[desrosj]

#49783 has been closed as fixed. This will significantly improve the performance of the phpcs and phpcompat commands (by 95%+). So performance is no longer a primary benefit of this suggested change.

The more I think about this, the more I lean towards wontfix. While it does happen occasionally, ​committers are told to run the necessary CS checks prior to committing, and to monitor all CI builds for their commits.

I don't like enforcing GH pull requests because everyone is free to use whichever workflow they like.

[costdev]

@desrosj That's excellent news about the performance improvements!

However, in terms of this ticket, performance wasn't the primary benefit of the suggested change. The primary benefit is that coding standards, or other coding failures, will no longer cause CI failures across the board because commits to core aren't fully checked before submitting. The remaining benefits are in the initial ticket description.

I agree that the information in the handbook surrounding commits should be followed, but one of the reasons that we have automated checks for tests and standards on PRs is because we accept that mistakes happen and we need to verify that everything is as it should be. However, commits to core don't have the same pre-merge enforcement of automated checks, otherwise they'd never make it into core.

As @johnbillion said, the ideal is to focus on the root cause, so while the original proposal in this ticket would prevent failures spreading to GH pull requests, any implementation of automated checks for each workflow that run before merging to core would achieve the benefits that this ticket is advocating for. It would also reduce the number of manual checks that committers have to do before committing to core and therefore reduce the number of opportunities for mistakes to occur.

Even a local helper script that committers can execute to run all of the tests/standards checks before they commit to core would reduce those separate steps to remember down to just one. It's even likely that many committers have already written such a script that could be adapted to the different environments/workflows.

[jorbin]

As @desrosj said, the undrerlying problem of slow performance has been fixed. Therefore, I am marking this as maybelater though since if WordPress Core ever switched to github first, this is something that should be reconsidered. Discussion can continue, but barring a major change like that or some other piece of new information, I think closure is the best route.