fix: Prevent Engine.destroy test intermittent failure #12765
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. |
mcmire
changed the title
mcmire
added
the
No QA Needed
mcmire
force-pushed
the
resolve-intermittent-engine-destroy-test
branch
2 times, most recently
from
db43ba3 to
326dd67
Compare
Some unit tests for the Engine do not properly wait for the NetworkController provider to be fully initialized after initializing the engine before attempting to destroy it. This prevents NetworkController's `lookupNetwork` from functioning because it removes an event listener that NetworkController had previously created, causing an error. This commit patches NetworkController to not throw if the controller is destroyed before `lookupNetwork` has a chance to remove the event listener. It also updates Engine to create a promise that resolves when NetworkController's provider finishes initialization. This promise is then awaited in `Engine.destroyEngineInstance`. Finally this commit also ensures that any calls to `Engine.destroyEngineInstance` in tests are properly awaited.
mcmire
force-pushed
the
resolve-intermittent-engine-destroy-test
branch
from
326dd67 to
78cab4a
Compare
| let networkChanged = false; | ||
| const listener = () => { | ||
| networkChanged = true; | ||
| - this.messagingSystem.unsubscribe('NetworkController:networkDidChange', listener); |
There was a problem hiding this comment.
I plan on updating @metamask/network-controller with this change in another PR so we don't need this patch long-term.
There was a problem hiding this comment.
Could we handle this upstream instead, and fix it here by updating? It looks like we're on the latest major version, so it should be relatively easy to do
There was a problem hiding this comment.
That would be better for sure. I don't have a lot of time today and just wanted to get this person unblocked for the time being or at least give them a solution they can use to complete their draft PR, but I can make a change upstream later today or possibly tomorrow.
There was a problem hiding this comment.
@mcmire , don't stress yourself over this! While I really appreciate the effort to unblock me (🫶), this was only a draft PR to test a core change that I'm planning to merge later this week. The extension draft PR in itself will be closed. I'd rather go with the classic update flow instead of adding a patch as well, so let's take our time to do this :)
There was a problem hiding this comment.
I've submitted a PR to fix this in NetworkController here: MetaMask/core#5116
Why does it matter if |
Fair enough, and that was the conclusion I eventually came to, hence the NetworkController patch. |
What I meant was "why is this resulting in an error?". That is not clear from looking at the patch. Though even in terms of just the patch changes, it seems that it's intended to ensure the operation succeeds. |
|
|
@Gudahtt It's resulting in an error because the messenger event that it's trying to unsubscribe has been removed (because the controller has been destroyed). There is no real reason to continue if the controller is destroyed, you are right. We should probably just return instead. |
|
I see, so it's causing the test to fail because it's an unhandled promise rejection? I was having trouble following how this caused the test failure. But it looks like Jest doesn't identify unhandled Promise rejections as such, so that could explain it. |
## Explanation This PR moves the logic related to account syncing from `UserStorageController` to separated files in the `account-syncing` folder. It also improves test coverage related to account syncing to 100%. ## References Related to #4923 - Extension draft PR: MetaMask/metamask-extension#29316 - CI & E2E Passes ✅ - I needed to add the new `isAccountSyncingInProgress` state key at various places to make it pass CI (as expected) - Account syncing is enabled on extension - Mobile draft PR: MetaMask/metamask-mobile#12755 - CI passes BUT ✅ ☝️ - I needed to add the new isAccountSyncingInProgress state key at various places to make it pass CI (as expected) - UTs were broken. It seems to be linked to the latest version of `NetworkController` and its `NetworkController:networkDidChange` event. This latest version is requested by our controller as part of the upcoming network syncing feature. - This will require a separate PR that bumps `NetworkController` to `v22.1.1` (or another incriminated dependency TBD) - This has started to be addressed here: MetaMask/metamask-mobile#12765 - This was fixed by this PR: #5116 - In any case, account syncing is **NOT** enabled on mobile yet ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/profile-sync-controller` - **CHANGED**: moved account syncing logic to its own files - **BREAKING**: added a new `isAccountSyncingInProgress` state key ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
## Explanation This PR moves the logic related to account syncing from `UserStorageController` to separated files in the `account-syncing` folder. It also improves test coverage related to account syncing to 100%. ## References Related to #4923 - Extension draft PR: MetaMask/metamask-extension#29316 - CI & E2E Passes ✅ - I needed to add the new `isAccountSyncingInProgress` state key at various places to make it pass CI (as expected) - Account syncing is enabled on extension - Mobile draft PR: MetaMask/metamask-mobile#12755 - CI passes BUT ✅ ☝️ - I needed to add the new isAccountSyncingInProgress state key at various places to make it pass CI (as expected) - UTs were broken. It seems to be linked to the latest version of `NetworkController` and its `NetworkController:networkDidChange` event. This latest version is requested by our controller as part of the upcoming network syncing feature. - This will require a separate PR that bumps `NetworkController` to `v22.1.1` (or another incriminated dependency TBD) - This has started to be addressed here: MetaMask/metamask-mobile#12765 - This was fixed by this PR: #5116 - In any case, account syncing is **NOT** enabled on mobile yet ## Changelog <!-- If you're making any consumer-facing changes, list those changes here as if you were updating a changelog, using the template below as a guide. (CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or FIXED. For security-related issues, follow the Security Advisory process.) Please take care to name the exact pieces of the API you've added or changed (e.g. types, interfaces, functions, or methods). If there are any breaking changes, make sure to offer a solution for consumers to follow once they upgrade to the changes. Finally, if you're only making changes to development scripts or tests, you may replace the template below with "None". --> ### `@metamask/profile-sync-controller` - **CHANGED**: moved account syncing logic to its own files - **BREAKING**: added a new `isAccountSyncingInProgress` state key ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes
Description
Some unit tests for the Engine do not properly wait for the NetworkController provider to be fully initialized after initializing the engine before attempting to destroy it. This prevents NetworkController's
lookupNetworkfrom functioning because it removes an event listener that NetworkController had previously created, causing an error.This commit resolves the issue by patching NetworkController to not throw if the controller is destroyed before
lookupNetworkhas a chance to remove the event listener.Related issues
This issue was found recently when making another, unrelated update.
Manual testing steps
All unit tests should pass, particularly the test in
Engine.test.tscalled "calling Engine.destroy deletes the old instance".Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist