Version v10.34.5 #20482
Merged
Version v10.34.5 #20482
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
|
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. |
* Log before and after each migration run * Use loglevel
* Reorganize Sentry error e2e 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. * Add error e2e test for background and UI errors 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. * Add error e2e tests for Sentry error state 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. * Rename throw test error function * Only setup debug/test state hooks in dev/test builds 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. * Add state snapshot assertions 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). * Ensure login page has rendered during setup This fixes an intermittent failure on Firefox * Format snapshots with prettier before writing them * Use defined set of date fields rather than infering from name * Remove waits for error screen The error screen only appears after a long timeout, and it doesn't affect the next test steps at all.
* Add AppMetadataController so current and previous application and migration version can be captured in sentry * Add currentAppVersion, previousAppVersion, previousMigrationVersion, currentMigrationVersion to SENTRY_OBJECT * Update app/scripts/controllers/app-metadata.ts Co-authored-by: Mark Stacey <[email protected]> * Update app/scripts/controllers/app-metadata.ts Co-authored-by: Mark Stacey <[email protected]> * Update app/scripts/controllers/app-metadata.ts Co-authored-by: Mark Stacey <[email protected]> * Fix types * Add tests for app-metadata.test.ts * Lint fixes * Modify loadStateFromPersistence to return the whole versionData object, so that the migration version can be passed to the metamask-controller on instantiation * Remove reference to implementation details in test descriptions in app/scripts/controllers/app-metadata.test.ts * Reset all mocks afterEach in AppMetadataController * Refactor AppMetadataController to be passed version instead of calling platform.version directly (for ease of unit testing the MetaMask Controller) * Make maybeUpdateAppVersion and maybeUpdateMigrationVersion private, and remove unit tests of those specific functions --------- Co-authored-by: Mark Stacey <[email protected]>
The state fixtures in the Sentry e2e tests became invalid in #20458 due to a conflict with that change (the new state properties were missing). The state fixtures have been updated.
|
No top level dependency changes detected. Learn more about Socket for GitHub ↗︎ |
* Bump SES to fix audit failure * Freeze Symbol
The state mask used to anonymize the Sentry state snapshots has been split into UI and background masks. This was done to simplify later refactors. There should be no functional changes.
The unflattened background state is now attached to any Sentry errors from the background process. This provides a clearer picture of the state of the wallet, and unblocks further improvements to Sentry state which will come in later PRs.
danjm
force-pushed
the
Version-v10.34.5
branch
from
08f27c7 to
98cac94
Compare
…grations (#20427) * capture exception for sentry when invariant conditions are met in migration 82 * Code cleanup * Capture exceptions in invariant conditions for migrations 83,84,85,86,89,91,93,94 * Update app/scripts/migrations/082.test.js Co-authored-by: Mark Stacey <[email protected]> * Code cleanup * Fix SentryObject type declaration * Stop throwing error if preferences controller is undefined * Refactor 084 and 086 to remove double negative * Capture exceptions for invariant states in in migrations 87,88,90 and 92 * lint fix * log warning in migration 82 when preferences controller is undefined --------- Co-authored-by: Mark Stacey <[email protected]>
Additional validation has been added for persisted state metadata. Beforehand we just checked that the state itself wasn't falsy. Now we ensure that the metadata is an object with a valid version as well.
* Add types of hidden properties to Sentry data The masked wallet state object sent to Sentry has been updated to include the type of each property omitted from the mask. This lets us at least see the full state shape, making it easier to see when errors are caused by invalid state. Relates to #20449 * Remove inconsistent state snapshot properties The state snapshot tests have been updated to exclude properties that were shown to differ between runs.
danjm
force-pushed
the
Version-v10.34.5
branch
from
98cac94 to
805ce29
Compare
* Improve Sentry state pre-initialization Previously the masked state snapshot sent to Sentry would be blank for errors that occured during initialization. Instead we'll now include some basic information in all cases, and a masked copy of the persisted state if it happens after the first time the persisted state is read. * Add test * Fix crash when persisted state not yet fetched * Add descriptions for initial state hooks * Update comments to reflect recent changes * Re-order imports to follow conventions * Move initial state hooks back to module-level The initial state hooks are now setup at the top-level of their module. This ensures that they're setup prior to later imports. Calling a function to setup these hooks in the entrypoint module wouldn't accomplish this even if it was run "before" the imports because ES6 imports always get hoisted to the top of the file. The `localStore` instance wasn't available statically, so a new state hook was introduced for retrieving the most recent retrieved persisted state. * Fix error e2e tests
Migration #77 would set the `TokenListController.tokensChainsCache` state to `undefined` if it wasn't already set to anything when that migration was run. This is probably harmless except that it results in Sentry errors during migrations, and it results in that property having a value (at least temporarily) that doesn't match its type. Migration #77 has been updated to prevent this property from being set to `undefined` going forward. A new migration has been added to delete this value for any users already affected by this problem. The new migration was named "92.1" so that it could run after 92 but before 93, to make backporting this to v10.34.x easier (v10.34.x is currently on migration 92). "92.1" is still a valid number so this should work just as well as a whole number.
* Fix Sentry MetaMetrics detection The refactor of the Sentry state in #20491 accidentally broke our opt- in detection. The opt-in detection has been updated to look for both types of application state (during and after initialization). * Continue suppressing breadcrumbs during onboarding * Fix how onboarding status is retrieved The check for whether the user had completed onboarding assumed that the application state was post-initialization UI state. It has been updated to handle background state and pre-initialization state as well. * Remove unnecessary optional chain operators * Add missing optional chain operator * Fix JSDoc description parameter type
* Initialize composable observable store after update The composable observable store now updates state immediately when the structure is updated. Previously each store would only be updated after the first state change. This ensures that the composable observable store state is always complete. * SUpport falsy controller state We now use the nullish coalescing operator when checkint store.state, so that we don't accidentally ignore falsy state. Co-authored-by: Frederik Bolding <[email protected]> * Add test for falsy controller state * Update state snapshots A change on `develop` required another state update. --------- Co-authored-by: Frederik Bolding <[email protected]>
* Fix and test log.info calls run for each migration In migrator/index.js, log.info is called before an after each migration. These calls are intended to produce breadcrumbs to be captured by sentry in cases where errors happen during or shortly after migrations are run. These calls were not causing any output to the console because the log.setLevel calls in ui/index.js were setting a 'warn value in local storage that was being used by logLevel in the background. This commit fixes the problem by setting the `persist` param of setLevel to false, so that the background no longer reads the ui's log level. Tests are added to verify that these logs are captured in sentry breadcrumbs when there is a migration error due to an invariant state. * Improve breadcrumb message matching The test modified in this commit asserts eqaulity of messages from breadcrumbs and hard coded expected results. This could cause failures, as sometimes the messages contain whitespace characters. This commit ensures the assertions only check that the expected string is within the message string, ignoring extra characters.
… output no longer flags that advisory
|
I have tested everything mentioned in the manual QA plan above, including seeing all the sentry errors in my personal instance of sentry. I also verified that you can still create and reject transactions from uniswap. I also verified that This is QA approved by me |
Builds ready [016a1ef]
Page Load Metrics (1578 ± 42 ms)
|
In the case where an error is thrown in the UI before initialization has finished, we aren't capturing the application state correctly for Sentry errors. We had a test case for this, but the test case was broken due to a mistake in how the `network-store` was setup (it was not matching the behavior of the real `local-tstore` module). The pre-initialization state capture logic was updated to rely solely upon the `localStore` instance used by Sentry to determine whether the user had opted-in to metrics or not. This simplifies the logic a great deal, removing the need for the `getMostRecentPersistedState` state hook. It also ensures that state is captured corretly pre- initialization in both the background and UI.
Sentry breadcrumb collection during initialization was broken in #20529 because we failed to consider that the `getSentryState` check was also used for an opt-in check in the `beforeBreadcrumb` hook. I had assumed that `getSentryState` was only used to get state to add additional context to an error report. But the function has a second purpose: to get state for the purposes of checking whether the user has opted into MetaMetrics. In this second case, `mostRecentRetrievedState` is sometimes unset (which violates an assumption made in #20529) The `getMostRecentPersistedState` hook removed in #20529 has been restored, ensuring that the `getSentryState` function returns Sentry state after loading state for the first time, but before the first error has occurred. This mistake didn't cause e2e tests to fail because multiple errors are currently thrown in the background upon initialization on `develop` (relating to Snow scuttling). These errors were early enough that they happened before the console logs that our breadcrumb test was testing for. When #20529 was ported onto the v10.34.5 RC, these errors were not present so the test failed correctly.
|
I redid these tests, and they continue to pass:
This is QA approved by me |
Builds ready [f83c7ff]
Page Load Metrics (1496 ± 41 ms)
|
Gudahtt
approved these changes
Aug 18, 2023
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.
Manual test plan:
(A) To validate #20424 and #20472
(B) To validate #20458
currentAppVersion,previousAppVersion,currentMigrationVersionandpreviousMigrationVersionproperties(C) To validate #20428, #20491 and #20457
SENTRY_BACKGROUND_STATEinsetupSentry.js, and they should point to strings that name thetypeofthe value for properties not inSENTRY_BACKGROUND_STATE(D) To validate #20462
(E) To validate #20426
if (process.env.METAMASK_DEBUG || process.env.IN_TEST) {in ui/index.js so that the code inside the if block is runwindow.stateHooks.throwTestError()(F) See manual testing steps in #20495 to validate that PR