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

build: Code Coverage for Nightly Build Workflows #6635

Merged
merged 12 commits into from
Feb 13, 2025

Conversation

stanbrub
Copy link
Contributor

Added java coverage to the Nightly Check CI according to the following contract:

  • Coverage only runs on JVM 21
  • Coverage runs on a schedule for any main repo branch
  • Coverage runs On Push on any branch (including forks) that starts with "coverage/"
  • Coverage is collected from separate jobs into a combined report uploaded as an Github artifact

@stanbrub stanbrub self-assigned this Feb 10, 2025
@stanbrub stanbrub changed the title Coverage workflow 2 build: Coverage workflow 2 Feb 10, 2025
@stanbrub stanbrub changed the title build: Coverage workflow 2 build: Coverage for Nightly Builds Feb 10, 2025
@stanbrub stanbrub changed the title build: Coverage for Nightly Builds build: Code Coverage for Nightly Build Workflows Feb 10, 2025
Comment on lines 168 to 169
./gradlew --scan --continue -Pcoverage.enabled=true jacocoTestReport
./gradlew --scan --continue -Pcoverage.enabled=true coverage-merge
Copy link
Member

Choose a reason for hiding this comment

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

I don't think --scan nor --continue are very relevant.

We use --continue in the case of testing so that the relevant tasks still run if some test task fails, in this case we should probably fail ASAP any of the tasks fail.

--scan is mostly relevant for debugging gradle build / configuration issues, which we can probably get from the --scan on the runs themselves.

How long do each of these commands take?

Copy link
Member

Choose a reason for hiding this comment

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

-Pcoverage.enabled does not matter, right? That only effects whether the Test JVMs create the .exec or not, and we are not running any tests 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.

Even though jacocoTestReport does not run the tests again, it does trigger a compileJava. I really don't know how the "classes" are used on the reporting side, but there is a setting in the jacoco-merge that defines classDirectories.

The other thing to note is that "coverage.enabled" isn't just about Java. There will be other coverage like Python, Go, et al. It seemed prudent to leave it in there until we get coverage of other languages working.

The combined-coverage-report job takes less than 5 minutes.

.github/workflows/nightly-check-ci.yml Outdated Show resolved Hide resolved

combined-coverage-report:
if: ${{ github.event_name == 'schedule' || startsWith(github.ref_name, 'coverage/') }}
needs: nightly
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this properly requires all the matrix steps in the nightly job to finish. What happens if one of those steps fails? I'm assuming that means this job does not run? (Which is probably what we want.)

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 tested by causing a failing step in exactly one matrix job. The combined-coverage-report was then skipped, which is the expected behavior.

@stanbrub stanbrub requested a review from devinrsmith February 11, 2025 03:07
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

Was this tested out in CI? Can you link to the resulting workflows / artifacts?

I tried something simple locally along the lines of

./gradlew -Pcoverage.enabled=true jacocoTestReport
./gradlew -Pcoverage.enabled=true coverage:coverage-merge

I didn't have many tests that were run w/ coverage.enabled, so I only expected that a few .exec files would be picked up and aggregated. I had to kill the coverage-merge job after a few minutes... I would have thought it would be relatively simple?

Turns of that it also create >30GB of data in the coverage/build directory. Is this expected? I expect this would cause breakage of CI since it's only supposed to have 14GB.

@stanbrub
Copy link
Contributor Author

stanbrub commented Feb 12, 2025

Was this tested out in CI? Can you link to the resulting workflows / artifacts?

I tried something simple locally along the lines of

./gradlew -Pcoverage.enabled=true jacocoTestReport
./gradlew -Pcoverage.enabled=true coverage:coverage-merge

I didn't have many tests that were run w/ coverage.enabled, so I only expected that a few .exec files would be picked up and aggregated. I had to kill the coverage-merge job after a few minutes... I would have thought it would be relatively simple?

Turns of that it also create >30GB of data in the coverage/build directory. Is this expected? I expect this would cause breakage of CI since it's only supposed to have 14GB.

The issue you were experiencing is fixed in this PR. I discovered it while working on the workflow, since it does not happen on my local system.

Here is a recent Nightly Check CI run from my fork: https://github.com/stanbrub/deephaven-core/actions/runs/13255990731. I ran with coverage/ (collect coverage) and with nightly/ (do not collect coverage). I also caused errors to make sure the combined job was skipped on those.

@stanbrub stanbrub requested a review from devinrsmith February 12, 2025 19:43
@stanbrub stanbrub merged commit 953c91e into deephaven:main Feb 13, 2025
17 checks passed
@stanbrub stanbrub deleted the coverage-workflow-2 branch February 13, 2025 19:38
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants