Add additional Sentry E2E tests #20425
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## develop #20425 +/- ##
===========================================
- Coverage 68.66% 68.64% -0.02%
===========================================
Files 990 990
Lines 38274 38286 +12
Branches 10260 10262 +2
===========================================
Hits 26279 26279
- Misses 11995 12007 +12
|
|
Lots of distinct changes in this PR. I was tempted to split it up into separate PRs, but instead I left each change as a separate commit. Reviewing one commit at a time is advised. |
8 tasks
Builds ready [bcc40c2]
Page Load Metrics (1806 ± 56 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
danjm
reviewed
Aug 8, 2023
app/scripts/metamask-controller.js
Outdated
| * @deprecated This is only mean to facilitiate E2E testing. We should not | ||
| * use this for handling errors. | ||
| */ | ||
| throwError(message) { |
There was a problem hiding this comment.
nit: maybe make this a noop if we are not in dev or in test?
There was a problem hiding this comment.
We could! If this was called in a production build it would legitimately be an error though. Maybe it'd be better to throw, in case someone doesn't heed the deprecation notice.
Though... I should at least skip setting up the hook in the UI in test builds. We don't want people calling that in the web console. I'll update that to only set it up on test or dev builds.
There was a problem hiding this comment.
Revised here: f4f1932
Let me know if that makes sense
Gudahtt
commented
Aug 8, 2023
| // Wait for Sentry request | ||
| await driver.wait(async () => { | ||
| const isPending = await mockedEndpoint.isPending(); | ||
| return isPending === false; | ||
| }, 10000); | ||
| }, 3000); |
There was a problem hiding this comment.
This was made 3000 to match the other test. If it takes longer than 3 seconds here for the request to arrive, then it implies the test where we're expecting it to never arrive isn't waiting long enough.
Gudahtt
commented
Aug 8, 2023
| // Trigger error | ||
| driver.executeScript('window.stateHooks.throwTestError()'); |
There was a problem hiding this comment.
These two lines were missing awaits
Builds ready [f4f1932]
Page Load Metrics (1581 ± 40 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Gudahtt
force-pushed
the
add-sentry-metric-state-tests
branch
from
bec7666 to
8b1f49e
Compare
Gudahtt
commented
Aug 14, 2023
| @@ -131,11 +383,10 @@ describe('Sentry errors', function () { | |||
| }, | |||
| async ({ driver, mockedEndpoint }) => { | |||
| await driver.navigate(); | |||
| await driver.fill('#password', 'correct horse battery staple'); | |||
There was a problem hiding this comment.
The login step was producing indeterminacy in the state snapshots because we didn't wait for the login process to finish. Rather than adding a waiting step, the login was removed (it wasn't necessary for the test).
Gudahtt
force-pushed
the
add-sentry-metric-state-tests
branch
3 times, most recently
from
16d947c to
c4cd27d
Compare
Builds ready [c4cd27d]
Page Load Metrics (1697 ± 48 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Gudahtt
force-pushed
the
add-sentry-metric-state-tests
branch
from
c4cd27d to
78b025f
Compare
Builds ready [78b025f]
Page Load Metrics (1521 ± 28 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Gudahtt
force-pushed
the
add-sentry-metric-state-tests
branch
2 times, most recently
from
f2564db to
d74cbdc
Compare
Builds ready [1d1557e]
Page Load Metrics (1563 ± 49 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
8 tasks
danjm
reviewed
Aug 15, 2023
| for (const field of dateFields) { | ||
| if (has(data, field)) { | ||
| set(data, field, typeof get(data, field)); | ||
| } |
danjm
reviewed
Aug 15, 2023
| return; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
I was a bit confused by this function, wondering why we don't communicate anything about the deepStrictEqual failure and why we just update the snapshot instead of failing the test. But then I re-read the PR description and the default value of the update param and this makes sense. We do throw there error except in cases where UPDATE_SNAPSHOTS has been explicitly set, which would be done intentionally when the snapsshots are to be updated.
👍
danjm
approved these changes
Aug 15, 2023
|
Looks good to me. Nice improvements to the sentry error tests. |
The tests have been reorganized into different describe blocks. Each describe block is for either before or after initialization, and either with or without opting into metrics. This was done to simplify later test additions. The conditions for each test are now in the describe block, letting us test additional things in each of these conditions. The conditions were flattened to a single level to avoid excessive indentation.
The Sentry e2e tests before initialization only tested background errors, and the after initialization tests only tested UI errors. Now both types of errors are tested in both scenarios.
E2E tests have been added to test the state object sent along with each Sentry error. At the moment this object is empty in some circumstances, but this will change in later PRs.
The state hooks used for debugging and testing are now only included in dev or test builds. The function name was updated and given a JSDoc description to explain this more clearly as well.
State snapshot assertions have been added to the e2e error tests. These snapshots will be very useful in reviewing a few PRs that will follow this one. We might decide to remove these snapshots after this set of Sentry refactors, as they might be more work to maintain than they're worth. But they will be useful at least in the short-term. The login step has been removed from a few tests because it introduced indeterminacy (the login process continued asynchronously after the login, and sometimes was not finished when the error was triggered).
This fixes an intermittent failure on Firefox
The error screen only appears after a long timeout, and it doesn't affect the next test steps at all.
Gudahtt
force-pushed
the
add-sentry-metric-state-tests
branch
from
1d1557e to
ad557db
Compare
Builds ready [ad557db]
Page Load Metrics (1453 ± 50 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
darkwing
approved these changes
Aug 16, 2023
| ); | ||
|
|
||
| // Wait for Sentry request | ||
| await driver.delay(3000); |
There was a problem hiding this comment.
Non-blocking nit but we may want to stick to known delay constants rather than an arbitrary number. Can be done in a follow up though.
There was a problem hiding this comment.
yeah good call. This was copied from pre-existing tests in this file, and they should all be using a matching delay time, so I'll leave this for a follow-up.
| await driver.wait(async () => { | ||
| const isPending = await mockedEndpoint.isPending(); | ||
| return isPending === false; | ||
| }, 3000); |
There was a problem hiding this comment.
Same nit re: milliseconds
metamaskbot
added
the
release-10.36.0
danjm
added
release-10.34.5
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Additional E2E tests have been added for our Sentry error handling.
Before this PR, the Sentry e2e tests before initialization only tested background errors, and the after initialization tests only tested UI errors. Now both types of errors are tested in both scenarios.
Tests have been added to test the state object sent along with each Sentry error as well. At the moment this object is empty in some circumstances, but this will change in later PRs.
To trigger background errors for testing, a new controller API method has been added that throws a test error. This mirrors a similar test function in the UI. A
@deprecatedtag was used to warn against using this accidentally in real code.State snapshots have been added to make it easier to track changes to the Sentry state in future PRs. This will be very useful for the next few PRs, but might be less valuable long-term. We should consider removing it after the next few Sentry state changes to reduce maintenance burdens. State snapshots can be updated by setting the environment variable
UPDATE_SNAPSHOTStotruewhen running tests, e.g.Relates to #20449
Manual Testing Steps
N/A
Pre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.