test(framework): fix test quality issues

[Little-Peony]

What does this PR do?

Systematically fix five categories of test quality issues across framework, actuator, and plugins modules (44 files, 6 commits):

1. Silent-pass tests

• Add Assert.fail() after code expected to throw exceptions (FetchInvDataMsgHandlerTest, MerkleTreeTest, CredentialsTest, etc.)
• Fix assertEquals(actual, expected) argument order → assertEquals(expected, actual)

2. Thread / resource leaks

• Replace bare Executors.newFixedThreadPool() / new Thread[] with ExecutorServiceManager in LogBlockQueryTest, TransactionRegisterTest, ConcurrentHashMapTest; add shutdownAndAwaitTermination
• Fix InterruptedException swallowed silently → restore interrupt flag in PropUtilTest, DBIteratorTest, SnapshotImplTest, ProposalControllerTest, BlockStoreTest, PbftTest
• Fix SolidityNode not shutting down correctly in SolidityNodeTest; patch SolidityNode.java shutdown path

3. Dead code / phantom fixtures

• Remove @After methods cleaning paths never written by any test (DbMoveTest)
• Remove never-called private static helpers (writeProperty(), deleteDir()) in ArchiveManifestTest, DbArchiveTest
• Remove unreachable test code in FreezeTest, EventParserTest, RelayServiceTest, etc.

4. Stale / obsolete tests

• Remove moreThanFrozenNumber test in FreezeBalanceActuatorTest — the corresponding actuator error path no longer exists

5. Test infrastructure unification

• Extract ClassLevelAppContextFixture (with shutdownChannel / shutdownChannels helpers) to unify AppContext and gRPC channel lifecycle across service tests
• Migrate CredentialsTest from JUnit 3 (junit.framework.TestCase) to JUnit 4 + Mockito; remove dependency on real SecureRandom
• Fix package typo: keystroe → keystore
• Add Mockito.verify() assertions in InventoryMsgHandlerTest, PeerStatusCheckMockTest

Why 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:

• All changed tests pass locally (./gradlew :framework:test :actuator:test :plugins:test)

Breaking Changes

None. Test-only changes except SolidityNode.java shutdown fix (additive only).

[warku123]

[NIT] EventPluginLoader.filterQuery is subject to the same cross-test pollution and is not reset here. FilterQueryTest.testMatchFilter() sets filterQuery = {fromBlock=100, toBlock=190} with no @After cleanup. When the two tests share the same Gradle forkEvery JVM batch, matchFilter() returns false for block 1, and processTrigger gets 0 coverage even though all assertions pass.

Consider adding alongside the existing reset:

EventPluginLoader.getInstance().setFilterQuery(null);

[Little-Peony]

Fixed in c8e7ef5. Added EventPluginLoader.getInstance().setFilterQuery(null) to @Before alongside the existing ConfigLoader.disable = false reset.

[warku123]

[NIT] Assert.assertNotNull(peerStatusCheck) is a dead assertion — peerStatusCheck is a local variable created at the top of the test and never reassigned, so it is trivially non-null. PeerStatusCheck.init() wraps statusCheck() in a try-catch, so scheduledTask.run() always returns normally and this line is always reached. Was this assertion intended to verify something specific, or can it be removed?

[Little-Peony]

Fixed in c8e7ef5. Removed the dead assertion and the now-unused import org.junit.Assert.

[bladehan1]

Re: @halibobo1205 #3077488830 — concrete findings on the shutdown-time verification you asked for:

[SHOULD] shutdown() worst-case latency can reach 240s

Both shutdownAndAwaitTermination calls are sequential, and each waits up to 60s + 60s (see common/src/main/java/org/tron/common/es/ExecutorServiceManager.java). If a worker is blocked inside databaseGrpcClient.getBlock(...) (no deadline set), Thread.interrupt() is not guaranteed to unblock the underlying Netty I/O, so the total shutdown time can reach 240s.

The current SolidityNodeTest uses a mocked node so CI is not affected, but any integration test or operational restart that runs the real worker will see this latency.

Suggestion: run the two shutdownAndAwaitTermination calls in parallel (the two pools are independent), and track a follow-up to add a gRPC deadline on DatabaseGrpcClient.getBlock.

[bladehan1]

[SHOULD] Use the project's standard shutdown helper

Every other test in this PR routes executor shutdown through ExecutorServiceManager.shutdownAndAwaitTermination(...), but this @After calls subscriberExecutor.shutdownNow() + awaitTermination(2, SECONDS) directly. ZMQ's subscriber.recv() does not always respond to Thread.interrupt() within 2s, so the subscriber thread may leak across test runs and occasionally interfere with the next test.

Suggestion: replace the manual shutdown block with ExecutorServiceManager.shutdownAndAwaitTermination(subscriberExecutor, "zmq-subscriber") to align with the rest of the codebase.

[bladehan1]

[SHOULD] The close()/start() race is only partially closed, and the remaining control flow is confusing.

1. The new if (shutdown) break; at line 81 only covers a narrow window — the case where close() has already set shutdown=true but has not yet read channel. The other window is still open: if close() runs while b.bind(port).sync() is still in progress, close() sees the previous iteration's (already-closed) channel (or null) and its channel.close().await(10s) is a no-op on the channel that is about to be bound. The newly-bound channel is then closed indirectly by group.shutdownGracefully().sync() in start()'s finally, meaning there are two distinct close paths with two different timeout semantics (10s vs. group's default quiet period).

2. Ordering in close(): backupManager.stop() runs before channel.close(). Any in-flight UDP datagram already picked up by the event loop will still be dispatched through the pipeline into a stopped BackupManager. The usual order is to stop ingress (channel) first, then business state (backupManager).

3. channel.close().await(10, SECONDS) is effectively redundant — ExecutorServiceManager.shutdownAndAwaitTermination(executor, name) already waits for start() to return, and start() only returns after group.shutdownGracefully().sync() has closed the channel. Its only real job is to wake up the channel.closeFuture().sync() in start(); a fire-and-forget channel.close() would do that.

4. The while (!shutdown) + logger.warn("Restart backup server ...") branch is practically dead code for a UDP NioDatagramChannel: closeFuture() only completes on close() (→ shutdown=true → break) or on an exception (→ outer catch → finally). There is no tested path that loops back and rebinds, so the three shutdown checks + restart loop read as defensive code that will never run.

Suggestion: drop the restart loop and extra if (shutdown) break; guards, call channel.close() (fire-and-forget) in close(), rely on ExecutorServiceManager.shutdownAndAwaitTermination(executor, name) as the single sync barrier, and stop backupManager only after the executor has terminated.

[bladehan1]

[NIT] Three small follow-ups on the relayNodes rewrite

Moving relayNodes from a class-load-time static field to a lazy instance field is a meaningful behaviour change — it avoids capturing an empty Args.getInstance().getFastForwardNodes() before Spring wiring completes. The new shape is race-free today, but three small improvements would make the intent durable:

1. Document why lazy. Without a one-line comment above the null-check, a future reader may "optimise" it back to a static or eager field and silently reintroduce the original empty-list bug. The if (relayNodes == null) guard also lets tests pre-inject a value via ReflectUtils.setFieldValue before setChannel runs; that contract is worth stating.

2. Tighten publication. The safety argument is non-local: it relies on (a) PeerConnection being @Scope("prototype"), (b) PeerManager.add(...) being public static synchronized and being the only caller of setChannel, and (c) PeerManager.peers being a Collections.synchronizedList so that readers obtained via getPeers() inherit a lock release/acquire edge. If any of those change, the field is neither volatile nor final and @Getter already exposes it publicly, so a concurrent reader could observe a stale null with no data-race detector to warn about it. Prefer either volatile, a single unconditional assignment (no guard), or — best — final initialised at declaration / in the constructor, since the prototype bean is created after Spring finishes parsing Args.

3. Narrow the getter. @Getter on a field that exists to support tests exposes it as a public API. @VisibleForTesting (from com.google.common.annotations) or @Getter(AccessLevel.PACKAGE) signals intent and keeps future changes to the field free of external consumers.

Suggestion: add a one-line comment above the null-check explaining the Args-init ordering + test-injection contract; make the field final (or at least volatile); and narrow the getter's visibility.

[bladehan1]

[NIT] Make the test executor daemon

ExecutorServiceManager.newFixedThreadPool(EXECUTOR_NAME, 4) defaults to non-daemon. Normal runs are fine (finally shuts the pool down), but if the JVM exits abruptly, non-daemon workers can hold it open. Other project tests using this helper with short-lived test scope typically pass isDaemon=true.

Suggestion: switch to newFixedThreadPool(EXECUTOR_NAME, 4, true).

[bladehan1]

[QUESTION] Why does this test need 90s?

The globalTimeout goes 60s → 90s and the method-level @Test(timeout=60_000) is removed. If this is driven by the 60s+60s wait inside ExecutorServiceManager.shutdownAndAwaitTermination, say so in a comment; otherwise the +30s is unexplained and future maintainers won't know how to tune it.

Suggestion: add a short comment explaining the new upper bound, or tighten the shutdown path so 60s still fits.

[bladehan1]

[NIT] Signal test-only visibility

awaitShutdown(Application, SolidityNode) is package-private purely to support the two new mock-based tests. Marking it @VisibleForTesting communicates that intent and discourages accidental external use.

Suggestion: add @VisibleForTesting (from com.google.common.annotations).