Conversation
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
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?
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.ConfigLoader.disable,VMConfigthread-local snapshot, global snapshot);PeerManagerstatic peers/counters and a permanently shut-down executorAllowTvmLondonTest/ValidateMultiSignContractTestfail on stale VM config;MetricsApiServiceTestRejectedExecutionException; peer-count assertions off by prior-class residueVMConfigRulerestores the whole VM config domain around every test;PeerManagerStateResetterrestores cold-start static state before each contextBackupServerTesttimes out after 60s stuck inshutdownAndAwaitTermination;BackupServer:WAITINGthread survives the classShieldedReceiveTestintermittentRt is invalid.;TransactionsMsgHandlerTestintermittentWrongTypeOfReturnValuePrometheusApiServiceTestBindException;EventLoaderTestZMQErrno 48; native queue never stopped inBlockEventGetTestchooseRandomPort()convention; queues stopped in teardownWhy 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#testProcessMessagehad a vacuous smart-contract section: its assertion checked the previous transaction's request map, the scheduler-submit verification was commented out, and two preloadedTrxEvent(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
clean build→:framework:testWithRocksDb→jacocoTestReport) 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)Follow up
Metricshas nostop()/getPort()andNativeMessageQueueremaps 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.