Add Zombienet battery station upgrade test and Update Integration Test #1459
Conversation
WalkthroughAdds a Node.js WebSocket shim and loads it in multiple integration tests, extends txStatus with a timeout and unified cleanup, and introduces a new GitHub Actions job for Battery Station zombienet post-upgrade tests with NODE_OPTIONS requiring the shim. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Chralt98
added
s:review-needed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/integration-tests.yml (1)
167-168: Consider updating GitHub Actions to latest versions.Static analysis flags
actions/checkout@v2andactions/setup-node@v3as outdated. While this is consistent with existing jobs in this file, consider updating all jobs to useactions/checkout@v4andactions/setup-node@v4in a follow-up to ensure continued compatibility with GitHub Actions runners.Also applies to: 176-176
integration-tests/pnpmfile.cjs (1)
22-27: Consider combining the two conditions.These two conditions apply the same transformation and could be consolidated for clarity.
- if (pkg.name === "polkadot-api_ws-provider_web") { - redirectToNode(pkg); - } - if (pkg.name === "polkadot-api_ws-provider") { + if ( + pkg.name === "polkadot-api_ws-provider_web" || + pkg.name === "polkadot-api_ws-provider" + ) { redirectToNode(pkg); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/integration-tests.yml(1 hunks)integration-tests/pnpmfile.cjs(1 hunks)integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (1)
node/src/cli.rs (1)
status(451-453)
🪛 actionlint (1.7.9)
.github/workflows/integration-tests.yml
168-168: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
176-176: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Parachain
- GitHub Check: Summary
🔇 Additional comments (4)
.github/workflows/integration-tests.yml (1)
162-216: LGTM!The new
zombienet_battery_station_upgradejob correctly mirrors the structure of the existingzombienet_zeitgeist_upgradejob, with appropriate path adjustments for battery-station-runtime artifacts.integration-tests/pnpmfile.cjs (1)
1-35: Well-documented workaround for Node.js compatibility.The hook clearly explains the problem (Moonwall expects global WebSocket) and solution (redirect web imports to node build). The implementation safely handles string fields while passing through non-strings unchanged.
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (2)
120-181: Good addition of timeout and comprehensive status handling.The
txStatusfunction now properly handles:
- Configurable timeout with cleanup
- Centralized cleanup via
finishhelper- All terminal failure states (
isDropped,isInvalid,isUsurped,isRetracted,isFinalityTimeout)- Success on
isInBlockorisFinalizedThis is a robust improvement over the previous implementation.
140-153: Good logging improvements.Extracting
statusto a local variable and includingstatus?.type ?? "unknown"in the log output improves debuggability, especially when investigating failed upgrade tests.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1459 +/- ##
=======================================
Coverage 93.28% 93.28%
=======================================
Files 181 181
Lines 35049 35049
=======================================
Hits 32696 32696
Misses 2353 2353
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mergify
bot
added
s:in-progress
Chralt98
added
s:review-needed
mergify
bot
added
s:in-progress
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/integration-tests.yml (1)
169-169: Consider updating GitHub Actions versions across the workflow.
actions/checkout@v2andactions/setup-node@v3are outdated; the current stable versions arev6for both. While the new job follows the existing pattern in this file, updating all action versions in a follow-up PR would improve security, compatibility, and support.Also applies to: 177-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/integration-tests.yml(3 hunks)integration-tests/scripts/ws-shim.cjs(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/integration-tests.yml
169-169: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
177-177: the runner of "actions/setup-node@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Fuzz
- GitHub Check: Test parachain build
- GitHub Check: Checks (clippy)
- GitHub Check: Checks (parachain)
- GitHub Check: Test standalone build
- GitHub Check: Quick check benchmarks
- GitHub Check: Checks (standalone)
- GitHub Check: Format
- GitHub Check: Coverage
- GitHub Check: Coverage
- GitHub Check: Quick check benchmarks
- GitHub Check: Test parachain build
- GitHub Check: Fuzz
- GitHub Check: Test standalone build
- GitHub Check: Format
- GitHub Check: Checks (standalone)
- GitHub Check: Checks (clippy)
- GitHub Check: Checks (parachain)
- GitHub Check: Build Parachain
- GitHub Check: Summary
🔇 Additional comments (6)
.github/workflows/integration-tests.yml (5)
160-160: LGTM! WebSocket polyfill correctly configured.The NODE_OPTIONS export properly preloads the ws-shim.cjs to ensure WebSocket support is available before test execution.
217-217: LGTM! WebSocket polyfill correctly configured.The NODE_OPTIONS export properly preloads the ws-shim.cjs to ensure WebSocket support is available before test execution.
266-266: LGTM! WebSocket polyfill correctly configured.The NODE_OPTIONS export properly preloads the ws-shim.cjs to ensure WebSocket support is available before test execution.
322-322: LGTM! WebSocket polyfill correctly configured.The NODE_OPTIONS export properly preloads the ws-shim.cjs to ensure WebSocket support is available before test execution.
163-219: LGTM! Battery Station test job properly structured.The new
zombienet_battery_station_upgradejob correctly mirrors the existing Zeitgeist upgrade test pattern with appropriate path adjustments for the battery-station runtime. All necessary setup steps are included, and the WebSocket polyfill is properly configured.integration-tests/scripts/ws-shim.cjs (1)
1-4: LGTM! The ws dependency is properly configured.The WebSocket polyfill is correctly implemented. The ws package (v8.17.1) is installed as a devDependency in integration-tests, and the conditional check properly ensures existing WebSocket implementations aren't overridden.
Chralt98
added
s:review-needed
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
integration-tests/tests/common-tests.ts (1)
65-95:canSendBalanceTransfernever breaks on success becausetxHashis an unawaited promise.At line 70,
const txHash = tx.signAndSend(alice);is missingawait. Without it,txHashis aPromiseobject rather than the actual extrinsic hash. When line 91 comparesincludedTxHashes.includes(txHash.toString()), it's comparing real hashes against"[object Promise]", which always fails. The loop then retries up toMAX_BALANCE_TRANSFER_TRIEStimes even when the first transfer succeeds.Also at line 66, the
awaitonparaApi.tx.balances.transferAllowDeath(...)is unnecessary since it's a synchronous call that returns an extrinsic object immediately.Fix:
while (tries < MAX_BALANCE_TRANSFER_TRIES) { - const tx = await paraApi.tx.balances.transferAllowDeath( + const tx = paraApi.tx.balances.transferAllowDeath( randomAccount.address, amount ); - const txHash = tx.signAndSend(alice); + const txHash = await tx.signAndSend(alice);
🧹 Nitpick comments (1)
integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (1)
102-181:txStatustimeout/cleanup is robust; consider fixing hash log labels and a small edge case.The refactored
txStatuslooks solid:
- Central
finishhelper reliably clears the timeout, logs an optional message, unsubscribes, and settles the promise.- Timeouts now reject with a clear error, preventing tests from hanging indefinitely.
- You explicitly treat dropped/invalid/usurped/retracted/finality-timeout statuses as failures, which makes debugging much easier.
- Success is cleanly gated on
isInBlockorisFinalized.Two small refinements you might want to make:
Swap the “current” vs “new” runtime hash logging (diagnostic clarity only).
Around the “Runtime not upgraded, proceeding with test” branch,
codeStringis the on-chain code andrtHexis read from the upgrade file, but the labels are reversed:log( "Current runtime hash: " + rtHex.slice(0, 10) + "..." + rtHex.slice(-10) ); log( "New runtime hash: " + codeString.slice(0, 10) + "..." + codeString.slice(-10) );Swapping them keeps logs intuitive:
log("Current runtime hash: " +rtHex.slice(0, 10) +"..." +rtHex.slice(-10));log("New runtime hash: " +codeString.slice(0, 10) +"..." +codeString.slice(-10));
log("Current runtime hash: " +codeString.slice(0, 10) +"..." +codeString.slice(-10));log("New runtime hash: " +rtHex.slice(0, 10) +"..." +rtHex.slice(-10));
Optional: guard against a synchronous throw from
tx.signAndSend.In the unlikely event that
tx.signAndSendthrows synchronously (before returning its promise), the executor will reject the outer promise but the timeout will still fire later and log a misleading “timed out” error. Wrapping the call in a try/catch would avoid that:
tx.signAndSend(alice, (result: any) => {
try {tx.signAndSend(alice, (result: any) => {@@
}).then((unsub: () => void) => {unsubscribe = unsub;}).catch((err: any) => {clearTimeout(timeout);reject(err);});
}).then((unsub: () => void) => {unsubscribe = unsub;}).catch((err: any) => {clearTimeout(timeout);reject(err);});} catch (err: any) {clearTimeout(timeout);reject(err);}Both points are small, but they improve debuggability and make the helper fully defensive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
integration-tests/tests/common-tests.ts(1 hunks)integration-tests/tests/rt-upgrade-battery-station-chopsticks/test-battery-station-chopsticks-runtime-upgrade.ts(1 hunks)integration-tests/tests/rt-upgrade-zeitgeist-chopsticks/test-zeitgeist-chopsticks-runtime-upgrade.ts(1 hunks)integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(2 hunks)integration-tests/tests/setup-websocket.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Cargo Deny
- GitHub Check: Fuzz
- GitHub Check: Quick check benchmarks
- GitHub Check: Format
- GitHub Check: Build Parachain
- GitHub Check: Summary
🔇 Additional comments (5)
integration-tests/tests/setup-websocket.ts (1)
1-7: Global WebSocket shim is minimal and idempotent (LGTM).The guard around
g.WebSocketkeeps this side-effect safe across repeated imports and avoids clobbering any existing implementation.integration-tests/tests/common-tests.ts (1)
19-19: WebSocket setup import placement looks good.Placing
import "./setup-websocket";first ensures the global WebSocket is initialized before the rest of the test utilities run.integration-tests/tests/rt-upgrade-zeitgeist-chopsticks/test-zeitgeist-chopsticks-runtime-upgrade.ts (1)
19-19: WebSocket setup import correctly ordered.Importing
"../setup-websocket"first ensures the WebSocket shim is in place before the chopsticks tests interact with the chain APIs.integration-tests/tests/rt-upgrade-battery-station-chopsticks/test-battery-station-chopsticks-runtime-upgrade.ts (1)
19-19: Consistent WebSocket setup for battery station tests.Bringing in
"../setup-websocket"at the top mirrors the other upgrade suites and ensures a global WebSocket is available before these tests run.integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts (1)
19-19: WebSocket setup import aligns zombienet test with other suites.The side-effect import of
"../setup-websocket"ensures the runtime upgrade test has a global WebSocket available before creating API connections.
What does it do?
It adds the zombienet battery station IT test and fixes #1456 (comment)
What important points should reviewers know?
Is there something left for follow-up PRs?
What alternative implementations were considered?
Are there relevant PRs or issues?
References
Summary by CodeRabbit
Tests
Chores
CI
✏️ Tip: You can customize this high-level summary in your review settings.