-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: improve logs for e2e errors #28479
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
@@ -900,6 +900,7 @@ describe('Test Snap Metrics', function () { | |||
testSpecificMock: mockSegment, | |||
ignoredConsoleErrors: [ | |||
'MetaMask - RPC Error: Failed to fetch snap "npm:@metamask/bip32-example-snap": Failed to fetch tarball for package "@metamask/bip32-example-snap"..', | |||
'Failed to fetch snap "npm:@metamask/bip32-example-…ball for package "@metamask/bip32-example-snap"..', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this error is expected in this test. It was a blank empty error before (see screenshot in dscription) and didn't cause the test to fail. Now this is needed, as we are logging also this error message instead of an empty string and makes the test fail
@@ -86,6 +86,9 @@ describe('lockdown', function (this: Mocha.Suite) { | |||
{ | |||
fixtures: new FixtureBuilder().build(), | |||
ganacheOptions, | |||
ignoredConsoleErrors: [ | |||
'Error: Could not establish connection.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this error is expected in this test. It was a blank empty error before and didn't cause the test to fail. Now this is needed, as we are logging also this error message instead of an empty string and makes the test fail
Builds ready [d839f0e]
Page Load Metrics (1766 ± 55 ms)
Bundle size diffs
|
@@ -86,6 +86,7 @@ describe('lockdown', function (this: Mocha.Suite) { | |||
{ | |||
fixtures: new FixtureBuilder().build(), | |||
ganacheOptions, | |||
ignoredConsoleErrors: ['Error: Could not establish connection.'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this error is expected in this test. It was a blank empty error before (see screenshot in dscription) and didn't cause the test to fail. Now this is needed, as we are logging also this error message instead of an empty string and makes the test fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
This PR adds an improvement on our logs when the errors do not have the expected form of a.value, leading to displaying empty errors and not failing the test. Those are happening for RPC and some snap errors types, which currently are displayed as empty (see below screenshots):
value
property but adescription
, so we were seeing empty errors in the logsWith this change we are now able to see better error logs in our e2e.
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3648
Manual testing steps
yarn test:e2e:single test/e2e/tests/request-queuing/ui.spec.js --browser=chrome
Screenshots/Recordings
Before
See empty RPC error logs:
See empty snap error logs (the 1st one type is logged but the 2nd one is empty):
After
See complete RPC error logs
See complete snaps error logs
Pre-merge author checklist
Pre-merge reviewer checklist