search

department-of-veterans-affairs / va.gov-cms

Editor-centered management for Veteran-centered content.
https://prod.cms.va.gov
GNU General Public License v2.0
97 stars 69 forks source link

Investigate/implement PHP_CodeSniffer Baseline #15954

Closed ndouglas closed 10 months ago

ndouglas commented 10 months ago

Description

We can add a baseline report for PHP_CodeSniffer to prevent existing errors from cluttering the report and confusing the engineer if they run composer va:test:php_codesniffer.

See here for one option: https://github.com/123inkt/php-codesniffer-baseline

Acceptance Criteria

JunTaoLuo commented 10 months ago

cc @BerniXiongA6, this should be a small/quick task.

JunTaoLuo commented 10 months ago

Hmm I'm not sure if CodeSniffer is being run on the CI correctly. Maybe the failures are being hidden by reviewdog?

image
ndouglas commented 10 months ago

Yep, that's a sure sign of things not working 🤦🏻

JunTaoLuo commented 10 months ago

Took a further look, reviewdog filters errors from the current changes. Effectively it uses the failures in the parent branch as a baseline so as long as you don't introduce new errors, it will quietly ignore old ones. I only noticed these errors since I was running the phpcs commands directly without reviewdog and that's why I saw the failures when most folks who rely on the CI tests tend to not see these errors.

TL;DR the CI tests are "working" as intended for the most part. I think we should still update the baselines so local dev and CI are aligned.

ndouglas commented 10 months ago

Yeah, running the commands locally should not output errors unrelated to the outstanding changes.

ndouglas commented 10 months ago

Sorry, I misunderstood your comment.

I thought you were referring to the reviewdog: This GitHub token... message. I double-checked and saw that your PR is opened from a fork -- we don't really do that here anymore. We prefer opening PRs from a branch on this fork (department-of-veterans-affairs/va.gov-cms) because of GitHub Actions and how the default token works with forks on public repositories. If you open a PR from a branch on this repository, that should work correctly.

And yeah, as you mentioned, it'll filter out messages that don't correspond to changes in the PR.

JunTaoLuo commented 10 months ago

Ah I see, yea I prefer working with branches in the repo instead of forks as well. I think I read in some doc to setup my fork so I'll go hunt that down and update the language there.