feat(e2e): Migrated more wallet tests #17232
Conversation
WalkthroughThe pull request primarily introduces changes to the wallet testing setup and related functionalities. It re-enables a wallet test group in the continuous integration workflow by updating the configuration to include specific container environments. Additionally, new utility and action methods have been added to support CSV to JSON conversion and transaction exports within the wallet module. The end-to-end test suites have been expanded in the desktop project to cover scenarios such as account type additions, transaction exports in multiple formats, CSV imports for Bitcoin, and Bitcoin send actions in a regtest environment. Concurrently, similar tests in the web project have been removed. The commit also updates a submodule reference to point to a newer commit of a common library, ensuring that an updated version is used across the project. Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (8)
✨ Finishing Touches
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 (
|
![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 (12)
packages/suite-desktop-core/e2e/support/csvToJson.ts (1)
1-16: Add error handling and support for more complex CSV formats.The current implementation makes several assumptions about the CSV format that might not hold true in all cases. Consider enhancing this function with:
- Input validation to ensure the CSV is properly formatted
- Handling for cases where row length doesn't match header length
- Support for quoted values that may contain commas
- Handling for empty lines, especially at the end of the file
export const csvToJson = (data: string) => { + if (!data || typeof data !== 'string') { + return []; + } + const lines = data.split('\n'); + // Filter out empty lines + const nonEmptyLines = lines.filter(line => line.trim().length > 0); + + if (nonEmptyLines.length < 2) { + return []; + } + const result = []; - const headers = lines[0].split(','); + const headers = nonEmptyLines[0].split(',').map(header => header.trim()); - for (let i = 1; i < lines.length; i++) { + for (let i = 1; i < nonEmptyLines.length; i++) { const obj: Record<string, string> = {}; - const currentline = lines[i].split(','); + const currentline = nonEmptyLines[i].split(',').map(value => value.trim()); - for (let j = 0; j < headers.length; j++) { + for (let j = 0; j < Math.min(headers.length, currentline.length); j++) { obj[headers[j]] = currentline[j]; } result.push(obj); } return result; };packages/suite-desktop-core/e2e/support/pageActions/walletActions.ts (1)
139-143: Add type safety and documentation for exportTransactions method.The method currently accepts any string without validation. Consider:
- Creating an enum or union type for valid export formats (CSV, PDF, JSON)
- Adding JSDoc to document the purpose and allowed values
+/** + * Export types supported by the wallet + */ +export type ExportType = 'csv' | 'pdf' | 'json'; @step() -async exportTransactions(typeOfExport: string) { +/** + * Exports transactions in the specified format + * @param typeOfExport The format to export (csv, pdf, json) + */ +async exportTransactions(typeOfExport: ExportType) { await this.page.getByTestId('@wallet/accounts/export-transactions/dropdown').click(); await this.page.getByTestId(`@wallet/accounts/export-transactions/${typeOfExport}`).click(); }packages/suite-desktop-core/e2e/tests/wallet/import-btc-csv.test.ts (2)
31-37: Consider adding error handling for file operations.The test assumes that the CSV file exists and is accessible. Consider adding error handling for the file operations to make the test more robust.
- const csvFilePath = path.join(__dirname, '../../fixtures/btcTest.csv'); - await dashboardPage.modal.locator('input[type=file]').setInputFiles(csvFilePath); - await dashboardPage.modal.waitFor({ state: 'detached' }); - - const csvData = fs.readFileSync(csvFilePath, 'utf8'); + const csvFilePath = path.join(__dirname, '../../fixtures/btcTest.csv'); + try { + await dashboardPage.modal.locator('input[type=file]').setInputFiles(csvFilePath); + await dashboardPage.modal.waitFor({ state: 'detached' }); + + const csvData = fs.readFileSync(csvFilePath, 'utf8'); + const convertedData = csvToJson(csvData); + // Continue with assertions... + } catch (error) { + throw new Error(`Error during CSV file operations: ${error.message}`); + }
37-41: Consider verifying all CSV entries, not just the first two.The test only verifies the first two addresses from the CSV file. If the file contains more entries, consider either:
- Verifying all entries in a loop
- Adding a comment explaining why only the first two are verified
This would make the test more comprehensive and clear about its intent.
packages/suite-desktop-core/e2e/tests/wallet/export-transactions.test.ts (3)
34-38: Inconsistent BTC filter logic.The code filters out BTC when enabling networks but includes it in the testing loop. This might be intentional if BTC is enabled by default, but a clarifying comment would improve readability.
const symbols: NetworkSymbol[] = ['btc', 'ltc', 'eth', 'ada']; await settingsPage.navigateTo('coins'); + // BTC is enabled by default, so we only need to enable other networks for (const symbol of symbols.filter(s => s !== 'btc')) { await settingsPage.coins.enableNetwork(symbol); }
46-58: Extract export types to a constant and improve file verification.The export types are defined inline and the file verification only checks that the size is greater than 0. Consider:
- Moving the export types to a constant at the top of the file
- Adding more specific content verification for each export type
This would improve maintainability and test robustness.
+ const EXPORT_TYPES = ['pdf', 'csv', 'json']; + test.describe('Export transactions', { tag: ['@group=wallet', '@webOnly'] }, () => { // ... test('Go to account and try to export all possible variants (pdf, csv, json)', async ({ // ... }) => { // ... for (const symbol of symbols) { await walletPage.accountButton({ symbol }).click(); - const typesOfExport = ['pdf', 'csv', 'json']; - for (const type of typesOfExport) { + for (const type of EXPORT_TYPES) { // ...
55-57: Add cleanup for downloaded files.The test downloads files but doesn't clean them up afterward. Consider adding cleanup logic to avoid accumulating files during multiple test runs.
const downloadPath = await download.path(); const stats = fs.statSync(downloadPath); expect(stats.size).toBeGreaterThan(0); + // Clean up downloaded file + fs.unlinkSync(downloadPath);packages/suite-desktop-core/e2e/tests/wallet/add-account-types.test.ts (2)
68-73: Reduce code duplication in account counting logic.There's repetition in how accounts are counted before and after addition. Consider extracting this into a helper function.
+ async function getAccountCount(page, selector) { + return await page + .getByTestId(selector) + .locator(':scope > *:not([data-testid="@account-menu/account-item-skeleton"])') + .count(); + } + // Then use it like: - const numberOfAccountsBefore = await page - .getByTestId(`@account-menu/${type}/group`) - .locator(':scope > *:not([data-testid="@account-menu/account-item-skeleton"])') - .count(); + const numberOfAccountsBefore = await getAccountCount(page, `@account-menu/${type}/group`);Also applies to: 83-88, 133-137, 145-149
114-117: Document the purpose of hardcoded derivation paths.The test uses specific derivation paths for ADA and ETH. It would be helpful to include a comment explaining why these specific paths are used and what they represent.
const coins = [ + // Using standard derivation paths for ADA and ETH to verify account creation { symbol: 'ada', path: `m/1852'/1815'/1'` }, { symbol: 'eth', path: `m/44'/60'/0'/0/1` }, ];packages/suite-desktop-core/e2e/tests/wallet/send-form-regtest.test.ts (3)
3-5: Document the purpose of hardcoded address constant.The
ADDRESS_INDEX_1constant is used throughout the tests. Include a comment explaining what this address represents and why it's used.test.describe('Send form for bitcoin', { tag: ['@group=wallet'] }, () => { + // Test address used for sending and receiving BTC in regtest environment const ADDRESS_INDEX_1 = 'bcrt1qkvwu9g3k2pdxewfqr7syz89r3gj557l374sg5v';
89-90: Use more descriptive OP_RETURN data.The test uses
meowas the OP_RETURN data, which is not descriptive of the test's intent. Consider using a more meaningful string.- await page.getByTestId('outputs.1.dataAscii').fill('meow'); + await page.getByTestId('outputs.1.dataAscii').fill('test_opreturn_data');Similarly, update the assertion on line 101:
- await expect(page.getByTestId('@wallet/accounts/transaction-list/pending/group/0')).toContainText('OP_RETURN (meow)'); + await expect(page.getByTestId('@wallet/accounts/transaction-list/pending/group/0')).toContainText('OP_RETURN (test_opreturn_data)');
98-102: Add verification for transaction confirmation.The test verifies that the transaction appears in the pending list but doesn't verify that it gets confirmed. Consider mining a block and checking that the transaction moves from pending to confirmed status.
await devicePrompt.sendButton.click(); await expect( page.getByTestId('@wallet/accounts/transaction-list/pending/group/0'), ).toContainText('OP_RETURN (meow)'); + + // Mine a block and verify transaction confirmation + await trezorUserEnvLink.mineBlocks({ block_amount: 1 }); + await expect( + page.getByTestId('@wallet/accounts/transaction-list/pending/group/0'), + ).not.toBeVisible({ timeout: 10000 }); + await expect( + page.getByTestId('@wallet/accounts/transaction-list/confirmed/group/0'), + ).toBeVisible();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/suite-desktop-core/e2e/fixtures/btcTest.csvis excluded by!**/*.csv
📒 Files selected for processing (12)
.github/workflows/test-suite-web-e2e-pw.yml(1 hunks)packages/suite-desktop-core/e2e/support/csvToJson.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/walletActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/metadata/output-labeling.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/wallet/add-account-types.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/export-transactions.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/import-btc-csv.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/send-form-regtest.test.ts(1 hunks)packages/suite-web/e2e/tests/wallet/add-account-types.test.ts(0 hunks)packages/suite-web/e2e/tests/wallet/export-transactions.test.ts(0 hunks)packages/suite-web/e2e/tests/wallet/send-form-regtest.test.ts(0 hunks)submodules/trezor-common(1 hunks)
💤 Files with no reviewable changes (3)
- packages/suite-web/e2e/tests/wallet/add-account-types.test.ts
- packages/suite-web/e2e/tests/wallet/send-form-regtest.test.ts
- packages/suite-web/e2e/tests/wallet/export-transactions.test.ts
✅ Files skipped from review due to trivial changes (1)
- submodules/trezor-common
⏰ 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: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (9)
.github/workflows/test-suite-web-e2e-pw.yml (1)
119-120: LGTM! Re-enabling wallet tests with appropriate containers.The re-enabling of the wallet test group with the required containers looks good. This change allows for running wallet-related tests as part of the web e2e test suite.
packages/suite-desktop-core/e2e/tests/metadata/output-labeling.test.ts (2)
13-19: LGTM! Adding walletPage parameter to utilize the new exportTransactions method.Good refactoring to utilize the new method from the WalletActions class.
82-82: LGTM! Using abstracted method instead of direct element interaction.This change improves maintainability by leveraging the new exportTransactions method.
packages/suite-desktop-core/e2e/tests/wallet/import-btc-csv.test.ts (2)
1-6: Good import organization and separation of concerns.The imports are well organized with clear separation between standard Node.js modules and application-specific imports. The use of the custom
csvToJsonutility function is appropriate for this test case.
9-14: Test setup looks good with proper initialization sequence.The test setup follows best practices by:
- Starting the metadata provider mock
- Completing onboarding
- Ensuring discovery finishes before each test
This provides a consistent starting state for all tests in the suite.
packages/suite-desktop-core/e2e/tests/wallet/export-transactions.test.ts (1)
7-17: Good test setup with clear configuration.The test uses a specific mnemonic for deterministic testing and properly sets up the initial state with onboarding completion and discovery finishing. This provides a reliable test environment.
packages/suite-desktop-core/e2e/tests/wallet/add-account-types.test.ts (1)
152-158: Good use of analytics verification.The test properly verifies that the expected analytics events are fired with the correct parameters. This ensures that the application correctly tracks user actions.
packages/suite-desktop-core/e2e/tests/wallet/send-form-regtest.test.ts (2)
32-58: Good test structure with clear actions and verifications.The test has a clear structure with descriptive comments, appropriate inputs, and verifications of UI elements and application state.
60-75: Complete the transaction sending process in the second test.Unlike the first and third tests, the second test doesn't complete the transaction sending process. Either add the missing steps to complete the transaction or add a comment explaining why the transaction is not sent.
Is it intentional that the second test doesn't complete the transaction sending process? If not, consider adding the following steps:
await marketPage.sendAddressInput.fill(ADDRESS_INDEX_1); + + // Send the transaction + await marketPage.sendButton.click(); + await trezorUserEnvLink.pressYes(); + await trezorUserEnvLink.pressYes(); + await trezorUserEnvLink.pressYes(); + + await expect(page.getByTestId('@send/copy-raw-transaction')).toBeVisible();
| await page.waitForTimeout(500); | ||
| await page.getByTestId(`@add-account-type/select/option/${type}`).click(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid arbitrary timeouts in tests.
Using a hardcoded timeout is not recommended as it can lead to flaky tests. Wait for a specific UI event or element instead.
- await page.getByTestId('@add-account-type/select/input').click();
- await page.waitForTimeout(500);
- await page.getByTestId(`@add-account-type/select/option/${type}`).click();
+ await page.getByTestId('@add-account-type/select/input').click();
+ await page.getByTestId(`@add-account-type/select/option/${type}`).waitFor({ state: 'visible' });
+ await page.getByTestId(`@add-account-type/select/option/${type}`).click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page.waitForTimeout(500); | |
| await page.getByTestId(`@add-account-type/select/option/${type}`).click(); | |
| await page.getByTestId('@add-account-type/select/input').click(); | |
| await page.getByTestId(`@add-account-type/select/option/${type}`).waitFor({ state: 'visible' }); | |
| await page.getByTestId(`@add-account-type/select/option/${type}`).click(); |
There was a problem hiding this comment.
What is happening without it?
There was a problem hiding this comment.
@Vere-Grey Without the timeout, the test likely tries to click on the dropdown option (@add-account-type/select/option/${type}) before the dropdown menu has fully opened or rendered. This would cause the test to fail intermittently because the element might not be clickable yet.
The suggested approach (waitFor({ state: 'visible' })) is more reliable because it explicitly waits for the dropdown option to become visible before attempting to click it, rather than using an arbitrary timeout which might be too short or unnecessarily long depending on system performance.
There was a problem hiding this comment.
But the click method does this implicitly. This doesn't help unfortunately.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
HajekOndrej
force-pushed
the
migrate-wallet-2
branch
from
9de320c to
43b1428
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/suite-desktop-core/e2e/tests/wallet/add-account-types.test.ts (1)
79-80: 🛠️ Refactor suggestionAvoid arbitrary timeouts in tests.
Using a hardcoded timeout is not recommended as it can lead to flaky tests. Wait for a specific UI event or element instead.
- await page.getByTestId('@add-account-type/select/input').click(); - await page.waitForTimeout(500); - await page.getByTestId(`@add-account-type/select/option/${type}`).click(); + await page.getByTestId('@add-account-type/select/input').click(); + await page.getByTestId(`@add-account-type/select/option/${type}`).waitFor({ state: 'visible' }); + await page.getByTestId(`@add-account-type/select/option/${type}`).click();
🧹 Nitpick comments (2)
packages/suite-desktop-core/e2e/tests/wallet/add-account-types.test.ts (2)
59-62: Consider more explicit waiting for chevrons.When expanding all account chevrons, it might be more robust to ensure each expansion is complete before clicking the next one.
const chevrons = await walletPage.accountChevron.all(); for (const chevron of chevrons) { await chevron.click(); + // Consider waiting for expansion to complete + await page.waitForTimeout(100); // Or better, wait for a specific element to appear }
27-94: Consider refactoring common test logic into helper functions.Both test cases contain similar patterns for adding accounts and verifying counts. Consider extracting this logic into helper functions to improve maintainability and reduce duplication.
You could create a helper function like:
async function addAndVerifyAccount( page: Page, coin: string, type: string = 'normal', analytics?: any ) { // Get count before const numberOfAccountsBefore = await getAccountCount(page, coin, type); // Add account await page.getByTestId('@account-menu/add-account').click(); await expect(page.getByTestId('@modal')).toBeVisible(); await page.getByTestId(`@settings/wallet/network/${coin}`).click(); // Select type if not normal if (type !== 'normal') { await page.getByTestId('@add-account-type/select/input').click(); await page.getByTestId(`@add-account-type/select/option/${type}`).waitFor({ state: 'visible' }); await page.getByTestId(`@add-account-type/select/option/${type}`).click(); } await page.getByTestId('@add-account').click(); // Get count after and verify const numberOfAccountsAfter = await getAccountCount(page, coin, type); expect(numberOfAccountsAfter).toEqual(numberOfAccountsBefore + 1); // Verify analytics if provided if (analytics) { // Analytics verification logic } }Also applies to: 107-161
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/suite-desktop-core/e2e/fixtures/btcTest.csvis excluded by!**/*.csv
📒 Files selected for processing (13)
.github/workflows/test-suite-web-e2e-pw.yml(1 hunks)packages/suite-desktop-core/e2e/support/csvToJson.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/walletActions.ts(1 hunks)packages/suite-desktop-core/e2e/tests/metadata/output-labeling.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/wallet/add-account-types.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/export-transactions.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/import-btc-csv.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/send-form-regtest.test.ts(1 hunks)packages/suite-web/e2e/tests/wallet/add-account-types.test.ts(0 hunks)packages/suite-web/e2e/tests/wallet/export-transactions.test.ts(0 hunks)packages/suite-web/e2e/tests/wallet/import-btc-csv.test.ts(0 hunks)packages/suite-web/e2e/tests/wallet/send-form-regtest.test.ts(0 hunks)submodules/trezor-common(1 hunks)
💤 Files with no reviewable changes (4)
- packages/suite-web/e2e/tests/wallet/import-btc-csv.test.ts
- packages/suite-web/e2e/tests/wallet/export-transactions.test.ts
- packages/suite-web/e2e/tests/wallet/add-account-types.test.ts
- packages/suite-web/e2e/tests/wallet/send-form-regtest.test.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- submodules/trezor-common
- packages/suite-desktop-core/e2e/support/pageActions/walletActions.ts
- .github/workflows/test-suite-web-e2e-pw.yml
- packages/suite-desktop-core/e2e/tests/metadata/output-labeling.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/import-btc-csv.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/export-transactions.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/send-form-regtest.test.ts
- packages/suite-desktop-core/e2e/support/csvToJson.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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/wallet/add-account-types.test.ts (4)
1-16: Good test setup with comprehensive preparation.The setup looks well-structured with appropriate imports, emulator configuration, and proper onboarding completion before each test. The use of a predefined mnemonic ensures consistent testing environment.
18-26: Well-documented test case with clear steps.The test case documentation clearly outlines the test steps and verification strategy, which improves readability and maintainability.
33-47: Good data-driven test approach.Using structured data to define test scenarios makes the test more maintainable and allows for easy extension to include additional currencies or account types in the future.
107-161: Great integration of analytics validation.The test properly verifies analytics events when adding accounts, which ensures both UI functionality and analytics tracking work correctly. The test also appropriately resets analytics requests between different coin tests.
packages/suite-desktop-core/e2e/support/pageActions/walletActions.ts
Outdated
Show resolved
Hide resolved
| .locator( | ||
| ':scope > *:not([data-testid="@account-menu/account-item-skeleton"])', | ||
| ) |
There was a problem hiding this comment.
This one took me a while to understand.
There was a problem hiding this comment.
We have a method in walletPage for something similiar with little bit nicer locator
getAccountsCount(symbol: NetworkSymbol) {
return await this.page
.locator(`[data-testid*="@account-menu/${symbol}"][tabindex]`)
.count();
}
```
There was a problem hiding this comment.
There are different coins so it can't be used, unless I iterate through them and then make a sum...
packages/suite-desktop-core/e2e/tests/wallet/add-account-types.test.ts
Outdated
Show resolved
Hide resolved
packages/suite-desktop-core/e2e/tests/wallet/export-transactions.test.ts
Show resolved
Hide resolved
HajekOndrej
force-pushed
the
migrate-wallet-2
branch
from
43b1428 to
c5a06c8
Compare
Description
Migrated more wallet tests
Related Issue
Resolve
Screenshots: