Refactor PermissionLogController tests#3937
Conversation
packages/permission-log-controller/tests/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
It seems that this controller is more complex than I had anticipated. If you look at #logRequest, #getRequestedMethods, #logResponse, and #logPermissionsHistory you will see that there is an incredible amount of logic hidden in the middleware. I don't think we are testing all of the code paths; however, in order to achieve true coverage, I think we would have to rewrite this entire file, which seems out of scope for this ticket. Given this, I tried to leave some comments that seem easier to tackle. I think the other changes you've made here make sense, though, and we can spend more time making this better in another PR (but, later on).
packages/permission-log-controller/tests/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
packages/permission-log-controller/tests/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
packages/permission-log-controller/tests/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
| it('records activity for a successful restricted method request', () => { | ||
| const controller = initController(); | ||
| const logMiddleware = initMiddleware(controller); | ||
| const req = RPC_REQUESTS.test_method(SUBJECTS.a.origin); |
There was a problem hiding this comment.
It's not completely clear in this test that test_method and eth_accounts are in the list of restricted methods that we are passing to the controller; that information is hidden and we have to consult other places in this file or other files entirely to learn this. It would make these tests easier to follow if they were more isolated and if that information which directly related to the behavior being tested was clearly visible. That said, at least RPC_REQUESTS, RESTRICTED_METHODS, and ORIGIN are located in one file, so that is good. I think we can make these tests even simpler than they are now, but at least the behavior is being tested appropriately, which is the main goal here. Perhaps so as not to bloat this PR, I'll create a separate PR which proposes more improvements I am thinking about, but I wanted to note some of my thoughts here at least.
There was a problem hiding this comment.
@mcmire good point for clarification at least we should leverage specifying restrictedMethods upon each controller initialisation to have a broader view of how it should work.
I just updated it to specify both state and restrictedMethods
packages/permission-log-controller/tests/PermissionLogController.test.ts
Show resolved
Hide resolved
packages/permission-log-controller/tests/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
packages/permission-log-controller/tests/PermissionLogController.test.ts
Outdated
Show resolved
Hide resolved
…23182) ## **Description** Following the successful migration of PermissionLogController to the core monorepo (MetaMask/core#1826), this commit removes the redundant PermissionLogController logic from the extension. All future developments and maintenance will be concentrated on the core implementation to streamline efforts and enhance functionality coherence across the platform. [](https://codespaces.new/MetaMask/metamask-extension/pull/23182?quickstart=1) ## **Related issues** - Fixes: 23181 ## **Changes** The transition of this controller from the extension repo to the core monorepo unfolded in three phases: 1. The controller was integrated into Core, with more information available at MetaMask/core#1871 2. The logic of the controller was streamlined, with additional details at MetaMask/core#3662 3. The tests for the controller were overhauled, with further information at MetaMask/core#3937 ## **Manual testing steps** These instructions outline the process for conducting manual testing locally. 1. Launch the extension from the latest development branch. 2. Navigate to the [test-dapp](https://metamask.github.io/test-dapp/). 3. Initiate the REQUEST_PERMISSIONS action from the Permissions Actions menu. 4. Open the background.html inspect window. 5. Execute the script `chrome.storage.local.get(null, ({data}) => console.log(data.PermissionLogController))` in the console. 6. Record the output from the previous step. 7. Switch to the branch named `feature/23181-remove-Permissionlogcontroller`. 8. Repeat steps 2 through 6 for this branch. 9. Compare the outputs from step 6 for both the development and feature branches. Look for matching entries in `permissionHistory` and `permissionActivityLog` from the initial run in the second run's output. Note that the log history is limited to 100 entries. ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [ ] I've included screenshots/recordings if applicable - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [ ] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] 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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
Explanation
This pull request is designed to align the tests for
PermissionLogControllerwith the guidelines outlined in our contributor documentation. The objective is to refactor these tests in accordance with best practices recommended for unit testing, as detailed in the provided documentation link.References
Changes
Checklist