Skip to content

[base-controller] Add RestrictedControllerMessenger tests for runtime error handling#2058

Merged
MajorLift merged 4 commits intomainfrom
231117-add-restricted-controller-messenger-tests
Nov 17, 2023
Merged

[base-controller] Add RestrictedControllerMessenger tests for runtime error handling#2058
MajorLift merged 4 commits intomainfrom
231117-add-restricted-controller-messenger-tests

Conversation

@MajorLift
Copy link
Copy Markdown
Contributor

@MajorLift MajorLift commented Nov 17, 2023

Explanation

  • Previously, error handling branches for RestrictedControllerMessenger methods were not being tested on the basis of compile-time checks: "Branches unreachable with valid types".
  • However, given that these methods are being called by our JavaScript clients, we also need assurances on runtime error-handling behavior.
  • This PR adds jest tests for the previously ignored error handling branches: coverage is brought up to 100%.
  • @ts-expect-error is used to suppress the compile-time type errors to simulate a JavaScript environment.

References

Changelog

@metamask/base-controller

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

@MajorLift MajorLift added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Nov 17, 2023
@MajorLift MajorLift self-assigned this Nov 17, 2023
@MajorLift MajorLift requested a review from a team as a code owner November 17, 2023 17:04
@MajorLift MajorLift changed the title [base-controller] Add RestrctedControllerMessenger tests for runtime error handling [base-controller] Add RestrictedControllerMessenger tests for runtime error handling Nov 17, 2023
@MajorLift MajorLift force-pushed the 231117-add-restricted-controller-messenger-tests branch from 1a1ca55 to caa5c6d Compare November 17, 2023 17:12
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.

One thing, but otherwise looks good.

Comment on lines +303 to +306
// @ts-expect-error suppressing to test runtime error handling
restrictedControllerMessenger.registerActionHandler(
'CountController:count',
);
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.

Doesn't registerActionHandler take two arguments? Should the // @ts-expect-error be added above the action not above the method call?

Suggested change
// @ts-expect-error suppressing to test runtime error handling
restrictedControllerMessenger.registerActionHandler(
'CountController:count',
);
restrictedControllerMessenger.registerActionHandler(
// @ts-expect-error suppressing to test runtime error handling
'CountController:count',
() => {
// do nothing
}
);

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.

Good point! Fixed here 42a614e

Co-authored-by: Elliot Winkler <elliot.winkler@gmail.com>
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.

Great!

@MajorLift MajorLift merged commit bb7f605 into main Nov 17, 2023
@MajorLift MajorLift deleted the 231117-add-restricted-controller-messenger-tests branch November 17, 2023 23:36
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.

[base-controller] Add RestrictedControllerMessenger tests for runtime error handling

2 participants