MetaMask / MetaMask/metamask-mobile

Chore: refactor state export

Open
#12,642 1 comment 0 reactions 0 assignees View on GitHub
Sev2-normal team-mobile-platform
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.