From a4ef7a9f9e3a75526572180f4d7d55ca23338f5f Mon Sep 17 00:00:00 2001 From: miroslavpojer Date: Tue, 7 Apr 2026 13:15:19 +0200 Subject: [PATCH 01/13] feat: add unit tests for JMF rules and improve documentation in copilot instructions and review rules --- .github/copilot-instructions.md | 101 ++++++++++++++++++++++ .github/copilot-review-rules.md | 47 +++++++++++ .gitignore | 1 + jmf-rules.txt | 144 +++++++------------------------- 4 files changed, 180 insertions(+), 113 deletions(-) create mode 100644 .github/copilot-instructions.md create mode 100644 .github/copilot-review-rules.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..3b64025 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,101 @@ +Copilot instructions + +Purpose +- Define consistent style; keep externally-visible behavior stable unless intentionally changing the contract. +- Sections order, bullet lists, and constraint words (Must / Must not / Prefer / Avoid) must be consistent across repos. +- Must keep one blank line at end of file. + +Context +- Scala library consumed via sbt in CI/CD; treat JDBC connections and DB state as boundary concerns. +- Must keep library API backward-compatible unless a version bump is intentional. + +Coding guidelines +- Must keep changes small and focused; prefer explicit over clever. +- Must keep pure logic free of environment access; route I/O through a single boundary module. +- Must keep externally-visible strings, formats, and exit codes stable unless intentional. + +Output discipline +- Aim ≤ 10 lines in final recap; link and summarize deltas rather than pasting large content. +- Prefer actionable bullets; avoid deep rationale unless requested. +- Code changes must end with: what changed / why / how to verify. + +PR Body Management +- Treat PR description as a changelog; append under `## Update [YYYY-MM-DD]` or `## Changes Added`. +- Must not rewrite the entire PR body; must reference the commit hash that introduced the change. + +Configuration +- Must read DB connection details from `database.properties` test resource; centralize in `ConnectionInfo`. +- Must not hard-code credentials; document required vs optional `ConnectionInfo` fields with defaults. + +Language and style +- Must target Scala 2.12 primary; cross-compile 2.11 and 2.13 where configured. +- Must add ScalaDoc for new public methods and classes. +- Must use slf4j (`LazyLogging` or explicit `Logger`), not `println`/`print`; pass logger arguments lazily (call-by-name), never pre-build interpolated strings. +- Must keep imports at top of file, grouped: stdlib → third-party → project-internal. +- Must include the Apache 2.0 copyright/license header in every source file using the project first-copyright year. +- Prefer `s"..."` interpolation for non-logging templates; avoid interpolation inside `logger.*` calls. + +Docstrings and comments +- ScalaDoc: short summary line first; prefer Parameters/Returns/Raises sections; avoid tutorials and long prose. +- Comments: comment intent/edge cases only; avoid restating the code. + +Patterns +- Prefer raising exceptions in leaf modules; translate to test/library failure at the entry point (`DBTestSuite.test()`). +- Must keep boundaries mockable; must not call real external systems (DB, APIs) in unit tests. + +Testing +- Must use ScalaTest under `balta/src/test/scala/`. +- Must test return values, exceptions, and error messages. +- Unit test files: name ends with `UnitTests`; must not require a DB connection; mock JDBC (`PreparedStatement`/`ResultSet`) if needed. +- Integration test files: name ends with `IntegrationTests`; must extend `DBTestSuite` or mix in `DBTestingConnection`. +- Must not access private members of the class under test. +- Prefer `AnyFunSuite`/`AnyFunSuiteLike`; use `AnyWordSpec` only when given/when/then adds clarity. +- Must place shared test helpers and fixtures in a dedicated `testing` sub-package and reuse them across tests. +- Must not add top-level comments in `*Tests.scala` outside test methods; use nested `describe`/`"..."` blocks to separate groups. +- Prefer TDD workflow: + - Must create or update `SPEC.md` before writing any code, listing scenarios, inputs, and expected outputs. + - Must propose the full test case set (name + intent + input + expected output) and wait for user confirmation before coding. + - Must write all failing tests first (red), then implement until all pass (green). + - Must cover all distinct combinations; each test must state its scenario in the ScalaDoc. + - Must update `SPEC.md` after all tests pass with the confirmed test case table. + +Tooling +- Must format with scalafmt (`.scalafmt.conf`); lint with scalastyle (`scalastyle-config.xml`) or wartremover as configured. +- Compiler warnings treated as errors where configured; coverage ≥ 80% via sbt-jacoco (excluding JMF-filtered methods in `jmf-rules.txt`). + +Coverage filtering (JMF) +- A method qualifies for a JMF filter rule if it meets at least one criterion: + - No added value: trivial delegate, one-liner factory, or pure field accessor — a test adds no assurance beyond testing the delegated method. + - Not coverable without integration tests: requires a DB/external system; same path already exercised by integration tests. +- Qualifying patterns: deprecated single-call delegates; `columnLabel: String` overloads that only call `columnNumber(label)` + Int-based overload; one-liner factory wrappers; implicit single-field accessors. +- Must not add JMF rules for methods with branching logic, error handling, or non-trivial transformations. + +Quality gates +- sbt "testOnly *UnitTests" # unit tests, no DB needed +- sbt test # all tests, DB must be running +- sbt jacoco # coverage → balta/target/scala-*/jacoco/ +- sbt scalafmtCheck # format check +- sbt scalastyle # lint + +Common pitfalls to avoid +- Dependencies: verify compatibility across all cross-compiled Scala versions. +- Unsafe casts: avoid `.asInstanceOf`; document why when unavoidable. +- Logging: never pre-build log strings; always pass args lazily (call-by-name). +- Cleanup: remove unused imports/variables (scalac `-Ywarn-unused`). +- Stability: avoid changing externally-visible method signatures, SQL output, or parameter binding order. + +Learned rules +- Must not change `QueryResultRow` getter return types or `stringPerConvention` output for existing scenarios. +- Must not change externally-visible SQL generation patterns without updating dependent tests. + +Repo additions +- Project name: balta +- Entry points: `build.sbt`, `balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala` +- Core package: `za.co.absa.db.balta`; supporting: `za.co.absa.db.mag` (naming conventions) +- DB boundary: `DBConnection` (JDBC wrapper), `DBTestSuite` (transaction rollback/commit) +- Contract-sensitive: SQL generation in `DBTable`/`DBFunction`; parameter binding order; `QueryResultRow` getter return types; `stringPerConvention` output +- Coverage tool: sbt-jacoco with JMF filter rules in `jmf-rules.txt` +- Commands: + - sbt "testOnly *UnitTests" # unit tests, no DB needed + - sbt test # all tests, DB must be running + - sbt jacoco # coverage report diff --git a/.github/copilot-review-rules.md b/.github/copilot-review-rules.md new file mode 100644 index 0000000..47fe590 --- /dev/null +++ b/.github/copilot-review-rules.md @@ -0,0 +1,47 @@ +# Copilot Review Rules + +Purpose +- Define consistent, concise, action-oriented review behavior. +- Use short headings and bullet lists; prefer do/avoid constraints over prose; point to code and impact. + +Mode: Default review +- Scope: single PR, normal risk. +- Priority order: correctness → security → tests → maintainability → style. +- Checks: + - Correctness: logic bugs, missing edge cases, regressions, contract changes. + - Security: unsafe input handling, secrets exposure, auth/authz issues, insecure defaults. + - Tests: tests exist for changed logic; success + failure paths covered. + - Maintainability: unnecessary complexity, duplication, unclear naming/structure. + - Style: flag only when readability or repo conventions are broken. +- Response format: short bullets; files + line ranges; severity groups (Blocker / Important / Nit); minimal actionable suggestions; no rewrites. + +Mode: Double-check review +- Scope: higher-risk PRs (security, infra/CI, wide refactors, data migrations, auth changes). +- Additional focus: + - Confirm previous review comments were addressed. + - Re-check: auth, permissions, secrets, persistence, external calls, concurrency. + - Hidden side effects: backward compatibility, failure modes, retries, idempotency. + - Safe defaults: least privilege, secure logging, safe error messages, missing-input behavior. +- Response format: comment only where risk/impact is non-trivial; no repeated style notes; call out explicit risk acceptance (what / why acceptable / mitigation). + +Commenting rules (all modes) +- Every comment must state: what the issue is · why it matters · minimal fix suggestion. +- Prefer linking to existing repo patterns over introducing new ones. +- If context is missing, ask a targeted question instead of assuming. + +Non-goals +- No refactors unrelated to the PR's intent. +- No formatting bikeshedding when the formatter/linter handles it. +- No architectural rewrites unless explicitly requested. + +Repo additions +- High-risk areas: + - DB transactions: `DBTestSuite` rollback/commit; do not change `persistData` behavior unintentionally. + - SQL injection: parameter binding uses `PreparedStatement`; no raw string concatenation of untrusted input. + - JDBC lifecycle: connections must be rolled back or committed; validate `try/finally` in `DBQuerySupport`. + - Contract-sensitive: `QueryResultRow` getter signatures; `stringPerConvention` output; `jmf-rules.txt`. + - Deprecations: do not extend `setParamNull`/`addNull` deprecated API surface. +- Required tests: + - `*UnitTests`: no DB connection; mock `PreparedStatement`/`ResultSet` if needed. + - `*IntegrationTests`: extend `DBTestSuite` or `DBTestingConnection`. + - New naming convention implementations must have a `UnitTests` covering all `LettersCase` variants. diff --git a/.gitignore b/.gitignore index 0a71d64..fbd2fef 100644 --- a/.gitignore +++ b/.gitignore @@ -55,3 +55,4 @@ build.log .hg* .bsp +TODO.md diff --git a/jmf-rules.txt b/jmf-rules.txt index 7c2ade2..5aa1b75 100644 --- a/jmf-rules.txt +++ b/jmf-rules.txt @@ -1,123 +1,13 @@ # jacoco-method-filter — Default Rules & HowTo (Scala) # [jmf:1.0.0] -# -# This file defines which methods should be annotated as *Generated so JaCoCo ignores them. -# One rule per line. -# -# ───────────────────────────────────────────────────────────────────────────── -# HOW TO USE (quick) -# 1) Replace YOUR.PACKAGE.ROOT with your project’s package root (e.g., com.example.app). -# 2) Start with the CONSERVATIVE section only. -# 3) If clean, enable STANDARD. Use AGGRESSIVE only inside DTO/auto‑generated packages. -# 4) Keep rules narrow (by package), prefer flags (synthetic/bridge) for compiler artifacts, -# and add `id:` labels so logs are easy to read. -# -# ───────────────────────────────────────────────────────────────────────────── -# ALLOWED SYNTAX (cheat sheet) -# -# General form: -# #() [FLAGS and PREDICATES...] -# -# FQCN_glob (dot form; $ allowed for inner classes): -# Examples: *.model.*, com.example.*, * -# -# method_glob (glob on method name): -# Examples: copy | $anonfun$* | get* | *_$eq -# -# descriptor_glob (JVM descriptor in (args)ret). You may omit it entirely. -# • Omitting descriptor ⇒ treated as "(*)*" (any args, any return). -# • Short/empty forms "", "()", "(*)" normalize to "(*)*". -# Examples: -# (I)I # takes int, returns int -# (Ljava/lang/String;)V # takes String, returns void -# () or (*) or omitted # any args, any return -# -# FLAGS (optional) — space or comma separated: -# public | protected | private | synthetic | bridge | static | abstract -# -# PREDICATES (optional): -# ret: # match return type only (e.g., ret:V, ret:I, ret:Lcom/example/*;) -# id: # identifier shown in logs/reports -# name-contains: # method name must contain -# name-starts: # method name must start with -# name-ends: # method name must end with -# -# Notes -# - Always use dot-form (com.example.Foo) for class names. -# - Comments (# …) and blank lines are ignored. -# -# ───────────────────────────────────────────────────────────────────────────── -# QUICK EXAMPLES -# -# Simple wildcards -# *#*(*) -# → Match EVERY method in EVERY class (any package). Useful only for diagnostics. -# "(*)" normalizes to "(*)*" ⇒ any args, any return. -# *.dto.*#*(*) -# → Match every method on any class under any package segment named "dto". -# Good when you treat DTOs as generated/boilerplate. -# Scala case class helpers -# *.model.*#copy(*) -# → Matches Scala case-class `copy` methods under `*.model.*`. -# Hides boilerplate clones with any parameter list and any return. -# *.model.*#productArity() -# → Matches zero-arg `productArity` (case-class/Product API). -# *.model.*#productElement(*) -# → Matches `productElement(int)` (or any descriptor form) on case classes. -# *.model.*#productPrefix() -# → Matches `productPrefix()`; returns the case class' constructor name. - -# Companion objects and defaults -# *.model.*$*#apply(*) -# → Matches companion `apply` factories under `*.model.*` (any args). -# BE CAREFUL: can hide real factory logic; keep the package scope narrow. -# *.model.*$*#unapply(*) -# → Matches extractor `unapply` methods in companions under `*.model.*`. -# *#*$default$*(*) -# → Matches Scala-generated default-argument helpers everywhere. -# Safe to keep enabled; they’re compiler-synthesized. - -# Anonymous / synthetic / bridge -# *#$anonfun$* -# → Matches any method whose name contains `$anonfun$` (Scala lambdas). -# Consider adding `synthetic` and/or a package scope in real configs. -# *#*(*):synthetic # any synthetic -# → Matches ANY method marked `synthetic` (compiler-generated). -# Powerful; scope by package to avoid hiding intentional glue code. -# *#*(*):bridge # any bridge -# → Matches Java generic bridge methods the compiler inserts. -# Usually safe globally, but scoping is still recommended. - -# Setters / fluent APIs -# *.dto.*#*_$eq(*) -# → Matches Scala var setters in DTO packages (e.g., `name_=(...)`). -# Good for excluding trivial field writes. -# *.builder.*#with*(*) -# → Matches builder-style fluent setters (`withXxx(...)`) in builder pkgs. -# Treats chainable configuration as boilerplate. -# *.client.*#with*(*) ret:Lcom/api/client/* -# → Like above but ONLY when the return type matches your client package. -# The `ret:` predicate protects real logic that returns other types. - -# Return-type constraints -# *.jobs.*#*(*):ret:V -# → Any method under `*.jobs.*` returning `void` (`V`). Often orchestration. -# *.math.*#*(*):ret:I -# → Any method under `*.math.*` returning primitive int (`I`). -# *.model.*#*(*):ret:Lcom/example/model/* -# → Any method under `*.model.*` that returns a type in `com.example.model`. -# Handy when the *return type* uniquely identifies boilerplate. - -# ───────────────────────────────────────────────────────────────────────────── # GLOBALS RULES -# ───────────────────────────────────────────────────────────────────────────── # ** all case class boilerplate # Scala case class helpers *#canEqual(*) id:case-canequal *#equals(*) id:case-equals -*#apply(*) id:case-apply +#*#apply(*) id:case-apply - disabled: method used in code *#unapply(*) id:case-unapply *#hashCode(*) id:case-hashcode *#copy(*) id:case-copy @@ -150,6 +40,34 @@ # lambda *#* synthetic name-contains:$anonfun$ id:scala-anonfun -# ───────────────────────────────────────────────────────────────────────────── # PROJECT RULES -# ───────────────────────────────────────────────────────────────────────────── \ No newline at end of file + +# proposal to new globals +*#productElementName(*) id:case-prod-element-name +*#productElementNames() id:case-prod-element-names + +# Deprecated single-call delegates: setParamNull / addNull instance methods +# Each method body is a single call to the non-deprecated replacement. +# A unit test would only re-test the replacement method; no assurance is added. +# Note: Params$ companion factory addNull methods are NOT filtered — they contain logic. +*#setParamNull(*) id:balta-deprecated-setnull +Params$NamedParams#addNull(*) id:balta-deprecated-addnull-named +Params$OrderedParams#addNull(*) id:balta-deprecated-addnull-ordered +DBTestSuite#addNull(*) id:balta-deprecated-addnull-suite + +# One-liner factory wrappers in DBTestSuite +# Each method is a single constructor call delegating to DBTable / DBFunction with no logic of its own. +DBTestSuite#table(*) id:balta-factory-table +DBTestSuite#function(*) id:balta-factory-function + +# Trivial implicit conversion: DBConnection → JDBC Connection +# Entire body returns in.connection; no branching or transformation; a test would only assert identity. +DBConnection$#dbConnectionToJdbcConnection(*) id:balta-implicit-dbconn + +# columnLabel: String overloads in QueryResultRow — pure label-resolution delegates +# Each overload resolves the column label to an index via columnNumber(label) then forwards to the +# Int-based overload. The Int-based overloads hold the extraction logic and are covered by integration tests. +# apply(String) and the 2-param getAs(String, transformer) follow the same delegation pattern. +QueryResultRow#get*(java.lang.String) id:balta-colname-getter +QueryResultRow#getAs(java.lang.String,*) id:balta-colname-getas-transformer +QueryResultRow#apply(java.lang.String) id:balta-colname-apply From 3c552dc778011056200f3ea88d795e316560499c Mon Sep 17 00:00:00 2001 From: miroslavpojer Date: Tue, 7 Apr 2026 14:40:15 +0200 Subject: [PATCH 02/13] Fixed tests folder structure. Partial test coverage of QueryResultRow. Fixes from self-review using copilot. --- .../classes => }/DBTestingConnection.scala | 4 +- .../za/co/absa/db/balta/MockResultSets.scala | 279 ++++++++++++++++++ .../classes/DBTableIntegrationTests.scala | 2 +- .../classes/QueryResultIntegrationTests.scala | 37 +++ .../QueryResultRowIntegrationTests.scala | 2 +- .../balta/classes/QueryResultUnitTests.scala | 93 ++++++ .../PostgresRowIntegrationTests.scala | 2 +- ...ryResultRowImplicitsIntegrationTests.scala | 2 +- jmf-rules.txt | 9 + 9 files changed, 424 insertions(+), 6 deletions(-) rename balta/src/test/scala/za/co/absa/db/balta/{testing/classes => }/DBTestingConnection.scala (96%) create mode 100644 balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala create mode 100644 balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultIntegrationTests.scala create mode 100644 balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultUnitTests.scala diff --git a/balta/src/test/scala/za/co/absa/db/balta/testing/classes/DBTestingConnection.scala b/balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala similarity index 96% rename from balta/src/test/scala/za/co/absa/db/balta/testing/classes/DBTestingConnection.scala rename to balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala index fd53ccc..27f2ccb 100644 --- a/balta/src/test/scala/za/co/absa/db/balta/testing/classes/DBTestingConnection.scala +++ b/balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala @@ -14,7 +14,7 @@ * limitations under the License. */ -package za.co.absa.db.balta.testing.classes +package za.co.absa.db.balta import za.co.absa.db.balta.classes.{DBConnection, QueryResult} import za.co.absa.db.balta.classes.inner.ConnectionInfo @@ -31,7 +31,7 @@ trait DBTestingConnection { val preparedStatement = connection.connection.prepareStatement("SELECT now() AS now") val prep = preparedStatement.executeQuery() val result = new QueryResult(prep).next().getOffsetDateTime("now").get - prep.close() + preparedStatement.close() result } diff --git a/balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala b/balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala new file mode 100644 index 0000000..b7d77c2 --- /dev/null +++ b/balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala @@ -0,0 +1,279 @@ +/* + * Copyright 2023 ABSA Group Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package za.co.absa.db.balta + +import java.io.{InputStream, Reader} +import java.math.BigDecimal +import java.net.URL +import java.sql._ + +/** + * Provides factory methods for minimal JDBC ResultSet stubs. + * Mix into any unit test suite that needs to construct a [[za.co.absa.db.balta.classes.QueryResult]] + * without a real database connection. + * + * Each factory method returns a fresh instance so tests do not share mutable state. + */ +trait MockResultSets { + + /** Returns a ResultSet that immediately reports no more rows. */ + def emptyMockResultSet: MockResultSets.MockResultSet = new MockResultSets.MockResultSet(0) + + /** Returns a ResultSet that emits exactly one row (rowNumber = 1). */ + def singleRowMockResultSet: MockResultSets.MockResultSet = new MockResultSets.MockResultSet(0) { + private var consumed = false + override def next(): Boolean = if (!consumed) { consumed = true; row = 1; true } else false + } + + /** Returns a ResultSet that emits exactly two rows (rowNumbers 1 and 2). */ + def twoRowMockResultSet: MockResultSets.MockResultSet = new MockResultSets.MockResultSet(0) { + private var count = 0 + override def next(): Boolean = if (count < 2) { count += 1; row = count; true } else false + } + + /** Returns an empty ResultSet with the given number of columns. */ + def mockResultSetWithCols(cols: Int): MockResultSets.MockResultSet = new MockResultSets.MockResultSet(cols) +} + +object MockResultSets { + + private class MockMetaData(cols: Int) extends ResultSetMetaData { + override def getColumnCount: Int = cols + override def getColumnName(column: Int): String = s"col$column" + override def getColumnType(column: Int): Int = Types.VARCHAR + override def getColumnTypeName(column: Int): String = "varchar" + override def isNullable(column: Int): Int = ResultSetMetaData.columnNullable + override def getColumnLabel(column: Int): String = getColumnName(column) + override def isAutoIncrement(column: Int): Boolean = false + override def isCaseSensitive(column: Int): Boolean = false + override def isSearchable(column: Int): Boolean = false + override def isCurrency(column: Int): Boolean = false + override def isSigned(column: Int): Boolean = false + override def getColumnDisplaySize(column: Int): Int = 0 + override def getSchemaName(column: Int): String = "" + override def getPrecision(column: Int): Int = 0 + override def getScale(column: Int): Int = 0 + override def getTableName(column: Int): String = "" + override def getCatalogName(column: Int): String = "" + override def isReadOnly(column: Int): Boolean = false + override def isWritable(column: Int): Boolean = false + override def isDefinitelyWritable(column: Int): Boolean = false + override def getColumnClassName(column: Int): String = "java.lang.String" + override def unwrap[T](iface: Class[T]): T = throw new UnsupportedOperationException + override def isWrapperFor(iface: Class[_]): Boolean = false + } + + /** + * Minimal ResultSet stub whose `next()` always returns false. + * Subclasses override `next()` to control row availability. + * `getObject(Int)` returns null so that the general extractor wraps each field as None. + */ + class MockResultSet(cols: Int) extends ResultSet { + protected var row: Int = 0 + override def getMetaData: ResultSetMetaData = new MockMetaData(cols) + override def getRow: Int = row + override def next(): Boolean = false + override def getObject(columnIndex: Int): AnyRef = null + override def getObject[T](columnIndex: Int, `type`: Class[T]): T = null.asInstanceOf[T] + override def getArray(columnIndex: Int): java.sql.Array = null + override def getArray(columnLabel: String): java.sql.Array = null + override def close(): Unit = () + override def wasNull(): Boolean = false + override def getString(columnIndex: Int): String = null + override def getBoolean(columnIndex: Int): Boolean = false + override def getByte(columnIndex: Int): Byte = 0 + override def getShort(columnIndex: Int): Short = 0 + override def getInt(columnIndex: Int): Int = 0 + override def getLong(columnIndex: Int): Long = 0L + override def getFloat(columnIndex: Int): Float = 0f + override def getDouble(columnIndex: Int): Double = 0d + override def getBigDecimal(columnIndex: Int, scale: Int): BigDecimal = null + override def getBytes(columnIndex: Int): scala.Array[Byte] = null + override def getDate(columnIndex: Int): Date = null + override def getTime(columnIndex: Int): Time = null + override def getTimestamp(columnIndex: Int): Timestamp = null + override def getAsciiStream(columnIndex: Int): InputStream = null + override def getUnicodeStream(columnIndex: Int): InputStream = null + override def getBinaryStream(columnIndex: Int): InputStream = null + override def getString(columnLabel: String): String = null + override def getBoolean(columnLabel: String): Boolean = false + override def getByte(columnLabel: String): Byte = 0 + override def getShort(columnLabel: String): Short = 0 + override def getInt(columnLabel: String): Int = 0 + override def getLong(columnLabel: String): Long = 0L + override def getFloat(columnLabel: String): Float = 0f + override def getDouble(columnLabel: String): Double = 0d + override def getBigDecimal(columnLabel: String, scale: Int): BigDecimal = null + override def getBytes(columnLabel: String): scala.Array[Byte] = null + override def getDate(columnLabel: String): Date = null + override def getTime(columnLabel: String): Time = null + override def getTimestamp(columnLabel: String): Timestamp = null + override def getAsciiStream(columnLabel: String): InputStream = null + override def getUnicodeStream(columnLabel: String): InputStream = null + override def getBinaryStream(columnLabel: String): InputStream = null + override def getWarnings: SQLWarning = null + override def clearWarnings(): Unit = () + override def getCursorName: String = null + override def getObject(columnLabel: String): AnyRef = null + override def findColumn(columnLabel: String): Int = 0 + override def getCharacterStream(columnIndex: Int): Reader = null + override def getCharacterStream(columnLabel: String): Reader = null + override def getBigDecimal(columnIndex: Int): BigDecimal = null + override def getBigDecimal(columnLabel: String): BigDecimal = null + override def isBeforeFirst: Boolean = false + override def isAfterLast: Boolean = false + override def isFirst: Boolean = false + override def isLast: Boolean = false + override def beforeFirst(): Unit = () + override def afterLast(): Unit = () + override def first(): Boolean = false + override def last(): Boolean = false + override def absolute(row: Int): Boolean = false + override def relative(rows: Int): Boolean = false + override def previous(): Boolean = false + override def setFetchDirection(direction: Int): Unit = () + override def getFetchDirection: Int = 0 + override def setFetchSize(rows: Int): Unit = () + override def getFetchSize: Int = 0 + override def getType: Int = ResultSet.TYPE_FORWARD_ONLY + override def getConcurrency: Int = ResultSet.CONCUR_READ_ONLY + override def rowUpdated(): Boolean = false + override def rowInserted(): Boolean = false + override def rowDeleted(): Boolean = false + override def updateNull(columnIndex: Int): Unit = () + override def updateBoolean(columnIndex: Int, x: Boolean): Unit = () + override def updateByte(columnIndex: Int, x: Byte): Unit = () + override def updateShort(columnIndex: Int, x: Short): Unit = () + override def updateInt(columnIndex: Int, x: Int): Unit = () + override def updateLong(columnIndex: Int, x: Long): Unit = () + override def updateFloat(columnIndex: Int, x: Float): Unit = () + override def updateDouble(columnIndex: Int, x: Double): Unit = () + override def updateBigDecimal(columnIndex: Int, x: BigDecimal): Unit = () + override def updateString(columnIndex: Int, x: String): Unit = () + override def updateBytes(columnIndex: Int, x: scala.Array[Byte]): Unit = () + override def updateDate(columnIndex: Int, x: Date): Unit = () + override def updateTime(columnIndex: Int, x: Time): Unit = () + override def updateTimestamp(columnIndex: Int, x: Timestamp): Unit = () + override def updateAsciiStream(columnIndex: Int, x: InputStream, length: Int): Unit = () + override def updateBinaryStream(columnIndex: Int, x: InputStream, length: Int): Unit = () + override def updateCharacterStream(columnIndex: Int, x: Reader, length: Int): Unit = () + override def updateObject(columnIndex: Int, x: AnyRef, scaleOrLength: Int): Unit = () + override def updateObject(columnIndex: Int, x: AnyRef): Unit = () + override def updateNull(columnLabel: String): Unit = () + override def updateBoolean(columnLabel: String, x: Boolean): Unit = () + override def updateByte(columnLabel: String, x: Byte): Unit = () + override def updateShort(columnLabel: String, x: Short): Unit = () + override def updateInt(columnLabel: String, x: Int): Unit = () + override def updateLong(columnLabel: String, x: Long): Unit = () + override def updateFloat(columnLabel: String, x: Float): Unit = () + override def updateDouble(columnLabel: String, x: Double): Unit = () + override def updateBigDecimal(columnLabel: String, x: BigDecimal): Unit = () + override def updateString(columnLabel: String, x: String): Unit = () + override def updateBytes(columnLabel: String, x: scala.Array[Byte]): Unit = () + override def updateDate(columnLabel: String, x: Date): Unit = () + override def updateTime(columnLabel: String, x: Time): Unit = () + override def updateTimestamp(columnLabel: String, x: Timestamp): Unit = () + override def updateAsciiStream(columnLabel: String, x: InputStream, length: Int): Unit = () + override def updateBinaryStream(columnLabel: String, x: InputStream, length: Int): Unit = () + override def updateCharacterStream(columnLabel: String, x: Reader, length: Int): Unit = () + override def updateObject(columnLabel: String, x: AnyRef, scaleOrLength: Int): Unit = () + override def updateObject(columnLabel: String, x: AnyRef): Unit = () + override def insertRow(): Unit = () + override def updateRow(): Unit = () + override def deleteRow(): Unit = () + override def refreshRow(): Unit = () + override def cancelRowUpdates(): Unit = () + override def moveToInsertRow(): Unit = () + override def moveToCurrentRow(): Unit = () + override def getStatement: Statement = null + override def getObject(columnIndex: Int, map: java.util.Map[String, Class[_]]): AnyRef = null + override def getRef(columnIndex: Int): Ref = null + override def getBlob(columnIndex: Int): Blob = null + override def getClob(columnIndex: Int): Clob = null + override def getObject(columnLabel: String, map: java.util.Map[String, Class[_]]): AnyRef = null + override def getRef(columnLabel: String): Ref = null + override def getBlob(columnLabel: String): Blob = null + override def getClob(columnLabel: String): Clob = null + override def getDate(columnIndex: Int, cal: java.util.Calendar): Date = null + override def getDate(columnLabel: String, cal: java.util.Calendar): Date = null + override def getTime(columnIndex: Int, cal: java.util.Calendar): Time = null + override def getTime(columnLabel: String, cal: java.util.Calendar): Time = null + override def getTimestamp(columnIndex: Int, cal: java.util.Calendar): Timestamp = null + override def getTimestamp(columnLabel: String, cal: java.util.Calendar): Timestamp = null + override def getURL(columnIndex: Int): URL = null + override def getURL(columnLabel: String): URL = null + override def updateRef(columnIndex: Int, x: Ref): Unit = () + override def updateRef(columnLabel: String, x: Ref): Unit = () + override def updateBlob(columnIndex: Int, x: Blob): Unit = () + override def updateBlob(columnLabel: String, x: Blob): Unit = () + override def updateClob(columnIndex: Int, x: Clob): Unit = () + override def updateClob(columnLabel: String, x: Clob): Unit = () + override def updateArray(columnIndex: Int, x: java.sql.Array): Unit = () + override def updateArray(columnLabel: String, x: java.sql.Array): Unit = () + override def getRowId(columnIndex: Int): RowId = null + override def getRowId(columnLabel: String): RowId = null + override def updateRowId(columnIndex: Int, x: RowId): Unit = () + override def updateRowId(columnLabel: String, x: RowId): Unit = () + override def getHoldability: Int = 0 + override def isClosed: Boolean = false + override def updateNString(columnIndex: Int, nString: String): Unit = () + override def updateNString(columnLabel: String, nString: String): Unit = () + override def updateNClob(columnIndex: Int, nClob: NClob): Unit = () + override def updateNClob(columnLabel: String, nClob: NClob): Unit = () + override def getNClob(columnIndex: Int): NClob = null + override def getNClob(columnLabel: String): NClob = null + override def getSQLXML(columnIndex: Int): SQLXML = null + override def getSQLXML(columnLabel: String): SQLXML = null + override def updateSQLXML(columnIndex: Int, xmlObject: SQLXML): Unit = () + override def updateSQLXML(columnLabel: String, xmlObject: SQLXML): Unit = () + override def getNString(columnIndex: Int): String = null + override def getNString(columnLabel: String): String = null + override def getNCharacterStream(columnIndex: Int): Reader = null + override def getNCharacterStream(columnLabel: String): Reader = null + override def updateNCharacterStream(columnIndex: Int, x: Reader, length: Long): Unit = () + override def updateNCharacterStream(columnLabel: String, x: Reader, length: Long): Unit = () + override def updateAsciiStream(columnIndex: Int, x: InputStream, length: Long): Unit = () + override def updateBinaryStream(columnIndex: Int, x: InputStream, length: Long): Unit = () + override def updateCharacterStream(columnIndex: Int, x: Reader, length: Long): Unit = () + override def updateAsciiStream(columnLabel: String, x: InputStream, length: Long): Unit = () + override def updateBinaryStream(columnLabel: String, x: InputStream, length: Long): Unit = () + override def updateCharacterStream(columnLabel: String, x: Reader, length: Long): Unit = () + override def updateBlob(columnIndex: Int, inputStream: InputStream, length: Long): Unit = () + override def updateBlob(columnLabel: String, inputStream: InputStream, length: Long): Unit = () + override def updateClob(columnIndex: Int, reader: Reader, length: Long): Unit = () + override def updateClob(columnLabel: String, reader: Reader, length: Long): Unit = () + override def updateNClob(columnIndex: Int, reader: Reader, length: Long): Unit = () + override def updateNClob(columnLabel: String, reader: Reader, length: Long): Unit = () + override def updateNCharacterStream(columnIndex: Int, x: Reader): Unit = () + override def updateNCharacterStream(columnLabel: String, x: Reader): Unit = () + override def updateAsciiStream(columnIndex: Int, x: InputStream): Unit = () + override def updateBinaryStream(columnIndex: Int, x: InputStream): Unit = () + override def updateCharacterStream(columnIndex: Int, x: Reader): Unit = () + override def updateAsciiStream(columnLabel: String, x: InputStream): Unit = () + override def updateBinaryStream(columnLabel: String, x: InputStream): Unit = () + override def updateCharacterStream(columnLabel: String, x: Reader): Unit = () + override def updateBlob(columnIndex: Int, inputStream: InputStream): Unit = () + override def updateBlob(columnLabel: String, inputStream: InputStream): Unit = () + override def updateClob(columnIndex: Int, reader: Reader): Unit = () + override def updateClob(columnLabel: String, reader: Reader): Unit = () + override def updateNClob(columnIndex: Int, reader: Reader): Unit = () + override def updateNClob(columnLabel: String, reader: Reader): Unit = () + override def getObject[T](columnLabel: String, `type`: Class[T]): T = null.asInstanceOf[T] + override def unwrap[T](iface: Class[T]): T = throw new UnsupportedOperationException + override def isWrapperFor(iface: Class[_]): Boolean = false + } +} diff --git a/balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala b/balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala index b84902c..94a3045 100644 --- a/balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala +++ b/balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala @@ -18,7 +18,7 @@ package za.co.absa.db.balta.classes import org.scalatest.funsuite.AnyFunSuiteLike import za.co.absa.db.balta.classes.inner.Params -import za.co.absa.db.balta.testing.classes.DBTestingConnection +import za.co.absa.db.balta.DBTestingConnection import za.co.absa.db.balta.implicits.OptionImplicits.OptionEnhancements import DBTableIntegrationTests.QueryResultRowAssertion diff --git a/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultIntegrationTests.scala b/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultIntegrationTests.scala new file mode 100644 index 0000000..42dfb0f --- /dev/null +++ b/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultIntegrationTests.scala @@ -0,0 +1,37 @@ +/* + * Copyright 2023 ABSA Group Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package za.co.absa.db.balta.classes + +import org.scalatest.funsuite.AnyFunSuiteLike +import za.co.absa.db.balta.DBTestingConnection + +class QueryResultIntegrationTests extends AnyFunSuiteLike with DBTestingConnection { + + test("noMore is true after all rows are consumed") { + DBTable("testing.base_types").all() { qr => + while (qr.hasNext) qr.next() + assert(qr.noMore) + } + } + + test("resultSetMetaData exposes the correct column count") { + DBTable("testing.base_types").all() { qr => + assert(qr.resultSetMetaData.getColumnCount == 14) + } + } + +} diff --git a/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala b/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala index bd82773..6cc163d 100644 --- a/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala +++ b/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scala @@ -17,7 +17,7 @@ package za.co.absa.db.balta.classes import org.scalatest.funsuite.AnyFunSuiteLike -import za.co.absa.db.balta.testing.classes.DBTestingConnection +import za.co.absa.db.balta.DBTestingConnection import java.sql.{Date, ResultSetMetaData, Time} import java.text.SimpleDateFormat diff --git a/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultUnitTests.scala b/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultUnitTests.scala new file mode 100644 index 0000000..43facdf --- /dev/null +++ b/balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultUnitTests.scala @@ -0,0 +1,93 @@ +/* + * Copyright 2023 ABSA Group Limited + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package za.co.absa.db.balta.classes + +import org.scalatest.funsuite.AnyFunSuiteLike +import za.co.absa.db.balta.MockResultSets + +import java.sql.SQLException + +class QueryResultUnitTests extends AnyFunSuiteLike with MockResultSets { + + test("hasNext returns false for an empty result set") { + val qr = new QueryResult(emptyMockResultSet) + assert(!qr.hasNext) + } + + test("hasNext returns true when at least one row is available") { + val qr = new QueryResult(singleRowMockResultSet) + assert(qr.hasNext) + } + + test("hasNext returns true after repeated calls without consuming the row") { + val qr = new QueryResult(singleRowMockResultSet) + assert(qr.hasNext) + assert(qr.hasNext) + } + + test("hasNext returns false after the only row has been consumed") { + val qr = new QueryResult(singleRowMockResultSet) + qr.next() + assert(!qr.hasNext) + } + + test("hasNext swallows SQLException and returns false") { + val throwingResultSet = new MockResultSets.MockResultSet(0) { + private var called = false + override def next(): Boolean = { + if (!called) { + called = true + throw new SQLException("simulated failure") + } + false + } + } + val qr = new QueryResult(throwingResultSet) + assert(!qr.hasNext) + } + + test("next returns the first row when result set is non-empty") { + val qr = new QueryResult(singleRowMockResultSet) + val row = qr.next() + assert(row.rowNumber == 1) + } + + test("next advances through all rows in order") { + val qr = new QueryResult(twoRowMockResultSet) + val first = qr.next() + val second = qr.next() + assert(first.rowNumber == 1) + assert(second.rowNumber == 2) + } + + test("next throws NoSuchElementException when result set is exhausted") { + val qr = new QueryResult(emptyMockResultSet) + assertThrows[NoSuchElementException](qr.next()) + } + + test("next throws NoSuchElementException after consuming all rows") { + val qr = new QueryResult(singleRowMockResultSet) + qr.next() + assertThrows[NoSuchElementException](qr.next()) + } + + test("columnCount reflects the value reported by ResultSetMetaData") { + val qr = new QueryResult(mockResultSetWithCols(3)) + assert(qr.columnCount == 3) + } + +} diff --git a/balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala b/balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala index e2bb630..fb18d3d 100644 --- a/balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala +++ b/balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala @@ -20,7 +20,7 @@ import org.scalatest.funsuite.AnyFunSuiteLike import za.co.absa.db.balta.classes.{DBTable, QueryResultRow} import za.co.absa.db.balta.postgres.implicits.Postgres.PostgresRow import za.co.absa.db.balta.postgres.classes.SimpleJsonString -import za.co.absa.db.balta.testing.classes.DBTestingConnection +import za.co.absa.db.balta.DBTestingConnection import java.sql.ResultSetMetaData diff --git a/balta/src/test/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicitsIntegrationTests.scala b/balta/src/test/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicitsIntegrationTests.scala index 65e8b0d..fdd7676 100644 --- a/balta/src/test/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicitsIntegrationTests.scala +++ b/balta/src/test/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicitsIntegrationTests.scala @@ -21,7 +21,7 @@ import java.util.UUID import org.scalatest.funsuite.AnyFunSuiteLike import za.co.absa.db.balta.classes.DBFunction import za.co.absa.db.balta.implicits.QueryResultRowImplicits.ProductTypeConvertor -import za.co.absa.db.balta.testing.classes.DBTestingConnection +import za.co.absa.db.balta.DBTestingConnection import za.co.absa.db.mag.naming.NamingConvention import za.co.absa.db.mag.naming.implementations.MapBasedNaming diff --git a/jmf-rules.txt b/jmf-rules.txt index 5aa1b75..d5323fb 100644 --- a/jmf-rules.txt +++ b/jmf-rules.txt @@ -71,3 +71,12 @@ DBConnection$#dbConnectionToJdbcConnection(*) id:balta-implicit-dbconn QueryResultRow#get*(java.lang.String) id:balta-colname-getter QueryResultRow#getAs(java.lang.String,*) id:balta-colname-getas-transformer QueryResultRow#apply(java.lang.String) id:balta-colname-apply + +# QueryResult#noMore — trivial negation of hasNext +# Body is exactly `!hasNext`; testing it adds no assurance beyond the hasNext unit tests. +QueryResult#noMore() id:balta-nomore + +# QueryResult#resultSetMetaData — implicit field accessor (public val) +# Scala compiles a public val to a 0-arg getter that returns the backing field. +# No logic; a test would only assert identity of what was passed to the constructor. +QueryResult#resultSetMetaData() id:balta-resultsetmetadata-accessor From 8ca828514cc6d273746dae799072cb54cefd9426 Mon Sep 17 00:00:00 2001 From: miroslavpojer Date: Tue, 7 Apr 2026 15:10:05 +0200 Subject: [PATCH 03/13] fix: update testing guidelines and filename exclusions in workflows --- .github/copilot-instructions.md | 4 ++-- .github/workflows/test_filenames_check.yml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 3b64025..37e28ed 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -50,8 +50,8 @@ Testing - Integration test files: name ends with `IntegrationTests`; must extend `DBTestSuite` or mix in `DBTestingConnection`. - Must not access private members of the class under test. - Prefer `AnyFunSuite`/`AnyFunSuiteLike`; use `AnyWordSpec` only when given/when/then adds clarity. -- Must place shared test helpers and fixtures in a dedicated `testing` sub-package and reuse them across tests. -- Must not add top-level comments in `*Tests.scala` outside test methods; use nested `describe`/`"..."` blocks to separate groups. +- Must place shared test helpers and fixtures in the root test package (`za.co.absa.db.balta`) and reuse them across tests; shared files are excluded from the filename-inspector check. +- Must not add top-level comments in `*Tests.scala` outside test methods; use nested `"..."` / `should` / `when` blocks (idiomatic to the chosen suite style) to separate groups. - Prefer TDD workflow: - Must create or update `SPEC.md` before writing any code, listing scenarios, inputs, and expected outputs. - Must propose the full test case set (name + intent + input + expected output) and wait for user confirmation before coding. diff --git a/.github/workflows/test_filenames_check.yml b/.github/workflows/test_filenames_check.yml index 804f280..0fe8418 100644 --- a/.github/workflows/test_filenames_check.yml +++ b/.github/workflows/test_filenames_check.yml @@ -36,6 +36,6 @@ jobs: name-patterns: '*UnitTests.*,*IntegrationTests.*' paths: '**/src/test/scala/**' report-format: 'console' - excludes: 'balta/src/test/scala/za/co/absa/db/balta/testing/*' + excludes: 'balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala,balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala' verbose-logging: 'false' fail-on-violation: 'true' From 0c236f75b011ba244fdb23041f0d69bda771d39f Mon Sep 17 00:00:00 2001 From: miroslavpojer Date: Mon, 13 Apr 2026 19:24:28 +0200 Subject: [PATCH 04/13] Improved to clean solution as reaction to review comment. --- .github/workflows/test_filenames_check.yml | 2 +- .../za/co/absa/db/balta/{ => testing}/DBTestingConnection.scala | 0 .../za/co/absa/db/balta/{ => testing}/MockResultSets.scala | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename balta/src/test/scala/za/co/absa/db/balta/{ => testing}/DBTestingConnection.scala (100%) rename balta/src/test/scala/za/co/absa/db/balta/{ => testing}/MockResultSets.scala (100%) diff --git a/.github/workflows/test_filenames_check.yml b/.github/workflows/test_filenames_check.yml index 0fe8418..44d296c 100644 --- a/.github/workflows/test_filenames_check.yml +++ b/.github/workflows/test_filenames_check.yml @@ -36,6 +36,6 @@ jobs: name-patterns: '*UnitTests.*,*IntegrationTests.*' paths: '**/src/test/scala/**' report-format: 'console' - excludes: 'balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala,balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala' + excludes: 'balta/src/test/scala/za/co/absa/db/balta/testing/**' verbose-logging: 'false' fail-on-violation: 'true' diff --git a/balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala b/balta/src/test/scala/za/co/absa/db/balta/testing/DBTestingConnection.scala similarity index 100% rename from balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala rename to balta/src/test/scala/za/co/absa/db/balta/testing/DBTestingConnection.scala diff --git a/balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala b/balta/src/test/scala/za/co/absa/db/balta/testing/MockResultSets.scala similarity index 100% rename from balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala rename to balta/src/test/scala/za/co/absa/db/balta/testing/MockResultSets.scala From 992f929d8da873a13a9401b25379a4c564c2d908 Mon Sep 17 00:00:00 2001 From: miroslavpojer Date: Tue, 14 Apr 2026 08:17:30 +0200 Subject: [PATCH 05/13] Add unit tests and database configuration for DBFunction and QueryResultRow - 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. --- .github/copilot-instructions.md | 26 +- JMF-NOTES.md | 429 ++++++++++++++++++ .../resources/database-persist.properties | 7 + .../absa/db/balta/DBTestSuiteUnitTests.scala | 31 ++ .../balta/classes/DBFunctionUnitTests.scala | 71 +++ .../classes/QueryResultRowUnitTests.scala | 310 +++++++++++++ .../balta/classes/inner/ParamsUnitTests.scala | 94 ++++ .../testing/RecordingPreparedStatement.scala | 134 ++++++ .../typeclasses/QueryParamTypeUnitTests.scala | 131 ++++++ .../QueryParamValueUnitTests.scala | 67 +++ .../SnakeCaseNamingUnitTests.scala | 13 + jmf-rules.txt | 321 ++++++++++++- 12 files changed, 1618 insertions(+), 16 deletions(-) create mode 100644 JMF-NOTES.md create mode 100644 balta/src/test/resources/database-persist.properties create mode 100644 balta/src/test/scala/za/co/absa/db/balta/classes/DBFunctionUnitTests.scala create mode 100644 balta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowUnitTests.scala create mode 100644 balta/src/test/scala/za/co/absa/db/balta/classes/inner/ParamsUnitTests.scala create mode 100644 balta/src/test/scala/za/co/absa/db/balta/testing/RecordingPreparedStatement.scala create mode 100644 balta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamTypeUnitTests.scala create mode 100644 balta/src/test/scala/za/co/absa/db/balta/typeclasses/QueryParamValueUnitTests.scala diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 37e28ed..2af2f79 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -58,17 +58,32 @@ Testing - Must write all failing tests first (red), then implement until all pass (green). - Must cover all distinct combinations; each test must state its scenario in the ScalaDoc. - Must update `SPEC.md` after all tests pass with the confirmed test case table. +- When a unit test adds value — write one: + - Method has any own logic: branching (`if`/`match`), exception handling or swallowing, non-trivial transformation, config/resource read, or reflection. +- When to add to `jmf-rules.txt` instead of writing a unit test: + - Body is a single call with no own logic: forwards to another overload, calls its non-deprecated replacement, returns a field, or wraps a constructor with no transformation. + - Litmus: "Does this method have any logic of its own?" — No → JMF. +- Review rule — JMF drift check: when modifying a method that appears in `jmf-rules.txt`, verify its body still qualifies; if own logic has been added, remove the JMF rule and write a unit test instead. Tooling - Must format with scalafmt (`.scalafmt.conf`); lint with scalastyle (`scalastyle-config.xml`) or wartremover as configured. - Compiler warnings treated as errors where configured; coverage ≥ 80% via sbt-jacoco (excluding JMF-filtered methods in `jmf-rules.txt`). Coverage filtering (JMF) -- A method qualifies for a JMF filter rule if it meets at least one criterion: - - No added value: trivial delegate, one-liner factory, or pure field accessor — a test adds no assurance beyond testing the delegated method. - - Not coverable without integration tests: requires a DB/external system; same path already exercised by integration tests. -- Qualifying patterns: deprecated single-call delegates; `columnLabel: String` overloads that only call `columnNumber(label)` + Int-based overload; one-liner factory wrappers; implicit single-field accessors. +- A method qualifies when its body is a single call with no own logic (see Testing section for the full decision rule and qualifying patterns). - Must not add JMF rules for methods with branching logic, error handling, or non-trivial transformations. +- Rules file: `jmf-rules.txt` (version `[jmf:1.0.0]`); one rule per line, `#` comments and blank lines ignored. +- Rule syntax: `#() [FLAGS] [PREDICATES]` + - FQCN_glob: dot-form class pattern (`*`, `*.model.*`, `com.example.*`; `$` for inner/companion classes). + - method_glob: glob on method name (`copy`, `get*`, `$anonfun$*`, `*_$eq`). + - descriptor_glob: JVM descriptor `(args)ret`; omitting or `(*)` means any args/any return. Must use JVM internal format: `(I)*` not `(int)`, `(Z)*` not `(boolean)`, `(Ljava/lang/String;)*` not `(java.lang.String)`. Non-matching descriptors are silently ignored — no warning is emitted. + - FLAGS (space/comma separated): `public`, `protected`, `private`, `synthetic`, `bridge`, `static`, `abstract`. + - PREDICATES: `ret:` (return type), `id:` (log label), `name-contains:`, `name-starts:`, `name-ends:`. +- Every rule Must include an `id:` label for traceability. +- Adoption order: start with CONSERVATIVE (case-class boilerplate, compiler synthetics), then STANDARD; use AGGRESSIVE only for DTO/auto-generated packages. +- Prefer narrow package scopes; prefer `synthetic`/`bridge` flags for compiler artifacts over broad wildcards. +- Must prefix project-specific FQCN globs with `*` so they match the full qualified class name (e.g. `*QueryResult#noMore()`, not `QueryResult#noMore()`). +- When adding a project-specific rule, add it under the `# PROJECT RULES` section with a comment explaining why the method qualifies. Quality gates - sbt "testOnly *UnitTests" # unit tests, no DB needed @@ -83,10 +98,13 @@ Common pitfalls to avoid - Logging: never pre-build log strings; always pass args lazily (call-by-name). - Cleanup: remove unused imports/variables (scalac `-Ywarn-unused`). - Stability: avoid changing externally-visible method signatures, SQL output, or parameter binding order. +- JMF silent failures: rules with non-matching FQCN or descriptor globs are silently ignored; always verify new rules take effect by comparing JaCoCo method-level output before and after. +- Scala reflection: case classes used with `currentMirror.reflectClass` in tests must be top-level (package scope), not inner classes — Scala reflection cannot handle inner class mirrors. Learned rules - Must not change `QueryResultRow` getter return types or `stringPerConvention` output for existing scenarios. - Must not change externally-visible SQL generation patterns without updating dependent tests. +- Scala 2.12 compiler artifacts that generate coverable bytecode: `$anonfun$` lambdas (ACC_SYNTHETIC), `$deserializeLambda$` (private static, not synthetic), Function1 mixin forwarders (`andThen`/`compose`), value class `$extension` methods, `$default$` parameter methods, Iterator trait mixin (~80+ forwarders). These are JMF candidates, not unit-test candidates. Repo additions - Project name: balta diff --git a/JMF-NOTES.md b/JMF-NOTES.md new file mode 100644 index 0000000..fa0df4c --- /dev/null +++ b/JMF-NOTES.md @@ -0,0 +1,429 @@ +# JMF (jacoco-method-filter) — Lessons Learned + +Practical notes for anyone maintaining or extending `jmf-rules.txt` in this project. +Accumulated from real debugging sessions; each entry caused at least one silent coverage regression. + +--- + +## 1. Descriptor format must be JVM internal, not human-readable + +**Wrong:** +``` +*QueryResultRow#apply(int)* id:qrr-apply-int +*QueryResultRow#getAs(java.lang.String,*)* id:qrr-getas-str +``` +**Right:** +``` +*QueryResultRow#apply(I)* id:qrr-apply-int +*QueryResultRow#getAs(Ljava/lang/String;*)* id:qrr-getas-str +``` + +JMF compares against raw JVM bytecode descriptors, not source-level types. +Common type mappings: + +| Source type | JVM descriptor | +|----------------|------------------------------| +| `int` | `I` | +| `boolean` | `Z` | +| `long` | `J` | +| `double` | `D` | +| `String` | `Ljava/lang/String;` | +| `Option[A]` | `Lscala/Option;` | +| `Unit` / void | `V` | +| array of int | `[I` | + +Use `javap -p -verbose ` to see the actual descriptors for any method. + +--- + +## 2. FQCN globs must start with `*` to match qualified class names + +**Wrong:** +``` +QueryResult#noMore() id:qr-nomore +``` +**Right:** +``` +*QueryResult#noMore() id:qr-nomore +``` + +Without the leading `*`, the glob is matched literally and will never match +`za.co.absa.db.balta.classes.QueryResult`. + +--- + +## 3. Non-matching rules are silently ignored — no warning, no error + +JMF does not report unmatched rules. A rule can be completely wrong and the tool will +load it, count it, and do nothing — while JaCoCo continues to count the method as missed. + +**Consequence:** Coverage numbers may look fine if another test happens to cover the same +method via a different path (e.g., an integration test). The rule appears to work for months, +then suddenly "breaks" when that test is removed or the method is called less often. + +**How to verify a rule is actually filtering:** +1. Run `sbt ++2.12.18 jacoco` and note the JMF log line: `Marked N methods`. +2. Add the new rule. +3. Run again and confirm `N` increased. +4. Cross-reference `balta/target/scala-2.12/jacoco/jacoco.xml` to confirm the specific + method now has `mi="0"` (missed instructions = 0) AND `ci` dropped by the expected amount. + +--- + +## 4. Diagnosing a missed-coverage method + +When JaCoCo reports missed instructions after you believe you have a rule: + +1. Find the method in `jacoco.xml`: + ``` + grep -A2 'name="myMethod"' balta/target/scala-2.12/jacoco/jacoco.xml + ``` +2. Check `mi` (missed instructions) and `nr` (not-reached branches). +3. Get the actual bytecode descriptor: + ``` + javap -p -verbose balta/target/scala-2.12/classes/za/co/absa/.../MyClass.class \ + | grep -A4 "myMethod" + ``` +4. Compare the descriptor in your rule against the bytecode output. + Common mistakes: `(int)` vs `(I)`, missing `;` after object type, `*` vs explicit return type. + +--- + +## 5. Scala 2.12 compiler-generated methods that produce coverable bytecode + +These arise automatically and are JMF candidates (no own logic), not unit-test candidates: + +| Pattern | Description | +|---------|-------------| +| `$anonfun$*` (ACC_SYNTHETIC) | Lambda bodies lifted to class methods | +| `$deserializeLambda$` (private static, NOT synthetic) | SerializedLambda support | +| `andThen` / `compose` | Function1 trait mixin forwarders | +| `*$extension` | Value class extension methods (boxed path) | +| `*$default$*` | Default parameter methods (`$default$1`, `$default$2`, …) | +| Iterator trait mixins (~80+) | `hasNext`, `next`, `drop`, `take`, `sliding`, etc. | +| `writeReplace` | Java serialization hook for case classes | + +When a method's coverage is missed and its source is compiler-generated, add a JMF rule +rather than writing a test. + +--- + +## 6. Verifying new rules end-to-end + +Minimal workflow before committing any new JMF rule: + +```bash +# 1. baseline — note "Marked N methods" in sbt output +sbt '++2.12.18; jacoco' + +# 2. add rule to jmf-rules.txt + +# 3. re-run — "Marked N+k methods" confirms the rule matched +sbt '++2.12.18; jacoco' + +# 4. quick coverage check +awk -F',' 'NR>1{m+=$5; c+=$6} END{printf "Missed: %d Covered: %d Total: %d Coverage: %.1f%%\n", m, c, m+c, c*100/(m+c)}' \ + balta/target/scala-2.12/jacoco/jacoco.csv +``` + +--- + +## 7. Avoid broad wildcards — prefer `synthetic`/`bridge` flags + +A rule like `*#*$anonfun$*(*)` with the `synthetic` flag is safer than an unqualified +glob that might accidentally suppress coverage for hand-written lambdas. + +``` +*#$anonfun$*(*) synthetic id:scala-anonfun +``` + +The `synthetic` flag restricts the rule to bytecode methods with ACC_SYNTHETIC set, +which is exactly what the Scala compiler emits for lifted lambdas. + +--- + +## 8. Scala reflection in tests — case classes must be top-level + +When writing unit tests that call `currentMirror.reflectClass` (e.g., for +`toProductType`-style tests), the case class **must** be declared at package scope, +not as a nested/inner class inside the test class. + +```scala +// WRONG: inner class, reflection fails at runtime +class MyUnitTests extends AnyFunSuite { + case class MyRecord(id: Int, name: String) + test("...") { ... currentMirror.reflectClass(mirror) ... } // NoHostException +} + +// RIGHT: top-level, reflection works +case class MyRecord(id: Int, name: String) +class MyUnitTests extends AnyFunSuite { + test("...") { ... currentMirror.reflectClass(mirror) ... } // OK +} +``` + +--- + +## 9. Rule file version header + +The first non-comment line must be `[jmf:1.0.0]` (or the appropriate version). +Missing or malformed version headers cause the entire rule file to be skipped silently. +Confirm the file is loaded by checking the sbt jacoco log for `Loaded N rules`. + +--- + +## 10. Misleading or incorrect hints in the official JMF tutorial + +The bundled HowTo comment block (`jmf-rules.txt` header) contains several claims that are +inconsistent, misleading, or contradicted by tested behaviour. Do not copy examples from it +without checking below. + +### 10a. Colon-prefix flag syntax in Quick Examples is wrong + +The Quick Examples section writes: +``` +*#*(*):synthetic # any synthetic +*#*(*):bridge # any bridge +``` +The colon before `synthetic`/`bridge` does **not** match the FLAGS syntax described just +above it, which says "space or comma separated". Our confirmed working syntax is: +``` +*#* synthetic id:some-label +*#$anonfun$*(*) synthetic name-contains:$anonfun$ id:scala-anonfun +``` +The colon-prefix format may be silently ignored (rule loads but never matches), which is +the same failure mode as wrong descriptors. Do not use `:` — use space-separated flags. + +### 10b. `ret:` predicate syntax is inconsistent within the tutorial + +The tutorial shows `ret:` two different ways: +``` +# inconsistent — colon-prefixed +*.jobs.*#*(*):ret:V + +# inconsistent — space-separated +*.client.*#with*(*) ret:Lcom/api/client/*; +``` +The space-separated form matches the PREDICATES spec. Use space-separated: +``` +*MyClass#myMethod(*) ret:V id:my-label +``` +Never use `:ret:` (double colon, colon-prefixed). + +### 10c. Tutorial does not warn about the FQCN `*` prefix requirement + +All Quick Examples use fully-anchored package globs like `*.model.*#copy(*)`, which +implicitly start with `*`. The tutorial never states that a bare class name +(`QueryResult#noMore()`) will NOT match a fully-qualified name +(`za.co.absa.db.balta.classes.QueryResult`). + +This silence has caused real bugs: rules written as `ClassName#method()` compile and load, +appear in the "N rules loaded" count, but never filter anything. + +**Always prefix project-specific class names with `*`:** +``` +# WRONG — silently never matches +QueryResult#noMore() id:qr-nomore + +# RIGHT +*QueryResult#noMore() id:qr-nomore +``` + +### 10d. Tutorial descriptor examples do not demonstrate JVM-format requirement + +The ALLOWED SYNTAX section correctly shows JVM format (`(I)I`, `(Ljava/lang/String;)V`) +but the Quick Examples section uses only `(*)` everywhere, giving the impression that +type-specific descriptors are rarely needed. In practice: + +- Any rule targeting a method that takes `int`, `boolean`, or any object type by specific + type **must** use JVM format. +- Writing `(int)`, `(boolean)`, `(java.lang.String)` will silently not match. +- The tutorial provides no warning that human-readable types produce silent failures. + +See section 1 of this file for the correct mapping table. + +### 10e. Tutorial claims empty/short descriptor forms are equivalent — verify before trusting + +The tutorial states: `"(*)" normalizes to "(*)*" ⇒ any args, any return.` +This is consistent with our tested experience for wildcard rules. However, when mixing +`(*)` with typed descriptors in the same class, `(*)` may match more broadly than intended. +Prefer an explicit descriptor when targeting a specific overload. + +### 10f. Empty parens `()` reads as "no args" but means "any args" due to normalisation + +The tutorial global rule examples include: +``` +*#productElement() +*#productArity() +*#productIterator() +``` +A reader naturally interprets `()` as "this method takes no arguments". But `productElement` +has the signature `def productElement(n: Int): Any` — its JVM descriptor is `(I)Ljava/lang/Object;`. +The rule still works because `()` normalises to `(*)*` (any args, any return). + +**Consequence:** These rules match more than you think. A rule written as `*#myMethod()` to +target a specific no-arg overload will also match every overload of `myMethod`, including +ones you may want to test. Use an explicit descriptor when targeting a single overload: +``` +*#mySpecificMethod()V # actually no-arg, returns void +*#myOverloadedMethod(I)* # specifically the int overload +``` + +### 10g. Tutorial's own Quick Example comments use human-readable types + +The formal ALLOWED SYNTAX section correctly shows JVM format, but the very next section's +inline explanatory comments contradict it: +``` +*.model.*#productElement(*) + → Matches `productElement(int)` (or any descriptor form) on case classes. + ^^^^ human-readable — not valid if used in a rule +``` +The parenthetical "(or any descriptor form)" hints at the issue but does not explain it. +Any reader who copies the comment text into a rule will get a silently non-matching rule. + +### 10h. `ret:` object type globs — semicolon inconsistency + +The tutorial shows `ret:` for object types with and without a trailing `;`: +``` +*.client.*#with*(*) ret:Lcom/api/client/*; # has semicolon +*.model.*#*(*):ret:Lcom/example/model/* # missing semicolon +``` +JVM object type descriptors always end with `;` (e.g., `Ljava/lang/String;`). The form +without `;` may silently fail to match for the same reason a wrong descriptor does. +Always include the trailing semicolon for object return type globs: +``` +*MyClass#myMethod(*) ret:Lcom/example/model/*; id:my-label +``` + +### 10i. `id:` listed under PREDICATES implies it is optional — it is not + +The tutorial lists `id:` in the PREDICATES section alongside `ret:`, `name-contains:`, +etc., with the description "identifier shown in logs/reports". Nothing in the tutorial says +it is required. + +In practice, omitting `id:` makes log output unreadable when rules fire — you see only the +method descriptor with no human-readable label. Treat `id:` as mandatory on every rule. +This project's instructions reflect this: "Every rule Must include an `id:` label for +traceability." + +### 10j. `#` is both the comment marker and the FQCN/method separator — inline comments are ambiguous + +The tutorial states "Comments (# …) and blank lines are ignored" but never clarifies that +`#` in the middle of a rule is the FQCN/method separator, not a comment. + +This creates an ambiguity for inline trailing comments: +``` +*$#(*) id:gen-ctor # constructors +``` +Is `# constructors` an inline comment that JMF drops, or does it confuse the parser? +In our tested rules it appears to be harmless, but the tutorial is completely silent on +this. To be safe, keep `#`-style inline comments only on dedicated comment lines. +Use the `id:` label as the sole annotation on rule lines: +``` +# constructors +*$#(*) id:gen-ctor +``` + +### 10k. CONSERVATIVE / STANDARD / AGGRESSIVE labels mentioned in intro but never applied to examples + +The Quick Start says: +> "2) Start with the CONSERVATIVE section only. 3) If clean, enable STANDARD. Use AGGRESSIVE +> only inside DTO/auto-generated packages." + +But the rule examples that follow are not labelled CONSERVATIVE, STANDARD, or AGGRESSIVE. +There is no way to know which category `*.model.*#copy(*)` or `*#$anonfun$*` belongs to +without guessing. In our project we use the section header comments to mark intent instead: +`# GLOBALS RULES`, `# PROJECT RULES`. + +### 10l. "Always use dot-form" note contradicts necessary `$` usage + +The "Notes" section says: +> "Always use dot-form (com.example.Foo) for class names." + +But inner classes and companion objects require `$`, not `.`: +- `com.example.Foo$Bar` — inner class `Bar` of `Foo` +- `com.example.Foo$` — companion object of `Foo` + +Both are used throughout the tutorial's own examples (`*.model.*$*#apply(*)`). The "dot-form" +note means "use `.` as the package separator" (not `/`), not "never use `$`". The note needs +the clarification: `$` is the required inner-class/companion separator and must be used as-is. + +### 10m. Universal wildcard `*#*(*)` missing warning about falsely inflated coverage + +The tutorial says: +> "`*#*(*)` — Match EVERY method in EVERY class. Useful only for diagnostics." + +It does not warn that enabling this globally will suppress JaCoCo for every method in the +codebase, producing artificially inflated (up to 100%) coverage numbers. This rule left +enabled in a CI pipeline would silently mask all regressions. It should carry an explicit +**DO NOT commit** warning. + +### 10n. Scala var setter compiled name not explained for `_$eq` glob + +The tutorial shows: +``` +*.dto.*#*_$eq(*) + → Matches Scala var setters in DTO packages (e.g., `name_=(...)`). +``` +The source-level name `name_=` is compiled to `name_$eq` in bytecode. The tutorial +uses `_$eq` as the glob suffix but never explains *why* — a reader unfamiliar with +Scala's name mangling might try `*_=(*)` (the source form), which will never match. +Always use the bytecode name in JMF globs, not the source name. + +--- + +## 12. Real bugs found in this project's own jmf-rules.txt via audit + +These were discovered by running the same JVM-descriptor audit described in section 1 +and cross-referencing against the knowledge in section 10d. Both rules silently failed +to match and were only "covered" because integration tests happened to exercise the +same methods. **Fixed in jmf-rules.txt.** + +| Rule (before fix) | Problem | Fix | +|---|---|---| +| `*DBFunction#execute(scala.Function1)` | Human-readable class name | `(Lscala/Function1;)*` | +| `*NamingConvention#fromClassNamePerConvention(java.lang.Object)` | Human-readable class name | `(Ljava/lang/Object;)*` | + +Audit command used to find these: +```bash +grep -n '[a-z]\.[A-Z]' jmf-rules.txt | grep -v '^#' +``` +This finds lines where a lower-case package segment is followed by an upper-case class +name in descriptor position — a strong indicator of human-readable format. + +--- + +## 13. Scala source name vs bytecode name — glob always targets bytecode + +JMF operates on bytecodes, so any glob must use the compiled method name, not the source +name. Common Scala name-mangling patterns: + +| Source name | Bytecode name | +|---|---| +| `name_=` (var setter) | `name_$eq` | +| `a_+_b` (operator) | `a_$plus_b` | +| `?` | `$qmark` | +| `!` | `$bang` | +| `++` | `$plus$plus` | +| `::` | `$colon$colon` | +| Inner class `Foo.Bar` | `Foo$Bar` | +| Companion object `Foo` | `Foo$` | +| Lambda lifted from `foo` | `$anonfun$foo$1` | + +Use `javap -p classfile` to confirm the exact bytecode name before writing a glob. + +--- + + +## 11. Quick reference — JMF rule anatomy + +``` +#() [FLAGS] [PREDICATES] id: