Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: Refactor/9083 logger class (#9307)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR refactors the `Logger` class's `error` method to take `Error` object. While we updated the first parameter of the `error` method to be type `Error`, the business logic still handles string inputs to prevent breakage (in case there are strings still being passed). Furthermore, this PR converts places where strings were being passed into the `error` method to pass `Error` objects instead. The reason for only allowing for `Error` objects as the input is because Sentry stack traces relies on the `Error` object being created at the origin of the error instead of being reconstructed in the `Logger` class. After these changes, we should see the correct stack trace for errors reported via `Logger` class. ## **Related issues** Fixes: #9083 ## **Manual testing steps** - I'd recommend setting up your own Sentry instance for testing - Once own Sentry instance is setup, update the `sentry.properties` file with your project values - Ensure `SENTRY_DISABLE_AUTO_UPLOAD` is "false" - Hardcode an error anywhere in the app to call `Logger.error` and pass in `Error` object - In the next line, hardcode an error anywhere in the app to call `Logger.error` and pass in `string` object - Build app in release mode - Trigger the errors - Notice the two errors show in Sentry - The error reported with a string will be missing the trace of the original error - The error reported with an error will show the correct trace of the original error ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> Stack trace doesn't show origin of error since `Logger.error` was passed a `string` <img width="1061" alt="image" src="https://github.com/MetaMask/metamask-mobile/assets/10508597/44ffb904-8504-4fed-a83b-67e5756ccde0"> ### **After** <!-- [screenshots/recordings] --> Stack trace shows origin of error after `Logger.error` is passed an `Error` object <img width="1058" alt="image" src="https://github.com/MetaMask/metamask-mobile/assets/10508597/8877206b-378b-43e1-8cbe-7269329f6f05"> ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
- Loading branch information
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import Logger from '.'; | ||
| import { | ||
| captureException, | ||
| withScope, | ||
| captureMessage, | ||
| } from '@sentry/react-native'; | ||
| import { AGREED, METRICS_OPT_IN } from '../../constants/storage'; | ||
| import DefaultPreference from 'react-native-default-preference'; | ||
|
|
||
| jest.mock('@sentry/react-native', () => ({ | ||
| captureException: jest.fn(), | ||
| captureMessage: jest.fn(), | ||
| withScope: jest.fn(), | ||
| })); | ||
| const mockedCaptureException = jest.mocked(captureException); | ||
| const mockedCaptureMessage = jest.mocked(captureMessage); | ||
| const mockedWithScope = jest.mocked(withScope); | ||
|
|
||
| describe('Logger', () => { | ||
| beforeEach(() => { | ||
| DefaultPreference.get = jest.fn((key: string) => { | ||
| switch (key) { | ||
| case METRICS_OPT_IN: | ||
| return Promise.resolve(AGREED); | ||
| default: | ||
| return Promise.resolve(''); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| describe('error', () => { | ||
| it('warns if error is not defined', async () => { | ||
| const warn = jest.spyOn(console, 'warn'); | ||
| await Logger.error(undefined as any); | ||
| expect(warn).toBeCalledWith('No error provided'); | ||
| }); | ||
|
|
||
| it('skips captureException if metrics is opted out', async () => { | ||
| DefaultPreference.get = jest.fn((key: string) => { | ||
| switch (key) { | ||
| case METRICS_OPT_IN: | ||
| return Promise.resolve(''); | ||
| default: | ||
| return Promise.resolve(''); | ||
| } | ||
| }); | ||
| const testError = new Error('testError'); | ||
| await Logger.error(testError); | ||
| expect(mockedCaptureException).not.toBeCalled(); | ||
| }); | ||
|
|
||
| it('calls captureException if metrics is opted in', async () => { | ||
| const testError = new Error('testError'); | ||
| await Logger.error(testError); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error)); | ||
| }); | ||
|
|
||
| it('calls withScope if extra is passed in', async () => { | ||
| const testError = new Error('testError'); | ||
| await Logger.error(testError, 'extraMessage'); | ||
| expect(mockedWithScope).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('calls withScope if extra is passed in', async () => { | ||
| const testError = new Error('testError'); | ||
| await Logger.error(testError, 'extraMessage'); | ||
| expect(mockedWithScope).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('calls captureException when string is passed instead of Error object', async () => { | ||
| const testError = 'testError' as any; | ||
| await Logger.error(testError); | ||
| expect(mockedCaptureException).toHaveBeenCalledWith(expect.any(Error)); | ||
| }); | ||
| }); | ||
|
|
||
| describe('message', () => { | ||
| it('skips captureMessage if metrics is opted out', async () => { | ||
| DefaultPreference.get = jest.fn((key: string) => { | ||
| switch (key) { | ||
| case METRICS_OPT_IN: | ||
| return Promise.resolve(''); | ||
| default: | ||
| return Promise.resolve(''); | ||
| } | ||
| }); | ||
| await Logger.message('testMessage'); | ||
| expect(mockedCaptureMessage).not.toHaveBeenCalled(); | ||
| }); | ||
| it('calls captureMessage if metrics is opted in', async () => { | ||
| await Logger.message('testMessage'); | ||
| expect(mockedCaptureMessage).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
| }); | ||
Oops, something went wrong.