test(framework): stabilize test fixtures and fix resource leaks

[bladehan1]

Summary

• Fix iterator resource leaks in DBIteratorTest by properly closing iterators
• Merge duplicate CredentialsTest coverage from keystroe (typo package) into keystore
• Stabilize CredentialsTest fixtures with dynamic temp file generation
• Update iterator exception assertions to match actual behavior

Test plan

• ./gradlew :framework:test --tests "org.tron.core.db.DBIteratorTest"
• ./gradlew :framework:test --tests "org.tron.keystore.CredentialsTest"
• Verify removed duplicate org.tron.keystroe.CredentialsTest has no unique coverage

🤖 Generated with Claude Code

---

Summary by cubic

Stabilizes keystore tests and fixes RocksDB iterator leaks to make the framework tests reliable. Also updates the reviewer auto-assign workflow with the correct event and permissions.

• Bug Fixes

  • Close RocksDB iterators with try-with-resources and assert expected exceptions via Assert.assertThrows in org.tron.core.db.DBIteratorTest.
  • Auto-assign reviewers: switch workflow to pull_request_target and grant required permissions.

• Refactors

  • Remove duplicate org.tron.keystroe.CredentialsTest and merge coverage into org.tron.keystore.CredentialsTest.
  • Use deterministic fixtures: mocked SignInterface with fixed addresses; assert Base58Check address, equals, and hashCode.
  • Normalize to JUnit Assert and drop legacy TestCase/Spring Assert.

^{Written for commit 276716e. Summary will update on new commits.}

Summary by CodeRabbit

Release Notes

• Tests - Enhanced test resource management and improved assertion methods for iterator and credential test coverage.
• Chores - Updated GitHub Actions workflow configuration for pull request review automation.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request updates a GitHub Actions workflow to improve PR handling by switching from pull_request to pull_request_target triggers and adding explicit permissions, while refactoring test implementations across two test classes to use modern assertion patterns and deterministic test setup instead of manual exception handling and randomness-based construction.

Changes

Cohort / File(s)	Summary
Workflow Configuration .github/workflows/pr-reviewer.yml	Changed PR trigger strategy from pull_request to pull_request_target, removed job-level conditional, and added explicit least-privilege permissions (contents: read, pull-requests: write).
Test Refactoring framework/src/test/java/org/tron/core/db/DBIteratorTest.java	Migrated RockStoreIterator resource management to try-with-resources blocks and replaced manual try/catch exception assertions with Assert.assertThrows() pattern for improved test clarity.
Test Improvements framework/src/test/java/org/tron/keystore/CredentialsTest.java	Replaced randomness-based credential construction with deterministic Mockito-based SignInterface mocks, strengthened exception validation, and expanded equality test coverage with multiple address and engine combinations.
Cleanup framework/src/test/java/org/tron/keystroe/CredentialsTest.java	Removed duplicate test file with incorrect directory path spelling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• fix(ci): fix reviewer assignment permissions for fork PRs #12: Implements the identical workflow configuration changes to .github/workflows/pr-reviewer.yml, including the pull_request_target trigger switch and explicit permission additions.

Poem

🐰 A workflow springs forth, more secure in its ways,
Tests now caught cleanly, no tangled delays,
With mocks in their places and try-blocks so neat,
The code hops along to a rhythmic beat! 🥕✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title 'test(framework): stabilize test fixtures and fix resource leaks' accurately summarizes the main changes: fixing iterator resource leaks in DBIteratorTest, stabilizing CredentialsTest with deterministic fixtures, and removing a duplicate test file.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/opt_test

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

framework/src/test/java/org/tron/keystore/CredentialsTest.java (1)

37-45: Use Assert.assertThrows instead of try/catch for this negative-path test.

JUnit 4.13.2 (configured in build.gradle) supports Assert.assertThrows(), which makes the expected exception contract explicit and simplifies failure behavior.

♻️ Proposed refactor

 `@Test`
 public void testCreateFromSM2() {
-  try {
-    Credentials.create(SM2.fromNodeId(ByteUtil.hexToBytes("fffffffffff"
-        + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"
-        + "fffffffffffffffffffffffffffffffffffffff")));
-    Assert.fail("Expected IllegalArgumentException");
-  } catch (Exception e) {
-    Assert.assertTrue(e instanceof IllegalArgumentException);
-  }
+  Assert.assertThrows(IllegalArgumentException.class, () ->
+      Credentials.create(SM2.fromNodeId(ByteUtil.hexToBytes("fffffffffff"
+          + "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"
+          + "fffffffffffffffffffffffffffffffffffffff"))));
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/src/test/java/org/tron/keystore/CredentialsTest.java` around lines
37 - 45, Replace the try/catch in the testCreateFromSM2 method with a direct
Assert.assertThrows call: use
Assert.assertThrows(IllegalArgumentException.class, () ->
Credentials.create(SM2.fromNodeId(ByteUtil.hexToBytes("fffffffffff" +
"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"
+ "fffffffffffffffffffffffffffffffffffffff")))); so the test explicitly expects
IllegalArgumentException from Credentials.create and removes the manual
try/catch/Assert.fail pattern.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@framework/src/test/java/org/tron/keystore/CredentialsTest.java`:
- Around line 37-45: Replace the try/catch in the testCreateFromSM2 method with
a direct Assert.assertThrows call: use
Assert.assertThrows(IllegalArgumentException.class, () ->
Credentials.create(SM2.fromNodeId(ByteUtil.hexToBytes("fffffffffff" +
"ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"
+ "fffffffffffffffffffffffffffffffffffffff")))); so the test explicitly expects
IllegalArgumentException from Credentials.create and removes the manual
try/catch/Assert.fail pattern.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 853a3d32-d58a-4af6-a455-fe5a61405449

📥 Commits

Reviewing files that changed from the base of the PR and between 82be367 and 276716e.

📒 Files selected for processing (4)

• .github/workflows/pr-reviewer.yml
• framework/src/test/java/org/tron/core/db/DBIteratorTest.java
• framework/src/test/java/org/tron/keystore/CredentialsTest.java
• framework/src/test/java/org/tron/keystroe/CredentialsTest.java

💤 Files with no reviewable changes (1)

• framework/src/test/java/org/tron/keystroe/CredentialsTest.java

[cubic-dev-ai]

No issues found across 4 files