chore: improve Engine tests stability

[Nodonisko]

Description

Previously only some tests called destroyEngine method after each test and that caused that tests actually relied on running in order etc. Now each tests destroys engine instance so it's much more stable.

I still have one more failing test but maybe I should just update that fixture? I am not sure.

Changelog

CHANGELOG entry:

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

After

Pre-merge author checklist

• I’ve followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards.
• I've completed the PR template to the best of my ability
• I’ve included tests if applicable
• I’ve documented my code using JSDoc format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

• I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

[github-actions]

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.

[tommasini]

hey @Nodonisko I think you've just found a bug, that initializing the AccountTrackerController with an empty state, it's not allowing it to initialize as it intends to do IMO. I will double check that, but doesn't make sense to me the default state be an empty object instead of getting the network from Network Controller

[Nodonisko]

@tommasini Thanks for feedback, so correct fix would to put some default network there instead of empty object?

[tommasini]

@Nodonisko The team responsible for addressing the issue was notified, feel free to address the test to the expected behaviour for now accountsByChainId: {}!
Thanks Nodonisko!

cc: @salimtb

[Cal-L]

@Nodonisko Thanks for looking into this. Just bouncing thoughts on your question about the failing unit test for non matching AccountTrackerController data - I'm looking at the controller repo code and by default, the state should resemble the expected state that the test is showing. I would consider this a real test failure if that is being overridden to an empty object. @tommasini It looks like that data is just being wiped right?

[tommasini]

Yeah, it seems that when I'm running the unit test on latest main right now, locally, I have the same error. I've talked with @salimtb and @salimtb confirmed that the initialization of AccountsTrackerController is wrong, and will fix it. The issue is here

I was suggesting to change the unit test since it's actually showing the actual result that is happening right now in Engine, because we are overriding if initial state is undefined

[Nodonisko]

@tommasini I hotfixed that failing test