Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Robustify permissions controller requestUserApproval tests #9064

Merged
merged 4 commits into from
Jul 27, 2020
Merged

Robustify permissions controller requestUserApproval tests #9064

merged 4 commits into from
Jul 27, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Jul 23, 2020 •
edited
Loading

This PR removes some reliance on timing artifacts in permissions controller tests that rely on requestUserApproval.

  • Stop needlessly mocking requestUserApproval in permissions controller tests
    • Let's actually test the underlying function!
  • Await the setting of pending user approval requests in permissions controller tests, instead of just hoping that they are set are set by the time we check for them

@rekmarks rekmarks requested a review from a team as a code owner July 23, 2020 17:09
@rekmarks rekmarks changed the title Make requestUserApproval synchronous Make requestUserApproval synchronous, robustify related tests Jul 23, 2020
@rekmarks rekmarks force-pushed the synchronous-requestUserApproval branch from b120978 to 0690178 Compare July 23, 2020 17:41
@metamaskbot
Copy link
Collaborator

Builds ready [0690178]
Page Load Metrics (638 ± 20 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29108472311
domContentLoaded5737236374120
load5757266384120
domInteractive5737236364120

@Gudahtt
Copy link
Member

Gudahtt commented Jul 23, 2020

  • Makes the permissions controller requestUserApproval function synchronous and Promise-returning
    • Also, wraps the entire function body in the Promise executor
      • The body of the executor is executed synchronously (spec)
    • With this function being synchronous, it will set the pending approval state on the permissions controller before returning its promise, which makes it more testable

Isn't all of this the case with async functions as well? 🤔 The function body should be executed eagerly - that's a fundamental property of Promises in JavaScript. I don't think the change to requestUserApproval here has any effect upon timing.

@rekmarks
Copy link
Member Author

rekmarks commented Jul 23, 2020 •
edited
Loading

@Gudahtt yeah, you're right: https://jsfiddle.net/k8ch1yqb/

The only time the entire function body, and any Promise executors, won't be eagerly executed is, obviously, if await is used.

The important thing about that function is that it's either async or synchronous with its entire body wrapped by the Promise executor. I think my confusion arose because I at one point made it synchronous without wrapping the entire body in the executor, which caused problems, and a bit of uncertainty about the spec.

So, except that, the form of requestUserApproval is up to style, but the changes made to the tests are still good, and still necessary for the pending json-rpc-engine update.

I'll update the PR description. Should we leave the requestUserApproval implementation as-is, or revert it?

@rekmarks rekmarks changed the title Make requestUserApproval synchronous, robustify related tests Robustify permissions controller requestUserApproval tests Jul 23, 2020
@rekmarks
Copy link
Member Author

@Gudahtt I just went with restoring the original requestUserApproval in 518a22c, because, if it ain't broke don't fix it and so forth.

@Gudahtt
Copy link
Member

Gudahtt commented Jul 23, 2020

I think the original is subtly better anyway in the case where an error is thrown, because we'll get an async stack trace (on Chrome at least)? Not entirely sure. It's at least probably not worse.

@metamaskbot
Copy link
Collaborator

Builds ready [518a22c]
Page Load Metrics (642 ± 21 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3091492211
domContentLoaded5847276404421
load5857296424421
domInteractive5847276404421

Gudahtt
Gudahtt approved these changes Jul 27, 2020
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks merged commit f6f8e5c into develop Jul 27, 2020
@rekmarks rekmarks deleted the synchronous-requestUserApproval branch July 27, 2020 21:35
@rekmarks rekmarks mentioned this pull request Jul 27, 2020
Merged
Gudahtt added a commit that referenced this pull request Jul 30, 2020
@Gudahtt
Merge remote-tracking branch 'origin/develop' into build-sys-2
1e042f2 
* origin/develop: (582 commits)
  Use async/await for seedPhraseVerifier.verifyAccounts (#9100)
  Use async/await for getRestrictedMethods (#9099)
  Update dependencies (#9105)
  update email us to contact us (#9104)
  Improve source maps (#9101)
  Update font family globally (#9073)
  [email protected] (#9103)
  Use environment variable for production Sentry DSN (#9097)
  Only log error on first occurrence of missing substitution (#9096)
  Use mixins for typography instead of placeholder selectors (#9072)
  Update css folder structure (#9071)
  Disable Sentry in development (#9095)
  Use environment variable for MetaMetrics project ID (#9094)
  Use development metametrics project during tests (#9093)
  [email protected] (#9091)
  fixup! call initializeProvider where necessary
  call initializeProvider where necessary
  Add euclid fontface (#9018)
  fix timing-reliant network controller test
  Robustify permissions controller requestUserApproval tests (#9064)
  ...
Gudahtt pushed a commit that referenced this pull request Aug 7, 2020
@rekmarks @Gudahtt
Robustify permissions controller requestUserApproval tests (#9064)
1591ed7 
* convert requestUserApproval mock to wrapper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rekmarks @metamaskbot @Gudahtt