-
-
Notifications
You must be signed in to change notification settings - Fork 187
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 NftDetectionController
tests
#3742
Conversation
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 a couple of comments, but otherwise looks good.
}); | ||
mockNfts.reset(); | ||
|
||
await controller.start(); |
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.
Is this necessary? It seems that the controller self-starts when useNftDetection
is 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.
Hmm. Good point. The way polling is setup right now doesn't really make sense. Not only does it auto-start, it also fails to start at all if the current network isn't Ethereum Mainnet (even if you later switch to Ethereum Mainnet).
For now I'll remove these unnecessary steps, but I'll follow up later with a PR that properly separates the disabled
state from the useNftDetection
setting, which will make this API make sense again.
Addressed in ac26f42
provider: jest.fn(), | ||
blockTracker: jest.fn(), |
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.
Hmm, these aren't functions. Could we create a FakeProvider and then use to construct a block tracker? If that's too much trouble, that's fine, but just a thought.
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, though I'm inclined to leave this alone for now because this mock is pre-existing. This PR just moves it into a helper function.
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.
Okay, no problem.
The `NftDetectionController` tests have been refactored to use mocks rather than full `AssetsContractController`, `PreferencesController`, and `NftController` references. The `withController` pattern has been introduced as well, to simplify the controller setup and ensure all polling has stopped after each test. A few tests were found to be testing functionality that was internal to the `NftController`, so they have been removed. Coverage for the `NftDetectionController` remains unchanged. Relates to #3708
9ccc441
to
9de452d
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.
LGTM!
Explanation
The
NftDetectionController
tests have been refactored to use mocks rather than fullAssetsContractController
,PreferencesController
, andNftController
references. ThewithController
pattern has been introduced as well, to simplify the controller setup and ensure all polling has stopped after each test.A few tests were found to be testing functionality that was internal to the
NftController
, so they have been removed. Coverage for theNftDetectionController
remains unchanged.References
Relates to #3708
Changelog
@metamask/assets-controllers
Added
getDefaultNftState
function to theNftController
Checklist