test(suite-native): app settings e2e #17494
Conversation
WalkthroughThe pull request implements a series of code refactorings and enhancements across end-to-end tests, page objects, utilities, and UI components. In several page object files, element selectors have been updated to consistently use the element wrapper, and method names have been refactored (for example, renaming waitForScreen to waitForInitScreen and enableNetwork to toggleNetwork). A new SettingsActions class was introduced to encapsulate settings-related interactions, and new navigation methods were added to support settings access via the tab bar. Test suites have been updated for consistency with these changes, including modifications to import statements and removal of platform-specific logic. Additionally, multiple files have been updated to include testID properties on components, improving their identifiability in testing, while utility functions were also adjusted for more flexible parameter handling. Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
suite-native/app/e2e/pageObjects/settingsActions.ts (1)
68-83: View-only mode toggle with error handlingThe
toggleWalletViewOnlymethod handles toggling the view-only mode with appropriate error handling for the alert sheet. The try-catch block gracefully handles the case where no alert appears when enabling view-only mode.Consider splitting this into two more explicit methods for better clarity:
- async toggleWalletViewOnly( - deviceName: string = TREZOR_E2E_DEVICE_LABEL, - walletIndex: number = 0, - ) { - const toggleButtonElement = element( - by.id(`@settings/view-only/toggle-button/${deviceName}/${walletIndex}`), - ); - await waitFor(toggleButtonElement).toBeVisible().withTimeout(10000); - await toggleButtonElement.tap(); - - try { - await onAlertSheet.tapPrimaryButton(); - } catch { - // Do nothing. In case of enabling view only mode, there is no alert sheet. - } - } + async enableWalletViewOnly( + deviceName: string = TREZOR_E2E_DEVICE_LABEL, + walletIndex: number = 0, + ) { + const toggleButtonElement = element( + by.id(`@settings/view-only/toggle-button/${deviceName}/${walletIndex}`), + ); + await waitFor(toggleButtonElement).toBeVisible().withTimeout(10000); + await toggleButtonElement.tap(); + // No alert sheet is shown when enabling view-only mode + } + + async disableWalletViewOnly( + deviceName: string = TREZOR_E2E_DEVICE_LABEL, + walletIndex: number = 0, + ) { + const toggleButtonElement = element( + by.id(`@settings/view-only/toggle-button/${deviceName}/${walletIndex}`), + ); + await waitFor(toggleButtonElement).toBeVisible().withTimeout(10000); + await toggleButtonElement.tap(); + + // Alert sheet is shown when disabling view-only mode + await onAlertSheet.tapPrimaryButton(); + } + + // Keep the original method for backward compatibility + async toggleWalletViewOnly( + deviceName: string = TREZOR_E2E_DEVICE_LABEL, + walletIndex: number = 0, + ) { + const toggleButtonElement = element( + by.id(`@settings/view-only/toggle-button/${deviceName}/${walletIndex}`), + ); + await waitFor(toggleButtonElement).toBeVisible().withTimeout(10000); + await toggleButtonElement.tap(); + + try { + await onAlertSheet.tapPrimaryButton(); + } catch { + // Do nothing. In case of enabling view only mode, there is no alert sheet. + } + }suite-native/app/e2e/tests/appSettingsWithDevice.test.ts (4)
37-41: Consider replacing fixed wait with deterministic alternativeUsing a fixed wait time might lead to flaky tests if the device connection takes longer than expected.
- await wait(5000); // wait for trezor device to start communicating with the app + // Consider using a deterministic wait that checks for device readiness + await waitFor(element(by.id('@device-manager/device-switch').withDescendant(by.text('Connected')))) + .toBeVisible() + .withTimeout(10000);
48-65: Remove outdated TODO comment and consider extracting navigation patternsThe test effectively validates coin enabling functionality, but has an outdated TODO.
- // TODO: rename to onCoinEnablingAlso, consider extracting the repeated back navigation pattern into a utility function for better maintainability:
async function navigateBackToHome(times = 2) { for (let i = 0; i < times; i++) { await device.pressBack(); } }
67-119: Comprehensive view-only mode test with detailed state verificationThe test thoroughly validates the view-only mode functionality with appropriate checks at each state transition. The test correctly verifies device connection states before and after toggling view-only mode and restarting the app.
However, the test contains multiple timeout values that are duplicated. Consider defining a constant for timeout values.
+ const DEVICE_CONNECTION_TIMEOUT = 10000; await waitFor( element( by.id('@device-manager/device-switch').withDescendant(by.text('Connected')), ), ) .toBeVisible() - .withTimeout(10000); + .withTimeout(DEVICE_CONNECTION_TIMEOUT);
108-119: Add a comment explaining the "Hi there!" expectationThe test checks for the text "Hi there!" but it's not immediately clear why this text appears after stopping the bridge in view-only mode.
await TrezorUserEnvLink.stopBridge(); + // In view-only mode, disconnecting shows the welcome message instead of "Disconnected" await waitFor( element( by.id('@device-manager/device-switch').withDescendant(by.text('Hi there!')), ), ) .toBeVisible() .withTimeout(10000);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
suite-native/app/e2e/pageObjects/accountImportActions.ts(1 hunks)suite-native/app/e2e/pageObjects/coinEnablingActions.ts(1 hunks)suite-native/app/e2e/pageObjects/deviceSettingsActions.ts(2 hunks)suite-native/app/e2e/pageObjects/homeActions.ts(2 hunks)suite-native/app/e2e/pageObjects/send/sendOutputsReviewActions.ts(1 hunks)suite-native/app/e2e/pageObjects/settingsActions.ts(1 hunks)suite-native/app/e2e/pageObjects/tabBarActions.ts(1 hunks)suite-native/app/e2e/tests/appSettings.test.ts(1 hunks)suite-native/app/e2e/tests/appSettingsWithDevice.test.ts(1 hunks)suite-native/app/e2e/tests/deeplinkPopup.test.ts(2 hunks)suite-native/app/e2e/tests/deviceSettings.test.ts(2 hunks)suite-native/app/e2e/tests/onboardAndConnect.test.ts(2 hunks)suite-native/app/e2e/tests/passphraseFlow.test.ts(2 hunks)suite-native/app/e2e/tests/receive.test.ts(2 hunks)suite-native/app/e2e/tests/send.test.ts(2 hunks)suite-native/app/e2e/utils.ts(3 hunks)suite-native/atoms/src/DiscreetText/DiscreetText.tsx(1 hunks)suite-native/atoms/src/DiscreetText/DiscreetTextTrigger.tsx(1 hunks)suite-native/atoms/src/Select/Select.tsx(2 hunks)suite-native/atoms/src/Select/SelectItem.tsx(1 hunks)suite-native/atoms/src/Select/SelectTrigger.tsx(2 hunks)suite-native/atoms/src/Sheet/BottomSheet.tsx(1 hunks)suite-native/formatters/src/components/FiatBalanceFormatter.tsx(2 hunks)suite-native/formatters/src/utils.ts(1 hunks)suite-native/graph/src/components/GraphFiatBalance.tsx(1 hunks)suite-native/module-settings/src/components/CryptoUnitsSelector.tsx(1 hunks)suite-native/module-settings/src/components/CurrencySelector.tsx(1 hunks)suite-native/module-settings/src/components/TouchableSwitchRow.tsx(3 hunks)suite-native/module-settings/src/components/ViewOnly/WalletRow.tsx(1 hunks)suite-native/module-settings/src/navigation/SettingsStackNavigator.tsx(2 hunks)suite-native/module-settings/src/screens/SettingsPrivacyAndSecurityScreen.tsx(2 hunks)suite-native/module-settings/src/screens/SettingsViewOnlyScreen.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (63)
suite-native/module-settings/src/components/ViewOnly/WalletRow.tsx (1)
124-124: Good addition of testID for automated testing purposes.This change enhances testability by adding a unique identifier to the Button component, making it easier to select this specific element during E2E tests. The construction of the testID follows a sensible hierarchical pattern and handles potential undefined values gracefully with the nullish coalescing operator.
suite-native/module-settings/src/components/TouchableSwitchRow.tsx (3)
14-14: Good addition of testID property to props typeAdding the optional testID to TouchableSwitchRowProps enhances testability, which aligns well with the PR objectives of implementing E2E tests for app settings.
32-32: Properly destructured the testID parameterThe parameter destructuring has been correctly updated to include the new testID property.
46-46: Correctly implemented testID on TouchableOpacityThe testID is properly passed to the TouchableOpacity component, which will allow for reliable element selection in E2E tests.
suite-native/module-settings/src/components/CryptoUnitsSelector.tsx (1)
33-33: Well-formatted testID for Bitcoin units selectorThe testID follows a clear naming convention with "@settings/localization/bitcoin-units-selector", making it easily identifiable in E2E tests. This directly supports the localization testing objectives mentioned in the PR summary.
suite-native/module-settings/src/components/CurrencySelector.tsx (1)
34-34: Consistent testID for currency selectorThe testID "@settings/localization/currency-selector" follows the same pattern as other selectors in the codebase, maintaining consistency for testing. This supports the localization testing for currency changes mentioned in the PR objectives.
suite-native/module-settings/src/screens/SettingsViewOnlyScreen.tsx (1)
14-14: Component renamed for better consistencyRenaming from
SettingsViewOnlytoSettingsViewOnlyScreenimproves naming consistency with other screen components, making the codebase more maintainable. This change supports the view-only mode testing objective mentioned in the PR.suite-native/module-settings/src/screens/SettingsPrivacyAndSecurityScreen.tsx (2)
50-50: Added testID for E2E testingAdding a testID to the DiscreetMode toggle is good for E2E test accessibility. This aligns with the PR objectives of implementing end-to-end tests for app settings, specifically for the discreet mode feature.
112-112: Renamed component for consistencyThe component has been renamed from
SettingsPrivacyAndSecuritytoSettingsPrivacyAndSecurityScreen, maintaining a consistent naming convention with other screen components in the codebase. This improves code readability and maintenance.suite-native/module-settings/src/navigation/SettingsStackNavigator.tsx (2)
15-16: Updated component imports after renamingThe import statements have been updated to reflect the renamed screen components, maintaining consistency with the component name changes in their respective files.
36-36: Updated component references in navigatorThe component references in the navigator have been correctly updated to use the renamed screen components, ensuring the navigation functionality remains intact after the component renames.
Also applies to: 41-41
suite-native/atoms/src/Sheet/BottomSheet.tsx (1)
123-123: Good addition of testID for improved testability.The addition of the
testIDproperty to the ScrollView component enables more reliable selection of this element in end-to-end tests, which aligns with the PR's objectives of enhancing the testing framework.suite-native/atoms/src/Select/SelectItem.tsx (1)
77-77: Excellent implementation of dynamic testID for Select items.The dynamic testID pattern
@select/item/${value}allows for precise targeting of specific select items in automated tests based on their values, which will be valuable for testing selection-based interactions like currency and Bitcoin unit changes.suite-native/formatters/src/utils.ts (1)
18-18: Good improvement to currency symbol handling.Adding the
trim()method improves the handling of whitespace in currency symbols, ensuring consistent currency display in the UI. The fallback tonullfor falsy values also provides more predictable behavior than passing the falsy value directly.suite-native/atoms/src/DiscreetText/DiscreetText.tsx (1)
61-61: Well-implemented conditional testID for discreet mode testing.The conditional testID that switches between 'discreet-text' and 'plain-text' based on the current state is particularly valuable for testing the discreet mode functionality, which is specifically mentioned as one of the features targeted by this PR's test suite.
suite-native/app/e2e/pageObjects/send/sendOutputsReviewActions.ts (1)
9-9: Appropriate timeout increase for E2E test reliabilityIncreasing the timeout from the default to 10 seconds is a good improvement for test stability. This change reduces the likelihood of flaky tests due to timing issues, especially when the SendOutputsReview screen might take longer to load under certain conditions.
suite-native/app/e2e/pageObjects/tabBarActions.ts (1)
12-18: Well-implemented navigation method for settingsThis new method follows the same pattern as the existing navigation methods, maintaining code consistency. The 10-second timeout aligns with other navigation methods, and the implementation correctly verifies that navigation was successful by checking the visibility of the Settings screen.
This addition supports the PR objective of implementing E2E tests for app settings features.
suite-native/atoms/src/Select/Select.tsx (2)
17-17: Good addition of testID to component propsAdding the optional testID property to the SelectProps type enhances component testability without changing functionality. This is a best practice for making components easily identifiable in automated tests.
25-25: Proper implementation of testID prop passingThe testID is correctly added to the props destructuring and properly passed to the SelectTrigger component, completing the implementation for test identification.
Also applies to: 56-56
suite-native/graph/src/components/GraphFiatBalance.tsx (1)
47-48: Well-structured testIDs for financial componentsThe addition of descriptive testIDs to both the DiscreetTextTrigger and FiatBalanceFormatter components follows good testing practices. The hierarchical naming convention (@home/portfolio/...) clearly identifies these components in the UI tree, making tests more maintainable and readable.
This is particularly important for financial components where accurate testing is critical.
suite-native/app/e2e/tests/deeplinkPopup.test.ts (2)
9-9: Import refactored to use the new class name.The import has been updated from
onCoinEnablingInittoonCoinEnablingto reflect the class renaming in the coin enabling actions page object.
65-67: Method names updated for clarity and consistency.The method calls have been updated to match the refactored implementation:
waitForScreen()→waitForInitScreen()- more clearly indicates this is for the initialization screenenableNetwork()→toggleNetwork()- better reflects the toggle functionalityThese changes align with the class renaming from
OnCoinEnablingInittoOnCoinEnabling.suite-native/app/e2e/tests/receive.test.ts (2)
7-7: Import updated to use refactored coin enabling class.The import has been updated from
onCoinEnablingInittoonCoinEnablingto match the renamed class in the page objects.
20-22: Method calls updated to match the refactored implementation.The method calls have been updated to match the refactored coin enabling page object:
waitForScreen()→waitForInitScreen()- provides more specific namingenableNetwork()→toggleNetwork()- better describes the action being performedThese changes are part of the broader refactoring effort to standardize test interactions.
suite-native/app/e2e/tests/deviceSettings.test.ts (2)
5-5: Import statement updated to use refactored page object.The import has been updated from
onCoinEnablingInittoonCoinEnablingto reflect the refactored class name.
26-28: Method calls updated to match the refactored page object implementation.The method calls have been updated for consistency with the refactored coin enabling page object:
waitForScreen()→waitForInitScreen()- more descriptive of the specific screenenableNetwork()→toggleNetwork()- better reflects the toggle functionalityThis consistency across test files improves maintainability.
suite-native/app/e2e/tests/passphraseFlow.test.ts (2)
8-8: Import statement updated to use refactored coin enabling class.The import has been updated from
onCoinEnablingInittoonCoinEnablingto reflect the renamed class.
77-79: Method calls updated to align with refactored implementation.The method calls have been updated to match the refactored coin enabling page object:
waitForScreen()→waitForInitScreen()- more specific namingenableNetwork()→toggleNetwork()- better reflects the action's purposeThese changes are consistent with the updates in other test files, ensuring uniformity across the test suite.
suite-native/atoms/src/Select/SelectTrigger.tsx (3)
15-15: Good addition of testID property.Adding the optional testID property to SelectTriggerProps enables better component testability in automated testing.
36-36: Function signature properly updated.The function signature has been correctly updated to include the new testID parameter in the destructured props.
40-40: TestID properly applied to TouchableOpacity.The testID is correctly passed to the TouchableOpacity component, making it identifiable in end-to-end tests.
suite-native/app/e2e/pageObjects/homeActions.ts (3)
3-5: Good element selector definition.The constant
graphHeaderDiscreetTextElementis well-defined and reusable, following proper Detox patterns for element selection with descendants.
21-23: Well-implemented discreet mode assertion.The method correctly checks that discreet mode is disabled by verifying the discreet text element is not visible, with an appropriate timeout.
25-27: Good implementation of discreet mode enabled assertion.The method correctly verifies that discreet mode is enabled by checking for the visibility of the discreet text element, maintaining consistency with the disabled check.
suite-native/app/e2e/pageObjects/accountImportActions.ts (1)
28-33: Improved element handling.Good refactoring to create a reusable element variable instead of duplicating the element selector. This approach is more maintainable and follows better practices for Detox testing.
suite-native/app/e2e/tests/send.test.ts (2)
6-6: Updated import for renamed module.Import statement correctly updated to use the renamed module
onCoinEnablinginstead ofonCoinEnablingInit.
80-82: Updated method calls for refactored API.Method calls have been properly updated to match the refactored API:
waitForScreen()→waitForInitScreen()enableNetwork('regtest')→toggleNetwork('regtest')- Using the new
onCoinEnablingobjectThese changes align with the broader refactoring of coin enabling functionality in the codebase.
suite-native/atoms/src/DiscreetText/DiscreetTextTrigger.tsx (3)
6-6: Props interface enhancement for testabilityAdding the optional
testIDproperty toDiscreetTextTriggerPropsis a good approach for improving the component's testability in E2E tests.
8-8: Function signature updated appropriatelyThe function signature has been updated to correctly destructure the new
testIDprop from the component props.
15-19: Test identifier correctly appliedThe
testIDprop is properly passed to the Pressable component, allowing it to be targeted in end-to-end tests. This change will help with testing the discreet mode functionality as mentioned in the PR objectives.suite-native/app/e2e/pageObjects/deviceSettingsActions.ts (2)
3-3: Improved element reference patternThe change from
checkAuthenticityButtonIdtocheckAuthenticityButtonElementusing theelement(by.id())pattern improves consistency with the rest of the file, where other button elements follow the same pattern.
34-34: Consistent element reference usageThe methods have been updated to use the
checkAuthenticityButtonElementvariable, maintaining the same functionality while improving code consistency. This approach creates a more consistent pattern throughout the test utilities.Also applies to: 38-39
suite-native/app/e2e/pageObjects/settingsActions.ts (4)
7-35: Well-structured settings navigation methodsThe new
SettingsActionsclass provides essential methods for navigating to different settings sections. Each method follows a consistent pattern of waiting for an element to be visible and then tapping it, which is a good practice for reliable end-to-end tests.These methods directly support the PR objectives by enabling tests to navigate to localization, privacy settings (discreet mode), coin enabling, and view-only mode sections.
36-42: Discreet mode toggle implementationThe
toggleDiscreetModemethod allows tests to interact with the discreet mode toggle, which is one of the specific features mentioned in the PR objectives. The implementation follows the same consistent pattern as other methods in this class.
44-66: Localization settings methodsThe methods for changing localization currency and Bitcoin units enable testing of the localization features mentioned in the PR objectives. Both methods follow a similar pattern:
- Tap on the selector trigger
- Find the desired option
- Tap the option
This implementation will allow for comprehensive testing of the localization functionality.
85-86: Properly exported instanceExporting a singleton instance of the
SettingsActionsclass asonSettingsfollows the pattern used by other page object files in the codebase, ensuring consistency.suite-native/formatters/src/components/FiatBalanceFormatter.tsx (3)
12-15: Props interface enhancement for testabilityAdding the optional
testIDproperty toBalanceFormatterPropstype is a good approach for improving the component's testability in E2E tests. The type definition is properly structured with the new property being optional.
23-27: Function signature updated appropriatelyThe function signature has been updated to correctly destructure the new
testIDprop from the component props.
40-40: Test identifier correctly appliedThe
testIDprop is properly passed to the Box component, allowing it to be targeted in end-to-end tests. This change will help with testing localization features and potentially discreet mode functionality as mentioned in the PR objectives.suite-native/app/e2e/tests/appSettings.test.ts (1)
1-84: Well-structured and comprehensive test suite for app settings!This new test suite provides good coverage for app settings functionality without requiring device interactions. It tests localization settings (both currency and Bitcoin units) and discreet mode, which aligns well with the PR objectives.
Some suggestions for minor improvements:
- Consider extracting the repeated timeout value of 10000ms into a constant to make maintenance easier
- The assertions rely on specific text values like "0 BTC" which could be brittle if the UI changes
- You might want to add a test for the View Only Mode mentioned in the PR objectives
Overall, the tests are well-organized with proper setup/teardown and clear assertions.
suite-native/app/e2e/tests/onboardAndConnect.test.ts (4)
7-7: Import updated to use the renamed page object.The import has been successfully updated from
onCoinEnablingInittoonCoinEnablingto match the refactored class name.
29-31: Simplified screen waiting logic is cleaner.The refactored code removes platform-specific conditional logic, making the test more consistent and easier to maintain.
33-39: Method names updated to match refactored API.The changes from
waitForScreen()towaitForInitScreen()andenableNetwork()totoggleNetwork()make the API more descriptive and better reflect the actual functionality.
40-42: Streamlined assertions with no platform dependency.Removing the platform-specific condition for the assertion makes the test more maintainable and consistent across platforms.
suite-native/app/e2e/pageObjects/coinEnablingActions.ts (3)
3-8: Class and method names refactored for clarity and consistency.Renaming the class from
OnCoinEnablingInittoOnCoinEnablingand the method fromwaitForScreentowaitForInitScreenmakes the API more descriptive and better reflects its specific functionality.
10-14: Method renamed and implementation improved.The method has been appropriately renamed from
enableNetworktotoggleNetworkwhich better reflects its functionality - toggling rather than just enabling. The implementation has also been improved by directly using the element reference rather than creating an intermediate matcher.
21-21: Updated export to match the new class name.The export has been correctly updated to reflect the new class name.
suite-native/app/e2e/utils.ts (3)
18-18: Improved constant naming and visibility.Renaming
TREZOR_DEVICE_LABELtoTREZOR_E2E_DEVICE_LABELclarifies that this label is specifically for E2E testing. Making it exported also increases its reusability across the codebase.
81-95: Enhanced flexibility in the scrollUntilVisible function.The function has been improved in several ways:
- Parameter changed from
Detox.NativeMatchertoDetox.IndexableNativeElementfor more direct usage- Added optional
scrollViewTestIdparameter with a sensible default value- Updated implementation to use the element parameter directly
- More consistent with how elements are handled throughout the codebase
These changes make the function more flexible and easier to use in different contexts.
114-114: Updated constant usage for consistency.The code now uses the renamed
TREZOR_E2E_DEVICE_LABELconstant, maintaining consistency with the earlier change.suite-native/app/e2e/tests/appSettingsWithDevice.test.ts (3)
1-19: Well-structured imports and organized dependenciesThe imports are well-organized, correctly separated by source (detox, common utilities, page objects, and local utilities). This makes the file structure clear and maintainable.
20-23: Appropriate platform-specific test isolationUsing
conditionalDescribeto run these tests only on Android is a good approach since some test actions (likedevice.pressBack()) are platform-specific.
24-35: Comprehensive test setup with appropriate validationThe setup logic appropriately initializes the environment, skips onboarding, and verifies initial state. The baseline check that Ethereum is not enabled initially (line 34) is particularly good as it establishes a clear starting point for the tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
suite-native/app/e2e/tests/appSettings.test.ts (3)
34-50: Consider extracting the timeout value to a constantThe hardcoded timeout value of 10000ms is used in multiple places throughout the tests. Extracting this to a named constant would improve maintainability and make it easier to adjust all timeouts at once if needed.
+const DEFAULT_ELEMENT_TIMEOUT = 10000; it('Localization - Currency', async () => { await waitFor( element(by.id('@home/portfolio/fiat-balance-header').withDescendant(by.text('$'))), ) .toBeVisible() - .withTimeout(10000); + .withTimeout(DEFAULT_ELEMENT_TIMEOUT); await onTabBar.navigateToSettings(); await onSettings.tapLocalization(); await onSettings.changeLocalizationCurrency('czk'); await device.pressBack(); await device.pressBack(); await waitFor( element(by.id('@home/portfolio/fiat-balance-header').withDescendant(by.text('CZK'))), ) .toBeVisible() - .withTimeout(10000); + .withTimeout(DEFAULT_ELEMENT_TIMEOUT); });
42-42: Consider using constants for currency codesUsing string literals like 'czk' directly in the code makes it harder to maintain. Consider creating an enum or constant object for currency codes to improve type safety and readability.
+// At the top of the file or in a separate constants file +enum Currency { + CZK = 'czk', + USD = 'usd', + // Add other currencies as needed +} // Then in the test: -await onSettings.changeLocalizationCurrency('czk'); +await onSettings.changeLocalizationCurrency(Currency.CZK);
69-83: Add a comment explaining the discreet mode toggle behaviorThe test toggles discreet mode on by tapping the balance header, but toggles it off through settings. It would be helpful to add a comment explaining if this asymmetric approach is intentional and why both methods need to be tested.
it('Privacy & Security - Discreet Mode', async () => { + // Verify discreet mode is initially disabled await onHome.assertIsDiscreetModeDisabled(); + // Toggle discreet mode ON by tapping the balance header (shortcut method) await element(by.id('@home/portfolio/fiat-balance-header')).tap(); await onHome.assertIsDiscreetModeEnabled(); + // Toggle discreet mode OFF through settings (explicit method) await onTabBar.navigateToSettings(); await onSettings.tapPrivacyAndSecurity(); await onSettings.toggleDiscreetMode(); await device.pressBack(); await device.pressBack(); await onHome.assertIsDiscreetModeDisabled(); });suite-native/app/e2e/tests/appSettingsWithDevice.test.ts (3)
40-40: Extract hardcoded wait time to a named constantThe hardcoded wait of 5000ms would be better as a named constant that explains why this specific delay is needed.
+const TREZOR_DEVICE_CONNECTION_DELAY = 5000; // Time needed for Trezor device to start communicating with the app beforeEach(async () => { await restartApp(); await appIsFullyLoaded(); - await wait(5000); // wait for trezor device to start communicating with the app + await wait(TREZOR_DEVICE_CONNECTION_DELAY); // wait for trezor device to start communicating with the app });
69-121: Add explanatory comments to the View Only Mode test stepsThe View Only Mode test has multiple steps and state transitions that would benefit from explanatory comments to clarify the expected behavior and the purpose of each step.
it('View Only Mode', async () => { + // Verify initial state: device should be connected await waitFor( element( by.id('@device-manager/device-switch').withDescendant(by.text('Connected')), ), ) .toBeVisible() .withTimeout(10000); + // Navigate to settings and enable View Only Mode await onTabBar.navigateToSettings(); await onSettings.tapViewOnly(); await onSettings.toggleWalletViewOnly(); + // Simulate device disconnection by stopping the bridge await TrezorUserEnvLink.stopBridge(); + // Verify device shows as disconnected when bridge is stopped await waitFor( element( by.id('@device-manager/device-switch').withDescendant(by.text('Disconnected')), ), ) .toBeVisible() .withTimeout(10000); + // Restart bridge and app to verify reconnection await TrezorUserEnvLink.startBridge(); await restartApp(); + // Verify device reconnected successfully await waitFor( element( by.id('@device-manager/device-switch').withDescendant(by.text('Connected')), ), ) .toBeVisible() .withTimeout(10000); + // Disable View Only Mode await onTabBar.navigateToSettings(); await onSettings.tapViewOnly(); await onSettings.toggleWalletViewOnly(); await device.pressBack(); await device.pressBack(); + // Simulate device disconnection again await TrezorUserEnvLink.stopBridge(); + // Verify welcome message is shown instead of connection status + // when in standard mode with bridge stopped await waitFor( element( by.id('@device-manager/device-switch').withDescendant(by.text('Hi there!')), ), ) .toBeVisible() .withTimeout(10000); + // Verify Ethereum is not visible when in standard mode without device connection await detoxExpect(element(by.text('Ethereum'))).not.toExist(); });
70-101: Extract timeout values to constantsSimilar to the other test file, the timeout value of 10000ms is used multiple times. Consider extracting this to a constant for better maintainability.
+const DEFAULT_ELEMENT_TIMEOUT = 10000; // Then use it in all waitFor calls .withTimeout(DEFAULT_ELEMENT_TIMEOUT);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
suite-native/app/e2e/pageObjects/coinEnablingActions.ts(1 hunks)suite-native/app/e2e/pageObjects/settingsActions.ts(1 hunks)suite-native/app/e2e/pageObjects/tabBarActions.ts(1 hunks)suite-native/app/e2e/tests/appSettings.test.ts(1 hunks)suite-native/app/e2e/tests/appSettingsWithDevice.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- suite-native/app/e2e/pageObjects/tabBarActions.ts
- suite-native/app/e2e/pageObjects/settingsActions.ts
- suite-native/app/e2e/pageObjects/coinEnablingActions.ts
🧰 Additional context used
🧬 Code Definitions (2)
suite-native/app/e2e/tests/appSettings.test.ts (5)
suite-native/app/e2e/utils.ts (3) (3)
openApp(61:70)restartApp(72:79)appIsFullyLoaded(97:101)suite-native/app/e2e/pageObjects/homeActions.ts (1) (1)
onHome(30:30)suite-native/app/e2e/pageObjects/accountImportActions.ts (1) (1)
onAccountImport(69:69)suite-native/app/e2e/pageObjects/tabBarActions.ts (1) (1)
onTabBar(21:21)suite-native/app/e2e/pageObjects/settingsActions.ts (1) (1)
onSettings(86:86)
suite-native/app/e2e/tests/appSettingsWithDevice.test.ts (4)
suite-native/app/e2e/utils.ts (6) (6)
prepareTrezorEmulator(103:120)openApp(61:70)restartApp(72:79)appIsFullyLoaded(97:101)wait(129:131)disconnectTrezorUserEnv(122:127)suite-native/app/e2e/pageObjects/coinEnablingActions.ts (1) (1)
onCoinEnabling(21:21)suite-native/app/e2e/pageObjects/tabBarActions.ts (1) (1)
onTabBar(21:21)suite-native/app/e2e/pageObjects/settingsActions.ts (1) (1)
onSettings(86:86)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
🔇 Additional comments (2)
suite-native/app/e2e/tests/appSettingsWithDevice.test.ts (2)
56-66: Reuse element selectors consistentlyGood job defining
ethereumTextElementas a variable for reuse. This follows the recommendation from previous reviews to use constants for elements used multiple times.
120-120:❓ Verification inconclusive
Explain why Ethereum should not be visible
Line 120 checks that Ethereum is not visible, but it's not clear why this is expected. Add a comment explaining this expectation.
Can you verify if Ethereum should be hidden when in standard mode without device connection? Is this behavior documented somewhere in your application requirements?
🏁 Script executed:
#!/bin/bash # Search for any code that might control the visibility of Ethereum based on connection status rg -A 10 -B 10 "Ethereum.*visibility|visible.*Ethereum|hide.*Ethereum|Ethereum.*hide" --type tsLength of output: 97
Action Required: Clarify "Ethereum" Hidden Expectation
The test at line 120 verifies that the "Ethereum" element is not visible. However, our search did not uncover any explicit documentation or code references that explain why Ethereum must be hidden when running in standard mode without a device connection. Please add an inline comment at line 120 (or nearby) in
suite-native/app/e2e/tests/appSettingsWithDevice.test.tsthat clarifies this expectation—whether it’s based on design requirements or a feature flag controlling UI behavior. Additionally, confirm manually that hiding Ethereum in standard mode without an active device connection is indeed the intended behavior based on the product requirements, and reference the relevant documentation if available.
| await disconnectTrezorUserEnv(); | ||
| }); | ||
|
|
||
| it('Coin Enabling', async () => { |
There was a problem hiding this comment.
I prefer test to it but not a big deal.
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13943618776 |
trezor-ci
force-pushed
the
test/native/settings-e2e
branch
from
e8ec5c1 to
53aaa5d
Compare
|
🚀 Expo preview is ready!
|
Description
E2E tests added for following app settings feature:
Screenshots:
Screen.Recording.2025-03-10.at.7.50.07.AM720.mov