CI: Run the code quality checks once, in the job that is meant to run them - #211
Open
MarkusPaulsen wants to merge 1 commit into
Open
CI: Run the code quality checks once, in the job that is meant to run them#211MarkusPaulsen wants to merge 1 commit into
MarkusPaulsen wants to merge 1 commit into
Conversation
Checkstyle, PMD, CPD and SpotBugs are bound to the package phase, so every command that reaches it runs them. Six jobs per pull request did: build, static analysis, CodeQL, both example jobs and the coverage report. Only the second was meant to. A new profile, without-quality-gates, turns the whole gate off for one invocation. The four jobs whose purpose is something else activate it, so Build answers only whether the project builds and Static Code Analysis answers whether the code is written the way this repository writes code. The profile is not active by default, so a local build still crosses the gate. The build job loses its Spotless step for the same reason: formatting is the analysis job's question. Spotless keeps no lifecycle binding of its own, so nothing changes for a local build or for the release path. Static Code Analysis was missing CPD. It runs pmd:check, which does not include cpd-check, so duplicated code was found only as a side effect of the package-bound execution the other jobs happened to run. Switching those off would have removed that check without a single test turning red. The step is added and cpd.xml is uploaded with the other three reports. Measured on JDK 17 from a clean target: mvn clean package -DskipTests takes 30.5s and writes all four report files, with the profile it takes 18.6s and writes none. In CI that is roughly 30 seconds off Build and off Coverage Report, both on the critical path, 30 off CodeQL and 25 off each example job. Two comments were wrong and are corrected. The one above the compile step claimed only SpotBugs needs compiled classes; PMD needs them too, and without them reports 18 unused wildcard imports it cannot resolve. The one above the analysis plugins ran to 890 characters, well past the 500 this repository allows.
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Summary
Ares checks its own code with four tools: Checkstyle, PMD, CPD and SpotBugs. They are
wired into the Maven build, so six CI jobs ran them when only one was meant to. A Maven
profile now switches them off for the jobs whose purpose is something else, so the build
job only builds and the analysis job only checks. The analysis job also gains the
duplicate-code check, which it never actually ran.
Linked issues
No linked issues
1. Problem
Nothing in Ares itself is broken, and no security part of Ares is at fault. This is about
how the project checks its own code.
Checkstyle, PMD, CPD and SpotBugs are attached to the Maven
packagephase, so anycommand that packages the project runs them. Six jobs per pull request did: Build, Static
Code Analysis, CodeQL, both Examples jobs and Coverage Report. Five of them paid for a
check that was not their job, about two and a half minutes of runner time per pull
request, and nobody reading the workflow files could tell which job checks what.
Worse, Static Code Analysis never ran the duplicate-code check. It calls
pmd:check,which is a different goal from
pmd:cpd-check. Duplicated code was found only byaccident, in the jobs that happened to package the project. Switching those off, which is
what this pull request does, would otherwise have removed that check with nothing turning
red.
2. Improvement from the user's perspective
No Improvement from the user's perspective
3. Improvement from the maintainer's perspective
Each job now answers one question. Build answers whether the project builds, Static Code
Analysis answers whether the code is written the way this repository writes code, and the
workflow files say which is which.
The duplicate-code check runs on purpose rather than by accident, and its report is
uploaded with the other three, so a reviewer can read it.
Measured against the same jobs on
main: Build drops from 1m32s to 54s and CoverageReport from 1m31s to 1m02s, both on the critical path, so the pipeline is about 67
seconds shorter. CodeQL drops from 3m30s to 2m48s, and the install
step of the two example jobs by 21 and 11 seconds. Static Code Analysis gains 5 seconds
for the CPD step it was missing. Runner time falls by about two minutes per pull request.
The list of checks lives in one place in
pom.xml. A job that does not want them says sowith one flag rather than five, and a check added later cannot creep back into those jobs
unnoticed.
4. Testing manual
Prerequisites
is enough.
newer, and this branch checked out. The change is build-tool independent on the
exercise side, because it touches only how Ares itself is built, never how an exercise
consumes it.
Steps
Not reproducible from an exercise. This changes the CI setup of Ares itself, so a reviewer
verifies it from this pull request's own workflow runs.
Read the latest attempt of each run. The first attempt of every run on this branch had an
empty dependency cache, because the cache key is a hash of
pom.xmland this pull requestchanges it, so its timings say nothing about the change.
step after the setup, called "Build", and no "Spotless Check" step. Search the log for
checkstyle:,pmd:andspotbugs:and expect no match, because the job now onlybuilds.
Checkstyle, PMD, CPD and SpotBugs. Open "CPD" and expect a line reading
--- pmd:3.28.0:cpd-check.checkstyle-result.xml,pmd.xml,cpd.xmlandspotbugsXml.xml. The third is thenew one.
spotbugs:andexpect no match, then open the job summary and confirm the "Aggregated code coverage"
table is still there.
run and its "Install Ares from this commit" step. Search both for
spotbugs:andexpect no match in either.
Expected result
Every job named above is green. The quality checks appear in exactly one job, Static Code
Analysis, and that job now reports five tools where it reported four. Nothing that was
checked before this pull request is unchecked after it.
Negative case (what must still be rejected)
This part needs a terminal, and it is the part worth doing, because the risk of this
change is a check that quietly stops running.
from, then run
mvn pmd:cpd-check. Expect it to fail and to name the duplication. Thisis the check that had no step of its own before this pull request.
line with spaces, then run
mvn spotless:check. Expect it to fail.mvn clean package -DskipTests -Pwithout-quality-gates. Expect it to succeed: theprofile switches the style checks off, and it must not hide anything else.
mvn clean package -DskipTestswithout the profile. Expect it to fail. That iswhat keeps a local build honest, and it is why the profile is not active by default.
Optional extra, the measurement behind section 3: from a clean checkout run
mvn clean package -DskipTestsfollowed byls target/*.xml, expecting the four reportfiles, then
mvn clean package -DskipTests -Pwithout-quality-gatesfollowed byls target/*.xml, expecting none and a noticeably shorter build. Locally on a JDK 17 thiswas 30.5 seconds against 18.6 seconds.
Modes exercised
No mode-specific behaviour changed.
5. Test case coverage regarding this PR
No production Java code changed.
Breaking changes and migration
No breaking changes or migration.
Checklist
Review progress