Java coverage fixes #3253
Java coverage fixes #3253
Conversation
There was a problem hiding this comment.
Copilot reviewed 2 out of 3 changed files in this pull request and generated no suggestions.
Files not reviewed (1)
- scripts/generate-java-code-coverage.sh: Language not supported
| @@ -17,18 +19,18 @@ jobs: | |||
|
|
|||
| - name: Generate C++ Coverage | |||
| working-directory: ./cpp | |||
| run: ../scripts/generate-code-coverage.sh | |||
| run: ../scripts/generate-cpp-code-coverage.sh | |||
| done | ||
|
|
||
| java -jar org.jacoco.cli-0.8.12-nodeps.jar report $(find . -name \*.jacoco.exec) $args --html coverage | ||
| ../scripts/generate-java-code-coverage.sh |
There was a problem hiding this comment.
This script does the build as well, like C++
| python3 allTests.py --all --workers=4 --debug --jacoco $(pwd)/org.jacoco.agent-runtime.jar | ||
|
|
||
| # We don't want any coverage for IceGridGUI | ||
| find src/IceGridGUI -name '*.java' -exec rm {} \; |
There was a problem hiding this comment.
Would be better if we don't need to remove files from the source tree. Specially if the script can be run locally as you can end up losing code changes.
There was a problem hiding this comment.
Could we remove the class files like we do for the generated code?
They should all live in src/IceGridGUI/build/classes.
So maybe it'd be easier to just rm -rf src/IceGridGUI/build instead of using find ... -exec rm.
There was a problem hiding this comment.
Yes, this is a bug. I was attempting to remove the class files.
| # make -C ../cpp srcs | ||
| # make |
There was a problem hiding this comment.
We need these uncommented to actually run the build, right?
| python3 allTests.py --all --workers=4 --debug --jacoco $(pwd)/org.jacoco.agent-runtime.jar | ||
|
|
||
| # We don't want any coverage for IceGridGUI | ||
| find src/IceGridGUI -name '*.java' -exec rm {} \; |
There was a problem hiding this comment.
Could we remove the class files like we do for the generated code?
They should all live in src/IceGridGUI/build/classes.
So maybe it'd be easier to just rm -rf src/IceGridGUI/build instead of using find ... -exec rm.
| # We don't want any coverage for generated code. We remove their class files and any inner classes. | ||
| jacocoArgs=() | ||
| for dir in src/*; do | ||
| [ -d "$dir" ] || continue |
There was a problem hiding this comment.
Actually, I think the most correct solution is to just continue when dir == IceGridGui or whatever.
This means we don't need to find rm anything, which would be nice.
Also, the current approach feels a little weird. find src/IceGridGUI -name '*.java' -exec rm {} \; removes all the source files, but not the class files. So, we're still passing all the IceGridGUI classes to jacoco. But, I guess it doesn't display any output for them because it doesn't know about the source files?
Feels sketchy. It'd be better to just not pass the IceGridGui directory or anything at all IMO.
This PR updates the coverage generation for Java (and adds a workflow schedule). I moved the coverage logic into a bash script because it's a little complicated and somebody might want to run it locally.