Refactor TokensController tests #4247
Conversation
mcmire
force-pushed
the
refactor-tokens-controller-tests
branch
2 times, most recently
from
fb59737 to
0884667
Compare
We want to change TokensController so that it relies on `selectedNetworkClientId` instead of `providerConfig` to know which chain is selected, and this will require updating the tests to match. However, the TokensController tests are written in such a way that makes updating them difficult. This commit rewrites each test to use the `withController` pattern that we've adopted for other controller tests. It also removes usage of `beforeEach` so that each test is isolated. Note that this commit isn't a complete refactor of the tests; there is more work we could do to make them easier to maintain, such as removing a reliance on ApprovalController and using `describe` to group tests by method. This step can be done later. In addition, this commit also adds some NetworkController-related and BaseController-related test helpers which the TokensController tests use. These helpers aren't defined in the public part of their respective packages, but in the private area. The TokensController tests can import them directly because they are private themselves, and therefore don't need to adhere to the same importing rules that the (public) implementation files do. Finally, this commit also exposes `BuiltInNetworkClientId` and `CustomNetworkClientId`. These are used in the aforementioned test helpers but are also useful on their own.
mcmire
force-pushed
the
refactor-tokens-controller-tests
branch
from
0884667 to
26e38f9
Compare
| * requests. | ||
| * @returns The fake network client. | ||
| */ | ||
| function buildFakeNetworkClient({ |
There was a problem hiding this comment.
This pattern of including test helpers specific to a controller inside the package for that controller is new. Previously we might have just put it within the test file that needed these helpers. But by putting them here, we define them once and then any package in the monorepo can use them.
| }; | ||
|
|
||
| const getNetworkClientById = buildMockGetNetworkClientById({ | ||
| // Since we access these networks so often in these tests, register |
There was a problem hiding this comment.
We could put these defaults directly in buildMockGetNetworkClientById. We do end up needing these in other test files. Thoughts? Would that be too much magic?
There was a problem hiding this comment.
Hmm. Seems fine to build them directly into buildMockGetNetworkClientById to me - I don't think it's too much magic.
| 'TokenListController:stateChange', | ||
| ], | ||
| }); | ||
| tokensController = new TokensController({ |
There was a problem hiding this comment.
Yikes, so many controllers. Really glad to see this get deleted!
There was a problem hiding this comment.
Well, I didn't want to go so far as to remove ApprovalController — that's still there. But, the TransactionController tests don't use a real ApprovalController, so we could copy the patterns we have there, or extract the helper functions so that we can reuse them here.
...and allow for setting custom network client configuration properties to `undefined`.
Explanation
We want to change TokensController so that it relies on
selectedNetworkClientIdinstead ofproviderConfigto know which chain is selected, and this will require updating the tests to match. However, the TokensController tests are written in such a way that makes updating them difficult.This commit rewrites each test to use the
withControllerpattern that we've adopted for other controller tests. It also removes usage ofbeforeEachso that each test is isolated.Note that this commit isn't a complete refactor of the tests; there is more work we could do to make them easier to maintain, such as removing a reliance on ApprovalController and using
describeto group tests by method. This step can be done later.In addition, this commit also adds some NetworkController-related and BaseController-related test helpers which the TokensController tests use. These helpers aren't defined in the public part of their respective packages, but in the private area. The TokensController tests can import them directly because they are private themselves, and therefore don't need to adhere to the same importing rules that the (public) implementation files do.
Finally, this commit also exposes
BuiltInNetworkClientIdandCustomNetworkClientId. These are used in the aforementioned test helpers but are also useful on their own.Tip
The diff for the tests will be easier to read if you hide whitespace.
References
This is a prerequisite to #4185.
Changelog
@metamask/network-controllerBuiltInNetworkClientIdandCustomNetworkClientId.Checklist