Closed Bug 1329045 Opened 7 years ago Closed 7 years ago

Rewrite mochitest test_use_with_hsts.html to use async/await

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(2 files)

The mochitest test_use_with_hsts.html does some loads inside of an iframe, and as a result, it involves some callback hell.  (It's written as a chain of callbacks.)

Now that we have async/await (in Firefox 52 according to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#Browser_compatibility ), we can rewrite this code to be more direct & avoid the ugly callback chaining.
So this bug is basically condensing code patterns like the following:
>  function() Step1() {
>   iframe.onload = Step2;
>   iframe.src = someURI;
>  }
>  function() Step2() {
>   // inspect iframe
>   // ...
>  }

...to this:
>  await LoadIframeAsync(someURI);
>  // inspect iframe  

...using my provided LoadIframeAsync helper and the magic of Async/Await.  One note: some online sources recomment wrapping "await" expressions in try{}catch{}, since await will throw if the promise is rejected.  I haven't bothered with that here, since:
 (a) the only promise in this test (the one returned by LoadIframeAsync) is never rejected under any conditions -- it's only resolved.
 (b) if it happens to be rejected, then an exception is fine -- it'll just trigger a test failure, which is what we want anyway. (If this were common then we could add try/catch for more graceful reporting, but I'm not expecting it to be possible in the first place.)
Comment on attachment 8824268 [details]
Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html.

https://reviewboard.mozilla.org/r/102794/#review103248

::: dom/svg/test/test_use_with_hsts.html:70
(Diff revision 1)
>  
> -  // TEST CODE BEGINS HERE.
> -  // Execution basically proceeds top-to-bottom through the functions
> -  // from this point on, via a chain of iframe onload-callbacks.
> -  function runTest() {
> +  // Convenience helper which makes |iframe| load the given |uri|. Returns
> +  // a promise that resolves when the load completes. This makes it handy to
> +  // use with 'await', to avoid onload callback hell.
> +  async function LoadIframeAsync(uri) {
> +    return new Promise(function(resolve, reject) {

Instead of `function(resolve, reject) {`, you can probably just do `resolve => {` given you are not using `reject` at all.

::: dom/svg/test/test_use_with_hsts.html:72
(Diff revision 1)
> +      iframe.onload = function() {
> +        iframe.onload = null; // (only invoke this callback for this one load)
> +        resolve();
> +      };

You can do `iframe.addEventListener('load', resolve, {once: true});` instead to avoid resetting the event handler.

::: dom/svg/test/test_use_with_hsts.html:110
(Diff revision 1)
> -    // Point iframe at insecure (HTTP) version of testcase (which should get
> -    // automatically upgraded to secure (HTTPS) under the hood), & wait for
> +    // Load insecure (http) version of testcase:
> +    await LoadIframeAsync(insecureURI);

The piece of text inside the parens ("which should get ... under the hood") doesn't seem to related to this simplification, so it probably shouldn't be removed.
Attachment #8824268 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8824269 [details]
Bug 1329045 part 2: Convert some global variables to local variables, in test_use_with_hsts.html.

https://reviewboard.mozilla.org/r/102796/#review103254
Attachment #8824269 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8824268 [details]
Bug 1329045 part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html.

https://reviewboard.mozilla.org/r/102794/#review103248

> Instead of `function(resolve, reject) {`, you can probably just do `resolve => {` given you are not using `reject` at all.

Done -- that does work & makes this more compact. Thanks!

> You can do `iframe.addEventListener('load', resolve, {once: true});` instead to avoid resetting the event handler.

Indeed! Thanks, fixed.

> The piece of text inside the parens ("which should get ... under the hood") doesn't seem to related to this simplification, so it probably shouldn't be removed.

Good point -- I've added it back. (I also reverted a few other needless wording tweaks, too.)
(Note: my "v2" of these patches (comment 6) included some bonus commit-message cruft left over from "qfold".  I noticed that just after pushing, & cleaned it up in "v3".)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa001860e4ab
part 1: Use async/await to simplify callbacks in mochitest test_use_with_hsts.html. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/1a3de277a1fc
part 2: Convert some global variables to local variables, in test_use_with_hsts.html. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/aa001860e4ab
https://hg.mozilla.org/mozilla-central/rev/1a3de277a1fc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.