MOSIP-44120 Corrected surefire and jacoco plugin #276
Conversation
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
WalkthroughAdds JaCoCo agent wiring and adjusts plugin lifecycles across three POMs: sets a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as Developer/CI
participant Maven as Maven Build
participant Jacoco as JaCoCo Plugin
participant Surefire as Surefire Plugin
participant Report as JaCoCo Report
CI->>Maven: run lifecycle (test → verify)
Maven->>Jacoco: execute prepare-agent (set `jacocoArgLine`)
Jacoco-->>Maven: expose `${jacocoArgLine}`
Maven->>Surefire: run tests with `${jacocoArgLine}`\n(includes --add-opens, --enable-preview, agent attached)
Surefire-->>Maven: tests complete
Maven->>Report: run jacoco:report (phase: verify)
Report-->>Maven: generate coverage report
Maven-->>CI: build finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
commons-packet/pom.xml (2)
26-26: Duplicate property for Jacoco version.
maven.jacoco.version(line 26) andjacoco.maven.plugin.version(line 37) both define the same value0.8.11. Consider consolidating to a single property name for clarity and maintainability. Additionally, child modules (commons-packet-managerandcommons-packet-service) also define their ownjacoco.maven.plugin.version, which is redundant since they inherit from this parent.Suggested consolidation
Remove the duplicate property and use a single consistent name:
- <maven.jacoco.version>0.8.11</maven.jacoco.version> <maven.sonar.plugin.version>3.7.0.1746</maven.sonar.plugin.version> <maven.javadoc.version>3.2.0</maven.javadoc.version> <maven-shade-plugin.version>2.3</maven-shade-plugin.version> <maven.deploy.plugin.version>2.8.1</maven.deploy.plugin.version> <maven.source.plugin.version>2.2.1</maven.source.plugin.version> <maven.gpg.plugin.version>1.5</maven.gpg.plugin.version> <central.publishing.maven.plugin.version>0.7.0</central.publishing.maven.plugin.version> <git.commit.plugin.version>3.0.1</git.commit.plugin.version> <sonar.coverage.exclusions>...</sonar.coverage.exclusions> <sonar.cpd.exclusions>**/dto/**,**/entity/**,**/config/**</sonar.cpd.exclusions> <jacoco.maven.plugin.version>0.8.11</jacoco.maven.plugin.version>Child modules can then remove their local
jacoco.maven.plugin.versiondefinitions since they inherit from this parent.Also applies to: 37-37
63-79: Parent surefire configuration will be overridden by child modules.The child modules (
commons-packet-managerandcommons-packet-service) each define their own completemaven-surefire-pluginconfiguration. Since they fully re-declare the plugin, this parent configuration is effectively unused for those modules. This is not a bug but creates maintenance overhead—changes need to be made in three places.Consider either:
- Remove surefire config from child poms and rely on parent inheritance, or
- Keep only in child poms and remove from parent (if modules truly need different configs)
commons-packet/commons-packet-service/pom.xml (1)
28-29: Upgrade maven-surefire-plugin to a 3.x version for improved Java 21 support.Version 2.22.0 from 2018 lacks modern Java version support improvements. Version 3.2.5 is recommended as a stable upgrade target, with 3.5.4 available as the current version.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
commons-packet/commons-packet-manager/pom.xmlcommons-packet/commons-packet-service/pom.xmlcommons-packet/pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-packet-manager / maven-build
🔇 Additional comments (5)
commons-packet/commons-packet-service/pom.xml (2)
261-277: Surefire configuration looks correct for Jacoco integration.The
${jacocoArgLine}placeholder is properly included before the JVM arguments, which will be populated by Jacoco'sprepare-agentgoal. The--add-opensflags address Java module encapsulation for testing.Note:
<skipTests>false</skipTests>and<skip>false</skip>are redundant (both default to false), but harmless.
283-303: Jacoco plugin configuration is correct.The
prepare-agentexecution properly populatesjacocoArgLinewhich is referenced in surefire'sargLine. Moving the report phase toverifyensures coverage data is collected after tests complete.commons-packet/commons-packet-manager/pom.xml (2)
166-182: Surefire and Jacoco integration is correctly configured.The configuration mirrors the service module with proper
${jacocoArgLine}placeholder and JVM arguments for Java 21 module access. Same note about surefire version 2.22.0 potentially being outdated for Java 21.
272-292: Jacoco configuration is consistent with the service module.The
prepare-agentandreportexecutions follow the same correct pattern.commons-packet/pom.xml (1)
80-107: Jacoco configuration is correct.The
prepare-agentexecution properly maps tojacocoArgLine, and movingreportto theverifyphase ensures tests complete before report generation.
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @commons-packet/pom.xml:
- Around line 69-76: Add a default empty property for jacocoArgLine in the POM
properties (define <jacocoArgLine></jacocoArgLine>) so ${jacocoArgLine} is never
undefined, and update the maven-compiler-plugin configuration (the plugin block
that defines compilerArgs / forked JVM args) to include --enable-preview
alongside test argLine so compilation uses the same preview flag as the
surefire/failsafe test execution.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commons-packet/pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-packet-manager / maven-build
🔇 Additional comments (2)
commons-packet/pom.xml (2)
84-104: JaCoCo configuration is correctly integrated.The
prepare-agentexecution withpropertyNameproperly exposes the agent arguments to Surefire, and moving the report generation to theverifyphase ensures coverage data is collected after tests complete. This is the recommended approach for JaCoCo + Surefire integration.
108-124: LGTM!The
maven-jar-pluginconfiguration correctly adds manifest metadata including classpath, default implementation entries, and project-specific attributes.
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @commons-packet/pom.xml:
- Line 68: Remove the hardcoded <skipTests>false</skipTests> element so Maven
respects the -DskipTests CLI property; alternatively replace it with a property
reference (e.g., use the skipTests Maven property like ${skipTests}) or remove
the element entirely to rely on Maven defaults, ensuring the <skipTests> element
is not hardcoded to "false".
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commons-packet/pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-packet-manager / maven-build
🔇 Additional comments (3)
commons-packet/pom.xml (3)
37-37: LGTM!The empty default for
jacocoArgLineprevents build failures when JaCoCo is skipped or disabled.
150-152: LGTM!The
--enable-previewflag correctly enables Java preview features for compilation, ensuring consistency with the Surefire test configuration.
98-104: This review comment is incorrect. Thereportgoal is the appropriate choice for this multi-module structure. Both child modules (commons-packet-managerandcommons-packet-service) have identical JaCoCo configurations using thereportgoal, confirming this is the intended design pattern. Each module generates independent coverage reports during the verify phase; no aggregation across modules is configured or needed in this codebase.Likely an incorrect or invalid review comment.
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @commons-packet/pom.xml:
- Line 69: Remove the hardcoded <skip>false</skip> element so command-line
test-skipping works; either delete the <skip> element entirely or replace its
value with a Maven property (e.g. <skip>${maven.test.skip}</skip> or
<skip>${skipTests}</skip>) so users can control test execution with
-Dmaven.test.skip=true or -DskipTests=true; update the plugin configuration that
contains the <skip> element accordingly (look for the <skip> tag in the plugin
configuration).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
commons-packet/pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-packet-manager / maven-build
🔇 Additional comments (4)
commons-packet/pom.xml (4)
37-37: LGTM!The empty default for
jacocoArgLineensures the build won't break if JaCoCo'sprepare-agentgoal is skipped.
70-77: LGTM!The argLine correctly integrates
${jacocoArgLine}with the necessary--add-opensflags for Java 21 module access and maintains consistency with the compiler's--enable-previewflag.
85-105: LGTM!The JaCoCo configuration is correct:
prepare-agentproperly binds the agent tojacocoArgLinefor Surefire integrationreportphase moved toverifyensures coverage data is collected after tests complete
150-152: LGTM!Adding
--enable-previewto the compiler ensures consistency between compilation and test execution, as addressed from the previous review.
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
Signed-off-by: kameshsr <kameshsr1338@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @commons-packet/commons-packet-service/pom.xml:
- Around line 258-276: The jacoco-maven-plugin is configured with the
prepare-agent goal but the Surefire test runner is not configured to use the
generated agent args, so tests won't be instrumented; update the
jacoco-maven-plugin prepare-agent execution to set a property name (e.g.,
jacocoArgLine) and then add a maven-surefire-plugin entry that sets its
<argLine> to reference ${jacocoArgLine} (and include any required JVM flags like
--add-opens/--enable-preview if needed), ensuring the prepare-agent populates
the property and Surefire uses it during test execution.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
commons-packet/commons-packet-service/pom.xmlcommons-packet/pom.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- commons-packet/pom.xml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-packet-manager / maven-build
🔇 Additional comments (4)
commons-packet/commons-packet-service/pom.xml (4)
160-232: LGTM! Standard Maven Central publishing configuration.The maven-deploy-plugin, central-publishing-maven-plugin, maven-source-plugin, maven-javadoc-plugin, and maven-gpg-plugin are configured correctly for publishing to Maven Central/OSSRH. The
autoPublish=falsesetting provides a safety check by requiring manual promotion.
233-257: LGTM! Git metadata plugin configured appropriately.The git-commit-id-plugin will generate build metadata in the validate phase with sensible property filtering.
279-298: LGTM! Project metadata sections added.The SCM, license, and developer information sections are correctly configured for Maven Central publishing requirements.
270-270: The technical reasoning in this review is incorrect. According to the Maven lifecycle, thetestphase completes before theprepare-packagephase begins. Coverage data from tests is available duringprepare-package, so the claim that this phase may generate reports "before all tests have run" is unfounded.That said, there is inconsistency in this repository:
commons-packet/commons-packet-manager/pom.xmluses theverifyphase with a comment indicating this was "fixed," while other modules useprepare-package. Whilejacoco:report(report generation) is valid in either phase per JaCoCo documentation, aligning all modules to the same phase would improve consistency.Consider either: (1) keeping
prepare-packageand removing the inconsistentverifyusage in commons-packet-manager, or (2) standardizing all modules toverifyphase for consistency with commons-packet-manager.Likely an incorrect or invalid review comment.
| <plugin> | ||
| <groupId>org.jacoco</groupId> | ||
| <artifactId>jacoco-maven-plugin</artifactId> | ||
| <version>${jacoco.maven.plugin.version}</version> | ||
| <executions> | ||
| <execution> | ||
| <goals> | ||
| <goal>prepare-agent</goal> | ||
| </goals> | ||
| </execution> | ||
| <execution> | ||
| <id>report</id> | ||
| <phase>prepare-package</phase> | ||
| <goals> | ||
| <goal>report</goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
Critical: JaCoCo agent won't be attached without Surefire plugin configuration.
The jacoco-maven-plugin's prepare-agent goal is configured, but there's no maven-surefire-plugin configuration to reference the JaCoCo agent property in its argLine. Without this, the JaCoCo agent won't instrument test execution, and no coverage data will be collected.
🔎 Required Surefire plugin configuration
Add the maven-surefire-plugin after the jacoco-maven-plugin (around line 277):
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<argLine>${jacocoArgLine} --add-opens java.base/java.lang=ALL-UNNAMED --enable-preview</argLine>
</configuration>
</plugin>Additionally, update the jacoco-maven-plugin's prepare-agent execution to explicitly set the property name:
<execution>
+ <configuration>
+ <propertyName>jacocoArgLine</propertyName>
+ </configuration>
<goals>
<goal>prepare-agent</goal>
</goals>
</execution>This ensures the JaCoCo agent arguments are stored in ${jacocoArgLine} and then referenced by Surefire.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <plugin> | |
| <groupId>org.jacoco</groupId> | |
| <artifactId>jacoco-maven-plugin</artifactId> | |
| <version>${jacoco.maven.plugin.version}</version> | |
| <executions> | |
| <execution> | |
| <goals> | |
| <goal>prepare-agent</goal> | |
| </goals> | |
| </execution> | |
| <execution> | |
| <id>report</id> | |
| <phase>prepare-package</phase> | |
| <goals> | |
| <goal>report</goal> | |
| </goals> | |
| </execution> | |
| </executions> | |
| </plugin> | |
| <plugin> | |
| <groupId>org.jacoco</groupId> | |
| <artifactId>jacoco-maven-plugin</artifactId> | |
| <version>${jacoco.maven.plugin.version}</version> | |
| <executions> | |
| <execution> | |
| <configuration> | |
| <propertyName>jacocoArgLine</propertyName> | |
| </configuration> | |
| <goals> | |
| <goal>prepare-agent</goal> | |
| </goals> | |
| </execution> | |
| <execution> | |
| <id>report</id> | |
| <phase>prepare-package</phase> | |
| <goals> | |
| <goal>report</goal> | |
| </goals> | |
| </execution> | |
| </executions> | |
| </plugin> |
🤖 Prompt for AI Agents
In @commons-packet/commons-packet-service/pom.xml around lines 258-276, The
jacoco-maven-plugin is configured with the prepare-agent goal but the Surefire
test runner is not configured to use the generated agent args, so tests won't be
instrumented; update the jacoco-maven-plugin prepare-agent execution to set a
property name (e.g., jacocoArgLine) and then add a maven-surefire-plugin entry
that sets its <argLine> to reference ${jacocoArgLine} (and include any required
JVM flags like --add-opens/--enable-preview if needed), ensuring the
prepare-agent populates the property and Surefire uses it during test execution.
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.