backdrop
GHA phpcs check can produce failures on unrelated files under certain circumstances
Description of the bug
It popped up here: https://github.com/backdrop/backdrop/actions/runs/3363918624/jobs/5577673674
All of a sudden there are check failures for totally unrelated files.
This happened, because the PR branch was far behind core, so in the meantime new files got added, which didn't exist in the branch.
Neither the Action nor the phpcs version changed, so I'm not sure yet, what's going on. There are no annotations on the PR files, but in the workflow log, loads of errors and even warnings (which are suppressed by the workflow) get logged.
Worst case: we have to move away from that action and write something custom
Thinking this over a bit... neither the action nor phpcs changed, so it might have to do with a change in GitHub workflows.
Some time back there was a (beta) feature to show annotations on unchanged files. That didn't seem to make it in (didn't see it recently), but maybe something similar has now been added. And that might break how thenabeel/action-phpcs catches unrelated coding standard failures. Wild guessing, though.
Or maybe it's something completely different...
@bugfolder your PR branch is extremely behind (9 commits ahead, 407 commits behind backdrop:1.x.)
The files that throw errors or warnings are files that have been added to core in the meantime - they do not exist in your branch. That explains:
- Check failures - somehow the action considers these missing files as "changed"
- That no annotation shows up - the files are missing in your branch
If that's really the reason, we should clearly document when a rebase is necessary.
Update: after a rebase everything's fine again, so it was indeed the outdated state of the PR branch that caused the false positives.
The question is now: should we document that somewhere - and where?
The question is now: should we document that somewhere - and where?
Seems like it should be documented, it's a new and potentially confusing behavior. In the case that led to this, although the branch was well behind, it was mergeable without conflicts, and so in pre-PHPCS days, wouldn't have needed a rebase.
And now, if you see PHPCS errors in code you didn't touch, it raises the question: "do I just fix these errors to get my code to pass, or might they go away if I rebase my code?" Providing some guidance on this would likely be helpful.
As for where to document it, https://docs.backdropcms.org/documentation/how-to-use-the-core-issue-queue might be a good place. A section on "Automated tests" could address the general topic, and then could mention this detail.
it's a new and potentially confusing behavior.
Definitely. It also caused me some head-scratching and I'm supposed to be familiar with how that action behaves. :stuck_out_tongue_winking_eye:
A section on "Automated tests" could address the general topic, and then could mention this detail.
Or maybe here: https://docs.backdropcms.org/documentation/contribute-to-backdrop-core#pull-requests
That section's more about PRs.
Fun fact: now it beats me: https://github.com/backdrop/backdrop/actions/runs/3386467794/jobs/5625907223
The annotated code part is "miles away" from what I changed. My branch is up-to-date with core. That file has loads of other violations, but seemingly a random one gets picked. I verified, that the scope of the action is still "blame". Not sure, what's the logic behind that...
Guess what: "git blame" is sort of correct... The last one touching the annotated line was me!
But that's not what we want. Unless we want to punish ourselves for former deeds. :laughing: No, seriously... what do we do in this case?
what do we do in this case?
Beyond my pay grade, but when the dust is settled and things are sorted, I'll update the docs.
We discussed this issue at length today in the developer meeting: https://www.youtube.com/watch?v=8AOl_edUKwA
Here are the take-aways:
- phpcs is still providing more benefit than harm and we don't want to turn off the action.
- It would be nice if we could keep the comments but not necessarily fail the state of the PR, is that possible?
- Our official policy towards phpcs failures should be:
- phpcs changes are suggestions and do not need to block the progress on a PR, especially if the suggested changes are outside the changed lines.
- Core contributors can ignore violations outside the changed lines. OR they update them if they wish to get complete compliance.
- Core committers are authorized to ignore coding violations outside of changed lines and merge even if a PR is failing phpcs checks.
- The best course of action to fix these unrelated suggestions would be to help contribute to the action itself and fix the problem at its source. It seems we're running https://github.com/thenabeel/action-phpcs, which was forked from https://github.com/tinovyatkin/action-php-codesniffer. Let's try working with @thenabeel first since their action has been more recently maintained.
Hello!
This is how it gets the changed files, in case you wanna look: https://github.com/thenabeel/action-phpcs/blob/master/src/get-changed-file.ts
Hi @thenabeel! Thanks for jumping in here!! And thank you for maintaining the action that our project is using! I think the current problem we're facing is when the same file has phpcs violations but the line wasn't actually touched in the current pull request. Looks like this line is where action-phpcs decides whether to report a problem, based on git blame matching the email address: https://github.com/thenabeel/action-phpcs/blob/master/src/run-on-blame.ts#L44 That's what we're seeing where if a contributor is filing a PR, they get syntax warnings on any lines they've previously changed. Seems like ideally we'd be able to get the list of commit hashes in a PR, and use that for the comparison here, rather than just checking the changed line's author's email address.
Would you be interested in turning on issue tracking in your repository so we can discuss outside of a PR?