feat(e2e): Introduce JS exception catcher to whole test suite #16788
Conversation
WalkthroughThe changes introduce a new asynchronous property named Tip 🌐 Web search-backed reviews and chat
✨ 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 (
|
| ); | ||
| } | ||
| }, | ||
| { auto: true }, |
There was a problem hiding this comment.
This means fixture is automatically used even if not imported to a test.
![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 (2)
packages/suite-desktop-core/e2e/support/fixtures.ts (2)
63-63: Use a more appropriate type for theexceptionLoggerproperty.The
voidtype is confusing outside a return type. Since this property is used for auto-running setup code, consider using a more appropriate type.Apply this diff to improve type clarity:
- exceptionLogger: void; + exceptionLogger: undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
221-238: Enhance error handling and reporting.The implementation effectively captures JavaScript errors, but could be improved in several ways:
Consider these enhancements:
exceptionLogger: [ async ({ page }, use) => { - const errors: Error[] = []; + const errors = new Map<string, Error>(); // Use Map for deduplication + const knownErrors = new Set(['Expected error message']); // Add known/expected errors + page.on('pageerror', error => { - errors.push(error); + // Skip known/expected errors + if (knownErrors.has(error.message)) return; + + // Deduplicate errors by message + if (!errors.has(error.message)) { + errors.set(error.message, error); + } }); await use(); - if (errors.length > 0) { + if (errors.size > 0) { + const errorsByType = new Map<string, Error[]>(); + + // Categorize errors + for (const error of errors.values()) { + const type = error.name || 'Unknown'; + if (!errorsByType.has(type)) { + errorsByType.set(type, []); + } + errorsByType.get(type)!.push(error); + } + + // Format error message with categories throw new Error( - `There was a JS exception during test run. - \n${errors.map(error => `${error.message}\n${error.stack}`).join('\n-----\n')}`, + `JavaScript exceptions occurred during test run:\n\n${ + Array.from(errorsByType.entries()) + .map(([type, typeErrors]) => + `${type} Errors:\n${ + typeErrors + .map(error => ` - ${error.message}\n ${error.stack}`) + .join('\n\n') + }` + ) + .join('\n\n') + }` ); } }, { auto: true }, ],These improvements:
- Deduplicate errors to avoid noise from repeated issues
- Filter out known/expected errors
- Categorize errors by type for better organization
- Enhance error message formatting for improved readability
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/suite-desktop-core/e2e/support/fixtures.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite-desktop-core/e2e/support/fixtures.ts
[error] 63-63: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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: Setup and Cache Dependencies
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
|
This is the only JS exception we caught so far. And it happens on not supported browser: |
Vere-Grey
force-pushed
the
feat/suite-desktop-core/e2e-catch-javascript-exceptions
branch
from
ffd73f8 to
826ffee
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/suite-desktop-core/e2e/support/common.ts (1)
175-180: Add JSDoc to clarify the fixture override pattern.The implementation looks good and follows Playwright's fixture pattern. However, consider adding JSDoc to better document the purpose and usage of this function.
Add JSDoc to explain the empty object pattern and usage:
+/** + * Helper function to skip automatic fixtures in specific tests. + * The empty object pattern is intentional as it follows Playwright's fixture override pattern. + * @example + * test.use({ exceptionLogger: skipFixture }); + */ /* eslint-disable no-empty-pattern, react-hooks/rules-of-hooks */ export async function skipFixture({}, use: (r: void) => Promise<void>) { await use(); }🧰 Tools
🪛 Biome (1.9.4)
[error] 177-177: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 177-177: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/suite-desktop-core/e2e/support/common.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-bridge-daemon.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-bridge.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts(1 hunks)packages/suite-desktop-core/e2e/tests/browser/safari.test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/suite-desktop-core/e2e/support/common.ts
[error] 177-177: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
[error] 177-177: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🔇 Additional comments (4)
packages/suite-desktop-core/e2e/tests/browser/safari.test.ts (1)
3-3: LGTM! Exception logging properly configured for Safari tests.The integration of the exception logger is correctly implemented, ensuring that any JavaScript exceptions during Safari compatibility testing will be caught and reported.
Also applies to: 19-24
packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-bridge-daemon.test.ts (1)
6-6: LGTM! Exception logging properly configured for bridge daemon tests.The exception logger integration will help catch any JavaScript errors during bridge daemon operations. Consider adding specific error scenarios to verify the exception logger's effectiveness.
To verify the exception logger's effectiveness with bridge daemon operations, consider adding a test that deliberately triggers a JavaScript error:
test('Exception logger catches JS errors in bridge daemon', async ({ request }) => { // Trigger a JavaScript error await expect(async () => { // @ts-ignore - Deliberately cause an error await undefined.someMethod(); }).toThrow(); });Also applies to: 9-9
packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-bridge.test.ts (1)
7-7: LGTM! Exception logging properly configured for bridge spawning tests.The exception logger integration will help catch JavaScript errors during bridge operations. Consider adding specific error scenarios to validate the exception catching mechanism.
To verify the exception logger's effectiveness with bridge operations, consider adding error scenarios:
test('Exception logger catches JS errors during bridge operations', async ({ request }) => { // Test error during bridge version check await expect(async () => { // @ts-ignore - Deliberately cause an error const version = await (undefined).getBridgeVersion(); }).toThrow(); });Also applies to: 13-13
✅ Verification successful
LGTM! Exception logger integration is correctly set up in the spawn-bridge tests.
- The file now imports
skipFixturealong with the existing functions.- Exception logging is properly configured to catch potential JS errors during bridge operations.
- As a future enhancement, consider adding tests for error scenarios (e.g., simulating a failure during a bridge version check as shown) to validate that exceptions are caught as expected. This suggestion applies similarly to the configurations on line 13.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
packages/suite-desktop-core/e2e/tests/bridge-tor/spawn-tor.test.ts (1)
3-3: LGTM! Exception logging properly configured for Tor tests.The exception logger integration will help catch JavaScript errors during Tor operations. Given the long-running nature of Tor tests (5-minute timeout), ensure the exception logger can handle delayed errors.
To verify the exception logger's effectiveness with long-running Tor operations:
test('Exception logger handles delayed errors in Tor operations', async () => { // Test delayed error scenario await expect(async () => { await new Promise(resolve => setTimeout(resolve, 1000)); // @ts-ignore - Deliberately cause an error await undefined.torMethod(); }).toThrow(); });Also applies to: 7-8
✅ Verification successful
LGTM! Exception logging is correctly integrated for the Tor tests.
- The added
skipFixtureimport and exception logger configuration follow the expected pattern.- The test’s 5-minute timeout is noted; ensure that the exception logger is monitored for any delayed errors during extended operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts or web queries found
HajekOndrej
deleted the
feat/suite-desktop-core/e2e-catch-javascript-exceptions
branch
Description
Introduces a JS exception catcher that fails the test in a teardown if there was any JS exception in the page during test run.
Screenshots:
Here you can see a at the top a failed test because of the JS catcher detect a JS exception during test run (I have sabotaged it). Then I commented out the JS catcher and run the tests again. You can see the test passed even tho there were JS exception thrown in the page.
