Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughIntroduces per-module JaCoCo support via two new sbt AutoPlugins, JMF method-filter docs and tooling, replaces legacy Jacoco wiring in build and plugins, and updates GitHub workflow to a newer jacoco-report action with adjusted thresholds and reporting paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant SBT as sbt (FilteredJacocoAgentPlugin)
participant JVM as Forked JVM (Test)
participant Agent as JaCoCo Agent (.exec)
participant JMF as JMF CLI/Rewrite
participant FS as Filesystem (report files)
participant GH as GitHub Action (jacoco-report)
Dev->>SBT: run `jacoco` / `test`
SBT->>JVM: inject -javaagent for module -> writes per-module .exec (Agent)
JVM->>Agent: run tests, collect coverage -> write .exec to FS
SBT->>JMF: (if enabled) jmfRewrite processes exec/class files -> outputs filtered exec/report artifacts to FS
SBT->>FS: generate per-module XML/HTML/CSV reports
GH->>FS: glob `balta/target/**/jacoco/report/jacoco.xml`
GH->>GH: evaluate coverage thresholds, post single comment (comment-mode: single)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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
|
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.
balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala
Outdated
Show resolved
Hide resolved
balta/src/main/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNaming.scala
Outdated
Show resolved
Hide resolved
| *#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.
| */ | ||
|
|
||
| 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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/check_pr_release_notes.yml:
- Around line 23-24: The workflow is currently gated by the paths filter "paths:
- '**/*.scala'", which restricts the release-notes check to Scala-only changes;
remove the "paths" block (or replace the pattern with a broader one like '**/*'
or add all relevant file globs) so the check runs for all PRs instead of only
Scala files; update the workflow's "paths" key (or delete it) to ensure the
release-notes check applies universally rather than only when '**/*.scala' is
changed.
2 participants
Overview
Introduce MoranaApps Jacoco measuring solution.
Release Notes
Summary by CodeRabbit
New Features
Chores
Documentation