test: comprehensive e2e and kittest UI testing infrastructure#778
test: comprehensive e2e and kittest UI testing infrastructure#778thepastaclaw wants to merge 4 commits intodashpay:v1.0-devfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
thepastaclaw
force-pushed
the
test/e2e-rebase
branch
from
6e3961e to
d38f15d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #778 adds a comprehensive E2E and kittest UI testing infrastructure. The code is well-structured with phase-based execution, retry logic, and emergency cleanup. However, there is one confirmed blocking bug: the E2E test clicks a 'Fetch Contracts' button that doesn't exist (the actual label is 'Add Contracts'). The remaining findings are architectural suggestions around test isolation and assertion quality.
Reviewed commit: 9b2e1c8
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(s)
3 additional findings
🟡 suggestion: Round-trip assertion is trivially satisfied before return payment arrives
tests/backend-e2e/send_funds.rs (lines 107-115)
Wallet A starts with 5M duffs, sends 2M to B, leaving ~3M. The wait_for_balance check at line 108 waits for A to reach send_amount (2M). Since A already has ~3M after sending, this predicate is true immediately — the test never actually verifies that B's return payment arrived. To properly verify the round trip, capture A's balance after the send and wait for it to increase.
💡 Suggested change
// Capture A's balance after the send, then wait for it to increase
let a_balance_after_send = {
let wallets = app_context.wallets().read().expect("lock");
wallets.get(&hash_a).map(|w| w.read().expect("lock").total_balance_duffs()).unwrap_or(0)
};
// Verify A received the return payment
let a_balance_after_return = wait_for_balance(
app_context,
hash_a,
a_balance_after_send + 1, // Any increase proves B->A arrived
Duration::from_secs(120),
)
.await
.expect("A should receive return funds from B");
🟡 suggestion: Withdrawal test only checks task result type, not actual state change
tests/backend-e2e/identity_withdraw.rs (lines 52-61)
The test verifies BackendTaskSuccessResult::WithdrewFromIdentity was returned but never checks that the identity's balance actually decreased or that the withdrawal amount matches expectations. While the task succeeding proves Platform accepted the state transition, verifying the balance delta would catch edge cases like partial withdrawals or fee miscalculations. At minimum, re-query the identity balance and assert it decreased by approximately withdraw_amount.
🟡 suggestion: Backend E2E suite commented out in CI — new test code can regress silently
.github/workflows/tests.yml (lines 83-92)
The backend E2E test execution is commented out with a TODO: "Re-enable after backend E2E tests are updated for TaskError migration". While these tests are #[ignore] and need network access, the fact that this PR adds/modifies them but they aren't exercised in CI means compilation failures or logic issues won't be caught. At minimum, consider running them with --ignored in a separate CI job (even if allowed to fail), or ensure cargo test --test backend-e2e --all-features (without --ignored) compiles successfully.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `tests/e2e/phases/phase_03_platform.rs`:
- [BLOCKING] lines 118-123: E2E test clicks non-existent 'Fetch Contracts' button — will panic every run
The test searches for a button labeled `"Fetch Contracts"` (line 120), but `add_contracts_screen.rs:339` renders the button as `"Add Contracts"`. The `.expect()` on line 122 will panic with `'Fetch Contracts' button must be visible on Add Contracts screen` every time this phase runs.
In `src/ui/mod.rs`:
- [SUGGESTION] line 86: Module visibility widened from pub(crate) to pub for test access
The `identities` module was changed from `pub(crate)` to `pub`, exposing internal UI types in the public API. The `wallets` module (line 91) correctly stays `pub(crate)` and uses `#[cfg(feature = "e2e")]` accessor methods instead. Phase 06 needs `RegisterDpnsNameSource` — this could be re-exported from `src/ui/mod.rs` (it's already referenced by `ScreenType`) and the identities module kept `pub(crate)`. The direct field access on `RegisterDpnsNameScreen` in phase_06 (lines 43-65) should use cfg-gated accessors like `AddNewIdentityScreen` does.
In `tests/backend-e2e/send_funds.rs`:
- [SUGGESTION] lines 107-115: Round-trip assertion is trivially satisfied before return payment arrives
Wallet A starts with 5M duffs, sends 2M to B, leaving ~3M. The `wait_for_balance` check at line 108 waits for A to reach `send_amount` (2M). Since A already has ~3M after sending, this predicate is true immediately — the test never actually verifies that B's return payment arrived. To properly verify the round trip, capture A's balance after the send and wait for it to increase.
In `tests/kittest/interactions.rs`:
- [SUGGESTION] lines 16-21: Kittest tests may use production database when run without --all-features
The kittest tests call `AppState::new()` which uses the production database unless the `testing` feature is active. Unlike the E2E tests (which have `required-features = ["e2e"]` in Cargo.toml), kittest has no `[[test]]` section enforcing `required-features = ["testing"]`. Running `cargo test --test kittest` without `--all-features` would read the real app data directory and database. CI is fine (uses `--all-features`), but developers could hit this. Consider adding a `[[test]]` section with `required-features = ["testing"]`.
In `tests/backend-e2e/identity_withdraw.rs`:
- [SUGGESTION] lines 52-61: Withdrawal test only checks task result type, not actual state change
The test verifies `BackendTaskSuccessResult::WithdrewFromIdentity` was returned but never checks that the identity's balance actually decreased or that the withdrawal amount matches expectations. While the task succeeding proves Platform accepted the state transition, verifying the balance delta would catch edge cases like partial withdrawals or fee miscalculations. At minimum, re-query the identity balance and assert it decreased by approximately `withdraw_amount`.
In `.github/workflows/tests.yml`:
- [SUGGESTION] lines 83-92: Backend E2E suite commented out in CI — new test code can regress silently
The backend E2E test execution is commented out with a TODO: "Re-enable after backend E2E tests are updated for TaskError migration". While these tests are `#[ignore]` and need network access, the fact that this PR adds/modifies them but they aren't exercised in CI means compilation failures or logic issues won't be caught. At minimum, consider running them with `--ignored` in a separate CI job (even if allowed to fail), or ensure `cargo test --test backend-e2e --all-features` (without `--ignored`) compiles successfully.
In `tests/e2e/phases/phase_04_tokens.rs`:
- [SUGGESTION] lines 9-62: Token search retry may accumulate text in input field
On retry, `navigate_to_screen` resets to the root token search screen but root screens persist across navigation (stored in `main_screens` BTreeMap). The text input from the previous attempt may still contain "dash". When `type_into_text_input(harness, 0, "dash")` runs on attempt 2, it appends to the existing text, producing "dashdash". This depends on whether `refresh_on_arrival()` clears the search input on `TokenSearchScreen`. If it doesn't, the retry will search for a concatenated string.
| // Click "Fetch Contracts" submit button | ||
| harness | ||
| .query_by_label("Fetch Contracts") | ||
| .or_else(|| harness.query_by_label_contains("Fetch Contracts")) | ||
| .expect("'Fetch Contracts' button must be visible on Add Contracts screen") | ||
| .click(); |
There was a problem hiding this comment.
🔴 Blocking: E2E test clicks non-existent 'Fetch Contracts' button — will panic every run
The test searches for a button labeled "Fetch Contracts" (line 120), but add_contracts_screen.rs:339 renders the button as "Add Contracts". The .expect() on line 122 will panic with 'Fetch Contracts' button must be visible on Add Contracts screen every time this phase runs.
💡 Suggested change
| // Click "Fetch Contracts" submit button | |
| harness | |
| .query_by_label("Fetch Contracts") | |
| .or_else(|| harness.query_by_label_contains("Fetch Contracts")) | |
| .expect("'Fetch Contracts' button must be visible on Add Contracts screen") | |
| .click(); | |
| // Click "Add Contracts" submit button | |
| harness | |
| .query_by_label("Add Contracts") | |
| .or_else(|| harness.query_by_label_contains("Add Contracts")) | |
| .expect("'Add Contracts' button must be visible on Add Contracts screen") | |
| .click(); |
source: ['claude-general', 'codex-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `tests/e2e/phases/phase_03_platform.rs`:
- [BLOCKING] lines 118-123: E2E test clicks non-existent 'Fetch Contracts' button — will panic every run
The test searches for a button labeled `"Fetch Contracts"` (line 120), but `add_contracts_screen.rs:339` renders the button as `"Add Contracts"`. The `.expect()` on line 122 will panic with `'Fetch Contracts' button must be visible on Add Contracts screen` every time this phase runs.
| pub mod dpns; | ||
| pub mod helpers; | ||
| pub(crate) mod identities; | ||
| pub mod identities; |
There was a problem hiding this comment.
🟡 Suggestion: Module visibility widened from pub(crate) to pub for test access
The identities module was changed from pub(crate) to pub, exposing internal UI types in the public API. The wallets module (line 91) correctly stays pub(crate) and uses #[cfg(feature = "e2e")] accessor methods instead. Phase 06 needs RegisterDpnsNameSource — this could be re-exported from src/ui/mod.rs (it's already referenced by ScreenType) and the identities module kept pub(crate). The direct field access on RegisterDpnsNameScreen in phase_06 (lines 43-65) should use cfg-gated accessors like AddNewIdentityScreen does.
source: ['claude-general', 'claude-rust-quality']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/mod.rs`:
- [SUGGESTION] line 86: Module visibility widened from pub(crate) to pub for test access
The `identities` module was changed from `pub(crate)` to `pub`, exposing internal UI types in the public API. The `wallets` module (line 91) correctly stays `pub(crate)` and uses `#[cfg(feature = "e2e")]` accessor methods instead. Phase 06 needs `RegisterDpnsNameSource` — this could be re-exported from `src/ui/mod.rs` (it's already referenced by `ScreenType`) and the identities module kept `pub(crate)`. The direct field access on `RegisterDpnsNameScreen` in phase_06 (lines 43-65) should use cfg-gated accessors like `AddNewIdentityScreen` does.
| let harness = Harness::builder().with_max_steps(200).build_eframe(|ctx| { | ||
| dash_evo_tool::app::AppState::new(ctx.egui_ctx.clone()) | ||
| .expect("Failed to create AppState") | ||
| .with_animations(false) | ||
| }); | ||
| (rt, harness) |
There was a problem hiding this comment.
🟡 Suggestion: Kittest tests may use production database when run without --all-features
The kittest tests call AppState::new() which uses the production database unless the testing feature is active. Unlike the E2E tests (which have required-features = ["e2e"] in Cargo.toml), kittest has no [[test]] section enforcing required-features = ["testing"]. Running cargo test --test kittest without --all-features would read the real app data directory and database. CI is fine (uses --all-features), but developers could hit this. Consider adding a [[test]] section with required-features = ["testing"].
source: ['codex-general', 'codex-rust-quality']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `tests/kittest/interactions.rs`:
- [SUGGESTION] lines 16-21: Kittest tests may use production database when run without --all-features
The kittest tests call `AppState::new()` which uses the production database unless the `testing` feature is active. Unlike the E2E tests (which have `required-features = ["e2e"]` in Cargo.toml), kittest has no `[[test]]` section enforcing `required-features = ["testing"]`. Running `cargo test --test kittest` without `--all-features` would read the real app data directory and database. CI is fine (uses `--all-features`), but developers could hit this. Consider adding a `[[test]]` section with `required-features = ["testing"]`.
| for attempt in 1..=PLATFORM_MAX_RETRIES { | ||
| // ─── 1. Navigate to token search screen (fresh each attempt) ─── | ||
| navigate_to_screen(harness, RootScreenType::RootScreenTokenSearch); | ||
|
|
||
| let on_screen = | ||
| wait_for_label(harness, "Enter Keyword", std::time::Duration::from_secs(10)); | ||
| assert!( | ||
| on_screen, | ||
| "'Enter Keyword' label must be visible on token search screen" | ||
| ); | ||
|
|
||
| if attempt == 1 { | ||
| println!(" Navigated to token search screen"); | ||
| } | ||
|
|
||
| type_into_text_input(harness, 0, "dash"); | ||
| println!( | ||
| " Typed 'dash' in search input (attempt {}/{})", | ||
| attempt, PLATFORM_MAX_RETRIES | ||
| ); | ||
|
|
||
| // ─── 2. Click search button and wait for results ─────────────── | ||
| harness | ||
| .query_by_label("Search") | ||
| .expect("Search button must be visible on token search screen") | ||
| .click(); | ||
| harness.run_steps(SETTLE_STEPS); | ||
|
|
||
| let completed = wait_until( | ||
| harness, | ||
| |h| { | ||
| h.query_by_label_contains("Contract ID").is_some() | ||
| || h.query_by_label_contains("No tokens match").is_some() | ||
| || h.query_by_label_contains("Error").is_some() | ||
| }, | ||
| TOKEN_SEARCH_TIMEOUT, | ||
| POLL_STEPS, | ||
| ); | ||
|
|
||
| if !completed { | ||
| println!( | ||
| " Token search timed out after {}s (attempt {}/{})", | ||
| TOKEN_SEARCH_TIMEOUT.as_secs(), | ||
| attempt, | ||
| PLATFORM_MAX_RETRIES | ||
| ); | ||
| if attempt == PLATFORM_MAX_RETRIES { | ||
| panic!( | ||
| "Token search timed out after {} attempts ({}s each)", | ||
| PLATFORM_MAX_RETRIES, | ||
| TOKEN_SEARCH_TIMEOUT.as_secs() | ||
| ); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
🟡 Suggestion: Token search retry may accumulate text in input field
On retry, navigate_to_screen resets to the root token search screen but root screens persist across navigation (stored in main_screens BTreeMap). The text input from the previous attempt may still contain "dash". When type_into_text_input(harness, 0, "dash") runs on attempt 2, it appends to the existing text, producing "dashdash". This depends on whether refresh_on_arrival() clears the search input on TokenSearchScreen. If it doesn't, the retry will search for a concatenated string.
source: ['claude-general']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `tests/e2e/phases/phase_04_tokens.rs`:
- [SUGGESTION] lines 9-62: Token search retry may accumulate text in input field
On retry, `navigate_to_screen` resets to the root token search screen but root screens persist across navigation (stored in `main_screens` BTreeMap). The text input from the previous attempt may still contain "dash". When `type_into_text_input(harness, 0, "dash")` runs on attempt 2, it appends to the existing text, producing "dashdash". This depends on whether `refresh_on_arrival()` clears the search input on `TokenSearchScreen`. If it doesn't, the retry will search for a concatenated string.
| const ERROR_PATTERNS: &[(ErrorCategory, &[&str])] = &[ | ||
| ( | ||
| ErrorCategory::Network, | ||
| &[ | ||
| "timeout", | ||
| "connection", | ||
| "network", | ||
| "unavailable", | ||
| "timed out", | ||
| "refused", | ||
| "unreachable", | ||
| ], | ||
| ), | ||
| ( | ||
| ErrorCategory::Validation, | ||
| &[ | ||
| "invalid", | ||
| "insufficient", | ||
| "already exists", | ||
| "not found", | ||
| "duplicate", | ||
| "too low", | ||
| "too high", | ||
| ], | ||
| ), | ||
| ( | ||
| ErrorCategory::TransientPlatform, | ||
| &[ | ||
| "consensus", | ||
| "retry", | ||
| "temporarily", | ||
| "try again", | ||
| "rate limit", | ||
| "internal error", | ||
| "transport error", | ||
| ], | ||
| ), | ||
| ]; | ||
|
|
||
| pub fn classify_error(error_text: &str) -> ErrorCategory { | ||
| let lower = error_text.to_lowercase(); | ||
| for (category, patterns) in ERROR_PATTERNS { | ||
| if patterns.iter().any(|p| lower.contains(p)) { | ||
| return *category; | ||
| } | ||
| } | ||
| ErrorCategory::Fatal | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Error classification intentionally biases toward retry — document the trade-off
The priority-ordered pattern matching (Network checked before Validation) means a validation error containing "connection" or "network" would be misclassified as retryable. The comment at lines 57-60 documents this as intentional for E2E resilience. This is a reasonable trade-off for test infrastructure, but consider adding an example to the comment (e.g., "invalid connection parameter" would classify as Network) so future maintainers understand the edge case.
source: ['claude-general', 'claude-rust-quality']
|
claw can we rename directory to frontend-e2e ? we already have backend-e2e, it will be consistent. |
thepastaclaw
force-pushed
the
test/e2e-rebase
branch
from
9b2e1c8 to
763d39c
Compare
|
Done — renamed |
thepastaclaw
force-pushed
the
test/e2e-rebase
branch
from
763d39c to
bc82804
Compare
Implements a multi-phase E2E test suite for wallet import, SPV sync, wallet UI operations, platform reads, token search, identity creation, DPNS registration, and teardown. Also ports kittest interactions and hardens existing test infrastructure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Aligns the frontend E2E test directory name with the existing tests/backend-e2e/ convention, as requested in PR review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
test/e2e-rebase
branch
from
bc82804 to
f112247
Compare
# Conflicts: # Cargo.lock # Cargo.toml
|
✅ Review complete (commit e90cc8b) |
Remove all #[cfg(feature = "e2e")] blocks from screen types per review feedback. Wallet import in e2e tests now goes through AppContext::register_wallet() instead of manipulating ImportMnemonicScreen fields. AddNewIdentityScreen and RegisterDpnsNameScreen accessors are now unconditionally public. Remove the `e2e` feature flag from Cargo.toml since no source files use it anymore. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Self-review of testing infrastructure PR. Verified 2 findings: (1) wallet reuse keyed only by alias creates a destructive collision risk — teardown deletes whatever wallet matched, even if it's a preexisting non-test wallet; (2) the identity/DPNS phase is explicitly skipped, so the PR description overstates e2e coverage.
Reviewed commit: e90cc8b
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `tests/frontend-e2e/phases/phase_00_setup.rs`:
- [SUGGESTION] lines 16-110: E2E wallet reuse is keyed only by alias, so the suite can adopt and later delete an unrelated wallet
`find_existing_e2e_wallet()` reuses the first wallet whose `alias` equals the fixed string `"E2E Test Wallet"`; it does not verify the mnemonic, seed hash, or any other test-owned marker before setting `ctx.wallet_seed_hash` and `ctx.wallet_reused` ([tests/frontend-e2e/phases/phase_00_setup.rs#L16](tests/frontend-e2e/phases/phase_00_setup.rs#L16), [tests/frontend-e2e/phases/phase_00_setup.rs#L110](tests/frontend-e2e/phases/phase_00_setup.rs#L110)). Teardown then unconditionally removes whatever seed hash is stored in the context, and the panic-path `emergency_cleanup()` does the same, with no guard for `ctx.wallet_reused` ([tests/frontend-e2e/phases/phase_07_teardown.rs#L47](tests/frontend-e2e/phases/phase_07_teardown.rs#L47), [tests/frontend-e2e/helpers/harness.rs#L414](tests/frontend-e2e/helpers/harness.rs#L414)). That makes alias collisions destructive: a preexisting non-test wallet with that alias can be hijacked for the run and then deleted from the DB.
In `tests/frontend-e2e/phases/phase_05_identity.rs`:
- [SUGGESTION] line 15: The phased E2E journey does not execute the identity/DPNS flow it claims to add coverage for
The main E2E entrypoint runs Phase 5, but `phase_05_identity::run()` explicitly skips actual identity creation and only performs client-side validation checks, so it never sets `ctx.identity_id` ([tests/frontend-e2e/phases/phase_05_identity.rs#L15](tests/frontend-e2e/phases/phase_05_identity.rs#L15)). `tests/frontend-e2e/main.rs` then explicitly skips Phase 6 DPNS registration for that reason ([tests/frontend-e2e/main.rs#L41](tests/frontend-e2e/main.rs#L41)). So the landed phased suite currently covers platform reads and UI validation, but not the end-to-end identity creation and DPNS registration path implied by the PR description.
| /// Check if a wallet with the E2E alias already exists that we can reuse. | ||
| /// Only matches by exact alias -- never grabs unrelated wallets. | ||
| fn find_existing_e2e_wallet(harness: &Harness<'_, AppState>) -> Option<WalletSeedHash> { | ||
| let app_ctx = harness.state().current_app_context(); | ||
| let wallets = app_ctx.wallets().read().unwrap(); | ||
| wallets | ||
| .iter() | ||
| .find(|(_, wallet)| wallet.read().unwrap().alias.as_deref() == Some(E2E_WALLET_ALIAS)) | ||
| .map(|(seed_hash, _)| *seed_hash) | ||
| } | ||
|
|
||
| /// Import a wallet through the AppContext/model layer (no UI screen setters). | ||
| /// Parses the mnemonic, creates a Wallet, and registers it via AppContext. | ||
| fn import_wallet_via_context( | ||
| harness: &mut Harness<'_, AppState>, | ||
| ctx: &mut TestContext, | ||
| words: &[&str], | ||
| ) { | ||
| let initial_wallet_keys: BTreeSet<WalletSeedHash> = { | ||
| let app_ctx = harness.state().current_app_context(); | ||
| app_ctx.wallets().read().unwrap().keys().copied().collect() | ||
| }; | ||
|
|
||
| let phrase = words.join(" "); | ||
| let mnemonic = bip39::Mnemonic::parse_normalized(&phrase) | ||
| .unwrap_or_else(|e| panic!("Invalid mnemonic: {}", e)); | ||
| let seed = mnemonic.to_seed(""); | ||
|
|
||
| let app_ctx = harness.state().current_app_context().clone(); | ||
| let wallet = Wallet::new_from_seed( | ||
| seed, | ||
| app_ctx.network(), | ||
| Some(E2E_WALLET_ALIAS.to_string()), | ||
| None, | ||
| ) | ||
| .unwrap_or_else(|e| panic!("Wallet creation failed: {}", e)); | ||
|
|
||
| println!( | ||
| " Parsed {} mnemonic words, alias '{}'", | ||
| words.len(), | ||
| E2E_WALLET_ALIAS | ||
| ); | ||
|
|
||
| let (new_seed_hash, _wallet_arc) = app_ctx | ||
| .register_wallet(wallet) | ||
| .unwrap_or_else(|e| panic!("Wallet registration failed: {}", e)); | ||
| println!(" Wallet registered via AppContext"); | ||
|
|
||
| // Let the UI pick up the new wallet | ||
| harness.run_steps(SETTLE_STEPS); | ||
|
|
||
| // Verify wallet appeared | ||
| { | ||
| let wallets = app_ctx.wallets().read().unwrap(); | ||
| assert!( | ||
| wallets.len() > initial_wallet_keys.len(), | ||
| "Wallet count didn't increase after registration (still {})", | ||
| initial_wallet_keys.len() | ||
| ); | ||
| assert!( | ||
| wallets.contains_key(&new_seed_hash), | ||
| "Registered wallet not found in AppContext" | ||
| ); | ||
| ctx.wallet_seed_hash = Some(new_seed_hash); | ||
| } | ||
| println!( | ||
| " Wallet imported. Seed hash prefix: {}", | ||
| seed_hash_prefix(ctx.seed_hash()) | ||
| ); | ||
| } | ||
|
|
||
| pub fn run(harness: &mut Harness<'_, AppState>, ctx: &mut TestContext) { | ||
| // 1. Read & validate mnemonic | ||
| let mnemonic = | ||
| std::env::var("E2E_WALLET_MNEMONIC").expect("E2E_WALLET_MNEMONIC env var required"); | ||
| let words: Vec<&str> = mnemonic.split_whitespace().collect(); | ||
| assert!( | ||
| [12, 15, 18, 21, 24].contains(&words.len()), | ||
| "Mnemonic has {} words, expected 12/15/18/21/24", | ||
| words.len() | ||
| ); | ||
| println!(" Mnemonic: {} words", words.len()); | ||
|
|
||
| // 2. Dismiss welcome screen | ||
| dismiss_welcome_screen(harness); | ||
| harness.run_steps(SETTLE_STEPS); | ||
|
|
||
| // 3. Switch to testnet and enable SPV mode | ||
| harness.state_mut().change_network(Network::Testnet); | ||
| harness.run_steps(SETTLE_STEPS); | ||
| let app_ctx = harness.state().current_app_context().clone(); | ||
| app_ctx.set_core_backend_mode(CoreBackendMode::Spv); | ||
| println!(" Switched to testnet (SPV mode)"); | ||
|
|
||
| // 4. Check if wallet already exists (idempotent re-run support) |
There was a problem hiding this comment.
🟡 Suggestion: E2E wallet reuse is keyed only by alias, so the suite can adopt and later delete an unrelated wallet
find_existing_e2e_wallet() reuses the first wallet whose alias equals the fixed string "E2E Test Wallet"; it does not verify the mnemonic, seed hash, or any other test-owned marker before setting ctx.wallet_seed_hash and ctx.wallet_reused (tests/frontend-e2e/phases/phase_00_setup.rs#L16, tests/frontend-e2e/phases/phase_00_setup.rs#L110). Teardown then unconditionally removes whatever seed hash is stored in the context, and the panic-path emergency_cleanup() does the same, with no guard for ctx.wallet_reused (tests/frontend-e2e/phases/phase_07_teardown.rs#L47, tests/frontend-e2e/helpers/harness.rs#L414). That makes alias collisions destructive: a preexisting non-test wallet with that alias can be hijacked for the run and then deleted from the DB.
source: codex
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `tests/frontend-e2e/phases/phase_00_setup.rs`:
- [SUGGESTION] lines 16-110: E2E wallet reuse is keyed only by alias, so the suite can adopt and later delete an unrelated wallet
`find_existing_e2e_wallet()` reuses the first wallet whose `alias` equals the fixed string `"E2E Test Wallet"`; it does not verify the mnemonic, seed hash, or any other test-owned marker before setting `ctx.wallet_seed_hash` and `ctx.wallet_reused` ([tests/frontend-e2e/phases/phase_00_setup.rs#L16](tests/frontend-e2e/phases/phase_00_setup.rs#L16), [tests/frontend-e2e/phases/phase_00_setup.rs#L110](tests/frontend-e2e/phases/phase_00_setup.rs#L110)). Teardown then unconditionally removes whatever seed hash is stored in the context, and the panic-path `emergency_cleanup()` does the same, with no guard for `ctx.wallet_reused` ([tests/frontend-e2e/phases/phase_07_teardown.rs#L47](tests/frontend-e2e/phases/phase_07_teardown.rs#L47), [tests/frontend-e2e/helpers/harness.rs#L414](tests/frontend-e2e/helpers/harness.rs#L414)). That makes alias collisions destructive: a preexisting non-test wallet with that alias can be hijacked for the run and then deleted from the DB.
| // Run validation sub-tests first — these are pure client-side (no network | ||
| // calls) and should run regardless of SPV sync state. | ||
| run_validation_tests(harness); | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: The phased E2E journey does not execute the identity/DPNS flow it claims to add coverage for
The main E2E entrypoint runs Phase 5, but phase_05_identity::run() explicitly skips actual identity creation and only performs client-side validation checks, so it never sets ctx.identity_id (tests/frontend-e2e/phases/phase_05_identity.rs#L15). tests/frontend-e2e/main.rs then explicitly skips Phase 6 DPNS registration for that reason (tests/frontend-e2e/main.rs#L41). So the landed phased suite currently covers platform reads and UI validation, but not the end-to-end identity creation and DPNS registration path implied by the PR description.
source: codex
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `tests/frontend-e2e/phases/phase_05_identity.rs`:
- [SUGGESTION] line 15: The phased E2E journey does not execute the identity/DPNS flow it claims to add coverage for
The main E2E entrypoint runs Phase 5, but `phase_05_identity::run()` explicitly skips actual identity creation and only performs client-side validation checks, so it never sets `ctx.identity_id` ([tests/frontend-e2e/phases/phase_05_identity.rs#L15](tests/frontend-e2e/phases/phase_05_identity.rs#L15)). `tests/frontend-e2e/main.rs` then explicitly skips Phase 6 DPNS registration for that reason ([tests/frontend-e2e/main.rs#L41](tests/frontend-e2e/main.rs#L41)). So the landed phased suite currently covers platform reads and UI validation, but not the end-to-end identity creation and DPNS registration path implied by the PR description.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Addressed both findings from the self-review (commit e90cc8b):
Finding 1 — Wallet reuse keyed only by alias (fixed in 1ce396e):
find_existing_e2e_wallet() now takes expected_seed_hash: &WalletSeedHash and verifies both alias AND seed hash before reusing. The expected hash is computed from the mnemonic early in run() (SHA-256 of BIP39 seed bytes via ClosedKeyItem::compute_seed_hash). A wallet with a matching alias but different seed — i.e., a preexisting non-test wallet — will no longer be adopted and subsequently deleted by teardown.
Finding 2 — Phase 5 scope overstated (addressed in 1ce396e):
Added a doc comment to phase_05_identity::run() explicitly stating that the phase runs client-side UI validation sub-tests only and does not perform actual identity creation. Updated the Phase 5 banner in main.rs to match: "Phase 5: Identity Screen Validation (creation skipped — pending SPV mempool)".
Branch: thepastaclaw/dash-evo-tool@tracker-922 (1 commit on top of e90cc8b)
2 participants
Summary
Comprehensive end-to-end testing infrastructure for DET. Adds a phased e2e test suite that exercises the full user journey (wallet import → SPV sync → balance verification → platform reads → token search → identity validation → teardown) and kittest interaction tests for welcome screen behavior and navigation.
Continuation of #598, rebased onto current v1.0-dev.
E2E Test Suite (
cargo test --features e2e --test e2e -- --ignored --nocapture)e2eCargo feature gates test-only visibility on internal types.github/workflows/e2e.ymlwith environment-gated mnemonicKittest Interaction Tests
Source Changes
hint_texton TextEdit widgets for accessibility/test discovery#[cfg(feature = "e2e")]accessor methods on screen types (no production API changes)Dropimpl onImportMnemonicScreento zeroize seed wordsTest Results
--all-features --all-targetsSkipped (pending SPV mempool support)
Validation
cargo check --all-features✅cargo clippy --all-features --all-targets -- -D warnings✅cargo test --all-features— 90 passed, 0 failed ✅Co-authored-by: PastaPastaPasta pasta@dashboost.org