Skip to content

fix(test): isolate cross-test state leaks and stabilize flaky suites - #6974

Open
warku123 wants to merge 6 commits into
tronprotocol:release_v4.8.3from
warku123:fix/unit-test-cleanup-release
Open

warku123 wants to merge 6 commits into
tronprotocol:release_v4.8.3from
warku123:fix/unit-test-cleanup-release

Conversation

@warku123

@warku123 warku123 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes cross-test state isolation defects so the full test chain passes reliably with retries disabled. All changes are test-only (21 files under framework/src/test/), organized as five commits by theme.

Scope constraint: every defect is fixed from the test side only — production code is deliberately untouched, even where a production change would be shorter. This keeps the patch strictly inside the test source set with zero risk of changing node behavior; the trade-off is that a few fixes are more verbose than their production-side alternatives (reflection-based teardown assertions, test-local state resetters such as VMConfigRule / PeerManagerStateResetter / BackupTestUtils, and retry loops around mock pools). Follow-ups where a production API could simplify the tests are listed below.

Defect Symptom Fix
VM flags leaked across tests (ConfigLoader.disable, VMConfig thread-local snapshot, global snapshot); PeerManager static peers/counters and a permanently shut-down executor AllowTvmLondonTest/ValidateMultiSignContractTest fail on stale VM config; MetricsApiServiceTest RejectedExecutionException; peer-count assertions off by prior-class residue VMConfigRule restores the whole VM config domain around every test; PeerManagerStateResetter restores cold-start static state before each context
Backup manager/server/channel/executors never fully released in teardown BackupServerTest times out after 60s stuck in shutdownAndAwaitTermination; BackupServer:WAITING thread survives the class Exception-safe teardown with verified termination
Class-wide DPoS producer raced with manual Merkle writes; async worker touched a mock concurrently with stubbing ShieldedReceiveTest intermittent Rt is invalid.; TransactionsMsgHandlerTest intermittent WrongTypeOfReturnValue Witness schedule initialized via balanced start/stop with no background producer; mock pool keeps stubbing single-threaded
Metrics HTTP (9527) and native ZMQ queue (5555) bound fixed ports across parallel worker JVMs PrometheusApiServiceTest BindException; EventLoaderTest ZMQ Errno 48; native queue never stopped in BlockEventGetTest Random ports via existing chooseRandomPort() convention; queues stopped in teardown
expected/actual swapped in one assertion; shielded flag not restored Misleading failure output; DPS residue Argument order fix; snapshot/restore

Why are these changes required?

Test retry (maxRetries=5) was masking first-attempt failures: suites passed while state leaks accumulated across Spring contexts sharing worker JVMs (maxParallelForks=4). With retries disabled these defects produced deterministic or high-frequency red suites.

How these were found: the same commit was run repeatedly with retries disabled across JDK/arch lanes to expose first-attempt failures; failures were correlated with per-class residue of shared state (JVM shutdown diffing), and causality was confirmed by fault injection — injecting a suspected leaked state made the victim suite fail while control arms passed. The port conflicts and the Mockito race were traced from recurring CI failure fingerprints to their call sites.

One test required more than isolation. TransactionsMsgHandlerTest#testProcessMessage had a vacuous smart-contract section: its assertion checked the previous transaction's request map, the scheduler-submit verification was commented out, and two preloaded TrxEvent(null, null) entries were later drained into the real pool by the production scheduler. It now uses a non-executing mock pool (which also removes the Mockito cross-thread race) and drives both queue paths with real transactions; the asserted intent is unchanged or strictly strengthened.

This PR has been tested by

  • Full chain (clean build:framework:testWithRocksDbjacocoTestReport) with retries disabled, 3 rounds on JDK8/x86_64 and 3 rounds on JDK17/aarch64, all green: https://github.com/warku123/java-tron/actions/runs/35065905472 (identical test-file content to this branch, verified by zero-diff comparison)
  • Targeted retry-disabled runs per fixed suite; independent review of all 20 files with no findings at reporting threshold

Follow up

  • Production Metrics has no stop()/getPort() and NativeMessageQueue remaps port 0 to the default — test-side random-port probing could be replaced by real ephemeral binds if those APIs are ever added.
  • PeerManager.close()/init() is not restart-safe in production; making it so would allow the test resetter to be narrowed or removed.

Extra details

Open question — test retry configuration. All verification for this PR ran with retries disabled (-PmaxRetries=0): six full unit-test runs (3× JDK8/x86_64 + 3× JDK17/aarch64) with zero failures. The current CI default retries failed tests up to 5 times, which can mask precisely the cross-test state leaks this PR eliminates. I kept this change strictly test-side and did not touch CI configuration, so whether to lower (or zero-out) the retry count on some lanes is open for discussion — happy to include it here or split it into a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant