Skip to content

Refactor PermissionLogController tests#3937

Merged
cryptodev-2s merged 7 commits intomainfrom
refactor-permission-log-controller-tests
Feb 22, 2024
Merged

Refactor PermissionLogController tests#3937
cryptodev-2s merged 7 commits intomainfrom
refactor-permission-log-controller-tests

Conversation

@cryptodev-2s
Copy link
Copy Markdown
Contributor

@cryptodev-2s cryptodev-2s commented Feb 20, 2024

Explanation

This pull request is designed to align the tests for PermissionLogController with 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

  • Discontinued the use of beforeEach for the majority of the tests, with exceptions made for certain tests that utilize fake timers. Now, each test individually initializes a new controller and middleware within its own scope, enhancing test isolation and clarity.
  • Split long test scenarios for more focused test cases.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@cryptodev-2s cryptodev-2s added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Feb 20, 2024
@cryptodev-2s cryptodev-2s self-assigned this Feb 20, 2024
@cryptodev-2s cryptodev-2s requested a review from a team as a code owner February 20, 2024 14:56
@cryptodev-2s cryptodev-2s changed the title refactor: remove beforeEach Refactor PermissionLogController tests Feb 20, 2024
Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Copy Markdown
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@cryptodev-2s cryptodev-2s merged commit bcd5ae0 into main Feb 22, 2024
@cryptodev-2s cryptodev-2s deleted the refactor-permission-log-controller-tests branch February 22, 2024 20:07
cryptodev-2s added a commit to MetaMask/metamask-extension that referenced this pull request Mar 1, 2024
…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.


[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-wallet-framework Deprecated: Please use `team-core-platform` instead.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor PermissionLogController tests

3 participants