Feature/improve jmf rules by adding unit tests#79
Feature/improve jmf rules by adding unit tests#79miroslavpojer wants to merge 3 commits intomasterfrom
Conversation
…ot instructions and review rules
…. Fixes from self-review using copilot.
miroslavpojer
requested review from
benedeki,
lsulak,
salamonpavel and
tmikula-dev
as code owners
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR restructures test infrastructure by relocating test utilities to align with main source package structure, introducing mock JDBC ResultSet stubs for unit testing, creating dedicated unit and integration test files, updating JMF coverage rules with project-specific exclusions, and adding Scala-specific Copilot development guidance documents. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.gitignore (1)
58-58: Consider whether TODO.md files in subdirectories should also be ignored.The current pattern
TODO.mdonly matches files at the repository root. If developers might create TODO.md files in subdirectories (e.g.,src/TODO.mdordocs/TODO.md), consider using**/TODO.mdor simplyTODO.mdat any level.If the intent is specifically to ignore only a root-level TODO.md, the current pattern is correct.
📝 Optional pattern to ignore TODO.md anywhere in the repository
-TODO.md +**/TODO.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 58, The .gitignore entry currently lists the pattern "TODO.md" which only matches a root-level file; if you want to ignore TODO.md files in any subdirectory, replace or add the recursive pattern "**/TODO.md" (or use a leading slash if you must restrict to root), otherwise leave "TODO.md" as-is if you intentionally want root-only behavior—update the gitignore entry for the TODO.md pattern accordingly.balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala (1)
85-92: Consider documenting thenull.asInstanceOf[T]behavior for primitive types.
getObject[T](...): Treturningnull.asInstanceOf[T]will produce0,false, etc. for primitive type parameters at runtime due to Scala/JVM boxing semantics. This is acceptable for mock purposes, but a brief doc comment noting this behavior would help future maintainers.🤖 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/MockResultSets.scala` around lines 85 - 92, Add a brief Scaladoc comment to the MockResultSet class (or directly above the getObject[T](columnIndex: Int, `type`: Class[T]): T method) explaining that the implementation returns null.asInstanceOf[T], which on the JVM yields default primitive values (0, false, etc.) when T is a primitive type wrapper, and that this behavior is intentional for the mock; mention any implications for callers.
🤖 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:
- Line 53: Update the guidance that mandates a `testing` sub-package: change the
line "Must place shared test helpers and fixtures in a dedicated `testing`
sub-package and reuse them across tests." to reflect the new project structure
by instructing contributors to place shared test helpers and fixtures in the
root test package `za.co.absa.db.balta` (or the agreed test package) so it no
longer requires a `testing` sub-package; ensure the wording references the root
test package name `za.co.absa.db.balta` to avoid future conflicts.
- Around line 52-54: The style guidance is inconsistent: it recommends
AnyFunSuite/AnyFunSuiteLike and AnyWordSpec (lines mentioning AnyFunSuite,
AnyFunSuiteLike, AnyWordSpec) but then mandates describe blocks (which belong to
AnyFunSpec); update the document to align frameworks and idioms by either (a)
changing the describe requirement to recommend AnyFunSpec when describe is
required, or (b) removing/rewriting the describe mandate and instead document
idiomatic patterns for the recommended styles—e.g., for AnyFunSuite use flat
test("...") definitions, for AnyWordSpec use nested should/when/in blocks—so
references to describe only appear with AnyFunSpec and the test()/should/when/in
examples match the stated preferred suites.
In `@balta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scala`:
- Line 17: The test filename DBTestingConnection.scala violates the test-naming
pipeline rule; fix by either renaming the utility to follow allowed patterns
(e.g., DBTestingConnectionSupport.scala or DBTestingConnectionUtil.scala), or
moving the trait into an excluded subfolder such as support/ or fixtures/, or
update the filename-inspector config to exclude this pattern; locate the
trait/type in DBTestingConnection.scala (symbol DBTestingConnection) and apply
one of these changes so the file is no longer treated as a test by the checker.
In `@balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala`:
- Line 17: This test-source filename violates the test-naming rule (same as
DBTestingConnection.scala); since MockResultSets.scala is a test utility, either
move the file into a test support subpackage (e.g., a "support" or "testutils"
test subfolder) so the filename-inspector ignores it, or adjust your inspector
rules to exclude utility files, or if the checker requires test names, rename
the file to match the convention; target the file named MockResultSets.scala
(and related DBTestingConnection.scala) when applying the change.
---
Nitpick comments:
In @.gitignore:
- Line 58: The .gitignore entry currently lists the pattern "TODO.md" which only
matches a root-level file; if you want to ignore TODO.md files in any
subdirectory, replace or add the recursive pattern "**/TODO.md" (or use a
leading slash if you must restrict to root), otherwise leave "TODO.md" as-is if
you intentionally want root-only behavior—update the gitignore entry for the
TODO.md pattern accordingly.
In `@balta/src/test/scala/za/co/absa/db/balta/MockResultSets.scala`:
- Around line 85-92: Add a brief Scaladoc comment to the MockResultSet class (or
directly above the getObject[T](columnIndex: Int, `type`: Class[T]): T method)
explaining that the implementation returns null.asInstanceOf[T], which on the
JVM yields default primitive values (0, false, etc.) when T is a primitive type
wrapper, and that this behavior is intentional for the mock; mention any
implications for callers.
🪄 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: 850e4589-07c7-4fd6-8428-a6ac7d0a27ea
📒 Files selected for processing (12)
.github/copilot-instructions.md.github/copilot-review-rules.md.gitignorebalta/src/test/scala/za/co/absa/db/balta/DBTestingConnection.scalabalta/src/test/scala/za/co/absa/db/balta/MockResultSets.scalabalta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/QueryResultIntegrationTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/QueryResultRowIntegrationTests.scalabalta/src/test/scala/za/co/absa/db/balta/classes/QueryResultUnitTests.scalabalta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scalabalta/src/test/scala/za/co/absa/db/balta/implicits/QueryResultRowImplicitsIntegrationTests.scalajmf-rules.txt
benedeki
left a comment
benedeki
left a comment
There was a problem hiding this comment.
The test itself are fine, so as the other content as far as I can judge (can be tweaked in future, if needed).
But I oppose the class move and change in excludes.
| 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' |
There was a problem hiding this comment.
Why this change? This folder was created exactly for placing there files not to be tested. Visa-versa I would place the MockResultSets there.
So please move the file back, and add new one there, I would suggest testing/data folder.
2 participants
Overview
testing/classes/had no counterpart insrc/main, JMF was flagging trivially untestable methods as coverage gaps, and.github/copilot-instructions.mdstill referenced Python tooling.This PR restructures the shared test traits to the root test package, adds unit and integration tests for
QueryResult, tightens JMF rules with 9 project-specific entries, and migrates the Copilot instructions to Scala/sbt.Release Notes
DBTestingConnectionand newMockResultSetsstub trait fromtesting/classes/to root test packageza.co.absa.db.baltaQueryResultUnitTests(10 tests, no DB) andQueryResultIntegrationTests(split fromQueryResultRowIntegrationTests).github/copilot-instructions.mdand.github/copilot-review-rules.mdfrom Python to Scala/sbtRelated
Closes #78
Summary by CodeRabbit
Tests
Bug Fixes
Chores