feat(e2e): Finished onboarding tests migration #16828
Conversation
WalkthroughThis pull request introduces two new end-to-end test suites in the desktop core repository. The first suite, located in Possibly related issues
Suggested labels
Suggested reviewers
✨ 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 (
|
There was a problem hiding this comment.
Migrated previously, but was not deleted.
There was a problem hiding this comment.
No working in Cypress - I have created a new issue to do this properly #16807
| await analyticsPage.continueButton.click(); | ||
| await expect(page.getByTestId('@webusb-button')).toBeVisible(); | ||
| await page.getByTestId('@connect-device-prompt/no-device-detected').click(); | ||
| await expect(page.getByTestId('@collapsible-box/body')).toHaveAttribute( |
There was a problem hiding this comment.
Seemed proper to add some checking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/suite-desktop-core/e2e/tests/onboarding/transport.test.ts (2)
9-9: Make the test title more descriptive.Consider renaming the test to better describe the complete test flow, e.g., "Should display webusb as primary choice and expand options when no device detected".
10-12: Add error handling for click actions.Consider wrapping click actions in try-catch blocks to handle potential interaction failures gracefully.
- await analyticsPage.continueButton.click(); - await expect(page.getByTestId('@webusb-button')).toBeVisible(); - await page.getByTestId('@connect-device-prompt/no-device-detected').click(); + try { + await analyticsPage.continueButton.click(); + await expect(page.getByTestId('@webusb-button')).toBeVisible(); + await page.getByTestId('@connect-device-prompt/no-device-detected').click(); + } catch (error) { + throw new Error(`Failed to interact with transport options: ${error.message}`); + }packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts (3)
14-14: Improve test title formatting and clarity.The current test title is verbose and uses sentence case. Consider using a more concise title in Title Case, e.g., "Should Show Analytics Consent When Visiting /accounts Route".
19-22: Consider parameterizing the route and adding more test cases.The route is hardcoded and only tests the happy path. Consider:
- Parameterizing the route to test multiple entry points
- Adding test cases for analytics consent rejection
Example of parameterized test:
const routes = ['/accounts', '/settings', '/'] as const; for (const route of routes) { test(`Should Show Analytics Consent When Visiting ${route} Route`, async ({ page, analyticsPage, onboardingPage, }) => { page.goto(route); // ... rest of the test }); }
28-31: Consider adding timeout for visibility checks.Add appropriate timeouts for visibility checks to handle potential delays in UI updates.
- await expect(page.getByTestId('@suite-layout/body')).toBeVisible(); + await expect(page.getByTestId('@suite-layout/body')).toBeVisible({ timeout: 5000 }); - await page.getByTestId('@account-menu/btc/normal/0').click(); - await expect(page.getByTestId('@wallet/menu/wallet-send')).toBeVisible(); + await page.getByTestId('@account-menu/btc/normal/0').click(); + await expect(page.getByTestId('@wallet/menu/wallet-send')).toBeVisible({ timeout: 5000 });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/transport.test.ts(1 hunks)packages/suite-web/e2e/tests/onboarding/analytics-consent.test.ts(0 hunks)packages/suite-web/e2e/tests/onboarding/firmware-update.test.ts(0 hunks)packages/suite-web/e2e/tests/onboarding/t3t1/t3t1-create-wallet.test.ts(0 hunks)packages/suite-web/e2e/tests/onboarding/transport.test.ts(0 hunks)
💤 Files with no reviewable changes (4)
- packages/suite-web/e2e/tests/onboarding/analytics-consent.test.ts
- packages/suite-web/e2e/tests/onboarding/firmware-update.test.ts
- packages/suite-web/e2e/tests/onboarding/transport.test.ts
- packages/suite-web/e2e/tests/onboarding/t3t1/t3t1-create-wallet.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- 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: build-web
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
packages/suite-desktop-core/e2e/tests/onboarding/transport.test.ts (1)
9-17: Consider adding more test coverage.The test suite focuses only on the webusb option. Consider adding test cases for:
- Bridge transport option
- Error scenarios (e.g., webusb not supported)
- Device connection timeout scenarios
Would you like me to help generate additional test cases to improve coverage?
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts (1)
24-26: Add assertions for secure element check.The secure element check lacks assertions to verify the check was successful.
if (onboardingPage.isModelWithSecureElement()) { await onboardingPage.passThroughAuthenticityCheck(); + await expect(page.getByTestId('@authenticity/success-screen')).toBeVisible(); }
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts
Show resolved
Hide resolved
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts
Outdated
Show resolved
Hide resolved
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts
Outdated
Show resolved
Hide resolved
|
|
||
| test.describe( | ||
| 'Onboarding - transport webusb/bridge', | ||
| { tag: ['@group=device-management', '@webOnly'] }, |
There was a problem hiding this comment.
This one has the grouping problem too.
HajekOndrej
force-pushed
the
migrate-onboarding
branch
2 times, most recently
from
f2c1bd3 to
5d18350
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts (2)
11-11: Consider a more descriptive test title.The current title "analytics consent appears on any route that is visited initially. this time /accounts" is verbose and could be more concise. Consider something like "analytics consent screen is shown when visiting /accounts route".
11-30: Add test coverage for analytics consent rejection.The test only covers the happy path where the user accepts analytics consent. Consider adding another test case to verify the behavior when the user rejects analytics consent.
Would you like me to help generate a test case for the analytics consent rejection scenario?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/test-suite-web-e2e-pw.yml(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Releases revision Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Other Checks
- GitHub Check: Unit Tests
- GitHub Check: Linting and formatting
- GitHub Check: Type Checking
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- 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: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (2)
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts (1)
1-10: LGTM!The test suite setup is well-structured with appropriate tags and necessary test preparation in the
beforeEachhook..github/workflows/test-suite-web-e2e-pw.yml (1)
107-108: LGTM!The addition of the device management test group to the matrix is correctly configured and aligns with the new test file.
HajekOndrej
force-pushed
the
migrate-onboarding
branch
from
5d18350 to
b8e04c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-core/e2e/tests/onboarding/transport.test.ts (1)
15-23: Consider enhancing test robustness and clarity.While the test flow is logical, consider these improvements:
- Add pre-condition assertions before clicking buttons
- Make the test name more descriptive about verifying collapsible box behavior
Example enhancement:
- test('Offer webusb as primary choice on web', async ({ page, analyticsPage }) => { + test('Should expand collapsible box when no device detected', async ({ page, analyticsPage }) => { + await expect(analyticsPage.continueButton).toBeVisible(); await analyticsPage.continueButton.click(); + await expect(page.getByTestId('@webusb-button')).toBeVisible(); + + await expect(page.getByTestId('@connect-device-prompt/no-device-detected')).toBeVisible(); await page.getByTestId('@connect-device-prompt/no-device-detected').click(); + await expect(page.getByTestId('@collapsible-box/body')).toHaveAttribute( 'aria-expanded', 'true', ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/transport.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/suite-desktop-core/e2e/tests/onboarding/analytics-consent.test.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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=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: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (3)
packages/suite-desktop-core/e2e/tests/onboarding/transport.test.ts (3)
1-8: LGTM!The test suite is well-structured with appropriate tags and configuration for transport testing.
9-13: LGTM!The beforeEach hook ensures a clean state by properly managing the bridge and emulator lifecycle.
25-29: LGTM!The afterEach hook properly cleans up the bridge state after each test.
Description
Migrate the rest of Onboarding tests.
The mocked firmware update was not working in cypress, so I have decided to just create an issue for that: #16807
Related Issue
Resolve
Screenshots: