Skip to content
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.

Sign up for GitHub

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

Jump to bottom

TokenDetectionController: Write comprehensive unit tests #1615

Open
mcmire opened this issue Aug 18, 2023 · 4 comments
Open

TokenDetectionController: Write comprehensive unit tests #1615

mcmire opened this issue Aug 18, 2023 · 4 comments
Labels
team-wallet-framework

Comments

@mcmire
Copy link
Contributor

mcmire commented Aug 18, 2023 •
edited
Loading

We should review the tests for TokenDetectionController and ensure that the functionality and edge cases are well-covered by tests and that there are no duplicative or unnecessary tests. A new contributor should be able to use the tests as a guideline for what the controller does.

There were API changes made recently to TokenDetectionController, so we should focus on backfilling tests for those especially. This is no longer the case, but see comments below for updates.
@mcmire mcmire mentioned this issue Aug 18, 2023
Open
@mcmire mcmire mentioned this issue Oct 11, 2023
Closed
@MajorLift MajorLift mentioned this issue Jan 17, 2024
Merged
3 tasks
@MajorLift

This comment was marked as outdated.

Sign in to view
@desi
Copy link
Contributor

desi commented Jan 22, 2024

Hey team! Please add your planning poker estimate with Zenhub @Gudahtt @kanthesha @MajorLift @mcmire @mikesposito

@MajorLift MajorLift mentioned this issue Feb 6, 2024
Merged
3 tasks
MajorLift added a commit that referenced this issue Feb 9, 2024
@MajorLift
[token-detection-controller] Add tests for API changes from merge wit…
58d2bd7 
…h `DetectTokensController` (#3867)

## Explanation

- This PR adds unit tests for the token-detection-controller API changes
implemented in #3775.
- The following changes are covered in the new tests:
  - Don't detect if keyring-controller `isUnlocked` state is false.
- Subscribe to `KeyringController:unlock`, `KeyringController:lock`
events
  - Subscribe to `AccountsController:selectedAccountChange` event
- Detect tokens using `@metamask/contract-metadata` static token list if
on mainnet and `useTokenDetection` is false.
  - Call `trackMetaMetricsEvent` for every detected token.
- The aim of this PR isn't to achieve 100% test coverage for
token-detection-controller. That will be the goal of follow-up ticket
#1615

## References

- Closes #3626 
- Follows from:
  - #3775
  - #3690
- Followed by #1615 
  
## Changelog

N/A

## 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
@MajorLift
Copy link
Contributor

MajorLift commented Feb 20, 2024 •
edited
Loading

The scope of this ticket no longer includes the API changes caused by the consolidation with DetectTokensController.

However, it needs to update the tests in the extension detect-tokens.test.js file to align with the current controller API, and integrate these test cases into the core TokenDetectionController.test.ts file. This effort will include removing redundant or obsolete tests.

@MajorLift
Copy link
Contributor

@MajorLift MajorLift mentioned this issue Mar 10, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
team-wallet-framework
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@mcmire @desi @MajorLift