fix(e2e): Fixed flakiness of analytics tests #17012
Conversation
WalkthroughThe pull request introduces modifications to the analytics handling in the end-to-end tests. In the analytics support file, a new private property, 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (9)
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 (2)
packages/suite-desktop-core/e2e/tests/analytics/toggle.test.ts (1)
107-114: Consider adding test coverage for edge cases.The test verifies the happy path but could benefit from additional coverage:
- Test behavior when analytics events fail to arrive
- Test behavior with concurrent analytics events
- Test behavior with duplicate events
packages/suite-desktop-core/e2e/tests/analytics/events.test.ts (1)
41-47: Document the expected event sequence.While the code correctly waits for 3 events, the magic number could be more explicit.
Apply this diff to improve code clarity:
- await analytics.waitForAnalyticsRequests(3); + // Wait for the initial sequence of events: + // 1. suite-ready + // 2. device-connect + // 3. transport-type + const EXPECTED_INITIAL_EVENTS = 3; + await analytics.waitForAnalyticsRequests(EXPECTED_INITIAL_EVENTS);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/suite-desktop-core/e2e/support/analytics.ts(3 hunks)packages/suite-desktop-core/e2e/tests/analytics/events.test.ts(3 hunks)packages/suite-desktop-core/e2e/tests/analytics/toggle.test.ts(4 hunks)
⏰ 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=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: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- 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/support/analytics.ts (1)
19-19:❓ Verification inconclusive
Address the TODO comment.
The TODO comment references issue #15811 for refactoring. Let's verify if this issue is still relevant.
🏁 Script executed:
#!/bin/bash # Check if the referenced issue exists and is still open gh issue view 15811Length of output: 110
Action Required: Verify the Status of Issue #15811
The TODO in
packages/suite-desktop-core/e2e/support/analytics.ts(line 19) still references issue #15811 for refactoring. The automated check usinggh issue view 15811failed because the repository context was not recognized as a Git repository, so its current status couldn’t be confirmed automatically.• File:
packages/suite-desktop-core/e2e/support/analytics.ts(line 19)
• Issue Reference: #15811Recommended next steps:
Please manually verify if issue #15811 is still open or relevant. Based on that confirmation, update the TODO comment accordingly (e.g., remove, update, or address the refactor details).packages/suite-desktop-core/e2e/tests/analytics/toggle.test.ts (1)
24-35: LGTM! Improved test reliability.The changes enhance test reliability by:
- Using
waitForAnalyticsRequeststo ensure events are captured- Using optional chaining for safer property access
- Using type-safe event lookups with
findLatestRequestByTypepackages/suite-desktop-core/e2e/tests/analytics/events.test.ts (1)
77-78: LGTM! Race condition fix.The changes properly handle the device disconnect event by:
- Waiting for the event using
waitForAnalyticsRequests- Using type-safe
findLatestRequestByTypefor verification
| findLatestRequestByType(eventType: SuiteAnalyticsEvent['type']) { | ||
| return this.requests.reverse().find(req => req.c_type === eventType) | ||
| } |
There was a problem hiding this comment.
Fix array mutation in findLatestRequestByType.
The current implementation mutates the original array by using reverse(). This could affect other code that relies on the array order.
Apply this diff to create a non-mutating implementation:
- findLatestRequestByType(eventType: SuiteAnalyticsEvent['type']) {
- return this.requests.reverse().find(req => req.c_type === eventType)
+ findLatestRequestByType(eventType: SuiteAnalyticsEvent['type']) {
+ return [...this.requests].reverse().find(req => req.c_type === eventType)📝 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.
| findLatestRequestByType(eventType: SuiteAnalyticsEvent['type']) { | |
| return this.requests.reverse().find(req => req.c_type === eventType) | |
| } | |
| findLatestRequestByType(eventType: SuiteAnalyticsEvent['type']) { | |
| return [...this.requests].reverse().find(req => req.c_type === eventType) | |
| } |
HajekOndrej
force-pushed
the
fix-request-asserts
branch
from
3061be1 to
3591ff2
Compare
Description
Related Issue
Resolve
Screenshots: