Closed Bug 1346913 Opened 5 years ago Closed 5 years ago

Rewrite waitForRtpFlow in mochitests to use new async/await syntax

Categories

(Core :: WebRTC, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ng, Assigned: ng)

Details

Attachments

(1 file)

waitForRtpFlow is unnecessary complicated and it does not always cancel the retry loop. While not leading to failures it does create confusing logging.

It could be simplified significantly by using the new async/await syntax.[0]

[0] https://blog.mozilla.org/webrtc/fiddle-of-the-week-await/
Assignee: nobody → na-g
Rank: 45
Priority: -- → P3
Comment on attachment 8846785 [details]
Bug 1346913 - rewrite waitForRtpFlow using async await;

https://reviewboard.mozilla.org/r/119794/#review121784

r=me with nits. Thanks for fixing this. I see the old code was improperly silencing any errors in the setInterval code.

::: dom/media/tests/mochitest/pc.js:1427
(Diff revision 1)
> -   * @returns {Promise}
> -   *        A promise that resolves when media is flowing.
> +   * @returns {StatsReport}
> +   *        A StatsReport object with RTP stats.

An async function still returns a promise.

::: dom/media/tests/mochitest/pc.js:1449
(Diff revision 1)
>             rtp.type + " RTP packets.");
>        return nrPackets > 0;
>      };
>  
>      // Time between stats checks
> -    var retryInterval = 500;
> +    let retryInterval = 500;

const

::: dom/media/tests/mochitest/pc.js:1461
(Diff revision 1)
> -          }
> +      }
> -        });
> -      }, retryInterval);
> -    });
> +      catch (e) {
> +        warn(e);
> +      }

Just remove try/catch here, to let errors escape and kill the test if there's a codig error. We shouldn't proceed in spite of an error here I don't think.
Attachment #8846785 - Flags: review?(jib) → review+
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/266bc832c343
rewrite waitForRtpFlow using async await;r=jib
https://hg.mozilla.org/mozilla-central/rev/266bc832c343
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.