Feature/82 increase unit test coverage#83
Merged
miroslavpojer merged 15 commits intomasterfrom Apr 23, 2026
Merged
Conversation
…ot instructions and review rules
…. Fixes from self-review using copilot.
…ultRow - Introduced `database-persist.properties` for test database configuration. - Added `DBFunctionUnitTests` to validate the behavior of DBFunction with named and positioned parameters. - Created `QueryResultRowUnitTests` to test the functionality of QueryResultRow, including handling of various SQL types and conversion to case classes. - Implemented `ParamsUnitTests` to ensure correct behavior of named and ordered parameters. - Developed `RecordingPreparedStatement` to facilitate testing of parameter assignments without a real database. - Added `QueryParamTypeUnitTests` and `QueryParamValueUnitTests` to verify the correct assignment of various parameter types to the prepared statement.
WalkthroughAdds extensive unit tests and a test JDBC helper, updates JMF/JaCoCo configuration and rules (v2.1.0), tweaks CI guidance, and changes QueryResultRow.apply(column: Int) to use JDBC 1-based indexing by passing the column directly to getObject. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
…hod-filter plugin version - Added tests for DBFunction to verify behavior of setParamNull by name and position. - Enhanced QueryResult tests to check noMore method functionality. - Included tests for Params class to validate addNull method for both NamedParams and OrderedParams. - Updated jacoco-method-filter plugin from version 2.0.1 to 2.1.0 for improved coverage filtering.
…onfiguration in build.sbt
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: 6
🧹 Nitpick comments (2)
balta/src/test/scala/za/co/absa/db/balta/testing/RecordingPreparedStatement.scala (1)
42-133: Fail fast for unsupported JDBC interactions.The no-op/default stubs can make tests pass after unexpected
PreparedStatementoperations. Keep the explicit setter recorders, but make unsupported methods throw so this mock only accepts the interactions it is meant to verify.🧪 Suggested pattern
class RecordingPreparedStatement extends PreparedStatement { var lastCall: (String, Any, Any) = _ + + private def unsupported: Nothing = + throw new UnsupportedOperationException("RecordingPreparedStatement only supports parameter setter verification") override def setBoolean(parameterIndex: Int, x: Boolean): Unit = { lastCall = ("setBoolean", parameterIndex, x) } override def setInt(parameterIndex: Int, x: Int): Unit = { lastCall = ("setInt", parameterIndex, x) } override def setLong(parameterIndex: Int, x: Long): Unit = { lastCall = ("setLong", parameterIndex, x) } @@ - override def executeQuery(): ResultSet = null - override def executeUpdate(): Int = 0 - override def setNull(parameterIndex: Int, sqlType: Int): Unit = () + override def executeQuery(): ResultSet = unsupported + override def executeUpdate(): Int = unsupported + override def setNull(parameterIndex: Int, sqlType: Int): Unit = unsupported🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@balta/src/test/scala/za/co/absa/db/balta/testing/RecordingPreparedStatement.scala` around lines 42 - 133, RecordingPreparedStatement currently contains many no-op/default stubs (e.g., executeQuery(), executeUpdate(), execute(), getResultSet, getConnection, isClosed, executeBatch, setBlob(...), setClob(...), etc.) that silently allow unexpected JDBC calls; replace those no-op implementations with throws of UnsupportedOperationException (with a clear message including the method name) so unsupported JDBC interactions fail fast while leaving the explicit setter/recorder methods unchanged; update each stubbed method in RecordingPreparedStatement to throw new UnsupportedOperationException("Unsupported in RecordingPreparedStatement: <methodName>") instead of returning null/0/Unit.balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala (1)
36-309: LGTM — thorough reflection + JDBC-stub coverage.Top-level case classes (
SimpleRecord,OptionalRecord,MultiCtorRecord) are correctly placed outside the suite forcurrentMirror.reflectClasscompatibility (matches the Scala reflection learning). StubResultSetMetaData/java.sql.Arrayimplementations keep the unit tests DB-free.getArray,toProductType(including Option handling and the NPE path), andcreateExtractorsdispatch branches are all covered.Minor nit: the many
.asInstanceOf[Object]casts in field vectors can be avoided with an explicit type ascription on the vector literal (e.g.Vector[Option[Object]](Some("hello"), Some(Integer.valueOf(42)), ...)) — not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala` around lines 36 - 309, Many unnecessary .asInstanceOf[Object] casts are used when building test field vectors; simplify by giving the vector literal an explicit type so Scala will upcast elements automatically: replace occurrences like the sampleFields definition and any inline fields Vector(...) in tests with a typed literal Vector[Option[Object]](...) (refer to sampleFields, the various local fields in tests, and mkRow/row constructions) and remove the per-element .asInstanceOf[Object] casts, leaving only necessary boxed primitives (e.g. Integer.valueOf) where required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Around line 78-81: Update the JMF version token in the docs: replace the stale
header reference "[jmf:1.0.0]" in the copilot instructions text with the current
rules-file header version found in jmf-rules.txt ("[jmf:2.1.0]") so the
guideline matches the actual rules file; ensure the single-line rules-file
header token in the prose exactly matches the header on jmf-rules.txt.
In
`@balta/src/test/scala/za/co/absa/db/balta/classes/inner/ParamsUnitTests.scala`:
- Around line 44-102: The tests call deprecated APIs NamedParams.addNull and
OrderedParams.addNull which emit deprecation warnings; to silence warnings (or
future-proof) either replace those calls with the non-deprecated form
Params.add(QueryParamType.NULL) / .add(NULL) in the tests (e.g., swap
Params.addNull("missing") -> Params.add("missing", QueryParamType.NULL) or
equivalent) or add a suppression annotation `@nowarn`("cat=deprecation") to the
specific test methods; update the two NamedParams tests referencing
Params.addNull and the two OrderedParams tests referencing Params.addNull to use
one of these approaches so the deprecation warnings are removed.
In
`@balta/src/test/scala/za/co/absa/db/balta/testing/RecordingPreparedStatement.scala`:
- Around line 17-28: The RecordingPreparedStatement helper currently declares
package za.co.absa.db.balta but is located under a nested testing folder; ensure
the file location and package match by moving the RecordingPreparedStatement
source so its containing directory corresponds to package za.co.absa.db.balta
and/or update the file's package declaration to exactly "za.co.absa.db.balta";
verify the class name RecordingPreparedStatement remains unchanged and that test
imports referencing za.co.absa.db.balta.RecordingPreparedStatement resolve
without package/path mismatch.
In
`@balta/src/test/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNamingUnitTests.scala`:
- Around line 60-64: The test in SnakeCaseNamingUnitTests currently only checks
inclusion of "snake" which would pass even if a trailing '$' remains; update the
assertion for the companion-object case to assert the exact converted string
returned by SnakeCaseNaming.fromClassNamePerConvention(SnakeCaseNaming) (e.g.,
"snake_case_naming") so the test verifies the trailing '$' is stripped and the
full expected snake_case name is produced.
In `@jmf-rules.txt`:
- Around line 209-213: Two rules for QueryResultRow share the same id label
"id:balta-colname-get-char" (QueryResultRow#getChar and
QueryResultRow#getObject), causing duplicate traceability ids; update the
duplicate id on the QueryResultRow#getObject entry to a unique id (e.g.,
id:balta-colname-get-object or another consistent naming) so every rule has a
distinct `id:` label per the guideline, and ensure naming follows the existing
id convention used for QueryResultRow#getChar and QueryResultRow#getAs.
- Around line 78-86: The rule for scala-lzycompute currently active
("*#*$lzycompute(*) id:scala-lzycompute") contradicts the comment that it should
be disabled; either comment out or remove that exclusion so lazy-val
initializers are not silently hidden, or keep the rule but remove the "DISABLED"
note and add explicit include/rescue rules (prefixing with "+" for specific lazy
vals) to ensure any non-trivial lazy initializer is covered; update the comment
to reflect the chosen action so rule state and documentation match.
---
Nitpick comments:
In
`@balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala`:
- Around line 36-309: Many unnecessary .asInstanceOf[Object] casts are used when
building test field vectors; simplify by giving the vector literal an explicit
type so Scala will upcast elements automatically: replace occurrences like the
sampleFields definition and any inline fields Vector(...) in tests with a typed
literal Vector[Option[Object]](...) (refer to sampleFields, the various local
fields in tests, and mkRow/row constructions) and remove the per-element
.asInstanceOf[Object] casts, leaving only necessary boxed primitives (e.g.
Integer.valueOf) where required.
In
`@balta/src/test/scala/za/co/absa/db/balta/testing/RecordingPreparedStatement.scala`:
- Around line 42-133: RecordingPreparedStatement currently contains many
no-op/default stubs (e.g., executeQuery(), executeUpdate(), execute(),
getResultSet, getConnection, isClosed, executeBatch, setBlob(...), setClob(...),
etc.) that silently allow unexpected JDBC calls; replace those no-op
implementations with throws of UnsupportedOperationException (with a clear
message including the method name) so unsupported JDBC interactions fail fast
while leaving the explicit setter/recorder methods unchanged; update each
stubbed method in RecordingPreparedStatement to throw new
UnsupportedOperationException("Unsupported in RecordingPreparedStatement:
<methodName>") instead of returning null/0/Unit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bc8c6ca-f4d7-489c-9460-6fc1f55278da
📒 Files selected for processing (14)
.github/copilot-instructions.mdbalta/src/test/resources/database-persist.propertiesbalta/src/test/scala/za/co/absa/db/balta/DBTestSuiteUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/DBFunctionUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/QueryResultUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/inner/ParamsUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/testing/RecordingPreparedStatement.scalabalta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamTypeUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamValueUnitTests.scalabalta/src/test/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNamingUnitTests.scalabuild.sbtjmf-rules.txtproject/plugins.sbt
There was a problem hiding this comment.
Pull request overview
This PR aims to increase unit-test coverage in balta by adding new ScalaTest *UnitTests suites (mocking JDBC at the boundary), expanding jacoco-method-filter (JMF) rules, and adding JMF reporting configuration/documentation.
Changes:
- Added/expanded unit tests for key classes/typeclasses (e.g.,
DBTestSuite,DBFunction,QueryResultRow,Params,QueryParamType,QueryParamValue,SnakeCaseNaming) plus a reusableRecordingPreparedStatementstub. - Updated JMF tooling: bumped
jacoco-method-filter-sbt, expandedjmf-rules.txt, and enabled JSON JMF reporting. - Added an integration-test-oriented resource
database-persist.propertiesand updated contributor instructions around JMF usage.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| project/plugins.sbt | Bumps jacoco-method-filter-sbt plugin version. |
| jmf-rules.txt | Major rewrite/expansion of JMF global/include/project rules. |
| build.sbt | Configures JMF JSON report output. |
| balta/src/test/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNamingUnitTests.scala | Adds more naming-convention unit test scenarios. |
| balta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamValueUnitTests.scala | New unit tests for QueryParamValue assignment/equality semantics. |
| balta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamTypeUnitTests.scala | New unit tests for JDBC binding per QueryParamType. |
| balta/src/test/scala/za/co/absa/db/balta/testing/RecordingPreparedStatement.scala | Adds a minimal PreparedStatement stub for unit testing parameter binding. |
| balta/src/test/scala/za/co/absa/db/balta/classes/inner/ParamsUnitTests.scala | New unit tests for ordered/named params behavior. |
| balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultUnitTests.scala | Adds unit tests for QueryResult.noMore. |
| balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala | New unit tests for QueryResultRow extractors, getters, arrays, and product conversion. |
| balta/src/test/scala/za/co/absa/db/balta/classes/DBFunctionUnitTests.scala | New unit tests for DBFunction param-building APIs. |
| balta/src/test/scala/za/co/absa/db/balta/DBTestSuiteUnitTests.scala | Expands tests for ConnectionInfo loading/override and helper delegates. |
| balta/src/test/resources/database-persist.properties | Adds a test config enabling persistData=true. |
| .github/copilot-instructions.md | Documents expanded JMF workflow and decision criteria. |
…emove old implementation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
balta/src/test/scala/za/co/absa/db/balta/RecordingPreparedStatement.scala (1)
29-29: ConsiderOptioninstead of null-initializedvar.
var lastCall: (String, Any, Any) = _defaults tonull; any test that asserts onlastCallbefore a setter fires will get an NPE rather than a meaningful failure.Option[(String, Any, Any)]initialized toNonewould fail assertions with a clear message and avoids the_default-value idiom.♻️ Suggested change
- var lastCall: (String, Any, Any) = _ + var lastCall: Option[(String, Any, Any)] = None(Callers would then assert on
ps.lastCall.contains(("setBoolean", 1, true)).)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@balta/src/test/scala/za/co/absa/db/balta/RecordingPreparedStatement.scala` at line 29, Replace the null-initialized var lastCall: (String, Any, Any) = _ with an Option to avoid NPEs: change the field in RecordingPreparedStatement to lastCall: Option[(String, Any, Any)] = None, update any setter to assign Some((..., ..., ...)) and update tests/assertions to check contains or pattern-match (e.g., ps.lastCall.contains(("setBoolean", 1, true))) so callers get clear failures instead of null pointer exceptions.balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala (2)
263-294: Drop the redundant helper ScalaDocs.These comments restate the private stub class names and do not add intent or edge-case context.
Proposed cleanup
- /** - * Stub ResultSetMetaData for unit testing. - */ private class StubMetaData(columns: List[(String, Int, String)]) extends ResultSetMetaData { @@ - /** - * Stub java.sql.Array for unit testing getArray methods. - */ private class StubSqlArray(data: Array[Object]) extends java.sql.Array {As per coding guidelines,
**/*.scala: Comments: comment intent/edge cases only; avoid restating the code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala` around lines 263 - 294, Remove the redundant ScalaDoc lines that merely restate class names for the test stubs (e.g., the block comment above the private class StubMetaData and the following "Stub java.sql.Array" comment); either delete those comments entirely or replace them with a single-line intent comment that documents edge-case behavior or why the stub exists, referencing StubMetaData (and the subsequent stub for java.sql.Array) so reviewers can locate the stubs without duplicative documentation.
93-96: Avoid the unchecked cast in the transformer.Pattern matching keeps the test failure explicit if the value type changes.
Proposed safer transformer
- val result = row.getAs[Int](2, { obj: Object => obj.asInstanceOf[java.lang.Integer].intValue() }) + val result = row.getAs[Int](2, { + case value: java.lang.Integer => value.intValue() + case other => fail(s"Expected java.lang.Integer, got ${other.getClass.getName}") + })As per coding guidelines,
**/*.scala: Unsafe casts: avoid.asInstanceOf; document why when unavoidable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala` around lines 93 - 96, The transformer in the test uses an unsafe cast (obj.asInstanceOf[java.lang.Integer]); replace it with a pattern match to keep failures explicit: update the transformer passed to QueryResultRow.getAs (in the test "getAs with transformer applies the function") to match on the expected java.lang.Integer and return intValue(), and in the default case throw a clear error (e.g., MatchError or AssertionError) indicating the unexpected type so test failures remain explicit if the value type changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala`:
- Around line 19-25: Reorder the imports so standard library imports come first:
move the java.sql imports (ResultSetMetaData, Types) above third-party and
project imports; maintain grouping order stdlib → third-party → project-internal
and keep imports at the top of the file, i.e., ensure imports for
ResultSetMetaData and Types appear before AnyFunSuiteLike, FieldNames,
ProductTypeConvertor, AsIs and AsIsNaming.
- Around line 137-159: Replace the weak size-only assertions by invoking the
extractor returned from QueryResultRow.createExtractors (use extractors.head)
with a stubbed ResultSet and asserting the actual returned value/type: for the
timestamptz case use a ResultSet stub that returns a known timestamp and assert
the extractor returns an OffsetDateTime (or expected value), for the timetz case
stub a time value and assert an OffsetTime result, and for the ARRAY case stub
the SQL Array (or the driver-specific array payload) and assert the extractor
returns the expected Scala collection/type; keep using StubMetaData to select
the extractor and call the extractor with the same column index/name used in
production so the test verifies behavior, not just count.
- Around line 27-34: SimpleRecord, OptionalRecord, and MultiCtorRecord are
defined as public top-level test fixtures; move them into a private companion
object to avoid leaking test-only symbols into the package API. Locate the
declarations (SimpleRecord, OptionalRecord, MultiCtorRecord) and place them
inside a private object adjacent to the QueryResultRowUnitTests class (e.g.
private object QueryResultRowUnitTestsFixtures) and update the test class to
import or reference them from that private object; ensure MultiCtorRecord's
auxiliary constructor remains intact and remove any top-level definitions so
only QueryResultRowUnitTests and the private companion object remain public.
In `@balta/src/test/scala/za/co/absa/db/balta/RecordingPreparedStatement.scala`:
- Line 1: The filename-inspector exclusion is too narrow and is failing for
shared test helpers like RecordingPreparedStatement.scala; update the workflow
exclusion in .github/workflows/test_filenames_check.yml (around the
filename-inspector patterns) to also exclude files directly under
balta/src/test/scala/za/co/absa/db/balta/, e.g. add a pattern such as
balta/src/test/scala/za/co/absa/db/balta/*.scala or explicitly include
RecordingPreparedStatement.scala so shared helpers at the root test package are
skipped by the check.
---
Nitpick comments:
In
`@balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala`:
- Around line 263-294: Remove the redundant ScalaDoc lines that merely restate
class names for the test stubs (e.g., the block comment above the private class
StubMetaData and the following "Stub java.sql.Array" comment); either delete
those comments entirely or replace them with a single-line intent comment that
documents edge-case behavior or why the stub exists, referencing StubMetaData
(and the subsequent stub for java.sql.Array) so reviewers can locate the stubs
without duplicative documentation.
- Around line 93-96: The transformer in the test uses an unsafe cast
(obj.asInstanceOf[java.lang.Integer]); replace it with a pattern match to keep
failures explicit: update the transformer passed to QueryResultRow.getAs (in the
test "getAs with transformer applies the function") to match on the expected
java.lang.Integer and return intValue(), and in the default case throw a clear
error (e.g., MatchError or AssertionError) indicating the unexpected type so
test failures remain explicit if the value type changes.
In `@balta/src/test/scala/za/co/absa/db/balta/RecordingPreparedStatement.scala`:
- Line 29: Replace the null-initialized var lastCall: (String, Any, Any) = _
with an Option to avoid NPEs: change the field in RecordingPreparedStatement to
lastCall: Option[(String, Any, Any)] = None, update any setter to assign
Some((..., ..., ...)) and update tests/assertions to check contains or
pattern-match (e.g., ps.lastCall.contains(("setBoolean", 1, true))) so callers
get clear failures instead of null pointer exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5499421b-0302-4a40-bf62-45df76de617d
📒 Files selected for processing (6)
.github/copilot-instructions.mdbalta/src/test/scala/za/co/absa/db/balta/RecordingPreparedStatement.scalabalta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/inner/ParamsUnitTests.scalabalta/src/test/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNamingUnitTests.scalajmf-rules.txt
✅ Files skipped from review due to trivial changes (2)
- balta/src/test/scala/za/co/absa/db/balta/classes/inner/ParamsUnitTests.scala
- .github/copilot-instructions.md
🚧 Files skipped from review as they are similar to previous changes (2)
- balta/src/test/scala/za/co/absa/db/mag/naming/implementations/SnakeCaseNamingUnitTests.scala
- jmf-rules.txt
|
JaCoCo Coverage Report
|
benedeki
reviewed
Apr 22, 2026
| * @return - the value stored in the column, type `Any` is for warningless comparison with any type | ||
| */ | ||
| def apply(column: Int): Option[Any] = getObject(column - 1) | ||
| def apply(column: Int): Option[Any] = getObject(column) |
Contributor
There was a problem hiding this comment.
Why this change?
It might be breaking, AFAIK, JDBC has 0-based indexing of columns, which is counterintuitive.
Collaborator
Author
There was a problem hiding this comment.
Pls confirm if it is or is not a bug, I have found it during review using copilot. I have read a lot about it, and the bug looks legit. I am not deep familiar with this.
Contributor
There was a problem hiding this comment.
You are right, it's 1 based. So it's a bug.
Would mention it in the release notes, and mark it as breaking change.
| test.jdbc.username=mag_owner | ||
| test.jdbc.password=changeme | ||
|
|
||
| test.persist.db=true |
Collaborator
Author
There was a problem hiding this comment.
This file is used only in unit test fixture - explicity name is there.
Do you see some serious problem with its existence?
Contributor
There was a problem hiding this comment.
Sorry, I still don't get why is it needed?
Collaborator
There was a problem hiding this comment.
Yeah the only usecase for this setting to true in the past, for me, was if I wanted to query and see what's in the DB after the tests finished
Collaborator
Author
There was a problem hiding this comment.
yes, It is here from the begin to allow to keep state after test for manual review.
…rect package location
There was a problem hiding this comment.
🧹 Nitpick comments (1)
jmf-rules.txt (1)
108-114: ScopeandThen/composerules to the project package per JMF guidelines.Lines 113–114 use broad
*#andThen(*)and*#compose(*)patterns matching every class in every package. Per coding guidelines, "Prefer narrow package scopes in JMF rules; prefersynthetic/bridgeflags for compiler artifacts over broad wildcards." Since these target Function1 trait mixin forwarders (compiler-generated), narrow the FQCN glob to the project package:♻️ Suggested narrowing
-*#andThen(*) id:fn1-andthen -*#compose(*) id:fn1-compose +*za.co.absa.db.*#andThen(*) id:fn1-andthen +*za.co.absa.db.*#compose(*) id:fn1-composeThe CAUTION comment and INCLUDE RULES rescue mechanism mitigate the current risk (no domain classes in the project currently implement Function1), but narrowing aligns with best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jmf-rules.txt` around lines 108 - 114, The rules *#andThen(*) and *#compose(*) are too broad and match every class; narrow their FQCN glob to your project package (e.g. replace the leading wildcard with your project's package prefix like com.yourorg.yourproj/**#andThen(*) and com.yourorg.yourproj/**#compose(*)) so they only target the compiler-generated Function1 forwarders in your codebase; keep the CAUTION and existing INCLUDE rescue mechanism unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@jmf-rules.txt`:
- Around line 108-114: The rules *#andThen(*) and *#compose(*) are too broad and
match every class; narrow their FQCN glob to your project package (e.g. replace
the leading wildcard with your project's package prefix like
com.yourorg.yourproj/**#andThen(*) and com.yourorg.yourproj/**#compose(*)) so
they only target the compiler-generated Function1 forwarders in your codebase;
keep the CAUTION and existing INCLUDE rescue mechanism unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbdc410b-279e-471c-951e-dd573746b52c
📒 Files selected for processing (7)
.github/copilot-instructions.mdbalta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scalabalta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/testing/classes/RecordingPreparedStatement.scalabalta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamTypeUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamValueUnitTests.scalajmf-rules.txt
✅ Files skipped from review due to trivial changes (1)
- balta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamValueUnitTests.scala
🚧 Files skipped from review as they are similar to previous changes (3)
- balta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamTypeUnitTests.scala
- .github/copilot-instructions.md
- balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala
benedeki
previously approved these changes
Apr 22, 2026
lsulak
reviewed
Apr 22, 2026
| * @return - the value stored in the column, type `Any` is for warningless comparison with any type | ||
| */ | ||
| def apply(column: Int): Option[Any] = getObject(column - 1) | ||
| def apply(column: Int): Option[Any] = getObject(column) |
Collaborator
There was a problem hiding this comment.
I think that previously there was a bug in the codebase, but please document it. On the parameter level ideally, instead of this:
* @param column - the number of the column, 1 based
lsulak
reviewed
Apr 22, 2026
Collaborator
lsulak
left a comment
lsulak
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Just one thing, about that "-1 bug" I commented on, please improve the doc of that parameter. Otherwise I don't have any objections!
miroslavpojer
added
the
breaking change
There was a problem hiding this comment.
🧹 Nitpick comments (1)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala (1)
48-50: Keep the public ScalaDoc focused on the parameter contract.The 1-based convention is the key API detail; the “prior to this fix” history fits better in release notes than generated API docs.
Proposed ScalaDoc cleanup
- * `@param` column - the 1-based column number (JDBC convention); note: prior to this fix the parameter was - * incorrectly treated as 0-based (direct vector index), causing column 1 to silently return - * the value of the second column + * `@param` column - the 1-based column number, following the JDBC conventionAs per coding guidelines,
ScalaDoc: short summary line first; prefer Parameters/Returns/Raises sections; avoid tutorials and long prose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala` around lines 48 - 50, The ScalaDoc for the method in class QueryResultRow that documents the parameter named "column" is too verbose and includes historical implementation details; update the ScalaDoc to a short summary followed by a concise `@param` entry that states the API contract (that "column" is 1-based, following JDBC convention) and remove the "prior to this fix" history; keep optional Returns/Throws tags as needed but avoid tutorial or historical prose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala`:
- Around line 48-50: The ScalaDoc for the method in class QueryResultRow that
documents the parameter named "column" is too verbose and includes historical
implementation details; update the ScalaDoc to a short summary followed by a
concise `@param` entry that states the API contract (that "column" is 1-based,
following JDBC convention) and remove the "prior to this fix" history; keep
optional Returns/Throws tags as needed but avoid tutorial or historical prose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48260fdc-67ce-4493-9f3b-cb13a1e518bf
📒 Files selected for processing (1)
balta/src/main/scala/za/co/absa/db/balta/classes/QueryResultRow.scala
lsulak
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Increases unit-test coverage across several core classes by adding
UnitTestssuites that mock JDBC at the boundary, expanding JMF filter rules for compiler-generated methods, and documenting the JMF decision process.New test files cover
DBTestSuite,DBFunction,QueryResultRow,Params,QueryParamType,QueryParamValue, andSnakeCaseNaming. A reusableRecordingPreparedStatementJDBC mock is added as a shared test helper. Thedatabase-persist.propertiesresource supports integration tests that commit rather than roll back.jmf-rules.txtis significantly extended.Release Notes
DBFunction,QueryResultRow,Params,DBTestSuite,QueryParamType,QueryParamValue, andSnakeCaseNamingwithout requiring a live DBRecordingPreparedStatementshared JDBC mock for reuse across unit test suitesdatabase-persist.propertiestest resource for integration tests that persist DB statejmf-rules.txtwith compiler-generated method rules (synthetics, case-class boilerplate, value-class extensions)QueryResultRow.apply(Int)and all column-by-number accessors now use 1-based column indices (JDBC convention) viagetObject(column: Int); callers passing 0-based indices must increment by 1Related
Closes #82
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Chores