feat(e2e): Prepare for ability to test FW from main branch #17429
Conversation
WalkthroughThe changes focus on modifying test configurations and tag definitions across multiple test suites. In several files, the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (1)
✨ 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: 3
🔭 Outside diff range comments (1)
packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-recovery-advanced.test.ts (1)
43-43:⚠️ Potential issueEnsure consistency between emulator configurations across the test file.
Line 5 configuration removed the version parameter, but line 43 still explicitly specifies
version: '1-latest'when restarting the emulator. This inconsistency should be addressed.- await trezorUserEnvLink.startEmu({ model: 'T1B1', version: '1-latest' }); + await trezorUserEnvLink.startEmu({ model: 'T1B1' });
🧹 Nitpick comments (5)
packages/suite-desktop-core/e2e/tests/onboarding/t3t1/t3t1-create-wallet.test.ts (1)
26-26: Consider removing unnecessary timeoutThis line includes a hard-coded timeout of 500ms which could make tests less reliable and slower. Consider removing this timeout if it's not strictly necessary, or replacing it with a more robust waiting mechanism based on element visibility or state changes.
- await page.waitForTimeout(500);packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-fail.test.ts (1)
29-32: Consider removing unnecessary wait times.The test includes two explicit
waitForTimeoutcalls which may make the test brittle and slower. Consider replacing these with more deterministic waiting strategies, such as waiting for specific UI elements or state changes.- await page.waitForTimeout(1000); await trezorUserEnvLink.stopEmu(); - await page.waitForTimeout(500); await devicePrompt.connectDevicePromptIsShown();packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-persistence.test.ts (1)
75-80: Improved test reliability with explicit device confirmations.Adding multiple explicit calls to
confirmOnDevicePromptIsShown()before proceeding with the next steps improves the test reliability by ensuring proper synchronization with the device state.Consider standardizing this pattern across all device interaction sequences in your tests to ensure consistent behavior.
packages/trezor-user-env-link/src/api.ts (1)
195-201: Excellent approach to dynamic firmware version selection.This implementation provides a flexible way to determine firmware versions based on environment variables. The
CANARY_FIRMWAREtoggle allows switching between 'main' and 'latest' versions, which directly supports the PR objective of preparing for testing from the main branch.This approach could be extended in the future to support more granular version selection for specific test scenarios, perhaps through additional environment variables or a configuration file.
packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-success.test.ts (1)
38-39: Better synchronization with device state.Waiting for the device prompt to be confirmed before proceeding with the next step ensures proper test synchronization.
Consider adding a helper function that encapsulates the pattern of
confirmOnDevicePromptIsShown()followed bypressYes()since this sequence appears multiple times in your tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/suite-desktop-core/e2e/tests/firmware/update-firmware.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-create-wallet.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-recovery-advanced.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-recovery-fail.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-recovery-success.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-create-wallet.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-fail.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-persistence.test.ts(4 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-success.test.ts(2 hunks)packages/suite-desktop-core/e2e/tests/onboarding/t3t1/t3t1-create-wallet.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/recovery/t1b1-dry-run.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/settings/t1b1-device-settings.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/settings/t2b1-device-settings.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/settings/t2t1-device-settings.test.ts(1 hunks)packages/trezor-user-env-link/src/api.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- 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: transport-e2e-test
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (19)
packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-create-wallet.test.ts (1)
5-5:✅ Verification successful
Firmware version property removed from emulator configuration
The
versionproperty (previously set to '1-latest') has been removed from theemulatorStartConfobject. This change is consistent with modifications in other test files and aligns with the PR objective to prepare for testing firmware from the main branch.
🏁 Script executed:
#!/bin/bash # Description: Check if there's a centralized approach for firmware version management # Look for potential new implementation that might handle firmware versions echo "Searching for potential firmware version management implementation..." fd -e ts -e js | xargs grep -l "DEFAULT_.*_FIRMWARE" || echo "No centralized firmware version constants found"Length of output: 400
Firmware emulator configuration update verified
- The removal of the
versionproperty from theemulatorStartConfobject inpackages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-create-wallet.test.tsis confirmed.- An automated search revealed no centralized firmware version management constants in the codebase, supporting this change.
packages/suite-desktop-core/e2e/tests/onboarding/t3t1/t3t1-create-wallet.test.ts (1)
6-6:✅ Verification successful
Firmware version property removed from emulator configuration
The
versionproperty has been removed from theemulatorStartConfobject. This change is consistent with similar modifications across other test files and aligns with the PR objective to prepare for dynamically determining firmware versions.
🏁 Script executed:
#!/bin/bash # Description: Check for environment variables that might be used to determine firmware versions # Look for potential environment variable usage related to firmware versions echo "Searching for environment variables related to firmware versions..." fd -e ts -e js | xargs grep -l "process.env.*FIRMWARE" || echo "No environment variables related to firmware versions found"Length of output: 614
Firmware Version Removal Verified
- The
versionproperty has been intentionally removed from theemulatorStartConfobject to align with changes made in similar test files.- This modification supports the PR objective of preparing for dynamic firmware version determination. Notably, the absence of environment variable references for firmware versions in this code area confirms that the change is isolated to the configuration, as expected.
- Updated code snippet:
emulatorStartConf: { wipe: true, model: 'T3T1' },packages/suite-desktop-core/e2e/tests/recovery/t1b1-dry-run.test.ts (1)
9-9:❓ Verification inconclusive
Firmware version property removed from emulator configuration
The
versionproperty (previously set to '1-latest') has been removed from theemulatorStartConfobject. This change is part of a consistent pattern across test files, preparing for a more centralized approach to firmware version management.
🏁 Script executed:
#!/bin/bash # Description: Check for a potential central API or configuration file that might now control firmware versions # Look for files that might be handling the firmware version selection echo "Searching for potential central firmware version configuration..." fd -e ts -e js | grep -l "api" | xargs grep -l "firmware.*version" || echo "No central API handling firmware versions found"Length of output: 395
Firmware Version Handling Update
The removal of the
versionproperty from theemulatorStartConfobject inpackages/suite-desktop-core/e2e/tests/recovery/t1b1-dry-run.test.tsis consistent with similar changes across test files. Note, however, that our search did not reveal any centralized firmware version management mechanism in the repository. Please verify that the eventual centralized handling (if planned) is properly integrated so that removing the firmware version from individual emulators does not introduce any unintended issues.
- File:
packages/suite-desktop-core/e2e/tests/recovery/t1b1-dry-run.test.ts(line 9)- Snippet Confirmed:
emulatorStartConf: { model: 'T1B1', wipe: true },- Action: Ensure that if a central firmware version configuration is intended, it is correctly implemented; otherwise, double-check that current test behaviors remain unaffected.
packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-recovery-success.test.ts (1)
8-8: Removal of version property for emulator configurationConsistent with other changes, the
versionproperty has been removed from the emulator configuration. This appears to be part of a broader effort to prepare for testing with firmware from the main branch as mentioned in the PR title.packages/suite-desktop-core/e2e/tests/settings/t1b1-device-settings.test.ts (1)
4-4: Removal of version property for emulator configurationThis change aligns with the PR objective of preparing for testing firmware from the main branch by removing hardcoded version references.
packages/suite-desktop-core/e2e/tests/firmware/update-firmware.test.ts (2)
5-5: Verification needed: Hard-coded version with '@specificFirmware' tagThis test retains its hard-coded version (
version: '2.5.2') despite the tag addition and the removal of versions in other files. Since this is a firmware update test that explicitly tests version 2.5.2, it makes sense to keep the version specified, but should verify if this is consistent with the intended pattern for '@specificFirmware' tagged tests.Is the intention to have all tests tagged with '@specificFirmware' specify exact firmware versions while other tests use a default/latest version? If so, a comment explaining this pattern would be helpful for future maintainers.
4-4:❓ Verification inconclusive
Added '@specificFirmware' tag while keeping version hardcoded
The test now includes a new tag '@specificFirmware', which appears to be for categorizing tests that require specific firmware versions. Interestingly, this test still maintains a hardcoded version (
2.5.2) on line 5, unlike the other modified files that had their version properties removed.
🏁 Script executed:
#!/bin/bash # Description: Search for other tests using the @specificFirmware tag and check for a pattern # Find all tests with the @specificFirmware tag echo "Tests using @specificFirmware tag:" rg -A 3 "@specificFirmware" packages/suite-desktop-core/e2e/tests/ -t ts # Check if there's a consistent pattern for tests with @specificFirmware tag (do they all have hardcoded versions?) echo -e "\nChecking if @specificFirmware tests consistently use hardcoded versions:" rg -A 5 "@specificFirmware.*\n.*\{.*version:" packages/suite-desktop-core/e2e/tests/ -t tsLength of output: 1879
Action: Confirm Consistency in Firmware Version Handling
Our verification shows that tests tagged with
@specificFirmwarein the repository consistently specify a hardcoded firmware version—evidenced by both the firmware update test (using version2.5.2) and the T2T1 device settings test (using version2.5.3). This suggests that retaining a hardcoded version in these tests might be intentional, rather than an oversight in the firmware update test alone.
- Review both test files:
packages/suite-desktop-core/e2e/tests/firmware/update-firmware.test.ts(hardcoded version2.5.2)packages/suite-desktop-core/e2e/tests/settings/t2t1-device-settings.test.ts(hardcoded version2.5.3)- Confirm whether the hardcoded firmware version is expected for tests requiring specific firmware updates or if the version should be dynamically set (as seen in other parts of the codebase).
Please verify if the existing behavior is intentional. If a refactor is needed, ensure that all tests using the
@specificFirmwaretag handle version configuration consistently.packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-recovery-advanced.test.ts (1)
5-5: Version parameter removal improves flexibility for firmware testing.The removal of the
version: '1-latest'property from the emulator configuration aligns with preparing the framework for more flexible firmware version testing. This modification allows the test to run with different firmware versions without requiring code changes.packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-create-wallet.test.ts (1)
6-6: Version parameter removal improves testing flexibility.The removal of the
version: '2-latest'property from the emulator configuration is consistent with the project's goal to prepare for testing firmware from the main branch. This change allows the test to adapt to different firmware versions without modifying the test code.packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-fail.test.ts (2)
5-5: Version parameter removal enhances firmware testing flexibility.The removal of the
version: '2-latest'property from the emulator configuration is consistent with the pattern observed in other test files, making the framework more adaptable to testing with different firmware versions.
33-33: Emulator restart configuration updated consistently.The removal of the version parameter in the
startEmucall maintains consistency with the initial emulator configuration, which is appropriate.packages/suite-desktop-core/e2e/tests/settings/t2t1-device-settings.test.ts (3)
49-49: Added '@specificFirmware' tag to categorize version-specific tests.Adding the
@specificFirmwaretag to the test suite for firmware < 2.5.4 improves test organization and filtering capabilities. This allows for more granular control over which tests run based on firmware version requirements, supporting the goal of preparing for testing firmware from the main branch.
50-50: Retained specific version for test with explicit version dependency.Unlike other tests where version specifications were removed, this test correctly maintains its explicit version requirement (
version: '2.5.3') as it's testing functionality specific to firmware versions < 2.5.4.
57-58: Address TODO comments in future PRs.There are open TODO comments for testing custom image uploads and auto-lock settings. Consider creating issues to track these intended test cases to ensure they're not forgotten.
packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-persistence.test.ts (3)
51-51: Good removal of hardcoded firmware version.Removing the
versionproperty from the emulator configuration aligns with centralizing firmware version management, which helps with the PR objective of enabling testing from the main branch.
90-90: Appropriate removal of hardcoded version in startEmu call.Removing the version parameter from the emulator start configuration properly delegates version selection to the centralized logic in the API.
125-125: Consistent implementation of version-independent emulator initialization.This change correctly aligns with the other modifications in this PR, ensuring all emulator initializations are consistent and rely on the centralized version management.
packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-success.test.ts (2)
5-5: Good removal of hardcoded firmware version.Removing the version specification from the emulator config aligns with the centralized version management approach in this PR.
29-30: Improved test stability with explicit device confirmation.Adding an explicit wait for device prompt confirmation before proceeding improves test reliability.
packages/suite-desktop-core/e2e/tests/settings/t2b1-device-settings.test.ts
Show resolved
Hide resolved
packages/suite-desktop-core/e2e/tests/onboarding/t1b1/t1b1-recovery-fail.test.ts
Show resolved
Hide resolved
packages/suite-desktop-core/e2e/tests/onboarding/t2t1/t2t1-recovery-success.test.ts
Show resolved
Hide resolved
HajekOndrej
force-pushed
the
fw-canary-pw
branch
from
fde6f3d to
789b0d9
Compare
HajekOndrej
force-pushed
the
fw-canary-pw
branch
from
789b0d9 to
056e629
Compare
Description
Related Issue
Resolve
Screenshots: