Open Bug 1316200 Opened 8 years ago Updated 2 years ago

Make mochitests more flexible in what tests each executes

Categories

(Core :: WebRTC, defect, P3)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox52 --- wontfix
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: drno, Unassigned)

References

Details

Attachments

(1 file)

Bug 1316200: more flexability and several fixes.
58 bytes, text/x-review-board-request
jib
: review+
Details 
      No description provided.
backlog: --- → webrtc/webaudio+
Rank: 25
Depends on: 1316765
Depends on: 1320150
Comment on attachment 8808859 [details]
Bug 1316200: more flexability and several fixes.

https://reviewboard.mozilla.org/r/91592/#review95570

Need to fix redefinition problem on try. Otherwise lgtm with nits. Would like to see it again.

::: dom/media/tests/mochitest/dataChannel.js:176
(Diff revision 3)
> -    var options = {};
> +    var options = {
> +      sourceChannel: test.pcRemote.dataChannels[0],
> +      targetChannel: test.pcLocal.dataChannels[0]
> +    };
>      options.bufferedAmountLowThreshold = 64*1024;

Maybe pick same way to initialize all three options?

::: dom/media/tests/mochitest/head.js:10
(Diff revision 3)
> +const isAndroid = !!navigator.userAgent.includes("Android");
> +const isWinXP   = navigator.userAgent.indexOf("Windows NT 5.1") != -1;

These are showing up as redefinitions in your try in comment 7. Probably means you can just remove them.

