Add support for linting annotations and other static analysis workflow improvements#76120
Open
Add support for linting annotations and other static analysis workflow improvements#76120
Conversation
|
Size Change: 0 B Total Size: 6.87 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 900b6ca. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22675447925
|
This reverts commit 016c76c.
This reverts commit 7f45020.
desrosj
changed the title
Now that it only runs once, the step's `if:` can handle the logic.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Static Analysis GitHub Actions workflow to improve developer feedback by enabling GitHub UI annotations for ESLint (lint:js) and Stylelint (lint:css) results, and makes a few workflow execution improvements (step splitting, npm caching).
Changes:
- Split
npm run lintinto separate workflow steps and adjust ESLint/Stylelint output tocompactformat for annotation ingestion. - Add a matrix flag to run annotation-producing lint steps only once per workflow run.
- Enable npm caching via
actions/setup-node.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
Agreed 👍 As I recall, it was done this way because we don't have anywhere else in CI that we call |
aduth
approved these changes
Mar 4, 2026
Member
aduth
left a comment
aduth
left a comment
There was a problem hiding this comment.
This seems like a good change, and I agree with your assessment on the benefits and opportunities of splitting this up 👍
I wrote my comments before fully reading your original pull request message, so I recognize they repeat a lot of what you already wrote. But I'll keep them intact if at least to reaffirm your impressions 😄
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0 | ||
| with: | ||
| node-version: ${{ matrix.node }} | ||
| cache: npm |
Member
There was a problem hiding this comment.
This seems like it'll be a nice win as a standalone change 👍 Surprised we didn't have this or that it's not a default.
Though reading the relevant documentation, it does have me thinking whether we should have packageManager specified in package.json as a more standards-based way of activating this across all of the Node workflows.
Member
Author
There was a problem hiding this comment.
Though reading the relevant documentation, it does have me thinking whether we should have
packageManagerspecified inpackage.jsonas a more standards-based way of activating this across all of the Node workflows.
Ah, good catch. That is a relatively new addition to actions/seup-node.
I'm torn for this one. On one hand it's good to have it on by default for the sake of better performance. But on the other, making it an intentional addition makes it a conscious choice and could help guard against cache poisoning attacks when production assets are being built or commits are being pushed to the repository.
It's also possible that someone could submit a PR to add packageManager to package.json because it's just a good thing to do. In that case, it may unintentionally enable caching without awareness that caching ends up enabled within Actions.
I am leaning towards adding it now and auditing where actions/setup-node is being used to explicitly disable caching wherever necessary at the same time just to prevent that last scenario. I've made myself a note to create an issue to collect feedback once the dust settles from 7.0.
| run: npm install | ||
|
|
||
| - name: Lint JavaScript and Styles | ||
| run: npm run lint |
Member
There was a problem hiding this comment.
Noting a couple things about this change to use granular scripts:
- It duplicates the source of truth for "linting everything", i.e. I could foresee that we add new scripts to
npm run lintand forget to include them here, or vice versa. npm run lintusesconcurrentlyto run these in parallel, so this could have a performance impact. Although (a) linting generally isn't our biggest problem in CI as far as time spent, and (b) we might be overdoing the parallelization that this could end up being a good thing anyways.
Member
Author
There was a problem hiding this comment.
It duplicates the source of truth for "linting everything", i.e. I could foresee that we add new scripts to
npm run lintand forget to include them here, or vice versa.
Yes, that is possible. One approach to make sure this scenario is caught could be to run npm run lint anytime the scripts package is updated. But that would only ensure the PR introducing the new script runs it.
We likely need to just have better documentation and guidance around this within packages. Each package should have a section detailing where any automation or CI/CD testing should be added when updating the package as a reminder. Maybe that could be a reason to introduce a CONTRIBUTING.md file to individual packages.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
This adds support for contextual inline annotations for
lint:js(ESLint) andlint:css(Stylelint) results on pull requests and commits.Why?
Visual annotations within GitHub's UI makes it easier for contributors to investigate static analysis failures without having to dig through GitHub Actions logs or re-run the commands locally. Warnings and errors are also surfaced on the workflow summary UI, which makes them easier to find without having to dig through the workflow run logs.
How?
For annotations to work, the report needs to be output in a specific format that GitHub can understand and ingest. This can be accomplished using ESLint's
--format compactand Stylelint's--formatter compactarguments, which change the output format to match the pattern required by a problem matcher. Theactions/setup-nodeaction makes the required problem matchers available by default through thetoolkitdependency.The cleanest way to make this happen within the workflow is to avoid using
lint(which runs all linting tasks usingconcurrently) in favor of splitting each linting task out into it's own step. This may seem counterintuitive at first because the benefits of parallelization are lost, but it actually is not a significant difference. In my testing, the individual linting commands are as follows (all times onubuntu-based runners):lint:tsconfigandlint:lockfile: runs in 1 second or lesslint:pkg-json: Runs in 10-15 seconds.lint:styles: Runs in 15-20 seconds.Because these all take less than 1 minute combined, there's no significant benefit of running them all concurrently. The majority of the time within
run lintis spent onlint:js, which appears to be around ~4 minutes on average currently.While working on this, I also noticed that caching was not configured for
npm. Thesetup/node-jsaction has an input for this and handles it all under the hood for you. Since the workflow is not building any production assets there's no risk of cache poisoning, so I've gone and enabled that.Despite a natural assumption that total run time would increase with these changes, the total run time appears to be less overall with the addition of caching. It's worth noting, though, that the Windows-based jobs themselves do take slightly longer. I'm not exactly clear why just yet, but I think that some of the steps in this workflow should be skipped entirely in all but one of the jobs spawned by the strategy matrix.
In my opinion, testing different operating systems within a static analysis workflow is the wrong place. Coding standards-focused checks return consistent results regardless of the environment it's run in. The strategy matrix was expanded in #72946 as a short-term way to catch any potential compatibility issues, but most of the issues seem to have been a result of changes to the custom scripts maintained here. I think that eventually there should be some some proper tests for the scripts instead of having to run them directly to confirm the results. But that's an effort for another day keeping this mostly as is for now is fine short-term.
As for the recommendations to skip certain steps entirely. I think that the
lint:cssandlint:jsscripts can be skipped on all but one job because they directly call Stylelint and ESLint without any custom path construction.lint:pkg-jsonalso seems relatively safe, but since it only takes ~15 seconds, it's fine to just leave for now. This will decrease the overall runtime for windows jobs by ~3-4 minutes, and MacOS by ~2 minutes.A few other benefits of breaking the scripts up into separate steps:
concurrently. This restores the formatting for easier scanning of larger result sets and makes it significantly easier to scan the workflow run logs.It's reasonable to also consider the fact that warnings also bubble up to the workflow run UI a con. When there are unaddressed warnings, this could seem a bit noisy. But on the flip side, this is an added reminder to clean them up and an incentive to keep the repository warning free.
Screenshots
Because annotations disappear once they're fixed, I've taken screenshots of what they look like. I've also included screenshots of the differences in the log output due to
concurrentlyremoving the colors in the output.JavaScript Annotations
CSS Annotations
<img width="1204" height="987" alt="inline-css-annotations" src="https://github.com/user-attachments/assets/07fb3261-3802-4427-a69a-d5fb91d892
Log Output Before
Log Output After