feat(e2e): Migrated backup tests #17150
Conversation
WalkthroughThe changes introduce several modifications to the end-to-end testing components, including new test files and updates to existing classes. In the desktop suite, the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/suite-desktop-core/e2e/support/pageActions/settings/deviceActions.ts (1)
24-30: Consider adding error handling for click actions.While the implementation is correct, consider adding error handling and retry logic for click actions to improve test reliability.
@step() async proceedMultiShareBackupModal(): Promise<void> { + const retryOptions = { timeout: 5000, retries: 3 }; + try { await this.page.getByTestId('@multi-share-backup/checkbox/1').click(); await this.page.getByTestId('@multi-share-backup/checkbox/2').click(); await this.firstInfoSubmitButton.click(); await this.secondInfoSubmitButton.click(); + } catch (error) { + throw new Error(`Failed to proceed multi-share backup modal: ${error.message}`); + } }packages/suite-desktop-core/e2e/tests/backup/t2t1-fail.test.ts (1)
38-39: Consider reducing the timeout value.The 30-second timeout for error message visibility seems excessive. Consider reducing it to improve test execution time.
- await expect(page.getByTestId('@backup/error-message')).toBeVisible({ timeout: 30000 }); + await expect(page.getByTestId('@backup/error-message')).toBeVisible({ timeout: 10000 });packages/suite-desktop-core/e2e/support/pageActions/onboarding/backupActions.ts (1)
47-49: Consider replacing timeout with a more robust solution.The delay to mitigate race condition could be replaced with a more reliable approach.
- // Adding delay to mitigate race condition; avoids hitting the homescreen - await this.page.waitForTimeout(1000); + // Wait for specific condition instead of arbitrary timeout + await this.page.waitForSelector('[data-test="@backup/shamir-progress"]', { state: 'visible' }); await TrezorUserEnvLinkProxy.readAndConfirmShamirMnemonicEmu({ shares, threshold });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/suite-desktop-core/e2e/support/pageActions/dashboardActions.ts(2 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/backupActions.ts(2 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/deviceActions.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(3 hunks)packages/suite-desktop-core/e2e/tests/backup/t2t1-fail.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/backup/t2t1-misc.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/backup/t2t1-success.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/backup/t3t1-create-additional-share.test.ts(1 hunks)packages/suite-web/e2e/tests/backup/t2t1-fail.test.ts(0 hunks)packages/suite-web/e2e/tests/backup/t2t1-misc.test.ts(0 hunks)packages/suite-web/e2e/tests/backup/t2t1-success.test.ts(0 hunks)packages/suite-web/e2e/tests/backup/t3t1-create-addtional-share.test.ts(0 hunks)
💤 Files with no reviewable changes (4)
- packages/suite-web/e2e/tests/backup/t2t1-misc.test.ts
- packages/suite-web/e2e/tests/backup/t2t1-success.test.ts
- packages/suite-web/e2e/tests/backup/t3t1-create-addtional-share.test.ts
- packages/suite-web/e2e/tests/backup/t2t1-fail.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=passphrase, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (10)
packages/suite-desktop-core/e2e/support/pageActions/settings/deviceActions.ts (1)
5-22: LGTM! Well-structured class following page object model pattern.The class is well-organized with:
- Clear separation of public/private members
- Immutable properties using readonly
- Consistent test ID naming pattern
packages/suite-desktop-core/e2e/tests/backup/t3t1-create-additional-share.test.ts (2)
5-12: LGTM! Well-structured test setup.Good practices:
- Clear test group tagging
- Proper emulator configuration
- Consistent beforeEach setup
37-39:❓ Verification inconclusive
Replace hardcoded timeout with a more robust solution.
The hardcoded timeout is a workaround for a known issue. Consider implementing a more reliable solution.
🏁 Script executed:
#!/bin/bash # Check if the issue is still open gh issue view 17128 --json state,titleLength of output: 129
Manual Verification Needed: GH Issue Details
The initial verification failed because the GH CLI command was executed outside a Git repository context. This makes it hard to confirm the current state of issue #17128.
The review comment suggests replacing the hardcoded timeout in
packages/suite-desktop-core/e2e/tests/backup/t3t1-create-additional-share.test.ts(lines 37-39)
// [device screen] create additional backup? page.waitForTimeout(1000); // without this timeout, backup on device simply disappears, it stinks TODO: https://github.com/trezor/trezor-suite/issues/17128 await trezorUserEnvLink.pressYes();with a more robust solution that no longer relies on an arbitrary delay.
To move forward, please re-run the issue verification using the repository flag so that the GH CLI can fetch the proper details. For example, run the command below:
#!/bin/bash gh issue view 17128 --repo trezor/trezor-suite --json state,titleOnce you’ve confirmed whether the issue is still open or has been fixed, we can update the approach accordingly.
packages/suite-desktop-core/e2e/tests/backup/t2t1-fail.test.ts (1)
6-15: LGTM! Comprehensive test setup for failure scenarios.Good practices:
- Proper device model configuration
- Analytics interception setup
- Clear test group tagging
packages/suite-desktop-core/e2e/support/pageActions/onboarding/backupActions.ts (1)
8-10: LGTM! Clear and descriptive property names.Properties follow consistent naming pattern and clearly indicate their purpose.
packages/suite-desktop-core/e2e/tests/backup/t2t1-misc.test.ts (2)
14-35: LGTM! Good test coverage for modal state reset.The test effectively verifies that the backup modal resets its state when closed and reopened, ensuring a clean state for each backup attempt.
37-53: LGTM! Good error handling test.The test properly verifies that backup cannot be initiated when the device is disconnected, even if it's remembered.
packages/suite-desktop-core/e2e/tests/backup/t2t1-success.test.ts (1)
18-62: LGTM! Comprehensive test coverage for successful backup flow.The test effectively covers:
- Initial setup and preconditions
- User interactions with checkboxes
- Device interactions
- Final state verification using analytics events
packages/suite-desktop-core/e2e/support/pageActions/dashboardActions.ts (1)
36-36: LGTM! Clean addition of backup notification locator.The new locator property follows the established pattern and naming conventions.
Also applies to: 64-64
packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts (1)
6-6: LGTM! Clean integration of DeviceActions.The integration of DeviceActions follows good practices:
- Clean dependency injection
- Proper initialization in constructor
- Maintains existing patterns
Also applies to: 42-42, 78-78
8eac7a4 to
11e59df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/suite-desktop-core/e2e/tests/backup/t2t1-success.test.ts (3)
7-16: Add JSDoc documentation for the test suite.Consider adding JSDoc documentation to describe the test suite's purpose, prerequisites, and configuration details. This would help other developers understand the test setup and maintenance requirements.
+/** + * End-to-end test suite for validating successful backup process on T2T1 device. + * Prerequisites: + * - Device is wiped and requires backup + * - Uses specific mnemonic for consistent testing + * - Requires completed onboarding + */ test.describe('Backup success', { tag: ['@group=device-management'] }, () => {
18-34: Add error handling for device interactions.The test should handle potential device interaction failures gracefully. Consider adding try-catch blocks with appropriate error messages and cleanup steps.
test('Successful backup happy path', async ({ analytics, onboardingPage, dashboardPage, devicePrompt, trezorUserEnvLink, }) => { + try { // access from notification await dashboardPage.notificationNoBackupButton.click(); await onboardingPage.backup.undertandWhatSeedIsCheckbox.click(); await onboardingPage.backup.hasEnoughTimeCheckbox.click(); await onboardingPage.backup.isInPrivateCheckbox.click(); // Create backup on device await onboardingPage.backup.startButton.click(); + } catch (error) { + throw new Error(`Failed to initiate backup process: ${error.message}`); + }
50-62: Enhance verification steps for comprehensive test coverage.The current verification focuses on UI elements and analytics events but could be more thorough. Consider adding:
- Verification of device backup status
- Confirmation that the seed was properly stored
- Validation of the backup recovery process
await expect(onboardingPage.backup.closeButton).not.toBeDisabled(); + // Verify device backup status + const deviceState = await trezorUserEnvLink.getDeviceState(); + expect(deviceState.needs_backup).toBeFalsy(); + + // Verify analytics event const createBackupEvent = analytics.findAnalyticsEventByType< ExtractByEventType<EventType.CreateBackup> >(EventType.CreateBackup); expect(createBackupEvent.status).toEqual('finished'); expect(createBackupEvent.error).toEqual(''); + + // Optional: Verify backup recovery + // This could be a separate test case that validates the backup process worked
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/test-suite-web-e2e.yml(0 hunks)packages/suite-desktop-core/e2e/support/pageActions/dashboardActions.ts(2 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/backupActions.ts(2 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/deviceActions.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts(3 hunks)packages/suite-desktop-core/e2e/tests/backup/t2t1-fail.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/backup/t2t1-misc.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/backup/t2t1-success.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/backup/t3t1-create-additional-share.test.ts(1 hunks)packages/suite-web/e2e/tests/backup/t2t1-fail.test.ts(0 hunks)packages/suite-web/e2e/tests/backup/t2t1-misc.test.ts(0 hunks)packages/suite-web/e2e/tests/backup/t2t1-success.test.ts(0 hunks)packages/suite-web/e2e/tests/backup/t3t1-create-addtional-share.test.ts(0 hunks)
💤 Files with no reviewable changes (5)
- packages/suite-web/e2e/tests/backup/t2t1-success.test.ts
- .github/workflows/test-suite-web-e2e.yml
- packages/suite-web/e2e/tests/backup/t2t1-misc.test.ts
- packages/suite-web/e2e/tests/backup/t2t1-fail.test.ts
- packages/suite-web/e2e/tests/backup/t3t1-create-addtional-share.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/suite-desktop-core/e2e/support/pageActions/settings/deviceActions.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/suite-desktop-core/e2e/support/pageActions/dashboardActions.ts
- packages/suite-desktop-core/e2e/tests/backup/t2t1-fail.test.ts
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/backupActions.ts
- packages/suite-desktop-core/e2e/tests/backup/t2t1-misc.test.ts
- packages/suite-desktop-core/e2e/tests/backup/t3t1-create-additional-share.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
packages/suite-desktop-core/e2e/tests/backup/t2t1-success.test.ts (1)
37-49: Address the TODO comment about flaky test.The comment indicates that
readAndConfirmMnemonicEmuis flaky and needs to be fixed. Consider investigating and fixing the flakiness issue.packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.ts (3)
6-6: LGTM!The import statement is correctly placed and follows the established pattern.
42-42: LGTM!The property is correctly declared as readonly and follows the class's property declaration pattern.
78-78:❓ Verification inconclusive
Verify the integration with DeviceActions.
The initialization is correctly implemented. Since this is part of a backup tests migration, ensure that all necessary device-related actions are accessible through this integration.
Run the following script to verify the integration:
🏁 Script executed:
#!/bin/bash # Description: Verify the integration between SettingsActions and DeviceActions # by checking for any existing backup-related tests that might need updating. # Search for backup-related test files echo "Searching for backup-related test files..." fd -e ts -e js backup # Search for usage of SettingsActions in backup-related tests echo "Checking usage of SettingsActions in tests..." rg -l "SettingsActions" $(fd -e ts -e js backup) # Search for any TODO comments related to device actions echo "Checking for TODO comments related to device actions..." rg -i "todo.*device" $(fd -e ts -e js)Length of output: 67120
DeviceActions Initialization Looks Correct – Please Confirm Backup Flow Functionality
- The constructor call on line 78 in
packages/suite-desktop-core/e2e/support/pageActions/settings/settingsActions.tscorrectly instantiatesDeviceActionswith thepageparameter.- Our search confirms the presence of backup-related tests and references to
SettingsActions, suggesting that the integration pattern aligns with other parts of the codebase.- Since this change is part of a backup tests migration, please manually verify that all required device-related actions (as provided by
DeviceActions) are fully accessible and correctly drive backup flows during end-to-end testing.
| test('User disconnected device that is remembered. Should not be allowed to initiate backup', async ({ | ||
| page, | ||
| dashboardPage, | ||
| onboardingPage, | ||
| trezorUserEnvLink, | ||
| }) => { | ||
| await expect(dashboardPage.graph).toBeVisible(); | ||
| await dashboardPage.openDeviceSwitcher(); | ||
| await dashboardPage.walletAtIndex(0).click(); | ||
| await dashboardPage.notificationNoBackupButton.click(); | ||
| await onboardingPage.backup.undertandWhatSeedIsCheckbox.click(); | ||
| await onboardingPage.backup.hasEnoughTimeCheckbox.click(); | ||
| await onboardingPage.backup.isInPrivateCheckbox.click(); | ||
|
|
||
| await trezorUserEnvLink.stopEmu(); | ||
| await expect(page.getByTestId('@backup/no-device')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
based on the name, it looks like part of the test is completely missing.
There was a problem hiding this comment.
No it's fine. The last line checks that there is a modal over the whole suite, so you can't initiate backup until you reconnect. Maybe it's confusing wording though.
Description
Related Issue
Resolve
Screenshots: