feat(e2e): Convert db-migration test to pw #17240
Conversation
updates workflows one locator renaming
Vere-Grey
force-pushed
the
feat/suite-desktop-core/e2e-convert-migration
branch
from
4c55231 to
f401e08
Compare
WalkthroughThis update introduces modifications across workflow configurations and end-to-end test suites. In the GitHub Actions workflows, a new job named 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/suite-desktop-core/e2e/tests/suite/db-migration.test.ts (3)
20-30: Consider using a more reliable waiting mechanism.The test uses discovery bar visibility to determine when the application is ready, which is a good approach. However, the timeouts (45000ms) are quite long and hardcoded.
Consider extracting timeout values to named constants for better maintainability:
+const DISCOVERY_TIMEOUT = 45000; // ... -await discoveryBar.waitFor({ state: 'visible', timeout: 45000 }); -await discoveryBar.waitFor({ state: 'hidden', timeout: 45000 }); +await discoveryBar.waitFor({ state: 'visible', timeout: DISCOVERY_TIMEOUT }); +await discoveryBar.waitFor({ state: 'hidden', timeout: DISCOVERY_TIMEOUT });
55-60: Consider using waitForSelector instead of arbitrary timeout.There's an arbitrary timeout on line 58 with a comment explaining it's needed for a state save. This is a potential flakiness point.
Consider using a more deterministic approach:
await page.locator('[data-test="outputs[0].address"]').fill(btcAddress); -await page.waitForTimeout(500); // wait has to be for a state save to happen +// Wait for some indicator that the state has been saved, such as: +// await page.waitForSelector('[data-test="state-saved-indicator"]', { state: 'visible' }); +// If no explicit indicator exists, consider adding one to make tests more reliable await page.locator('[data-test="@wallet/menu/close-button"]').last().click();
82-92: Inconsistent selector patterns used throughout the test.This section mixes older selector patterns (
page.locator) with newer Playwright patterns (page.getByTestId). While this might be intentional if different versions of the app use different patterns, it makes the test harder to maintain.Consider standardizing on
page.getByTestIdwhere possible for better readability and maintainability:-await page.locator('[data-test="@menu/switch-device"]').click(); +await page.getByTestId('@menu/switch-device').click();
🛑 Comments failed to post (1)
packages/suite-desktop-core/e2e/tests/suite/db-migration.test.ts (1)
114-114: 💡 Verification agent
🧩 Analysis chain
Verify the need for skipSuiteLoadedCheck parameter.
The
disableFirmwareHashCheckmethod is called with{ skipSuiteLoadedCheck: true }. It would be helpful to add a comment explaining why this parameter is needed in this specific context.
🏁 Script executed:
#!/bin/bash # Let's check how disableFirmwareHashCheck is implemented to understand the skipSuiteLoadedCheck parameter fd -t f -e ts disableFirmwareHashCheck | xargs grep -l "disableFirmwareHashCheck" | xargs grep -A 10 -B 5 "disableFirmwareHashCheck"Length of output: 130
I'll run an additional search to locate where and how the
skipSuiteLoadedCheckparameter is used in the codebase. This should help us understand its role and whether a comment is needed for clarification.Once we see the results, we can determine if it's necessary to add an inline comment in
db-migration.test.tsexplaining the need for that parameter.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for all occurrences of the `skipSuiteLoadedCheck` parameter within TypeScript files to gather more context rg "skipSuiteLoadedCheck" -t tsLength of output: 606
Action: Clarify rationale for using skipSuiteLoadedCheck
It appears that the
skipSuiteLoadedCheckparameter in thedisableFirmwareHashCheckmethod (defined inpackages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts) controls whether the usual suite load verification is performed. In the DB migration test (packages/suite-desktop-core/e2e/tests/suite/db-migration.test.ts), passing{ skipSuiteLoadedCheck: true }bypasses this check—presumably because, in this context, the complete suite load is either already ensured or irrelevant to the migration scenario.Please add an inline comment above or alongside this call that briefly explains:
- Why the suite-loaded check is being bypassed in this test.
- Any assumptions (e.g., pre-configured state or non-essential nature of the check during migration) that justify disabling the check.
This additional context will help future maintainers understand the intent behind setting this parameter to
true.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-core/e2e/tests/suite/db-migration.test.ts (1)
55-61: Send form interaction could be improved.The step fills in the send form but includes a hardcoded timeout that might be fragile in certain environments.
Consider replacing the hardcoded timeout with a more reliable approach, such as waiting for a specific element state change that indicates the save has completed:
- await page.waitForTimeout(500); // wait has to be for a state save to happen + // Wait for a visual indication that the form has been saved, like a success message or button state change + await page.waitForSelector('[data-test="@form-saved-indicator"]', { state: 'visible', timeout: 3000 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/test-suite-web-e2e-pw.yml(2 hunks).github/workflows/test-suite-web-nightly.yml(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts(1 hunks)packages/suite-desktop-core/e2e/support/pageActions/walletActions.ts(2 hunks)packages/suite-desktop-core/e2e/tests/metadata/wallet-metadata.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/suite/db-migration.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/cardano.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/pending-transactions.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts(1 hunks)packages/suite-web/e2e/tests/suite/database-migration.test.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/suite-web/e2e/tests/suite/database-migration.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/suite-desktop-core/e2e/tests/wallet/send-doge.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/cardano.test.ts
- packages/suite-desktop-core/e2e/tests/wallet/pending-transactions.test.ts
- packages/suite-desktop-core/e2e/support/pageActions/walletActions.ts
- packages/suite-desktop-core/e2e/support/pageActions/onboarding/onboardingActions.ts
- .github/workflows/test-suite-web-e2e-pw.yml
- packages/suite-desktop-core/e2e/tests/metadata/wallet-metadata.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Releases revision Checks
- GitHub Check: Other Checks
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- GitHub Check: Build libs for publishing
- GitHub Check: Unit Tests
- 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: build-web
- 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: build-web
🔇 Additional comments (14)
.github/workflows/test-suite-web-nightly.yml (1)
1-1: Updated workflow name to reflect removal of migration tests.The workflow name has been updated to remove "migrations" from the title, which correctly aligns with the PR's objective of converting Cypress tests to Playwright. The migration tests have been removed from this workflow as they're now handled by the Playwright implementation instead.
packages/suite-desktop-core/e2e/tests/suite/db-migration.test.ts (13)
3-9: Good setup of migration test constants.The test setup clearly defines the versions for migration testing, wallet credentials, and necessary selectors. Having these defined as constants at the top improves maintainability.
10-12: Appropriate test configuration with meaningful tags.The test is properly tagged with '@group=migrations' and '@webonly', which allows it to be selectively run in the correct environments. The emulator setup with passphrase protection is also correctly configured.
13-21: Comprehensive test setup with necessary page objects.The test correctly imports all required page objects and establishes a locator for the discovery progress bar that will be used multiple times throughout the test.
22-31: Well-structured test step for loading the old version.This step effectively navigates to the older version of the application, completes the onboarding flow, and waits for the discovery process to complete. Good use of waitFor with appropriate timeouts.
32-40: Theme change verification is well implemented.The step changes the theme to dark and verifies the change was applied, which will be important to check during migration.
41-54: Thorough passphrase wallet setup process.The test properly sets up a wallet with passphrase protection, interacts with the device emulator, and waits for the discovery process to complete before proceeding.
62-71: Effective transaction address extraction.The test correctly captures the original transaction address for later comparison after migration.
82-93: Good migration verification steps.The test effectively navigates to the new version and verifies that the wallet state has been preserved, checking both the device status and wallet name.
94-101: Robust transaction address comparison.The test properly verifies that transaction data has been preserved during migration by comparing the address before and after.
104-110: Effective passphrase caching verification.This step verifies that the passphrase is correctly cached after migration by attempting to reveal an address and checking for the disconnected device warning instead of a passphrase prompt.
121-127: Improved button selector naming.The test now uses
openSendFormButtoninstead ofsendButton, which aligns with the PR's objective of renaming locators to be more descriptive.
129-132: Effective theme persistence verification.The test properly verifies that the dark theme setting persists across the migration by checking the CSS property.
1-134: Comprehensive migration test that covers all essential scenarios.This test effectively converts a Cypress migration test to Playwright while maintaining comprehensive coverage of important migration scenarios. It verifies that user preferences (theme), wallet state (addresses, transactions), and form data all correctly persist during migration between versions. The test is well-structured using test steps for clarity and maintainability.
A few additional points worth noting:
- The test correctly handles both the disconnected and connected states of the emulator
- Good use of assertions to verify each step completes as expected
- The test properly handles passphrase protection scenarios
This implementation fulfills the PR objective of converting one of the last Cypress tests to Playwright.
Vere-Grey
force-pushed
the
feat/suite-desktop-core/e2e-convert-migration
branch
4 times, most recently
from
f6ae488 to
0f68719
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test-suite-web-e2e-pw.yml (1)
96-117: Dynamic Matrix Generation ingenerate-e2e-matrixJobThe new job correctly builds a JSON array as a matrix for end-to-end test groups and conditionally appends the migrations group for scheduled events. Using
jqto enforce valid JSON is a practical approach. Please ensure that the constructed JSON output exactly meets the format expected by the downstreamfromJsoncall in thee2e-test-suite-webjob.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test-suite-web-e2e-pw.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (2)
.github/workflows/test-suite-web-e2e-pw.yml (2)
168-168: Simplified Docker Image Pull CommandThe updated command
docker compose pull trezor-user-env-unixdirectly references the container, which enhances clarity and aligns well with the PR objective of simplifying docker commands.
181-181: Direct Container Startup Command UpdateThe change to
docker compose up -d trezor-user-env-unixoffers a straightforward and maintainable approach to start the required service. This simplification is consistent with the improvements made throughout this workflow.
Vere-Grey
force-pushed
the
feat/suite-desktop-core/e2e-convert-migration
branch
from
0f68719 to
e8d77e1
Compare
| @@ -159,7 +166,7 @@ jobs: | |||
| echo -e "\nenableHardenedMode: false" >> .yarnrc.yml | |||
| yarn workspaces focus @trezor/suite-desktop-core | |||
| npx playwright install --with-deps | |||
| docker compose pull ${{ matrix.CONTAINERS }} | |||
| docker compose pull trezor-user-env-unix | |||
There was a problem hiding this comment.
Web isn't using any other containers atm. YAGNI
| @@ -115,13 +115,15 @@ export class OnboardingActions { | |||
| } | |||
|
|
|||
| @step() | |||
| async disableFirmwareHashCheck() { | |||
| async disableFirmwareHashCheck(options?: { skipSuiteLoadedCheck?: boolean }) { | |||
There was a problem hiding this comment.
migration test needs it when it navigates from old version to new one. The suite is loadead on dashboard.
So alternatively we could add here a different check that suite is loadead that is not based on element from onboarding flow
| const migrateFromVersion = 'release/22.5/web'; | ||
| const migrateToVersion = 'develop/web'; |
There was a problem hiding this comment.
I will ask around if we want to maybe do a different versions.
There was a problem hiding this comment.
Yep, it kind of useless :D
there is intermediate version thru everybody has to upgrade.
So the most value we will get from 24.5.4 -> develop
| }) => { | ||
| const discoveryBar = page.locator('[data-test="\\@wallet\\/discovery-progress-bar"] div'); | ||
|
|
||
| await test.step(`Load suite in old version ${migrateFromVersion}`, async () => { |
There was a problem hiding this comment.
The old version part has to rely on old testIds that are in the old version. And cannot be changed
Vere-Grey
force-pushed
the
feat/suite-desktop-core/e2e-convert-migration
branch
from
e8d77e1 to
58d90d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test-suite-web-e2e-pw.yml (1)
96-118: Enhance Dynamic Test Matrix Generation:
The newgenerate-e2e-matrixjob effectively builds a JSON matrix for test groups and conditionally adds the@group=migrationstag for scheduled events. While the manual JSON construction via shell scripting works, consider:
- Verifying that the JSON string built via
echoand the inlinejqmanipulation is robust, especially as the matrix grows in complexity.- Ensuring consistent quoting and safe variable expansion in the shell (e.g., checking that the comparison using
==in[ "${{ github.event_name }}" == "schedule" ]behaves as expected in your CI environment).Overall, the approach is clear and integrates well with the downstream job using
fromJson.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test-suite-web-e2e-pw.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: build-web
🔇 Additional comments (2)
.github/workflows/test-suite-web-e2e-pw.yml (2)
169-169: Simplified Docker Compose Pull Command:
Changing the pull command to directly referencetrezor-user-env-unixsimplifies the workflow since only that container is used. This aligns with prior feedback (see existing comment: "Web isn't using any other containers atm. YAGNI").
182-182: Simplified Docker Compose Up Command:
Directly usingdocker compose up -d trezor-user-env-unixfurther streamlines the setup by explicitly targeting the single container. Just verify that this hard-coded container name remains valid across all relevant CI configurations.
| generate-e2e-matrix: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| matrix: ${{ steps.set-matrix.outputs.matrix }} | ||
| steps: | ||
| - name: Set matrix | ||
| id: set-matrix | ||
| run: | | ||
| matrix='[ | ||
| {"TEST_GROUP": "@group=suite"}, | ||
| {"TEST_GROUP": "@group=device-management"}, | ||
| {"TEST_GROUP": "@group=settings"}, | ||
| {"TEST_GROUP": "@group=metadata1"}, | ||
| {"TEST_GROUP": "@group=metadata2"}, | ||
| {"TEST_GROUP": "@group=other"}, | ||
| {"TEST_GROUP": "@group=wallet"} | ||
| ]' | ||
|
|
||
| if [ "${{ github.event_name }}" == "schedule" ]; then | ||
| matrix=$(echo $matrix | jq '. += [{"TEST_GROUP": "@group=migrations"}]') | ||
| fi | ||
| echo "matrix={\"include\":$(echo $matrix)}" >> $GITHUB_OUTPUT | ||
|
|
There was a problem hiding this comment.
Let me know what you think.
I could improve readability if need be. Or add some comment explanation.
There was a problem hiding this comment.
We can use this for now
| @@ -129,7 +136,7 @@ jobs: | |||
| - name: Check previous runs | |||
| id: check-previous-runs | |||
| if: github.event_name == 'pull_request' && github.run_attempt == 1 | |||
| uses: './.github/actions/check-previous-test-runs' | |||
| uses: "./.github/actions/check-previous-test-runs" | |||
There was a problem hiding this comment.
probably done by prettier. I don't remember changing these
updates workflows
one locator renaming
Description
Converts one of the last cypress tests. This one has its own group that is run only as part of nightly and not normal PR runs
Related Issue
Resolve 15606
Screenshots:
https://app.currents.dev/projects/Og0NOQ/insights/tests/df9ed901634501e239d7a38a83ece5c8