Add MoranaApps Jacoco support #71
Conversation
WalkthroughRestructures JaCoCo integration to per-module reporting via two new sbt AutoPlugins, adds JaCoCo Method Filter docs and CI workflow updates, and applies cosmetic comment touches across many Scala sources to trigger coverage evaluation. Changes
Sequence DiagramsequenceDiagram
participant Test as Test Task
participant JVM as Forked JVM
participant Agent as JaCoCo Agent
participant JMF as JMF (Method Filter)
participant Exec as Exec File
participant Reporter as Report Generator
Test->>JVM: Fork with -javaagent (per-module)
activate JVM
JVM->>Agent: Load & attach agent (module-specific)
activate Agent
Agent->>JMF: Initialize filter rules
activate JMF
JMF-->>Agent: Ready
deactivate JMF
JVM->>JVM: Execute tests (instrumented)
Note over JVM: Coverage data collected
JVM->>Exec: Write module exec file
deactivate Agent
deactivate JVM
Reporter->>Exec: Read exec file
Reporter->>JMF: Apply filtering rules
activate JMF
JMF-->>Reporter: Filtered coverage data
deactivate JMF
Reporter->>Reporter: Generate HTML/XML/CSV reports (per-module)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
miroslavpojer
changed the title
|
Note: This is prior Jacoco report action result JaCoCo 'balta' module code coverage report - scala 2.12.18
|
|
JaCoCo Coverage Report
|
miroslavpojer
changed the title
miroslavpojer
requested review from
benedeki,
lsulak,
salamonpavel and
tmikula-dev
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.github/workflows/jacoco_report.yml:
- Around line 57-69: Replace the nonexistent action MoranaApps/jacoco-report@v2
with the known public action madrapps/jacoco-report@v1.7.2 (or the specific
version you want) and align the inputs to that action's documented parameters:
use the action's "token" and "files" (instead of "paths") for the report glob,
keep the overall/changed-files coverage inputs that madrapps supports (map your
env.coverage-overall and env.coverage-changed-files accordingly), and remove
unsupported inputs such as "sensitivity", "skip-unchanged" and
"min-coverage-per-changed-file"; alternatively, if MoranaApps/jacoco-report@v2
is a private/internal action, confirm it exists in your environment and document
its supported inputs before keeping it.
In
`@balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala`:
- Line 18: Remove the test/debug artifact comment "// jacoco-touch: simulate
change" from SimpleJsonString.scala (the file/class SimpleJsonString) so
production code contains no JaCoCo simulation lines; simply delete that comment
and ensure no other test-only markers remain in the SimpleJsonString source
before merging.
In
`@balta/src/main/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNaming.scala`:
- Line 18: Remove the test artifact comment "// jacoco-touch: simulate change"
from SnakeCaseNaming.scala (and any identical occurrences in other Scala source
files); locate the SnakeCaseNaming implementation (object/class/trait named
SnakeCaseNaming) and delete that non-functional comment line, then run a quick
grep/search across the repo for "jacoco-touch: simulate change" and remove all
matches before merging.
In `@jmf-rules.txt`:
- Around line 118-135: The global patterns using wildcard receiver `*#` are too
broad (e.g., symbols `apply(*)`, `unapply(*)`, `name()`, `groups()`,
`optionalAttributes()`) and will exclude non-boilerplate business methods;
narrow their scope by replacing the global `*#` selector with package-specific
selectors (for example target model or generated packages like
`*.model.*#apply(*)` / `*.model.*#unapply(*)`) or remove them from the globals
section and re-add under PROJECT RULES with appropriate package prefixes so only
intended boilerplate methods are excluded.
In `@project/FilteredJacocoAgentPlugin.scala`:
- Line 73: Remove the duplicate lazy val Jmf = config("jmf").extend(Compile)
definition (it shadows the autoImport Jmf); delete the second definition in
FilteredJacocoAgentPlugin and instead update the original autoImport Jmf
declaration to call config("jmf").extend(Compile) so there is a single,
correctly extended Jmf symbol used throughout.
- Line 36: There are two conflicting definitions of Jmf (val Jmf =
config("jmf").hide and config("jmf").extend(Compile)); remove the duplicate and
make the remaining definition reflect the intended semantics—update the single
Jmf definition (the one currently using .hide) to use
config("jmf").extend(Compile) and delete the other duplicate definition so Jmf
is defined once consistently.
- Line 358: The jacocoReport task uses a hardcoded "java" binary which is
inconsistent with jmfRewrite's resolution from java.home; update jacocoReport to
resolve the JVM binary the same way (e.g., read System.getProperty("java.home")
and construct javaBin or reuse an extracted helper) and replace the literal
"java" in the args Seq with that resolved javaBin so the task works when java is
not on PATH; reference the jacocoReport task and mirror the java resolution
logic used in jmfRewrite (lines building javaBin from java.home).
🧹 Nitpick comments (14)
balta/src/main/scala/za/co/absa/db/mag/naming/implementations/AsIsNaming.scala (1)
18-18: Remove the test artifact comment before merging.This comment appears to be a testing artifact used to trigger JaCoCo coverage evaluation during development. It adds no functional value and should be cleaned up before the PR is merged.
🧹 Proposed fix to remove the test comment
package za.co.absa.db.mag.naming.implementations -// jacoco-touch: simulate change import za.co.absa.db.mag.naming.LettersCase.AsIsbalta/src/main/scala/za/co/absa/db/balta/classes/inner/ConnectionInfo.scala (1)
18-18: Consider removing the test comment.This "jacoco-touch" comment appears to be a temporary artifact from testing the JaCoCo integration. If it served only to trigger coverage evaluation during testing, it can be removed before merging.
balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
18-18: Consider removing the test comment.This "jacoco-touch" comment appears to be a temporary artifact from testing the JaCoCo integration. If it served only to trigger coverage evaluation during testing, it can be removed before merging.
balta/src/main/scala/za/co/absa/db/balta/implicits/MapImplicits.scala (1)
18-18: Consider removing the test comment.This "jacoco-touch" comment appears to be a temporary artifact from testing the JaCoCo integration. If it served only to trigger coverage evaluation during testing, it can be removed before merging.
balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (1)
18-18: Consider removing the test comment.This "jacoco-touch" comment appears to be a temporary artifact from testing the JaCoCo integration. If it served only to trigger coverage evaluation during testing, it can be removed before merging.
balta/src/main/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicits.scala (1)
18-18: Consider removing the test comment.This "jacoco-touch" comment appears to be a temporary artifact from testing the JaCoCo integration. If it served only to trigger coverage evaluation during testing, it can be removed before merging.
balta/src/main/scala/za/co/absa/db/balta/postgres/implicits/Postgres.scala (1)
18-18: Consider removing the test comment.This "jacoco-touch" comment appears to be a temporary artifact from testing the JaCoCo integration. If it served only to trigger coverage evaluation during testing, it can be removed before merging.
balta/src/main/scala/za/co/absa/db/balta/classes/DBConnection.scala (1)
18-18: Consider removing the test comment.This "jacoco-touch" comment appears to be a temporary artifact from testing the JaCoCo integration. If it served only to trigger coverage evaluation during testing, it can be removed before merging.
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (1)
18-18: Remove debug artifact comment before merging.This
// jacoco-touch: simulate changecomment appears to be a development artifact used to force JaCoCo to process this file. Based on the commit history ("Fake change to see full output of Jacoco"), this should be removed before merging to master to avoid polluting the codebase with non-functional comments.The AI summary indicates similar comments were added across numerous Scala files—consider removing them all.
.github/workflows/jacoco_report.yml (2)
12-12:coverage-per-changed-file: 0.0effectively disables per-file threshold checking.Setting this threshold to
0.0means individual changed files won't fail the build regardless of their coverage. If this is intentional (e.g., to focus only on aggregate metrics), consider adding a comment explaining the rationale. Otherwise, a meaningful threshold would provide better coverage enforcement.
71-135: Remove commented-out code.This large block of commented-out workflow steps adds noise and maintenance burden. Since the code is preserved in version control history, it can be recovered if needed. Keeping it commented out suggests the migration may be incomplete or experimental.
If this is intentional for reference during the transition period, consider:
- Adding a TODO comment with a target date for removal, or
- Removing it entirely and referencing the previous commit in the PR description
♻️ Suggested change
min-coverage-per-changed-file: ${{ env.coverage-per-changed-file }} skip-unchanged: false - - # - name: Add coverage to PR - # if: steps.jacocorun.outcome == 'success' - ... (remove all commented lines 71-135)jmf-rules.txt (1)
141-144: Redundant rules duplicating global definitions.Lines 141-142 define
*$*#apply(*)and*$*#unapply(*)for companion objects, but lines 120-121 already define*#apply(*)and*#unapply(*)which match all classes including companions. The companion-specific rules are redundant unless you narrow the global rules.project/FilteredJacocoAgentPlugin.scala (2)
105-109: Redundant default forjacocoPluginEnabled.
jacocoPluginEnabled := falseis set here inbuildSettings(line 106) and again inprojectSettings(line 133), butJacocoBaseKeysPluginalready sets the default tofalsein itsprojectSettings. Consider removing the redundant assignments.
174-183: Unused variable and duplicate assignment.
- Line 175:
updis assigned but never used; line 189 re-fetches the same value- Line 182:
rulesFileduplicatesrulesfrom line 174🧹 Proposed cleanup
- val rules = jmfRulesFile.value - val upd = (Jmf / update).value // hoisted + val rules = jmfRulesFile.value // used for existence check and CLI arg val log = streams.value.log val outRoot = jmfOutDir.value val mainCls = jmfCliMain.value val dryRun = jmfDryRun.value val workDir = baseDirectory.value val classesIn = (Compile / classDirectory).value - val rulesFile = jmfRulesFile.value val enabled = jacocoPluginEnabled.value
| - name: Check coverage thresholds and add reports in PR comments | ||
| id: jacoco | ||
| uses: MoranaApps/jacoco-report@v2 | ||
| with: | ||
| paths: ${{ github.workspace }}/balta/target/scala-${{ env.scalaShort }}/jacoco/report/jacoco.xml | ||
| token: ${{ secrets.GITHUB_TOKEN }} | ||
| token: '${{ secrets.GITHUB_TOKEN }}' | ||
| paths: | | ||
| balta/target/**/jacoco/report/jacoco.xml | ||
| sensitivity: "detail" | ||
| comment-mode: 'single' | ||
| min-coverage-overall: ${{ env.coverage-overall }} | ||
| min-coverage-changed-files: ${{ env.coverage-changed-files }} | ||
| title: JaCoCo 'balta' module code coverage report - scala ${{ env.scalaLong }} | ||
| update-comment: true | ||
| - name: Get the Coverage info | ||
| if: steps.jacocorun.outcome == 'success' | ||
| run: | | ||
| echo "Total 'balta' module coverage ${{ steps.jacoco-balta.outputs.coverage-overall }}" | ||
| echo "Changed Files coverage ${{ steps.jacoco-balta.outputs.coverage-changed-files }}" | ||
| - name: Fail PR if changed files coverage is less than ${{ env.coverage-changed-files }}% | ||
| if: steps.jacocorun.outcome == 'success' | ||
| uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd | ||
| with: | ||
| script: | | ||
| const coverageCheckFailed = | ||
| Number('${{ steps.jacoco-balta.outputs.coverage-changed-files }}') < Number('${{ env.coverage-changed-files }}') | ||
| if (coverageCheckFailed) { | ||
| core.setFailed('Changed files coverage is less than ${{ env.coverage-changed-files }}%!'); | ||
| } | ||
| - name: Fail PR if overall files coverage is less than ${{ env.coverage-overall }}% | ||
| if: ${{ (steps.jacocorun.outcome == 'success') && (env.check-overall-coverages == 'true') }} | ||
| uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd | ||
| with: | ||
| script: | | ||
| const coverageCheckFailed = | ||
| Number('${{ steps.jacoco-balta.outputs.coverage-overall }}') < Number('${{ env.coverage-overall }}'); | ||
| if (coverageCheckFailed) { | ||
| core.setFailed('Overall coverage is less than ${{ env.coverage-overall }}%!'); | ||
| } | ||
| - name: Edit JaCoCo comments on build failure | ||
| if: steps.jacocorun.outcome != 'success' | ||
| uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd | ||
| with: | ||
| script: | | ||
| const issue_number = context.issue.number; | ||
| const owner = context.repo.owner; | ||
| const repo = context.repo.repo; | ||
| const jacocoReportRegExp = /^### JaCoCo .* code coverage report .*/; | ||
| min-coverage-per-changed-file: ${{ env.coverage-per-changed-file }} | ||
| skip-unchanged: false |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MoranaApps/jacoco-report GitHub action v2 documentation
💡 Result:
I couldn’t find any public GitHub Action repository or release at MoranaApps/jacoco-report (or a published @v2 major version for it). The widely used action with this name is madrapps/jacoco-report, whose latest published release is v1.7.2 and whose documentation is on the GitHub Marketplace page. [1]
madrapps/jacoco-report documentation (latest: v1.7.2)
What it does: Posts a JaCoCo coverage report as a PR comment (and/or workflow summary), with optional minimum coverage thresholds. [1]
Typical usage
- name: Add coverage to PR
uses: madrapps/jacoco-report@v1.7.2
with:
paths: ${{ github.workspace }}/**/build/reports/jacoco/**/testCoverage.xml
token: ${{ secrets.GITHUB_TOKEN }}
min-coverage-overall: 80
min-coverage-changed-files: 80
update-comment: true
title: Code Coverage[1]
Inputs (high-level)
paths(required): comma-separated JaCoCo XML report paths (supports glob). [1]token(required): token used to comment on PRs. [1]min-coverage-overall,min-coverage-changed-files(optional). [1]- Also supports:
update-comment,comment-type(pr_comment/summary/both),pr-number,title,skip-if-no-changes,pass-emoji,fail-emoji,continue-on-error,debug-mode. [1]
Outputs
coverage-overallcoverage-changed-files[1]
If you meant a different repo (e.g., a fork under MoranaApps) paste its GitHub URL and I’ll pull the exact v2 docs for that specific action.
Source:
[1] GitHub Marketplace – “JaCoCo Report” action documentation (Madrapps)
🏁 Script executed:
# Search for MoranaApps references in the codebase
rg -i "moranaapps" --type yaml --type yml
# Also check git history for this workflow file if available
git log --oneline .github/workflows/jacoco_report.yml | head -20Repository: AbsaOSS/balta
Length of output: 185
🏁 Script executed:
# Search for MoranaApps more broadly
rg "MoranaApps" --type-list | head -5
rg "MoranaApps"
# Also check the full workflow file to understand context
cat -n .github/workflows/jacoco_report.yml | head -80Repository: AbsaOSS/balta
Length of output: 4317
Action MoranaApps/jacoco-report@v2 does not exist on GitHub Marketplace and will cause workflow failure.
The action referenced on line 59 cannot be found publicly. The known JaCoCo reporting action is madrapps/jacoco-report (latest v1.7.2). Additionally, the parameters used (sensitivity, comment-mode, skip-unchanged, min-coverage-per-changed-file) do not match any documented inputs for available JaCoCo actions. Either correct the action name to a valid existing action and align the parameters, or confirm this is a private action available in your environment. The commented-out code (lines 71–80) suggests the previous implementation used madrapps/jacoco-report.
🤖 Prompt for AI Agents
In @.github/workflows/jacoco_report.yml around lines 57 - 69, Replace the
nonexistent action MoranaApps/jacoco-report@v2 with the known public action
madrapps/jacoco-report@v1.7.2 (or the specific version you want) and align the
inputs to that action's documented parameters: use the action's "token" and
"files" (instead of "paths") for the report glob, keep the overall/changed-files
coverage inputs that madrapps supports (map your env.coverage-overall and
env.coverage-changed-files accordingly), and remove unsupported inputs such as
"sensitivity", "skip-unchanged" and "min-coverage-per-changed-file";
alternatively, if MoranaApps/jacoco-report@v2 is a private/internal action,
confirm it exists in your environment and document its supported inputs before
keeping it.
There was a problem hiding this comment.
v2 changed to sha.
There was a problem hiding this comment.
@miroslavpojer, I see you've moved from @v2 to the specific SHA @54bfe284d1119dc917dddba80517c54c5bcf3627. However, this doesn't address the underlying concern—MoranaApps/jacoco-report doesn't appear to exist as a public GitHub Action.
Is this a private, internal, or forked action that your organization has access to? If so, that's perfectly fine, and the workflow should work as long as your GitHub Actions runner can access it.
The parameters you're using (sensitivity, comment-mode, min-coverage-per-changed-file, skip-unchanged) also differ from the standard madrapps/jacoco-report action, which further suggests this is a custom implementation.
If this action is accessible in your environment and the workflow runs successfully in your CI, then you're all set. Otherwise, you may need to verify the action's availability or consider using the public madrapps/jacoco-report action instead.
| */ | ||
|
|
||
| package za.co.absa.db.balta.postgres.classes | ||
| // jacoco-touch: simulate change |
There was a problem hiding this comment.
Remove test artifact before merging.
This comment was added solely to trigger JaCoCo coverage evaluation ("simulate change") and serves no functional purpose in the production codebase. Test and debugging artifacts should be removed before merging to master.
🧹 Proposed fix
package za.co.absa.db.balta.postgres.classes
-// jacoco-touch: simulate change
🤖 Prompt for AI Agents
In
`@balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala`
at line 18, Remove the test/debug artifact comment "// jacoco-touch: simulate
change" from SimpleJsonString.scala (the file/class SimpleJsonString) so
production code contains no JaCoCo simulation lines; simply delete that comment
and ensure no other test-only markers remain in the SimpleJsonString source
before merging.
| */ | ||
|
|
||
| package za.co.absa.db.mag.naming.implementations | ||
| // jacoco-touch: simulate change |
There was a problem hiding this comment.
Remove the test artifact comment before merging.
This non-functional comment appears to be a test artifact from the initial JaCoCo integration testing (as mentioned in the PR's "Fake change to see full output of Jacoco" commit). Test artifacts like this should not be committed to the production codebase.
🧹 Proposed fix to remove the test comment
package za.co.absa.db.mag.naming.implementations
-// jacoco-touch: simulate change
import za.co.absa.db.mag.naming.LettersCase.LowerCaseAccording to the enriched summary, similar // jacoco-touch: simulate change comments were added across multiple Scala source files. Please ensure all such test artifacts are removed across the entire codebase before merging.
🤖 Prompt for AI Agents
In
`@balta/src/main/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNaming.scala`
at line 18, Remove the test artifact comment "// jacoco-touch: simulate change"
from SnakeCaseNaming.scala (and any identical occurrences in other Scala source
files); locate the SnakeCaseNaming implementation (object/class/trait named
SnakeCaseNaming) and delete that non-functional comment line, then run a quick
grep/search across the repo for "jacoco-touch: simulate change" and remove all
matches before merging.
| *#canEqual(*) id:case-canequal | ||
| *#equals(*) id:case-equals | ||
| *#apply(*) id:case-apply | ||
| *#unapply(*) id:case-unapply | ||
| *#hashCode(*) id:case-hashcode | ||
| *#copy(*) id:case-copy | ||
| *#copy$default$*(*) id:case-copy-defaults | ||
| *#productElement() id:case-prod-element | ||
| *#productArity() id:case-prod-arity | ||
| *#productPrefix() id:case-prod-prefix | ||
| *#productIterator() id:case-prod-iterator | ||
| *#tupled() id:case-tupled | ||
| *#curried() id:case-curried | ||
| *#toString() id:case-tostring | ||
| *#name() id:case-name | ||
| *#groups() id:case-groups | ||
| *#optionalAttributes() id:case-optionalAttributes | ||
|
|
There was a problem hiding this comment.
Global rules may be overly aggressive and could hide real business logic.
Several rules use *# (all classes) which contradicts the documentation's own warning at line 74 about keeping "package scope narrow":
*#apply(*)and*#unapply(*)(lines 120-121) will exclude coverage for all apply/unapply methods, including custom factory logic*#name(),*#groups(),*#optionalAttributes()(lines 132-134) are very generic names that could match non-boilerplate methods
Consider scoping these to specific packages (e.g., *.model.*#apply(*)) or removing them from globals and adding them under PROJECT RULES with appropriate package prefixes.
🤖 Prompt for AI Agents
In `@jmf-rules.txt` around lines 118 - 135, The global patterns using wildcard
receiver `*#` are too broad (e.g., symbols `apply(*)`, `unapply(*)`, `name()`,
`groups()`, `optionalAttributes()`) and will exclude non-boilerplate business
methods; narrow their scope by replacing the global `*#` selector with
package-specific selectors (for example target model or generated packages like
`*.model.*#apply(*)` / `*.model.*#unapply(*)`) or remove them from the globals
section and re-add under PROJECT RULES with appropriate package prefixes so only
intended boilerplate methods are excluded.
| val jacocoSetUserDirToBuildRoot = settingKey[Boolean]("Mimic non-forked runs by setting -Duser.dir to the build root for forked tests") | ||
|
|
||
| val jmfCoreVersion = settingKey[String]("JMF core library version") | ||
| val Jmf = config("jmf").hide |
There was a problem hiding this comment.
Duplicate Jmf configuration definition with conflicting semantics.
Jmf is defined here with .hide, but is redefined at line 73 as config("jmf").extend(Compile). The outer definition at line 73 will shadow this one in the plugin's scope, making the .hide variant effectively unused.
Remove one of the definitions and ensure consistent semantics throughout.
🔧 Proposed fix: Remove the duplicate definition
Remove line 73 and update line 36 to use the desired semantics:
- val Jmf = config("jmf").hide
+ val Jmf = config("jmf").extend(Compile)Then remove line 73:
- lazy val Jmf = config("jmf").extend(Compile)🤖 Prompt for AI Agents
In `@project/FilteredJacocoAgentPlugin.scala` at line 36, There are two
conflicting definitions of Jmf (val Jmf = config("jmf").hide and
config("jmf").extend(Compile)); remove the duplicate and make the remaining
definition reflect the intended semantics—update the single Jmf definition (the
one currently using .hide) to use config("jmf").extend(Compile) and delete the
other duplicate definition so Jmf is defined once consistently.
There was a problem hiding this comment.
Will be fixed in another issue.
There was a problem hiding this comment.
@miroslavpojer, understood! Would you like me to create a GitHub issue to track the duplicate Jmf configuration definition fix?
| } | ||
| } | ||
|
|
||
| lazy val Jmf = config("jmf").extend(Compile) |
There was a problem hiding this comment.
Remove duplicate Jmf configuration.
This line redefines Jmf which was already defined at line 36 in autoImport. This definition shadows the autoImport version and creates confusion about which one is used where.
🔧 Proposed fix
- lazy val Jmf = config("jmf").extend(Compile)
-And update line 36 to use .extend(Compile) if that semantic is needed:
- val Jmf = config("jmf").hide
+ val Jmf = config("jmf").extend(Compile)📝 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.
| lazy val Jmf = config("jmf").extend(Compile) |
🤖 Prompt for AI Agents
In `@project/FilteredJacocoAgentPlugin.scala` at line 73, Remove the duplicate
lazy val Jmf = config("jmf").extend(Compile) definition (it shadows the
autoImport Jmf); delete the second definition in FilteredJacocoAgentPlugin and
instead update the original autoImport Jmf declaration to call
config("jmf").extend(Compile) so there is a single, correctly extended Jmf
symbol used throughout.
There was a problem hiding this comment.
Will be fixed in another issue.
There was a problem hiding this comment.
@miroslavpojer, understood! Would you like me to create a GitHub issue to track this duplicate Jmf configuration fix?
| reportDir | ||
| } else { | ||
| // repeat flags per path | ||
| val args = Seq("java","-jar", cli.getAbsolutePath, "report", execFile.getAbsolutePath) ++ |
There was a problem hiding this comment.
Inconsistent Java binary resolution.
jacocoReport uses hardcoded "java" while jmfRewrite (lines 195-198) properly resolves the Java binary from java.home. If java is not on PATH, this task will fail.
🔧 Proposed fix: Use consistent Java binary resolution
Extract the javaBin resolution to a helper or inline it here:
+ val javaBin = {
+ val h = sys.props.get("java.home").getOrElse("")
+ if (h.nonEmpty) new java.io.File(new java.io.File(h, "bin"), "java").getAbsolutePath else "java"
+ }
// repeat flags per path
- val args = Seq("java","-jar", cli.getAbsolutePath, "report", execFile.getAbsolutePath) ++
+ val args = Seq(javaBin, "-jar", cli.getAbsolutePath, "report", execFile.getAbsolutePath) ++📝 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.
| val args = Seq("java","-jar", cli.getAbsolutePath, "report", execFile.getAbsolutePath) ++ | |
| val javaBin = { | |
| val h = sys.props.get("java.home").getOrElse("") | |
| if (h.nonEmpty) new java.io.File(new java.io.File(h, "bin"), "java").getAbsolutePath else "java" | |
| } | |
| // repeat flags per path | |
| val args = Seq(javaBin, "-jar", cli.getAbsolutePath, "report", execFile.getAbsolutePath) ++ |
🤖 Prompt for AI Agents
In `@project/FilteredJacocoAgentPlugin.scala` at line 358, The jacocoReport task
uses a hardcoded "java" binary which is inconsistent with jmfRewrite's
resolution from java.home; update jacocoReport to resolve the JVM binary the
same way (e.g., read System.getProperty("java.home") and construct javaBin or
reuse an extracted helper) and replace the literal "java" in the args Seq with
that resolved javaBin so the task works when java is not on PATH; reference the
jacocoReport task and mirror the java resolution logic used in jmfRewrite (lines
building javaBin from java.home).
There was a problem hiding this comment.
Will be fixed in another issue.
There was a problem hiding this comment.
@miroslavpojer, understood! Would you like me to create a GitHub issue to track this Java binary resolution inconsistency for the follow-up fix?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/jacoco_report.yml:
- Around line 10-12: The workflow currently sets coverage-overall: 80.0 and
coverage-changed-files: 80.0 but leaves coverage-per-changed-file: 0.0 which
effectively disables per-file enforcement; update coverage-per-changed-file to
80.0 to match the other gates (or explicitly confirm intent and document why it
should remain 0.0). Locate the keys coverage-overall, coverage-changed-files and
coverage-per-changed-file in the jacoco_report.yml and make the change or add a
comment explaining the intended exception.
| coverage-overall: 80.0 | ||
| coverage-changed-files: 80.0 | ||
| coverage-per-changed-file: 0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/jacoco_report.yml | head -80Repository: AbsaOSS/balta
Length of output: 3278
🏁 Script executed:
# Search for references to coverage thresholds
rg "coverage-per-changed-file|per-file|per_file_coverage" -i --type yaml --type md --type txtRepository: AbsaOSS/balta
Length of output: 39
🏁 Script executed:
# Check git history for this file to understand context
git log --oneline .github/workflows/jacoco_report.yml | head -20Repository: AbsaOSS/balta
Length of output: 143
🏁 Script executed:
# Get the commit details for the change
git show c83994c --statRepository: AbsaOSS/balta
Length of output: 5486
🏁 Script executed:
# Search for README or documentation files
fd -t f "README|CONTRIBUTING|docs" -i | grep -E "\.(md|txt)$" | head -10Repository: AbsaOSS/balta
Length of output: 66
🏁 Script executed:
cat README.md | grep -i -A 5 -B 5 "coverage\|jacoco"Repository: AbsaOSS/balta
Length of output: 717
🌐 Web query:
MoranaApps jacoco-report action min-coverage-per-changed-file 0.0 behavior
💡 Result:
I couldn’t find any public docs/source for MoranaApps/jacoco-report that mention an input named min-coverage-per-changed-file (web search only turns up the upstream madrapps/jacoco-report, whose documented thresholds are overall and “all changed files combined”, not “per changed file”).[1]
Given how coverage thresholds are typically implemented, if min-coverage-per-changed-file exists in the MoranaApps fork and is compared as:
- fail if
fileCoverage < minCoveragePerChangedFile
then setting min-coverage-per-changed-file: 0.0 effectively disables the per-file gate, because coverage percentages normally can’t be negative, so every file will satisfy coverage >= 0.0. Other gates (e.g., overall coverage / combined changed-files coverage) would still apply if configured.
If you paste the action link (or the action.yml inputs section / the exact version tag you’re using), I can confirm the exact behavior from the fork’s actual implementation.
Source: [1]
🏁 Script executed:
# Fetch the action.yml from the specific commit
curl -s https://raw.githubusercontent.com/MoranaApps/jacoco-report/54bfe284d1119dc917dddba80517c54c5bcf3627/action.yml | head -100Repository: AbsaOSS/balta
Length of output: 3342
🏁 Script executed:
# Also search the repo for any related issues or discussions
rg "coverage-per-changed-file|per.file.*coverage" -i --type mdRepository: AbsaOSS/balta
Length of output: 39
Clarify intent: min-coverage-per-changed-file: 0.0 disables per-file coverage enforcement.
The min-coverage-per-changed-file parameter is supported by the MoranaApps action and setting it to 0.0 effectively disables the per-file coverage gate (any coverage ≥ 0.0 passes). All other thresholds are set to 80.0, so confirm whether this disparity is intentional or should be 80.0 to enforce per-file coverage consistently.
🤖 Prompt for AI Agents
In @.github/workflows/jacoco_report.yml around lines 10 - 12, The workflow
currently sets coverage-overall: 80.0 and coverage-changed-files: 80.0 but
leaves coverage-per-changed-file: 0.0 which effectively disables per-file
enforcement; update coverage-per-changed-file to 80.0 to match the other gates
(or explicitly confirm intent and document why it should remain 0.0). Locate the
keys coverage-overall, coverage-changed-files and coverage-per-changed-file in
the jacoco_report.yml and make the change or add a comment explaining the
intended exception.
| */ | ||
|
|
||
| package za.co.absa.db.balta.classes | ||
| // jacoco-touch: simulate change |
There was a problem hiding this comment.
will you revert all of these?
lsulak
left a comment
lsulak
left a comment
There was a problem hiding this comment.
Ok my review is finished, pretty much what we have elsewhere, thank you. Concerned about the code docs / comments there though, I would't prefer them to stay in master
Overview
Introduce MoranaApps Jacoco measuring solution.
Release Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.