Add Resource Leak Checker tests for JDBC and RowSet resource types#7480
Add Resource Leak Checker tests for JDBC and RowSet resource types#7480parameshn wants to merge 23 commits into
Conversation
This file contains tests for JDBC resource leak detection, including various scenarios for ResultSet, Statement, and PreparedStatement resource management.
Added tests for resource leak detection in various RowSet types, including JdbcRowSet, CachedRowSet, FilteredRowSet, WebRowSet, and JoinRowSet.
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughAdds two new test classes under 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🤖 Fix all issues with AI agents
In `@checker/tests/resourceleak/JdbcResourceLeak.java`:
- Around line 54-59: The expected error annotation is placed on the ResultSet
creation line but the Resource Leak Checker reports the missing close for the
Statement at end-of-scope; move the comment `// :: error:
(required.method.not.called)` from before `ResultSet rs =
stmt.executeQuery("SELECT 1");` to the line immediately before the closing brace
of the nestedStatementNotClosed method so the expected error is associated with
the end-of-scope where `stmt`’s required close is reported.
In `@checker/tests/resourceleak/RowSetResourceLeak.java`:
- Around line 11-47: In cachedRowSetToResultSetNotClosed, the error annotation
is misplaced: it currently sits before the crs.close() call but the Resource
Leak Checker reports the unclosed ResultSet rs at end-of-scope; move the comment
"// :: error: (required.method.not.called)" from before crs.close() to the line
immediately before the method's closing brace so the expected error lines up
with the actual violation for rs in cachedRowSetToResultSetNotClosed.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@checker/tests/resourceleak/JdbcResourceLeak.java`:
- Around line 54-59: The error annotation inside the nestedStatementNotClosed
method is mis-indented (6 spaces) compared to the file's 8-space convention;
update the comment "// :: error: (required.method.not.called)" in
nestedStatementNotClosed so it has 8-space indentation to match other
annotations in this file (ensure alignment with other error comments in the
class).
- Around line 45-59: Add a new test method called nestedResultSetNotClosed
following the existing nestedBothClosed and nestedStatementNotClosed methods.
The method should create a Statement and ResultSet (using the Connection
parameter and executeQuery), close only the Statement while leaving the
ResultSet open, and include the error annotation comment to verify the checker
detects the unclosed ResultSet. This mirrors the inverse scenario of the
nestedStatementNotClosed test and completes the coverage of nested resource
closing patterns.
In `@checker/tests/resourceleak/RowSetResourceLeak.java`:
- Around line 35-40: The error annotation for the test method
cachedRowSetToResultSetNotClosed() has inconsistent indentation; change the
leading whitespace before the comment marker "// :: error:
(required.method.not.called)" to match the other annotations (use 8-space
indentation) so it aligns with the rest of the file's annotations and keeps
style consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@checker/tests/resourceleak/RowSetResourceLeak.java`:
- Around line 35-47: Add a new test method cachedRowSetToResultSetBothNotClosed
that constructs a CachedRowSet (new CachedRowSetImpl()), calls crs.toResultSet()
to obtain a ResultSet, and then exits scope without calling rs.close() or
crs.close(); annotate the end-of-scope with two required.method.not.called
expectations (one for the unclosed ResultSet and one for the unclosed
CachedRowSet) so the checker verifies both obligations independently.
Added test for CachedRowSet to ResultSet without closing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@checker/tests/resourceleak/RowSetResourceLeak.java`:
- Line 48: Remove the trailing whitespace on the blank line in the
RowSetResourceLeak.java file (class RowSetResourceLeak) at the location flagged
(line 48) — open the file, delete the extra spaces on that blank line so it is a
clean empty line, then save and re-run formatting/linting to ensure no other
trailing-space occurrences remain.
|
@parameshn The tests do not compile because they use an internal class in the JDK. |
|
I can’t seem to find the issue. Is it possible the failures are due to the |
Refactor JdbcRowSet, CachedRowSet, FilteredRowSet, WebRowSet, and JoinRowSet test methods to remove parameters and create instances within the methods.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 45: Remove the stray placeholder text "foo." from CONTRIBUTING.md by
deleting that line (it's an accidental/test artifact); ensure no other content
or formatting is altered and commit the cleaned file.
In `@Makefile`:
- Around line 6-8: Remove the unused make target by deleting the .PHONY entry
and the empty target named "nothing" (remove the lines defining ".PHONY:
nothing" and the "nothing:" target); if the .PHONY list contains other targets
keep them but remove only "nothing", and if the target was intended for a
purpose instead of deletion add a brief comment above it explaining its purpose.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9f2f619-86ca-464f-888e-fe64e3bc71b6
📒 Files selected for processing (2)
CONTRIBUTING.mdMakefile
|
|
||
| When interacting with other people, please abide by the [Contributor | ||
| Covenant](https://www.contributor-covenant.org/version/2/1/code_of_conduct). | ||
| foo. |
There was a problem hiding this comment.
Remove the erroneous foo. text.
This line contains meaningless placeholder text that should not be in the contributing guidelines. It appears to be a test artifact that was accidentally included.
🗑️ Proposed fix to remove the erroneous line
When interacting with other people, please abide by the [Contributor
Covenant](https://www.contributor-covenant.org/version/2/1/code_of_conduct).
-foo.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foo. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CONTRIBUTING.md` at line 45, Remove the stray placeholder text "foo." from
CONTRIBUTING.md by deleting that line (it's an accidental/test artifact); ensure
no other content or formatting is altered and commit the cleaned file.
| .PHONY: nothing | ||
| nothing: | ||
|
|
There was a problem hiding this comment.
Remove the unused nothing target.
This phony target has no commands, no dependencies, and serves no apparent purpose in the Makefile. It appears to be a test artifact or debugging code that should not be committed.
If there is a specific reason for this target to exist, please add a comment explaining its purpose.
🧹 Proposed fix to remove the unused target
-.PHONY: nothing
-nothing:
-
PLUME_SCRIPTS=checker/bin-devel/.plume-scripts📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PHONY: nothing | |
| nothing: | |
| PLUME_SCRIPTS=checker/bin-devel/.plume-scripts |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 6 - 8, Remove the unused make target by deleting the
.PHONY entry and the empty target named "nothing" (remove the lines defining
".PHONY: nothing" and the "nothing:" target); if the .PHONY list contains other
targets keep them but remove only "nothing", and if the target was intended for
a purpose instead of deletion add a brief comment above it explaining its
purpose.
mernst
force-pushed
the
mustcall-jdbc-types
branch
from
79ed286 to
cf68d95
Compare
|
Could you confirm whether there are any remaining issues that need attention on this PR? |
|
@parameshn The tests fail. No pull request can be merged with failing tests. |
|
Thanks for the clarification |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@checker/tests/resourceleak/RowSetResourceLeak.java`:
- Line 5: The file contains an empty test placeholder class RowSetResourceLeak
with no test methods or annotations so it provides no coverage; either (A)
restore the original RowSet resource-leak tests by reintroducing the test
methods and proper `@Test` annotations and resolving the internal-JDK dependency
issues referenced in the review (locate RowSetResourceLeak and add the test
logic and imports so the harness runs it), or (B) delete the file if those tests
are not being used; if you choose to keep the empty class as a placeholder, add
a clear TODO comment inside RowSetResourceLeak explaining why it is
intentionally empty and exactly how/where it will be wired back into the test
harness once JDK/internal deps are addressed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe3de048-63bd-4f35-aa8d-dfcd728bdea0
📒 Files selected for processing (1)
checker/tests/resourceleak/RowSetResourceLeak.java
2 participants
Adds Resource Leak Checker tests for JDBC resource types, including
Statement, PreparedStatement, ResultSet, and RowSet implementations.
The tests validate must-call behavior by checking both missing and
proper close() calls.
Issue: #6354
Related: typetools/jdk#254