::: dom/media/tests/mochitest/head.js:375
(Diff revision 3)
>  
>  function runTestWhenReady(testFunc) {
>    setupEnvironment();
>    return testConfigured.then(options => testFunc(options))
>      .catch(e => {
> -      ok(false, 'Error executing test: ' + e +
> +      if (typeof e != 'undefined') {

Nit: I think

    if (e) {

should suffice here. Rejecting with 0 or "" is no better than rejecting with undefined IMHO. Applies throughout.

::: dom/media/tests/mochitest/head.js:381
(Diff revision 3)
> +         ok(false, 'Error execution test: ' + e +
> -        ((typeof e.stack === 'string') ?
> +            ((typeof e.stack === 'string') ?
> -        (' ' + e.stack.split('\n').join(' ... ')) : ''));
> +             (' ' + e.stack.split('\n').join(' ... ')) : ''));
> +      } else {
> +         ok(false, 'Caught exception without error object;' +
> +           ' dont use Promise.reject() without a reason string');

Maybe: 'don't use Promise.reject() without a new Error("some reason")'

::: dom/media/tests/mochitest/pc.js:32
(Diff revision 3)
>    "have-local-pranswer": ["stable", "closed", "have-local-pranswer"],
>    "closed": []
>  }
>  
> -var makeDefaultCommands = () => {
> -  return [].concat(commandsPeerConnectionInitial,
> +
> +var optionsToCmds = (options) => {

Redundant () on single arg. Applies throughout.

::: dom/media/tests/mochitest/pc.js:46
(Diff revision 3)
> +var addOptionalsToCmds = (cmds, options) => {
> +  return cmds.concat(optionsToCmds(options));
> +};
> +
> +var addOptionalsToChain = (chain, options) => {
> +  chain.append(optionsToCmds(options));
> +};

One-liner functions without reuse. Please inline.

::: dom/media/tests/mochitest/pc.js:54
(Diff revision 3)
> +
> +var addOptionalsToChain = (chain, options) => {
> +  chain.append(optionsToCmds(options));
> +};
> +
> +var addRenegotiation = (chain, options, pre, post) => {

When a function starts to take four args I find it hard to remember what they all are, and what their order is.

Have you considered passing in `test`, or even making addRenegotiation a method on PeerConnectionTest? Then instead of:

    addRenegotiation(test.chain, test.testOptions, [...], [...])

you'd have:

    test.addRenegotiation([...], [...])

Thought I'd mention it since you're already touching this everywhere.

::: dom/media/tests/mochitest/pc.js:98
(Diff revision 3)
>  function PeerConnectionTest(options) {
>    // If no options are specified make it an empty object
>    options = options || { };
> -  options.commands = options.commands || makeDefaultCommands();
> +  options.commands = options.commands || makeDefaultCommands(options);

While you're here, maybe make options a default argument. i.e.

    fuction PeerConnectionTest(options = {}) {

Then we can remove this thing:

    options = options || {};

Wouldn't normally mention it, but this seems like a cleanup bug, so maybe it's a good time to do it?

::: dom/media/tests/mochitest/pc.js:299
(Diff revision 3)
>      source.onbufferedamountlow = function() {
>        bufferlow_fired = true;
>      };
>    }
>  
> -  return new Promise(resolve => {
> +  return new Promise((resolve, reject) => {

Unnecessary change, as reject is unused.

::: dom/media/tests/mochitest/pc.js:534
(Diff revision 3)
> +      }
> +      })

wrong indent

::: dom/media/tests/mochitest/test_ondevicechange.html:44
(Diff revision 3)
>  
>  var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p});
>  
>  var videoTracks;
>  
> -SimpleTest.requestCompleteLog();
> +//SimpleTest.requestCompleteLog();

Remove commented-out code, or add a comment on why it's there.

::: dom/media/tests/mochitest/test_peerConnection_addDataChannelNoBundle.html:15
(Diff revision 3)
>    runNetworkTest(function (options) {
>      options = options || { };
>      options.bundle = false;
> +    options.enableIceChecks = true;

I'd take the opportunity to introduce default args throughout here, since you're touching the options stuff in every file already:

    runNetworkTest(function(options = {}) {

That should make the options setting in every file look smoother.

::: dom/media/tests/mochitest/test_peerConnection_addSecondAudioStream.html:17
(Diff revision 3)
>    });
>  
>    var test;
>    runNetworkTest(function (options) {
>      test = new PeerConnectionTest(options);
> -    addRenegotiation(test.chain,
> +    dump("test.options: " + JSON.stringify(test.options) + "\n");

Debug cruft?

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:34
(Diff revision 3)
>  }
>  
>  var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p});
>  var test;
>  
> -runNetworkTest(options =>
> +runNetworkTest((options) => {

No need to add () here

::: dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html:16
(Diff revision 3)
> -  var test;
> +  var pushPrefs = (...p) => new Promise(r => SpecialPowers.pushPrefEnv({set: p}, r));
> -  var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p});

SpecialPowers.pushPrefEnv() returns a promise [1], so this change is unnecessary.

[1] https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/testing/specialpowers/content/specialpowersAPI.js#1090

::: dom/media/tests/mochitest/test_peerConnection_simulcastOffer.html:26
(Diff revision 3)
>      var receiver = receivers[0];
>  
>      SpecialPowers.wrap(pc._pc).mozSelectSsrc(receiver, index);
>    }
>  
> -  runNetworkTest(() =>
> +  runNetworkTest((options) => {

Please use

    runNetworkTest((options = {}) {

here since you're actually changing this.
Attachment #8808859 - Flags: review?(jib) → review-
Comment on attachment 8808859 [details]
Bug 1316200: more flexability and several fixes.

https://reviewboard.mozilla.org/r/91592/#review95570

> These are showing up as redefinitions in your try in comment 7. Probably means you can just remove them.

I'm pretty sure the try run form comment 7 was not the same code as here in mozreview. And I fixed that issue already.
Will drop it and will check if it comes up again in the try run for the revision.
Looks like try got worse. Want to update before review?
Comment on attachment 8808859 [details]
Bug 1316200: more flexability and several fixes.

https://reviewboard.mozilla.org/r/91592/#review98428

Lgtm, just typos and same nit over and over.

I know I asked for default arg instead of 

    options = options || {};

and I definitely think that's an improvement. However, looking at all these:

    runNetworkTest(function(options = {}) {

it makes me wonder: Why can't runNetworkTest provide options here as an invariant instead? Just sayin'.

::: dom/media/tests/mochitest/head.js:381
(Diff revisions 3 - 5)
>           ok(false, 'Error execution test: ' + e +
>              ((typeof e.stack === 'string') ?
>               (' ' + e.stack.split('\n').join(' ... ')) : ''));
>        } else {
>           ok(false, 'Caught exception without error object;' +
> -           ' dont use Promise.reject() without a reason string');
> +           ' dont use Promise.reject() without a new Error("some reason")');

don't

::: dom/media/tests/mochitest/head.js:750
(Diff revisions 3 - 5)
>           ok(false, 'Error in test execution: ' + e +
>              ((typeof e.stack === 'string') ?
>               (' ' + e.stack.split('\n').join(' ... ')) : ''));
>        } else {
>           ok(false, 'Caught exception without error object;' +
> -           ' dont use Promise.reject() without a reason string');
> +           ' dont use Promise.reject() without a new Error("some reason")');

don't

::: dom/media/tests/mochitest/pc.js:524
(Diff revisions 3 - 5)
>          ok(false, 'Error in test execution: ' + e +
>             ((typeof e.stack === 'string') ?
>              (' ' + e.stack.split('\n').join(' ... ')) : ''))
>        } else {
>          ok(false, 'Caught exception without error object;' +
> -           ' dont use Promise.reject() without a reason string');
> +           ' dont use Promise.reject() without a new Error("some reason")');

don't

::: dom/media/tests/mochitest/test_dataChannel_basicAudioVideoNoBundle.html:15
(Diff revisions 3 - 5)
> -runNetworkTest(function (options) {
> +runNetworkTest(function(options = {}) {
>    options = options || { };

Remove second line.

::: dom/media/tests/mochitest/test_peerConnection_addSecondAudioStreamNoBundle.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_addSecondVideoStreamNoBundle.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioRequireEOC.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || {};

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideo.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || {};

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoNoBundleNoRtcpMux.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoNoRtcpMux.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_insertDTMF.html:55
(Diff revisions 3 - 5)
> -runNetworkTest((options) => {
> +runNetworkTest(function(options = {}) {
>    options = options || {};

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_noTrickleOfferAnswer.html:16
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || {};

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html:34
(Diff revisions 3 - 5)
> -runNetworkTest((options) => {
> +runNetworkTest(function(options = {}) {
>    options = options || {};

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_restartIceNoBundle.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_restartIceNoBundleNoRtcpMux.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_restartIceNoRtcpMux.html:15
(Diff revisions 3 - 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_setParameters.html:63
(Diff revisions 3 - 5)
> -runNetworkTest((options) => {
> +runNetworkTest(function(options = {}) {
>    options = options || {};

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioDynamicPtMissingRtpmap.html:15
(Diff revision 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioPcmaPcmuOnly.html:15
(Diff revision 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicAudioVideoNoBundle.html:14
(Diff revision 5)
> -  runNetworkTest(options => {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_basicH264Video.html:15
(Diff revision 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.

::: dom/media/tests/mochitest/test_peerConnection_bug1042791.html:15
(Diff revision 5)
> -  runNetworkTest(function (options) {
> +  runNetworkTest(function(options = {}) {
>      options = options || { };

Second line redundant.
Attachment #8808859 - Flags: review?(jib) → review+
Comment on attachment 8808859 [details]
Bug 1316200: more flexability and several fixes.

https://reviewboard.mozilla.org/r/91592/#review98428

I apologize for not removing the 'options = options || {};'. Somehow that dropped of my radar before committing.

Steeplechase is/was the only case which fed options into runNetworkTest(). I would suggest we discuss the removal of remaining Steeplechase left overs in our mochitests in a separate, new bug.
(In reply to Nils Ohlmeier [:drno] from comment #14)
> Steeplechase is/was the only case which fed options into runNetworkTest(). I
> would suggest we discuss the removal of remaining Steeplechase left overs in
> our mochitests in a separate, new bug.

Filled bug 1323137 for that follow up discussion.
Mass wontfix for bugs affecting firefox 52.
status-firefox52: affected → wontfix
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
I don't have the capacity any more to work on this any time soon.
Assignee: drno → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: