Skip to content

fix: decouple pmd, spotbugs, and checkstyle skip flags from skiptests - #1817

Open
aws-kevinrickard wants to merge 1 commit into
mainfrom
fix/decouple-static-analysis-skip-flags
Open

fix: decouple pmd, spotbugs, and checkstyle skip flags from skiptests#1817
aws-kevinrickard wants to merge 1 commit into
mainfrom
fix/decouple-static-analysis-skip-flags

Conversation

@aws-kevinrickard

Copy link
Copy Markdown
Member

Problem

maven-pmd-plugin, spotbugs-maven-plugin, and maven-checkstyle-plugin all had their <configuration><skip> wired to the single ${skipTests} property instead of their own idiomatic skip parameters. Concretely, in pom.xml:

<!-- maven-pmd-plugin -->
<skip>${skipTests}</skip>

<!-- spotbugs-maven-plugin -->
<skip>${skipTests}</skip>

<!-- maven-checkstyle-plugin -->
<skip>${skipTests}</skip>

This causes two problems:

  1. Each plugin's own documented skip flag silently does nothing. For example, -Dspotbugs.skip=true is the SpotBugs Maven plugin's own parameter, but since the POM never reads spotbugs.skip, passing it has zero effect — the analysis still runs. Same for -Dpmd.skip=true and -Dcheckstyle.skip=true.
  2. -DskipTests=true conflates two different intents: "skip running the test suite" and "skip static analysis." Passing it skips everything (tests + PMD + SpotBugs + Checkstyle), which is surprising if you only wanted to skip one of those.

Fix

Each plugin now reads its own skip property, added to <properties> with the same default (false) skipTests already had:

<pmd.skip>false</pmd.skip>
<spotbugs.skip>false</spotbugs.skip>
<checkstyle.skip>false</checkstyle.skip>

And each plugin's <configuration><skip> now points at its own property instead of ${skipTests}.

skipTests is untouched everywhere else (surefire, JaCoCo) — it continues to control only test execution, as before.

Behavior after this change

  • Default mvn test (no extra flags): identical to before — PMD, SpotBugs, Checkstyle, and the test suite all run, since the new properties default to false just like skipTests did.
  • -Dpmd.skip=true: now actually skips PMD, while Checkstyle/SpotBugs/tests still run.
  • -Dcheckstyle.skip=true: now actually skips Checkstyle, while PMD/SpotBugs/tests still run.
  • -Dspotbugs.skip=true: now actually skips SpotBugs, while PMD/Checkstyle/tests still run.
  • -DskipTests=true: still skips test execution (unchanged), but no longer skips PMD/Checkstyle/SpotBugs — each of those needs its own explicit flag now. This is the corrected, intended behavior: previously -DskipTests=true skipped everything, which was itself the confusing overload this PR removes.

Testing

Verified locally via full mvn test lifecycle runs:

  • Default run (no flags): PMD and Checkstyle execute and pass, matching pre-change behavior.
  • -Dpmd.skip=true: PMD produces no report / does not run (confirmed via missing target/pmd.xml and near-instant mojo execution time vs. the multi-second scan when enabled); Checkstyle still runs.
  • -Dcheckstyle.skip=true: Checkstyle mojo execution drops from ~4s to ~1s (confirming it no-ops) while PMD still runs and produces its report.
  • -DskipTests=true alone: test execution is skipped (Tests are skipped.), while PMD and SpotBugs still execute in the test phase as before.
  • Confirmed no regression to default behavior and no changes to the spotbugs-maven-plugin <version>, JaCoCo, or surefire configuration.

Note: this environment's JDK (11/17/21) triggers a known, pre-existing, unrelated SpotBugs 4.0.0 crash (Unsupported class file major version 61) when actually running the SpotBugs analysis — reproduced identically on unmodified main before this change, so it isn't something introduced here. -Dspotbugs.skip=true was used to work around it when verifying the other flags, and is itself one of the flags this PR fixes.

Each plugin's <skip> configuration was wired to the single ${skipTests}
property instead of its own idiomatic parameter. This meant passing a
plugin's documented skip flag (e.g. -Dspotbugs.skip=true) silently had
no effect, and -DskipTests=true skipped all static analysis in addition
to tests, conflating two different intents.

Each plugin now honors its own skip property, defaulting to false so
default behavior (mvn test with no flags) is unchanged:
- maven-pmd-plugin now reads pmd.skip
- spotbugs-maven-plugin now reads spotbugs.skip
- maven-checkstyle-plugin now reads checkstyle.skip

skipTests continues to control only test execution (surefire/jacoco),
as before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants