MetaMask / MetaMask/metamask-mobile
Chore: refactor state export
- Dominant language
- TypeScript
- Stars
- 3k
- Forks
- 1.7k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 669
Description
### What is this about?
State export feature in app/util/logs/index.ts and app/util/logs/index.test.ts are not following good practices and it makes it hard and complex to test.
Long term this will increase the risk of tests being hard to understand and lead to untested parts and possible data being included in export when they should not.
### Scenario
_No response_
### Design
_No response_
### Technical Details
- Two exported functions `downloadStateLogs` and `generateStateLogs`
- `downloadStateLogs` calls `generateStateLogs` internally
- Test is testing `generateStateLogs` output
- Test is testing `downloadStateLogs` output (and so testing also `generateStateLogs` again)
- tests can’t be isolated as both functions are in the same module
- No spying possible on `generateStateLogs` because the spy would be on the module export ref when `downloadStateLogs` uses the module internal ref. So your spy behave like it’s never called.
- We test `generateStateLogs` that should be private!
- `generateStateLogs` is exported only for test purpose! (avoid doing things only for test purpose, and here it's really not hard to prevent)
- the only way to properly test this without refactoring is a complex mock that increases risks of misunderstanding and not properly testing things (risk of testing the mock instead of the real code)
- The real solution is a refactoring of the original code either to only test public code (but requires to catch the downloaded base 64 encoded data and decode it, see #12621, not the best) or to export `generateStateLogs` as a util function and test is separately and be able to spy on it in the `downloadStateLogs` run.
The refactoring should make each part under test testable in an isolated way. This export feature is not complex enough to say it can't be refactored, so we should refactor it. All the code base will benefit from it and this provides a good example of how to write testable code.
### Threat Modeling Framework
- with this refactoring and making it better tested, we lower the risk of data being added in the export when it should not.
### Acceptance Criteria
- all code is properly tested without complicated workaround
- no artificial export for test use
- unit tests properly scoped to the code under test
- all tests pass
- not change on the feature behaviour
### Stakeholder review needed before the work gets merged
- [x] Engineering (needed in most cases)
- [ ] Design
- [ ] Product
- [ ] QA (automation tests are required to pass before merging PRs but not all changes are covered by automation tests - please review if QA is needed beyond automation tests)
- [x] Security
- [ ] Legal
- [ ] Marketing
- [ ] Management (please specify)
- [ ] Other (please specify)
### References
See also #12621
Contributor guide
Research direction
Start with app/util/logs/index.ts and app/util/logs/index.test.ts, focusing on downloadStateLogs and generateStateLogs and the testing concerns described in the issue. Refactor the structure so each part can be tested in isolation without an artificial test-only export or complicated mocks. Run the relevant tests and confirm all tests pass without changing feature behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- testing
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 68/100