Skip to content

CI testing coverage report and implementation with test summary#4

Closed
PeriodicallyZoneOut wants to merge 6 commits into
mainfrom
feature/ci-pipeline
Closed

CI testing coverage report and implementation with test summary#4
PeriodicallyZoneOut wants to merge 6 commits into
mainfrom
feature/ci-pipeline

Conversation

@PeriodicallyZoneOut

Copy link
Copy Markdown
Owner

CI testing coverage and report implementation

Copilot AI review requested due to automatic review settings April 30, 2026 07:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds CI test coverage reporting and a consolidated JUnit test summary to the repository’s GitHub Actions pipeline, plus enables JaCoCo report generation via Maven configuration.

Changes:

  • Add JaCoCo Maven plugin configuration to generate coverage reports.
  • Extend .github/workflows/ci-pipeline.yml to generate/upload JaCoCo reports, enforce a coverage threshold, and publish a unified JUnit test summary.
  • Add cart/mvnw (Maven wrapper shell script) for consistency with other modules.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.

File Description
pom.xml Adds JaCoCo plugin config in the root build section.
cart/mvnw Adds Maven wrapper shell script for the cart module.
.github/workflows/ci-pipeline.yml Generates JaCoCo reports, enforces coverage threshold, uploads artifacts, and publishes a cross-module JUnit summary.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +291 to +303
- name: Download test result artifacts
uses: actions/download-artifact@v4
with:
pattern: test-results-*
path: artifacts/test-results
merge-multiple: true

- name: Publish JUnit test report
uses: dorny/test-reporter@v1
with:
name: Unit Tests
path: artifacts/test-results/**/*.xml
reporter: java-junit

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This job can fail when a selected module has no JUnit XML reports (some modules in the reactor don't have src/test, so upload-artifact with if-no-files-found: ignore will skip creating test-results-* artifacts). In that case, actions/download-artifact (pattern test-results-*) and/or dorny/test-reporter can error due to missing/empty inputs, making the whole workflow fail even though builds/tests were successful. Consider making the download/report steps tolerant of missing artifacts (e.g., ignore missing artifacts and set the reporter to not fail on empty), or ensure a placeholder report file is always uploaded.

Copilot uses AI. Check for mistakes.
contents: read
actions: write
statuses: write
checks: write

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checks: write is granted at the workflow level, which elevates the GITHUB_TOKEN for all jobs/steps. Since only the JUnit publishing step needs this, consider scoping checks: write to just the test-summary job (job-level permissions:) to follow least-privilege and reduce the blast radius if a later step is compromised.

Suggested change
checks: write

Copilot uses AI. Check for mistakes.
Comment thread pom.xml
Comment on lines +424 to +446
<version>${jacoco-maven-plugin.version}</version>
<executions>
<execution>
<goals>
<goal>prepare-agent</goal>
</goals>
</execution>
<execution>
<id>report</id>
<phase>verify</phase>
<goals>
<goal>report</goal>
</goals>
</execution>
</executions>
<configuration>
<excludes>
<exclude>com/yas/**/*Application.class</exclude>
<exclude>com/yas/**/config/**</exclude>
<exclude>com/yas/**/exception/**</exclude>
<exclude>com/yas/**/constants/**</exclude>
</excludes>
</configuration>

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jacoco-maven-plugin is already defined (with the same version/executions/config) under <build><pluginManagement> earlier in this POM. Duplicating the full plugin configuration again under <build><plugins> is likely to cause redundant/duplicate executions in the parent (packaging pom) and can also lead to confusing/incorrect agent configuration in multi-module builds. Prefer keeping the configuration only in pluginManagement and, if you need it active in the parent, add a minimal <plugin> entry here that relies on pluginManagement, or remove this duplicate entirely and keep activation at the module level (each module already declares the plugin).

Suggested change
<version>${jacoco-maven-plugin.version}</version>
<executions>
<execution>
<goals>
<goal>prepare-agent</goal>
</goals>
</execution>
<execution>
<id>report</id>
<phase>verify</phase>
<goals>
<goal>report</goal>
</goals>
</execution>
</executions>
<configuration>
<excludes>
<exclude>com/yas/**/*Application.class</exclude>
<exclude>com/yas/**/config/**</exclude>
<exclude>com/yas/**/exception/**</exclude>
<exclude>com/yas/**/constants/**</exclude>
</excludes>
</configuration>

Copilot uses AI. Check for mistakes.
Comment on lines +166 to +167
echo "Attempt $attempt: mvn jacoco:report (${{ matrix.service }})"
if mvn -B -ntp -pl ${{ matrix.service }} jacoco:report; then

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mvn ... -pl ${{ matrix.service }} jacoco:report is run without -am. For services that depend on other reactor modules (e.g., many depend on common-library), this second Maven invocation can fail dependency resolution because required modules are not built/installed in this run (the prior mvn ... test run does not install artifacts). Consider adding -am to the report invocation (or switching to a single Maven invocation like test jacoco:report) so reactor dependencies are available reliably.

Suggested change
echo "Attempt $attempt: mvn jacoco:report (${{ matrix.service }})"
if mvn -B -ntp -pl ${{ matrix.service }} jacoco:report; then
echo "Attempt $attempt: mvn -am jacoco:report (${{ matrix.service }})"
if mvn -B -ntp -pl ${{ matrix.service }} -am jacoco:report; then

Copilot uses AI. Check for mistakes.
Comment on lines +285 to +304
test-summary:
name: Test Summary
needs: [detect-changes, test]
if: always() && needs.detect-changes.outputs.has_services == 'true'
runs-on: ubuntu-latest
steps:
- name: Download test result artifacts
uses: actions/download-artifact@v4
with:
pattern: test-results-*
path: artifacts/test-results
merge-multiple: true

- name: Publish JUnit test report
uses: dorny/test-reporter@v1
with:
name: Unit Tests
path: artifacts/test-results/**/*.xml
reporter: java-junit
fail-on-error: 'false'

Copilot AI Apr 30, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test-summary job publishes a GitHub Check via dorny/test-reporter, which requires checks: write and will generally fail on pull_request events from forks (GITHUB_TOKEN is read-only). Other workflows in this repo guard such write-permission steps with FROM_ORIGINAL_REPOSITORY (e.g., .github/workflows/cart-ci.yaml uses github.event.pull_request.head.repo.full_name == github.repository). Consider adding the same guard here (or conditionally skipping only the publishing step) so fork PRs don't fail CI.

Copilot uses AI. Check for mistakes.
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