-
-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(assets-controllers): Simplify TokenDetectionController
tests
#3744
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
6d24b1a
to
07da387
Compare
262b25e
to
000b301
Compare
946694b
to
665f73c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one suggestion, but otherwise looks good.
@@ -157,58 +145,6 @@ function buildTokenDetectionControllerMessenger( | |||
} | |||
|
|||
describe('TokenDetectionController', () => { | |||
let tokenDetection: TokenDetectionController; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at all this code go! 👋🏻
[sampleTokenA.address]: new BN(1), | ||
}); | ||
await tokenDetection.detectTokens(); | ||
describe('enabled', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using:
describe('enabled', () => { | |
describe('when the "disabled" option is false', () => { |
or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done in e80e936
The `TokenDetectionController` tests have been refactored to use mocks instead of real controller instances, and to use the `withController` pattern to simplify controller setup and ensure that polling has stopped after each test. A `getDefaultTokenListState` function has been added to the `TokenListController` to enable us to use that default state in tests. Relates to #3708
665f73c
to
0b9bede
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Explanation
The
TokenDetectionController
tests have been refactored to use mocks instead of real controller instances, and to use thewithController
pattern to simplify controller setup and ensure that polling has stopped after each test.A
getDefaultTokenListState
function has been added to theTokenListController
to enable us to use that default state in tests.One test was added to the
TokensController
tests to prevent a coverage drop (it was previously being tested indirectly via theTokenDetectionController
tests).References
Relates to #3708
Changelog
@metamask/assets-controllers
Added
getDefaultTokenListState
function to `TokenListControllerChecklist