Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JaCoCo for Code Coverage Reports in Pass Support #142

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

tsande16
Copy link
Contributor

@tsande16 tsande16 commented Dec 19, 2024

This PR will add JaCoCo (Java Code Coverage) to the pass-support project. It is mostly the default configuration for JaCoCo. The reports are aggregated into the jacoco-aggregate-report module. In my research this is the approach to take when setting up an aggregate report to be consumed by a report service like SonarQube.

  • Report can be found in target/site/jacoco/index.html
    • Reports on individual classes are found in subfolders such as target/site/jacoco/org.eclipse.pass.support.client etc. These can be nagivated to by using the index.html
  • I removed the coverage ratio so it wouldn't fail the build as we determined in our last status meeting, the first iteration will be just to view the reports and then determine what type of metrics to fail a build.

Other important notes: I had to update the dockerfile: pass-journal-loader/pass-journal-loader-nih/Dockerfile. I couldn't get it to work any other way locally, however I'm not sure if this is the best approach.

To generate the reports:

  • run mvn verify

@tsande16 tsande16 changed the title Test Validation Checks Add JaCoCo for Code Coverage Reports Dec 19, 2024
@tsande16 tsande16 linked an issue Dec 21, 2024 that may be closed by this pull request
@tsande16 tsande16 changed the title Add JaCoCo for Code Coverage Reports Add JaCoCo for Code Coverage Reports in Pass Support Dec 21, 2024
@tsande16 tsande16 marked this pull request as ready for review January 7, 2025 21:46
@tsande16 tsande16 self-assigned this Jan 7, 2025
@@ -2,12 +2,13 @@ FROM eclipse-temurin:17.0.13_11-jre-jammy

WORKDIR /app

COPY target/pass-journal-loader-nih-*-exec.jar pass-journal-loader-nih-exec.jar
COPY target/ /app/
Copy link
Contributor

@markpatton markpatton Jan 9, 2025

Choose a reason for hiding this comment

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

It would be better not to copy all of target into the image and instead just pick out what you need.

Copy link
Contributor

@markpatton markpatton Jan 9, 2025

Choose a reason for hiding this comment

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

Maybe that copy glob pattern doesn't work cross-platform? One solution would be to update the pom such that the executable jar which is produced has a set name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this change was made for this PR. Maybe a mistaken commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the comment in the description. I will work with Tim to troubleshoot. It must have been something strange in Tim's docker env. The other modules have the same pattern of copying. The * is used so we can leave the version in the exec jar file (there will only be one file that matches in target).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm running the latest docker which was recently update on 1/9 prior to me making this change. I tested running mvn verify on the pass-journal-loader module and it passed with the original docker statements. I'm now testing mvn verify on pass-support altogether.

Side note: Docker for Windows apparently just has intermittent problems. On some versions I never experience issues and then it will update and I'm plagued with a whole variety of problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rpoet-jh @markpatton Reverted back to the previous version. I tested a couple times and it's now passing.

@tsande16 tsande16 requested a review from markpatton January 13, 2025 19:46
@@ -182,6 +182,21 @@
<groupId>org.cyclonedx</groupId>
<artifactId>cyclonedx-maven-plugin</artifactId>
</plugin>

<plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this repeats in all modules, did you try defining this in a pluginManagement element in pass-support/pom.xml and then defining the plugin in the child modules (exception would be jacoco-aggregate-report which would remain as is)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I haven't, I can give this a try and see if it works!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very cool, this worked! Nice find!

Copy link
Contributor

@markpatton markpatton left a comment

Choose a reason for hiding this comment

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

Tests pass for me locally. I agree with Russ about checking if the pom changes can be consolidated.

@@ -91,7 +93,31 @@
</repositories>

<build>
<pluginManagement>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this pom looks like it requires the plugin declaration in the plugins element too (line 109), you can remove the pluginManagement element and put the <version>${jacoco-maven-plugin.version}</version> below in the plugins section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this 4 different ways, and here were the results:

With only the plugin without the agent in the root POM

image

<plugin>
  <groupId>org.jacoco</groupId>
  <artifactId>jacoco-maven-plugin</artifactId>
  <version>${jacoco-maven-plugin.version}</version>
</plugin>

With only the plugin in the pluginManagement tag in the root POM

image

<pluginManagement>
  <plugins>
    <plugin>
      <groupId>org.jacoco</groupId>
      <artifactId>jacoco-maven-plugin</artifactId>
      <version>${jacoco-maven-plugin.version}</version>
    </plugin>
  </plugins>
</pluginManagement>

Plugin with agent only in the root POM

Test now....


With pluginManagement and the agent in plugin tags in the root POM

image

<build>
  <pluginManagement>
    <plugins>
      <plugin>
        <groupId>org.jacoco</groupId>
        <artifactId>jacoco-maven-plugin</artifactId>
        <version>${jacoco-maven-plugin.version}</version>
      </plugin>
    </plugins>
  </pluginManagement>

  <plugins>
    <plugin>
      <groupId>org.jacoco</groupId>
      <artifactId>jacoco-maven-plugin</artifactId>
      <executions>
        <execution>
          <id>prepare-agent</id>
          <goals>
            <goal>prepare-agent</goal>
          </goals>
        </execution>
      </executions>
    </plugin>

@@ -182,6 +182,7 @@
<groupId>org.cyclonedx</groupId>
<artifactId>cyclonedx-maven-plugin</artifactId>
</plugin>

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove these line feeds in the children pom.xml file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

<plugin>
<groupId>org.jacoco</groupId>
<artifactId>jacoco-maven-plugin</artifactId>
<version>${jacoco-maven-plugin.version}</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the version element here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

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.

Setup JaCoCo in PASS Back-end Projects
3 participants