Skip to content

Robustify permissions controller requestUserApproval tests#9064

Merged
rekmarks merged 4 commits intodevelopfrom
synchronous-requestUserApproval
Jul 27, 2020
Merged

Robustify permissions controller requestUserApproval tests#9064
rekmarks merged 4 commits intodevelopfrom
synchronous-requestUserApproval

Conversation

@rekmarks
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

metamaskbot commented Jul 23, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

rekmarks commented Jul 23, 2020

@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
Copy Markdown
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
Copy Markdown
Collaborator

metamaskbot commented Jul 23, 2020

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
Copy Markdown
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)
  rpc-cap@3.1.0 (#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)
  json-rpc-engine@5.2.0 (#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