feat(ci): add integration test workflows (single-node + multinode)#6789
feat(ci): add integration test workflows (single-node + multinode)#6789warku123 wants to merge 9 commits into
Conversation
Pulls troninfra/troninfra-ci:latest from DockerHub on every PR/dispatch and runs the integration test suite against the FullNode.jar built from the PR. Uses JDK 8 inside the container so FullNode runs on the production runtime; JAVA_HOME_17 keeps Gradle on JDK 17 for the test tooling. Image is built and published from a separate repo (tron_integration-test/java-tron_integration-test) as part of that project's release pipeline (vX.Y.Z tag push).
Mirrors the single-node workflow but starts a 3-witness docker-compose stack via the integration-test image and runs the multinode test target against it. HOST_WORKDIR is set so the script's path-alignment logic resolves compose configs to host paths usable by the host docker daemon (DinD via socket mount). Builds the PR's FullNode.jar, wraps it in a local docker image that inherits from tronprotocol/java-tron:latest, and injects it into the multinode stack via TRON_IMAGE.
Both Integration Test workflows (Single Node / Multinode, Full) cover strictly more than the existing System Test runs. Drop System Test from the PR + push auto-trigger set so every commit doesn't pay the ~7 min runner cost twice for overlapping coverage. workflow_dispatch is kept so a regression suspect can still run stest manually from the Actions UI if needed. To re-enable for routine PR runs, uncomment the push + pull_request blocks at the top of the file.
| @@ -0,0 +1,117 @@ | |||
| name: Integration Test Multinode (Full) | |||
There was a problem hiding this comment.
[SHOULD] Sync pr-cancel.yml workflows list with the workflow changes in this PR
.github/workflows/pr-cancel.yml:20 hard-codes the workflows list as ['pr-build.yml', 'system-test.yml', 'codeql.yml']. This PR deletes system-test.yml and adds two new workflows, but does not touch pr-cancel.yml. Once this PR is merged:
github.rest.actions.listWorkflowRuns({workflow_id: 'system-test.yml', ...})will hit a 404 on the deleted workflow file.actions/github-script@v8has no try/catch around the loop, so the whole cancel job throws — meaningpr-build.yml,codeql.yml, and the two new integration-test workflows are never cancelled on a closed-unmerged PR.- Multinode timeout is 60 min — a single un-cancelled run consumes a full runner slot.
Suggestion: in this PR, update pr-cancel.yml:20 to remove 'system-test.yml', add 'integration-test-single-node.yml' and 'integration-test-multinode.yml', and wrap each workflow iteration in a try/catch to make the job resilient to future drift.
There was a problem hiding this comment.
Good catch — both issues are valid. Fixed in eed9dad7d:
- Updated the workflow list: dropped
system-test.yml, addedintegration-test-single-node.ymlandintegration-test-multinode.yml. - Wrapped each workflow iteration in a try/catch so a missing or renamed file logs a
Skipping ...line instead of taking down the whole cancel job. This makes the cancel job resilient to future drift, not just the current rename.
| name: Integration Test Single Node (Full) | ||
|
|
||
| on: | ||
| pull_request: |
There was a problem hiding this comment.
[DISCUSS] Is this workflow intended to be a hard merge gate, or an informational signal?
The PR description frames the new workflows as 'wiring it into PR CI catches integration regressions before merge instead of after' — i.e., a real gate. However, the workflow files alone do not block merge: even if Integration Test Single Node Full or Integration Test Multinode Full fails red, GitHub will still allow merge unless these check names are explicitly added to the repository's required status checks under Branches → Branch protection rules.
That configuration lives outside the repository tree and is not part of this PR, so two questions to clarify before / soon after merge:
- Intent: are these meant to be required checks on
release_v4.8.2/develop, or informational only for now (e.g., let the new suite bake for 1–2 release cycles before promoting to required)? - Rollout: if required, who owns adding
Integration Test Single Node Full (JDK 8 / x86_64)andIntegration Test Multinode Full (JDK 8 / x86_64)to the branch protection rules on which protected branches, and when? Without that follow-up, replacingsystem-test.yml(which is also not currently required, but conceptually was the gate) silently downgrades the gating posture — failures become advisory.
If gating is the goal, suggest tracking the branch-protection update as an explicit follow-up in the PR description (same vein as the existing 'two java.version assertions' and 'multinode cost' follow-ups). If informational-only is the goal, please say so in the PR description so reviewers don't assume blocking semantics.
There was a problem hiding this comment.
Confirming: hard merge gates on release_v4.8.2 and develop is the intent, not informational. You're right that the workflow files alone don't enforce that — the branch protection update is a follow-up that lives outside this PR.
The reasoning: system-test.yml has historically been the on-chain-behavior gate — gRPC/HTTP/JSON-RPC responses, governance lifecycle, V2 staking, transaction validation, multi-witness consensus boundaries. The new workflows cover the same surface with strictly tighter assertions (JUnit 5 + AssertJ hard assertions on exact values, instead of isNotNull/isNotEmpty checks). Replacing stest without making the new workflows required would silently downgrade the gating posture — which is exactly the risk you flagged.
The two workflows are complementary, not redundant:
- Integration Test Single Node (Full) runs the suite against one FullNode. Covers the bulk of on-chain behavior: API correctness (gRPC / HTTP / JSON-RPC response shapes and values), transaction validation, governance proposal lifecycle, V2 staking, smart contract execution, event subscription. Anything that depends on a single node's state machine.
- Integration Test Multinode (Full) spins up a 3-witness docker-compose stack. Covers what one node can't observe: witness rotation across maintenance boundaries, block propagation between peers, solidification dynamics with multiple confirmations, view-change behavior, isolated-land detection, multi-witness-specific config matrices (RocksDB on node2, native event queue on node3, prometheus on node1, etc.).
A regression in chain parameter logic typically shows up only in single-node; a regression in consensus / P2P / witness scheduling typically shows up only in multinode. Removing either one leaves a class of regressions undetectable at PR time — which is why both need to be required.
Hardcodes getMaintenanceTimeInterval to return 6000L instead of reading from DB. The Integration Test workflows added in this PR should catch this protocol-level regression and turn CI red. REVERT BEFORE MERGE.
This reverts commit 8ce5c51.
… gating Truncates ConsensusDelegate.getActiveWitnesses() to return only the first witness. Single-node setups have 1 witness so this is effectively a no-op there; multinode setups (3 witnesses) lose rotation entirely, which the Integration Test (Multinode Full) workflow should catch while the Single Node workflow stays green. REVERT BEFORE MERGE.
…cific CI gating" This reverts commit 703c087.
|
To address the concern about whether an integration test failure actually reaches the PR check status: The "Run integration tests" step runs the container via Verified end-to-end on this PR:
So a single bad line of Java did flip the PR from green to red, and the revert flipped it back. Combined with the repo's branch protection (all required checks green + ≥2 maintainer approvals), integration test failures block merge in practice. |
system-test.yml is removed in this PR and replaced by two new workflows (integration-test-single-node.yml + integration-test-multinode.yml). Update pr-cancel.yml's hard-coded workflow list to match: drop system-test.yml, add the two new ones. Also wrap each workflow iteration in try/catch so that a missing or renamed workflow file no longer takes down the whole cancel job — the remaining workflows still get processed and a clear "Skipping ..." line is logged for the offending one. Closes the resilience gap flagged in code review.
3 participants
What does this PR do?
Adds two GitHub Actions workflows that gate PRs to
release_v4.8.2(anddevelop,release_**) with the modernized integration test suite, and removessystem-test.ymlsince the new workflows supersede it. Single-node and multinode workflows buildFullNode.jarfrom the PR, pulltroninfra/troninfra-ci:latest(public DockerHub image), and run the full integration test target against the freshly-built jar.The integration test image is published from a separate upstream-owned repo (
tron_integration-test/java-tron_integration-teston internal GitLab); its release pipeline pushesvX.Y.Z+:latesttags to DockerHub. This PR's workflows just consume that published image — no source access required.Why are these changes required?
The integration test suite (JUnit 5 + AssertJ on
TronTestBasefixtures) covers gRPC / HTTP / JSON-RPC, V2 staking, governance, smart contracts, event subscription, multi-witness consensus boundaries, and several other areas thatsystem-testeither didn't touch or asserted only weakly on. Up to now it has been a pre-release manual gate; wiring it into PR CI catches integration regressions before merge instead of after.This PR has been tested by:
Follow up
java.version.startsWith("1.8"). The workflow works around it by settingJAVA_HOMEto JDK 8 inside the container; the assertions themselves will be relaxed in a separate MR on the integration-test repo.workflow_dispatchlater.Extra details
pr-build) — net +5 min on PR wall-clock.troninfra/troninfra-ci:latestis public and rolls forward on eachvX.Y.Ztag from the integration-test repo. Workflows can be pinned to a specific tag if needed.