test: fix several cucumbers - #1563
Conversation
WalkthroughThis PR reorganizes workspace dependencies, adds new Tari git dependencies, updates .gitignore, changes error handling in a wallet daemon process, modifies test concurrency and multiple feature scenarios, and refactors confidential transfer tests to use a new stealth transfer API with updated function signatures and call sites. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester
participant CLI as wallet_daemon_cli
participant WDC as Wallet Daemon Client
participant WD as Wallet Daemon
participant VN as Validator Node
Tester->>CLI: transfer_confidential(src, dest, amount, wallet, outputs)
CLI->>WDC: accounts_stealth_transfer(StealthTransferRequest{ XTR, ConfidentialOnly, ... })
WDC->>WD: POST /accounts/stealth_transfer
WD->>VN: Submit transaction
VN-->>WD: Wait response (accepted/rejected)
WD-->>WDC: Wait result
WDC-->>CLI: Result
CLI-->>Tester: Update substate ids or fail with reason
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Cargo.toml (1)
62-63: Remove duplicate workspace member entry
Remove one of the two identical"utilities/transaction_submitter"lines to avoid tooling noise.@@ Cargo.toml:62-63 - "utilities/transaction_submitter",integration_tests/tests/features/concurrency.feature (1)
28-28: Assertion likely failing with current engine behaviorExpecting "30" after concurrent calls contradicts the comment that only the first tx executes; this will fail the pipeline. Either (a) downgrade the expectation temporarily or (b) tag the scenario to skip until the lock bug is fixed.
Two options:
- Temporary expectation change:
- When I invoke on wallet daemon WALLET_D on account ACC on component COUNTER/components/Counter the method call "value" the result is "30" + When I invoke on wallet daemon WALLET_D on account ACC on component COUNTER/components/Counter the method call "value" the result is "1"
- Or ignore this scenario until the engine fix:
# add this at the scenario line @ignoreNote: The runner filters "@ignore" before execution, so fail_on_skipped won’t trip.
🧹 Nitpick comments (20)
.gitignore (2)
3-3: Anchor this ignore to repo root unless you intend a global match
cucumber-output-junit.xmlwithout a leading slash ignores this filename anywhere in the tree, which makes Line 61 redundant and may hide files in subprojects unintentionally. If you only want the root artifact ignored, anchor it.Apply:
-cucumber-output-junit.xml +/cucumber-output-junit.xml
64-65: Nit: make directory intent explicit for moon cache/dockerAdd trailing slashes to clarify these are directories (functionally similar, but clearer).
-.moon/cache -.moon/docker +.moon/cache/ +.moon/docker/applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs (1)
27-44: Avoid UTF-8 conversion; pass the path as OsStr directly
Command::argacceptsAsRef<OsStr>, so you can remove.to_str()and the extra error handling entirely.- command - .envs(context.environment()) - .arg("-b") - .arg(context.base_path()) - .arg("--network") - .arg(context.network().to_string()) - .args([ - "create-account", - "--name", - "Fees", - "--key", - "0", - "--set-active", - "--output", - output_path - .to_str() - .context("Non-UTF8 output path in WalletDaemonCreateAccount")?, - ]); + command + .envs(context.environment()) + .arg("-b") + .arg(context.base_path()) + .arg("--network") + .arg(context.network().to_string()) + .arg("create-account") + .arg("--name") + .arg("Fees") + .arg("--key") + .arg("0") + .arg("--set-active") + .arg("--output") + .arg(&output_path);integration_tests/tests/features/substates.feature (1)
25-27: Fix typos in commentsMinor nits.
- # We should get an error if we se as inputs the same component version thas has already been downed from previous transactions + # We should get an error if we see as inputs the same component version that's already been downed by previous transactionsintegration_tests/tests/features/counter.feature (1)
9-9: DRY the repeated precondition with a BackgroundOptional, but keeps scenarios lean.
Gherkin snippet:
Background: Given a network with registered validator VN and wallet daemon WALLET_DThen drop the duplicated Given in each scenario.
Also applies to: 29-29
integration_tests/tests/features/fungible.feature (2)
21-25: Add explicit assertions for TX1 effectsThe manifest performs actions but the scenario doesn’t assert outcomes at this step. Consider adding a Then/And that verifies ACC1’s balance increased as expected.
Does the harness validate
balance(...)return values automatically, or should we add explicit “the result is …” checks?
35-47: Stabilize resource addressing and assert final balances
- Referencing
FAUCET/resources/0can be brittle if resource indices change; prefer a named/resource handle if available from the faucet component or event output.- Consider asserting post-TX2 balances (ACC2: 50, ACC1: 9950) to ensure the transfer is actually verified.
Example (conceptual):
Then wallet daemon WALLET_D reports ACC2 balance of the faucet resource is "50" And wallet daemon WALLET_D reports ACC1 balance of the faucet resource is "9950"Cargo.toml (1)
130-141: Pin Tari git dependencies in Cargo.toml to the resolved commit
Cargo.lock already records all three new Tari git deps—tari_sidechain,tari_transaction_components, andtari_transaction_key_manager—at commit213a0c510457be9eae5a140f2a72fb0dc4d13764. To prevent branch‐head drift when runningcargo update, replace in Cargo.toml:-tari_sidechain = { git = "https://github.com/tari-project/tari.git", branch = "development" } +tari_sidechain = { git = "https://github.com/tari-project/tari.git", rev = "213a0c510457be9eae5a140f2a72fb0dc4d13764" }(and do likewise for
tari_transaction_componentsandtari_transaction_key_manager; you can also consolidate these into a shared[patch]block).integration_tests/src/wallet_daemon_cli.rs (4)
171-193: Stealth transfer happy-path is good; consider more tolerant input selection.
ConfidentialOnlycan flake if the source has insufficient confidential inputs. ConsiderPreferConfidential(keeps confidential when possible, falls back when needed).Apply:
- input_selection: ConfidentialTransferInputSelection::ConfidentialOnly, + input_selection: ConfidentialTransferInputSelection::PreferConfidential,If you expect strictly confidential spends here, ignore; otherwise this reduces intermittent insufficiency errors.
239-247: Also surface explicit fee rejections for parity with other helpers.Other functions check
fee_reject(). Add it here for clearer failures.Apply:
- if let Some(reason) = wait_resp + if let Some(reason) = wait_resp .result .as_ref() .expect("Transaction has timed out") .result .any_reject() { panic!("Transaction failed: {}", reason); } + if let Some(reason) = wait_resp + .result + .as_ref() + .and_then(|r| r.fee_reject().cloned()) + { + panic!("Transaction fee rejected: {}", reason); + }
195-233: Remove legacy commented block.The old manual-proofs path is now obsolete; keeping ~40 lines commented adds noise.
If you want to preserve it, move to docs or a gist and add a short comment with a link.
156-158: De-duplicate hard-coded 120s timeout.Define a single constant to keep timeouts consistent and easy to tweak.
Apply (top of file):
+const TX_WAIT_TIMEOUT_SECS: u64 = 120;Then:
- timeout_secs: Some(120), + timeout_secs: Some(TX_WAIT_TIMEOUT_SECS),Also applies to: 236-237, 361-363, 495-497, 579-582
integration_tests/tests/features/nft.feature (1)
9-9: Ensure network precondition is isolation-safe under concurrent scenarios.
- Verify that
Given a network with registered validator VN and wallet daemon WALLET_Dspins up fresh VN and WALLET_D instances per scenario (avoiding cross-test state bleed under @Concurrent).- (Optional) Move this step into a
Background:block to DRY it across scenarios.integration_tests/tests/features/concurrency.feature (2)
7-8: Tag mismatch: feature won't run as a “concurrent” scenarioRunner detects the tag "concurrent", but this feature uses "@concurrency" (Line 4). Either retag the feature to "@Concurrent" or update the selector in cucumber.rs to look for "concurrency". Otherwise this scenario will be scheduled serially.
24-24: High flake risk: write-lock conflicts under concurrent increasesGiven the known lock bug, blasting 30 concurrent "increase" calls will intermittently reject transactions, making the scenario nondeterministic. Consider reducing the concurrency or splitting into two scenarios (accept path vs. expected rejection path) until the engine fix lands.
Apply a temporary reduction to stabilize CI:
- When I invoke on wallet daemon WALLET_D on account ACC on component COUNTER/components/Counter the method call "increase" concurrently 30 times + When I invoke on wallet daemon WALLET_D on account ACC on component COUNTER/components/Counter the method call "increase" concurrently 1 timesintegration_tests/tests/features/wallet_daemon.feature (2)
51-53: Commented-out assertions leave the scenario without verificationThe TODO notes the check isn’t resource-specific. Prefer re-enabling assertions against the faucet resource to validate the transfer.
Proposed step wording for precision (to implement in steps/CLI):
- When I check the balance of ACCOUNT_X for resource "FAUCET/resources/0" on wallet daemon WALLET_D the amount is exactly N
65-66: Time-based mining/mempool waits can still flake“1 tx in mempool within 10s” + “mine 13 blocks” may be brittle across CI loads. Consider polling height/mempool until condition holds with an upper bound, then proceed (or compute required blocks dynamically from current tip).
integration_tests/tests/cucumber.rs (1)
65-65: Reduce parallelism to 2: stability > throughputGood call; this should lower contention and flakiness. Consider making it configurable via an env var (default 2) for local runs/CI.
integration_tests/tests/features/epoch_change.feature (2)
18-18: Remove commented-out legacy stepDead/commented step adds noise and can confuse future edits.
Apply:
-# When I call function "mint" on template "faucet" on VN with args "amount_10000" named "FAUCET"
22-24: Verify height/epoch assumptions for flakiness
Mining 15 blocks after “height ≥ 6” may not deterministically reach scanned height 40 or epoch 4 without knowing the pre-mined height and the configured epoch length. Confirm that these thresholds align with your epoch length and any seeded blocks or else derive expectations dynamically:
- Record initial height H₀, mine N blocks, then assert scanned height ≥ H₀ + N + k.
- Compute the expected epoch from the epoch-length constant and assert epoch ≥ target rather than exact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
.gitignore(2 hunks)Cargo.toml(1 hunks)applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs(2 hunks)clients/base_node_client/Cargo.toml(1 hunks)integration_tests/src/wallet_daemon_cli.rs(6 hunks)integration_tests/tests/cucumber.rs(2 hunks)integration_tests/tests/features/concurrency.feature(1 hunks)integration_tests/tests/features/counter.feature(2 hunks)integration_tests/tests/features/epoch_change.feature(2 hunks)integration_tests/tests/features/fungible.feature(1 hunks)integration_tests/tests/features/nft.feature(2 hunks)integration_tests/tests/features/substates.feature(1 hunks)integration_tests/tests/features/wallet_daemon.feature(3 hunks)integration_tests/tests/steps/wallet_daemon.rs(0 hunks)
💤 Files with no reviewable changes (1)
- integration_tests/tests/steps/wallet_daemon.rs
🧰 Additional context used
🧬 Code graph analysis (2)
integration_tests/tests/cucumber.rs (3)
crates/engine/tests/test.rs (2)
result(862-863)result(868-869)crates/template_test_tooling/src/template_test.rs (2)
result(431-436)result(464-469)utilities/tariswap_test_bench/src/tariswap.rs (8)
result(266-266)result(267-267)result(268-268)result(269-269)result(327-327)result(328-328)result(329-329)result(330-330)
integration_tests/src/wallet_daemon_cli.rs (6)
bindings/src/helpers/consts.ts (1)
XTR(10-10)bindings/src/types/UnsignedTransaction.ts (1)
UnsignedTransaction(4-4)bindings/src/types/wallet-daemon-client/StealthTransferRequest.ts (1)
StealthTransferRequest(8-17)integration_tests/src/util.rs (2)
cucumber_log(12-15)transaction_builder(8-10)bindings/src/types/wallet-daemon-client/ComponentAddressOrName.ts (1)
ComponentAddressOrName(4-4)bindings/src/types/ConfidentialTransferInputSelection.ts (1)
ConfidentialTransferInputSelection(3-7)
⏰ 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: fmt
- GitHub Check: check nightly
- GitHub Check: test
- GitHub Check: machete
- GitHub Check: check stable
- GitHub Check: clippy
🔇 Additional comments (15)
clients/base_node_client/Cargo.toml (1)
13-14: Confirm default‐features for tari_transaction_components and tari_node_components
These workspace dependencies now implicitly enable their default features (which may pull in additional behavior or transitive deps). If you intended to disable defaults, restoredefault-features = false; otherwise, explicitly declare your feature set.--- clients/base_node_client/Cargo.toml @@ [dependencies] -tari_transaction_components = { workspace = true } -tari_node_components = { workspace = true } +tari_transaction_components = { workspace = true, default-features = false } +tari_node_components = { workspace = true, default-features = false }applications/tari_swarm_daemon/src/process_definitions/wallet_daemon_create_key.rs (1)
6-6: Nice: replaced panic with contextual error propagationUsing
anyhow::Contextand?improves robustness vsexpect. Good change.integration_tests/tests/features/substates.feature (1)
9-16: Setup simplification looks goodSwitch to the shared network baseline and explicit template publish is clearer and should reduce flake.
integration_tests/tests/features/counter.feature (1)
18-19: Confirm name scoping to avoid clashesBoth scenarios name the component “COUNTER”. It’s unclear from the step definitions whether that identifier is scoped per scenario or shared globally—please verify that named components are scenario-isolated under @Concurrent to prevent collisions.
integration_tests/tests/features/fungible.feature (1)
8-9: Baseline refactor LGTMUsing the preconfigured network and wallet daemon simplifies setup and should reduce test flakiness.
Cargo.toml (1)
145-148: Networking crates group LGTM.Clear grouping and explicit paths improve readability and local builds.
integration_tests/src/wallet_daemon_cli.rs (5)
39-44: Imports align with new flow.Using
XTRand narrowing toUnsignedTransactionfits the updated API.
64-65: New type import OK.
StealthTransferRequestmatches the fields used below.
76-77: Switch to cucumber_log is good.Consistent test logging beats
eprintln!.
408-409: LGTM: structured logging for balances.
428-429: LGTM: structured logging for confidential balances.integration_tests/tests/features/wallet_daemon.feature (2)
9-9: Baseline step consolidation looks goodUnified network setup improves test readability and cuts boilerplate.
17-18: Verify funding assumptions and downstream balance checksACC_2 now starts with 100000 free coins and the check is "at least 10000". Ensure subsequent balance assertions (especially faucet-token specific ones) account for this larger starting balance to avoid masking transfer failures.
integration_tests/tests/features/epoch_change.feature (2)
21-21: Step wording OK“on VN VN” matches the usual pattern “on VN ”. No change needed.
9-9: Confirm per-scenario isolation for “Given a network with registered validator … and wallet daemon …”
Verify that this step definition spins up a fresh, isolated network (unique ports/directories or separateTariWorld) and fully tears it down between scenarios to prevent cross-test bleed under @Concurrent.
| match result.return_type { | ||
| Type::U32 => { | ||
| let u32_result: u32 = result.decode().unwrap(); | ||
| let u32_result: u32 = result.decode()?; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Safer decode with ? instead of unwrap
Nice improvement—propagates decoding errors instead of panicking. Consider aligning other decode/unwrap call sites in this file for consistency.
Run to find remaining risky sites:
🏁 Script executed:
#!/bin/bash
# Find decode() results unwrapped with unwrap() in tests
rg -nP --type=rust -C2 '\.decode::<[^>]+>\(\)\.unwrap\(\)' integration_tests/testsLength of output: 84
🏁 Script executed:
#!/bin/bash
# Search for any unwrapped decode() results in integration tests
rg -nP --type=rust '\.decode(?:::<[^>]+>)?\(\)\.unwrap\(\)' integration_tests/testsLength of output: 289
Replace unwrapped decode().unwrap() calls with ? across tests
- integration_tests/tests/cucumber.rs:352: change
.decode().unwrap()to.decode()? - integration_tests/tests/cucumber.rs:541: change
.decode().unwrap()to.decode()?
🤖 Prompt for AI Agents
In integration_tests/tests/cucumber.rs around lines 352, 395 and 541, replace
occurrences of .decode().unwrap() with .decode()?; ensure the enclosing test
functions return a Result type compatible with the decode() error (e.g.,
propagate using anyhow::Result or the existing test error type), update the
function signatures to return that Result if needed and adjust imports so the ?
operator compiles.
| # Initialize indexer and connect wallet daemon | ||
| Given an indexer IDX connected to base node BASE | ||
| Given a wallet daemon WALLET_D connected to indexer IDX | ||
| Given a network with registered validator VN and wallet daemon WALLET_D |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Same note for the second scenario.
Verify per-scenario isolation or mark this feature @serial if the step uses a shared instance.
🏁 Script executed:
#!/bin/bash
rg -nP '@serial|@concurrent' integration_tests/tests/features/nft.feature -n -C0Length of output: 96
Mark NFT feature as @serial or ensure per-scenario isolation The feature is annotated @concurrent (line 4) but uses a shared network instance (Given a network with registered validator VN and wallet daemon WALLET_D); change the annotation to @serial or refactor to spin up an isolated network per scenario.
🤖 Prompt for AI Agents
In integration_tests/tests/features/nft.feature around line 62, the feature is
marked @concurrent but uses a shared network instance via "Given a network with
registered validator VN and wallet daemon WALLET_D", which can cause
interference between parallel scenarios; either change the feature annotation to
@serial at the top of the file to force serial execution, or refactor the steps
to create and tear down an isolated network per scenario (e.g., replace the
shared "Given a network..." with a per-scenario setup/fixture that spins up a
fresh network and wallet and ensures cleanup after each scenario).
|
|
||
| // Withdraw 50 of the tokens and send them to acc2 | ||
| let tokens = acc1.withdraw(faucet_resource, Amount(50)); | ||
| let tokens = acc1.withdraw(faucet_resource, Amount(1000)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Transfer amount increased to 1000: align assertions and fees
Withdrawing 1000 changes expected end balances. Re-introduce explicit resource-scoped balance checks for ACC_1 and ACC_2 that account for fees, or assert on net delta. The current generic balance checks were commented out.
I can add a step that checks balance for a specific resource id on an account via wallet daemon to make this robust—want me to open a follow-up?
🤖 Prompt for AI Agents
integration_tests/tests/features/wallet_daemon.feature around line 44:
withdrawing 1000 changes expected end balances so restore explicit,
resource-scoped assertions for ACC_1 and ACC_2 (or assert net delta) that
account for transaction fees; update the test to query the wallet daemon for the
specific resource id balance after the withdrawal and assert expected_balance =
previous_balance - 1000 - fees (or compare net delta between before/after
balances), and adjust any commented-out generic balance checks to use
resource-scoped checks or a dedicated step that fetches balances by resource id
to make expectations robust.
Test Results (CI)419 tests +24 413 ✅ +18 1h 16m 8s ⏱️ + 30m 53s For more details on these failures, see this check. Results for commit 09b656e. ± Comparison against base commit bee07d3. |
2 participants
Description
test: fix several cucumbers
Motivation and Context
Fixed around 5 more cucumber scenarios
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores