test(framework): fix test quality issues#6653
Closed
Little-Peony wants to merge 9 commits intotronprotocol:developfrom
Closed
test(framework): fix test quality issues#6653Little-Peony wants to merge 9 commits intotronprotocol:developfrom
Little-Peony wants to merge 9 commits intotronprotocol:developfrom
Conversation
- FetchInvDataMsgHandlerTest: add Assert.fail() before catch blocks so tests fail when expected exceptions are not thrown; fix assertEquals argument order (expected, actual) - ConcurrentHashMapTest: restore commented-out Assert.fail() for ItemNotFoundException; replace printStackTrace() with interrupt flag restoration and proper logging; fail main thread on interrupted join - InventoryMsgHandlerTest: add Mockito.verify() to assert isBlockUnsolidified() is called in the unsolidified-block scenario - PeerStatusCheckMockTest: add Mockito.verify() to assert statusCheck() is invoked by the scheduler after init() - MerkleTreeTest: replace Assert.assertFalse(true) with Assert.fail() for clearer failure message Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- DbLiteTest: remove dead null-dbPath reassignment before init() call; restore thread interrupt flag on InterruptedException in generateSomeTransactions() - DbMoveTest: remove legacy @after destroy() that cleaned a hardcoded path never written by any test; remove dead deleteDir() helper and OUTPUT_DIRECTORY constant; remove now-unused After import - DbTest: narrow INPUT_DIRECTORY and cli fields from public to protected - ArchiveManifestTest, DbArchiveTest: remove copy-pasted writeProperty() and deleteDir() private static methods that were never called; clean up unused imports - DbLiteRocksDbV2Test: rename testToolsWithRocksDB to testToolsWithRocksDbV2 to match the LevelDB v1/v2 naming convention Note: testMvForRocksDB is a pre-existing failure unrelated to these changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lanceActuatorTest The method tested a "max frozen number is: X" error that no longer exists in FreezeBalanceActuator. The actuator was updated to check "frozenCount must be 0 or 1" instead, and a second freeze with frozenCount=1 now succeeds rather than throwing. Remove the now-unused ChainConstant import. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nd fix test quality issues - Replace Executors.newFixedThreadPool / bare new Thread[] with ExecutorServiceManager in LogBlockQueryTest, TransactionRegisterTest, ConcurrentHashMapTest; add proper shutdownAndAwaitTermination - Fix InterruptedException handling: restore interrupt flag instead of swallowing in PropUtilTest, DBIteratorTest, SnapshotImplTest, ProposalControllerTest, BlockStoreTest, PbftTest - Remove dead/unreachable test code in FreezeTest, EventParserTest, EventParserJsonTest, BlockFilterCapsuleTest, MessageTest, RelayServiceTest, DelegationServiceTest - Fix package typo: rename keystroe.CredentialsTest → keystore.CredentialsTest; rewrite test using JUnit Assert and Mockito instead of junit.framework.TestCase - Minor cleanup: unused imports, whitespace, AccountAssetStoreTest assertion style Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Little-Peony
requested review from
317787106,
bladehan1,
halibobo1205 and
xxo1shine
as code owners
Sunny6889
changed the title
Little-Peony
changed the title
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sunny6889
changed the title
Move to local .git/info/exclude instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sunny6889
changed the title
Author
|
Closing in favor of a clean PR with the same changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Systematically fix five categories of test quality issues across
framework,actuator, andpluginsmodules (44 files, 6 commits):1. Silent-pass tests
Assert.fail()after code expected to throw exceptions (FetchInvDataMsgHandlerTest,MerkleTreeTest,CredentialsTest, etc.)assertEquals(actual, expected)argument order →assertEquals(expected, actual)2. Thread / resource leaks
Executors.newFixedThreadPool()/new Thread[]withExecutorServiceManagerinLogBlockQueryTest,TransactionRegisterTest,ConcurrentHashMapTest; addshutdownAndAwaitTerminationInterruptedExceptionswallowed silently → restore interrupt flag inPropUtilTest,DBIteratorTest,SnapshotImplTest,ProposalControllerTest,BlockStoreTest,PbftTestSolidityNodenot shutting down correctly inSolidityNodeTest; patchSolidityNode.javashutdown path3. Dead code / phantom fixtures
@Aftermethods cleaning paths never written by any test (DbMoveTest)writeProperty(),deleteDir()) inArchiveManifestTest,DbArchiveTestFreezeTest,EventParserTest,RelayServiceTest, etc.4. Stale / obsolete tests
moreThanFrozenNumbertest inFreezeBalanceActuatorTest— the corresponding actuator error path no longer exists5. Test infrastructure unification
ClassLevelAppContextFixture(withshutdownChannel/shutdownChannelshelpers) to unify AppContext and gRPC channel lifecycle across service testsCredentialsTestfrom JUnit 3 (junit.framework.TestCase) to JUnit 4 + Mockito; remove dependency on realSecureRandomkeystroe→keystoreMockito.verify()assertions inInventoryMsgHandlerTest,PeerStatusCheckMockTestWhy are these changes required?
These issues caused tests to pass silently even when the code under test was broken, leaked threads between test runs (causing flakiness in parallel execution), and kept dead code that obscured the real test intent.
This PR has been tested by:
./gradlew :framework:test :actuator:test :plugins:test)Breaking Changes
None. Test-only changes except
SolidityNode.javashutdown fix (additive only).