1119593 - Overhaul PC test infrastructure

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Martin Thomson [:mt:]]

There is a lot of flaky code in the test setup and it is hard to write new tests.

This won't fix every problem we have, but it does end up in a lot of code being deleted.

Outline:
 - use promises for everything consistently
 - use modern Javascript (with arrow functions, there are no more declarations of self)
 - rework asynchronous logic throughout
 - add a few regression tests for our callback-based functions

Comment 1

[Martin Thomson [:mt:]]

Attachment: MozReview Request: bz://1119593/mt

Comment 2

[Martin Thomson [:mt:]]

/r/2241 - Bug 1119593 - Reduce log spam on catastrophic failure
/r/2243 - Bug 1119593 - Update WebRTC tests to use promises more consistently
/r/2245 - Bug 1119593 - Update WebRTC data channel tests
/r/2247 - Bug 1119593 - Update PeerConnection tests
/r/2249 - Bug 1119593 - Update identity tests
/r/2251 - Bug 1119593 - Update gUM tests to use promises consistently
/r/2253 - Bug 1119593 - Adding test for legacy PC callback functions
/r/2255 - Bug 1119593 - Adding test for legacy navigator.mozGetUserMedia

Pull down these commits:

hg pull review -r bf18a249d41237f3b6cfafe4600269f3a2b39bc9

Comment 3

[Martin Thomson [:mt:]]

This reviewboard thing is a little screwy; I can't ask for "feedback?" easily.  Apologies for the bogus r?  This works locally, but it hasn't been rigorously vetted yet.

Comment 4

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5243436bfced

Note to self: figure out where the relevant tests are for Android and B2G.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

https://reviewboard.mozilla.org/r/2239/#review1477

Overall, I am not the person to be reviewing this.  I am not familiar with this code or set of features/tests.  Nothing jumped out at me as obviously wrong.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +    info('looking for ' + JSON.stringify(props));

this looks to be for local debugging, not for useful test output, same with the info statement two lines down.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8546282 [details]
MozReview Request: bz://1119593/mt

hmm, I commented in reviewboard, this is really not my area to be an effective reviewer, although I did see two statements which looked to be debugging related and not related to test changes.

Comment 7

[Martin Thomson [:mt:]]

:jmaher, who should I ask about https://reviewboard.mozilla.org/r/2241/ ?

Apologies for not making this clearer, I hoped that RB would help with that, but I can see now that it doesn't work that way (RB gripe #1000).  I only wanted feedback on the small piece that touches SimpleTest.  Thanks for finding my mistake though :)

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

https://reviewboard.mozilla.org/r/2241/#review1479

A few small issues here.  The concept of switching from _alreadyFinished -> _finishedCalled is just fine, I would like to be more explicit about the value of it and not treating it as true false if it is an integer.  Also there is the confusion around supressing messages >3.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
(Diff revision 1)
> +    }

I am not sure why we would call finish >1 time?  That seems like we might be doing the wrong thing.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
(Diff revision 1)
> -        if (!SimpleTest._alreadyFinished && arguments.length > 1 && arguments[1] > 0) {
> +        if (!SimpleTest._finishedCalled && arguments.length > 1 && arguments[1] > 0) {

Where is _finishedCalled defined or set to an initial value of 0 ?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

updated reviewboard feedback, not 100% excited about the patch as it is, but I like the general concept.

Comment 10

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2241/#review1489

Ack on the integer stuff.  I'll try to ensure that the value is properly initialized and checked more robustly.

It's always the case that calling finish multiple times is wrong.  We definitely need to record the second call to .finish() as an error.  The problem is that there are cases where tests continue to record results.  Some of the WebRTC tests were recording many results every second continuously when the test failed.  Finding the original fault is challenging.

The multiple records are merely a development affordance.  I chose 3 somewhat arbitrarily; I wanted to ensure that the first error was captured correctly.  And there are a few cases where a few extra messages can provide critical context that helps in debugging the issue (sometimes the second call to .finished() is the same as the first, but subsequent ones might be different).  I think that a few more won't hurt.  Do you think that 5 or 10 would be reasonable numbers?

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

ok, if we could do a better job of documenting the condition in the code, I would be happier with leaving something in place.

Yes, we will get an error on each subsequent finish() call - that is good and for tests this should yield an orange on treeherder.

Is there any value in keeping track of the finish() calls?  Will that help debug things in the future?  I am concerned that maybe we would ignore some finish messages and not be able to put context around the failure condition.  My initial thought is setting it to 5 so we can see what is going on.

Is there a bug on file for documenting which tests call finish() 3+ times?

Comment 12

[Martin Thomson [:mt:]]

:jmaher, this has only come up in local testing and development.  For me at least.  I have seen it on try pushes sometimes when I screw up, but rarely.  The problem with a local run is that I have often seen 10,000 calls to finish().  BTW, I did you a disservice by attaching that here.  I've opened bug 1119952 for this.  I'll have an updated patch ready soon.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/2239/#review1521

Monumental cleanup! \o/

Going through all this code, I couldn't help but come up with some more suggestions, but please don't let any of that delay this. We can always do more later.

Wished some of the global formatting and renaming had been in a separate patch to ease reviewing, but no biggie. 

Personal peeve: I wish we'd rename pcLocal and pcRemote to pc1 and pc2. Each one has things that are local and remote to it, which gives me point-of-view vertigo every time I go in here.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  var self = this;
> -  this.chain.onFinished = function () {
> -    self.teardown();
> -  };
> +function timerGuard(p, time, message) {
> +  var thandle;
> +  return Promise.race([
> +    p.then(r => {
> +      if (thandle) {
> +        clearTimeout(thandle);
> +      }
> +      return r;
> +    }),
> +    new Promise((resolve, reject) => (thandle = setTimeout(() => {
> +      ok(false, message);
> +      reject();
> +    }, time)))
> +  ]);

Does anything bad happen if you don't clear the timer? How about just:

var wait = ms => new Promise(r => setTimeout(r, ms));
var timeout = (p, ms) => Promise.race([p, wait(ms).then(() => {
  throw new Error("timeout after " + ms/1000 + " seconds");
})]);

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +    var sad = new Promise(resolve => (thandle = setTimeout(resolve, 60000)))
> +        .then(() => {
> +          ok(this.onAddStreamFired, this + " checkMediaTracks() timed out waiting" +
> +             " for onaddstream event to fire");
> +        });
> +    return Promise.race([ happy, sad ]);
>    },

This is the timeout pattern again in a slightly different form.

::: dom/media/tests/mochitest/templates.js
(Diff revision 1)
> +  if (!pc.isIceConnectionPending()) {
> +    dumpSdp(test);
> +    ok(false, pc + ": ICE is already in bad state: " + pc.iceConnectionState);
> +    return Promise.resolve();
> +  }

reject?

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStream.html
(Diff revision 1)
> -  <title>mozGetUserMedia Stop Video Stream</title>
> +  <title>mozGetUserMedia Stop Video Audio Stream</title>

video

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 1)
> -    [
> -      'PC_LOCAL_VERIFY_DATA_CHANNEL_STATE',
> -      function (test) {
> -        test.waitForInitialDataChannel(test.pcLocal, function() {
> -          test.next();
> +    function PC_LOCAL_VERIFY_DATA_CHANNEL_STATE(test) {
> +      return test.pcLocal.dataChannels[0].opened
> +        .then(() => {
> +          ok(true, test.pcLocal + " dataChannels[0] switched to 'open'");
> +        }, e => {
> -        }, function() {
>            ok(false, test.pcLocal + " initial dataChannels[0] failed to switch to 'open'");
> -          //TODO: use stopAndExit() once bug 1019323 has landed
> +          unexpectedEventAndFinish(this, 'timeout');

Now that this is nice and clean, it looks a bit manual that individual steps do .then(ok(true, "step succeeded"), ok(false, "step failed"), when this perhaps is something that could be done by the harness itself?

Also, could we make unexpectedEventAndFinish() be the default behavior if a step fails? If a step doesn't want that it can just .catch() the error. Then if a step uses catch just to output something before failing, then it could (re-)throw e.

::: dom/media/tests/mochitest/nonTrickleIce.js
(Diff revision 1)
>    chain.replace('PC_LOCAL_SETUP_ICE_HANDLER', [
> -    ['PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER',
> +    function PC_LOCAL_SETUP_NOTRICKLE_ICE_HANDLER(test) {

Love it!

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -    var index = this.indexOf(id);
> +    var index = this.indexOf(id) + 1;
>  
>      if (index > -1) {

Uh-oh if indexOf returns -1 here.

::: dom/media/tests/mochitest/mediaStreamPlayback.js
(Diff revision 1)
> -          onError("timeUpdate event never fired");
> +          reject("timeUpdate event never fired");

Would it be a good idea to reject with new Error("...") throughout, or throw?

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
>      SimpleTest.finish();
> +    // if this is part of a promise chain, then we might as well try to abort
> +    throw new Error('Aborting test');

Now that we return a promise to the harness, do we still need the SimpleTest.finish() function? Said differently: Since we throw, could we rework the harness so that we don't need SimpleTest.finish() here? One more thing to forget.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  waitForMediaFlow : function MEC_WaitForMediaFlow(onSuccess) {
> -    var self = this;
> +  waitForMediaFlow: function() {
> +    var elementId = this.element.getAttribute('id');
> -    var elementId = self.element.getAttribute('id');
>      info('Analyzing element: ' + elementId);
>  
> -    if(self.canPlayThroughFired && self.timeUpdateFired && self.timePassed) {
> +    if(this.canPlayThroughFired && this.timeUpdateFired && this.timePassed) {
>        ok(true, 'Media flowing for ' + elementId);
> -      onSuccess();
> +      return Promise.resolve();
>      } else {
> -      setTimeout(function() {
> -        self.waitForMediaFlow(onSuccess);
> +      return new Promise(r => setTimeout(r, 100))
> +        .then(() => this.waitForMediaFlow());
> -      }, 100);
>      }
>    },

(Just a comment, not a beef with your patch) This function highlights what I dislike about the test harness. The harness wraps peerConnection and invents a higher-level API, which is what you'd do if you wanted to abstract (hide) details of the underlying engine from a caller. Unfortunately, that's opposite of what we need IMHO to test all corners of the engine.

A recent test [1] shows that testing a whole call without a harness isn't a big deal anymore. In particular, its media-flow test is two lines (look for waitUntil), and is done on the data of that specific test.

waitForMediaFlow(), in contrast, depends on assumptions about use, state and data cached centrally by middle-ware; Unobvious dependencies that limit for no benefit I can see. For example: I had to "lie" to the harness to make it work with other (non-gUM) input. [2]

So I'd be for any steps that help decentralize the harness and makes individual pieces useful without all the other pieces.

Finally, a code comment: Using waitUntil() in waitForMediaFlow to reduce the scope of recursion might be more readable? Also, there was another place you used setTimeout as well where I wondered if it would be worth defining a wait() function.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_promiseSendOnly.html?force=1#20

[2] http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_capturedVideo.html?rev=99d65d6cb0b1&force=1&mark=40-40#29

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  closeEverything();
> +  return timerGuard(Promise.all([
> +    closeIt(this.pcLocal),
> +    closeIt(this.pcRemote)
> +  ]), 60000, "failed to close peer connection");

Nice.

Nothing wrong here, but this begs looking at the elephant in the room which is that pcLocal and pcRemote themselves could benefit from having totally independent queues.

E.g. http://lists.w3.org/Archives/Public/public-webrtc/2014Sep/0063.html

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  setRemoteDescriptionAndFail : function PCW_setRemoteDescriptionAndFail(desc, onFailure) {
> -    var self = this;
> +  setRemoteDescriptionAndFail : function(desc) {
> +    return this._pc.setRemoteDescription(desc).then(
> -    this._pc.setRemoteDescription(desc,
>        generateErrorCallback("setRemoteDescription should have failed."),
> -      function (err) {
> -        info(self + ": As expected, failed to set the remote description");
> -        onFailure(err);
> +      err => {
> +        info(this + ": As expected, failed to set the remote description");
> +        return err;
>      });
>    },

I wouldn't be sad to see this function go. It is used in three places.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  storeOrAddIceCandidate : function PCW_storeOrAddIceCandidate(candidate) {
> -    var self = this;
> -
> +  storeOrAddIceCandidate : function(candidate) {
> +    this._remote_ice_candidates.push(candidate);
> +    if (this.signalingState === 'closed') {
> -    self._remote_ice_candidates.push(candidate);
> -    if (self.signalingState === 'closed') {
>        info("Received ICE candidate for closed PeerConnection - discarding");
>        return;
>      }
> -    if (!self.holdIceCandidates) {
> -      self.addIceCandidate(candidate);
> +    if (!this.holdIceCandidates) {
> +      this.addIceCandidate(candidate);
>      } else {
> -      self._ice_candidates_to_add.push(candidate);
> +      this._ice_candidates_to_add.push(candidate);
>      }
>    },
>  
> -  addStoredIceCandidates : function PCW_addStoredIceCandidates() {
> -    var self = this;
> -
> -    self.holdIceCandidates = false;
> +  addStoredIceCandidates : function() {
> +    this.holdIceCandidates = false;
> +    if ((this._ice_candidates_to_add) &&
> +        (this._ice_candidates_to_add.length > 0)) {
> -    if ((self._ice_candidates_to_add) &&
> -        (self._ice_candidates_to_add.length > 0)) {
>        info("adding stored ice candidates");
> -      for (var i = 0; i < self._ice_candidates_to_add.length; i++) {
> -        self.addIceCandidate(self._ice_candidates_to_add[i]);
> +      for (var i = 0; i < this._ice_candidates_to_add.length; i++) {
> +        this.addIceCandidate(this._ice_candidates_to_add[i]);
>        }
> -      self._ice_candidates_to_add = [];
> +      this._ice_candidates_to_add = [];
>      }
>    },

Fyi storeOrAddIceCandidate and addStoredIceCandidates can be replaced in entirety with a single promise!

See last part of http://lists.w3.org/Archives/Public/public-webrtc/2014Sep/0063.html

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  addIceCandidateAndFail : function PCW_addIceCandidateAndFail(candidate, onFailure) {
> -    var self = this;
> +  addIceCandidateAndFail : function(candidate) {
> +    return this._pc.addIceCandidate(candidate).then(
> -
> -    this._pc.addIceCandidate(candidate,
>        generateErrorCallback("addIceCandidate should have failed."),
> -      function (err) {
> -        info(self + ": As expected, failed to add an ICE candidate");
> -        onFailure(err);
> -    }) ;
> +      err => {
> +        info(this + ": As expected, failed to add an ICE candidate");
> +        return err;
> +      });
>    },

I wouldn't be sad to see this function go. It is used in one place.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #13)
> Nothing wrong here, but this begs looking at the elephant in the room which
> is that pcLocal and pcRemote themselves could benefit from having totally
> independent queues.

Fyi here's an example of parallel setup: http://jsfiddle.net/jib1/fwfLbnqe
Maybe in a separate patch.

Comment 15

[Martin Thomson [:mt:]]

I definitely think that we need to split the two paths in two, but I decided not to with this change.  The data channel (and identity) tests don't split in two cleanly and so I think that the best plan would be to get them running separately before splitting the paths in two.

It's fairly trivial to setup two chains and then use Promise.all() to in the darkness bind them.

Comment 16

[Nils Ohlmeier [:drno]]

Still working on the feedback... just so big that it takes time to chew through it.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

For all its bells and whistles, I wish ReviewBoard had something like splinter's "reviewed" checkbox.

Comment 18

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2239/#review1553

> Uh-oh if indexOf returns -1 here.

Oops, trying too hard to refactor.  Not that we should be failing silently here though...

> (Just a comment, not a beef with your patch) This function highlights what I dislike about the test harness. The harness wraps peerConnection and invents a higher-level API, which is what you'd do if you wanted to abstract (hide) details of the underlying engine from a caller. Unfortunately, that's opposite of what we need IMHO to test all corners of the engine.
> 
> A recent test [1] shows that testing a whole call without a harness isn't a big deal anymore. In particular, its media-flow test is two lines (look for waitUntil), and is done on the data of that specific test.
> 
> waitForMediaFlow(), in contrast, depends on assumptions about use, state and data cached centrally by middle-ware; Unobvious dependencies that limit for no benefit I can see. For example: I had to "lie" to the harness to make it work with other (non-gUM) input. [2]
> 
> So I'd be for any steps that help decentralize the harness and makes individual pieces useful without all the other pieces.
> 
> Finally, a code comment: Using waitUntil() in waitForMediaFlow to reduce the scope of recursion might be more readable? Also, there was another place you used setTimeout as well where I wondered if it would be worth defining a wait() function.
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_promiseSendOnly.html?force=1#20
> 
> [2] http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/test_peerConnection_capturedVideo.html?rev=99d65d6cb0b1&force=1&mark=40-40#29

Yes, there is definitely more work to do here.  That hack in capturedVideo.html is horrific.

> Does anything bad happen if you don't clear the timer? How about just:
> 
> var wait = ms => new Promise(r => setTimeout(r, ms));
> var timeout = (p, ms) => Promise.race([p, wait(ms).then(() => {
>   throw new Error("timeout after " + ms/1000 + " seconds");
> })]);

That works.  I'll preserve the message though.

> Fyi storeOrAddIceCandidate and addStoredIceCandidates can be replaced in entirety with a single promise!
> 
> See last part of http://lists.w3.org/Archives/Public/public-webrtc/2014Sep/0063.html

Done.

> I wouldn't be sad to see this function go. It is used in three places.

Un-factored.

> This is the timeout pattern again in a slightly different form.

Yeah, this one was a little screwy due to the check on `onAddStreamFired`.  I've moved to throw here, which allows this to get a lot cleaner.

> Now that this is nice and clean, it looks a bit manual that individual steps do .then(ok(true, "step succeeded"), ok(false, "step failed"), when this perhaps is something that could be done by the harness itself?
> 
> Also, could we make unexpectedEventAndFinish() be the default behavior if a step fails? If a step doesn't want that it can just .catch() the error. Then if a step uses catch just to output something before failing, then it could (re-)throw e.

I think that this is right.  The trick here is to ensure that we don't end up swallowing errors anywhere.  I think that throwing everywhere that an error occurs is right, then we only have to ensure that we don't catch them too often (and consequently lose them).

Comment 19

[Martin Thomson [:mt:]]

/r/2241 - Bug 1118398 - Dispatch data channel onclose unconditionally on reset
/r/2243 - Bug 1119593 - Update WebRTC tests to use promises more consistently
/r/2245 - Bug 1119593 - Update WebRTC data channel tests
/r/2247 - Bug 1119593 - Update PeerConnection tests
/r/2249 - Bug 1119593 - Update identity tests
/r/2251 - Bug 1119593 - Update gUM tests to use promises consistently
/r/2253 - Bug 1119593 - Adding test for legacy PC callback functions
/r/2255 - Bug 1119593 - Adding test for legacy navigator.mozGetUserMedia
/r/2373 - Bug 1119593 - Re-enable per-data-channel close

Pull down these commits:

hg pull review -r 314b7d959a79d85a8afc8a6bfbac176f9deecedf

Comment 20

[Martin Thomson [:mt:]]

:drno, I updated based on :jib's comments.  Hopefully RB will allow you to continue to review as you were then look at the interdiff.

Comment 21

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/2239/#review1501

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> -function getBlobContent(blob, onSuccess) {
> +function getBlobContent(blob) {

As this is only used from dataChannel.js I think we should move it over to that file.

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
> +    // if this is part of a promise chain, then we might as well try to abort

The if suggests that there are cases where this is not part of promise chain. Is that the case? And if so how would this new Error behave then?

::: dom/media/tests/mochitest/head.js
(Diff revision 1)
>      SimpleTest.finish();

Why don't you throw the same Error here as you do up in line 253?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
>   * executing them synchronously.

You should probably update this comment with what the chain is doing now.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
>     * Execute the next command in the chain.

Update the comment

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
>      if (index > -1) {

In case of no such element this is going to insert the new function right at position 0!

You should probably only add to the index within the if clause.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -      this.append(commands);
> +        this.commands.slice(index));

This code looks identical to the lines 99-102. Maybe one should call the other, or both call an underlying helper function with the exact position?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -    if ((self.pcLocal) && (self.pcLocal.signalingState !== "closed")) {
> +  var isClosed = pc => !pc || pc.signalingState === "closed";

What is this line good for?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -
> +  // return timerGuard(

Don't you need to call the timerGuard function after you set up the onclose callbacks below?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -  }
> +    return new Promise(resolve => {

Isn't this redundant to the onclose handler up in line 569 to 576?

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> - * Close the specified data channels
> + * Close the specified data channels on both sides

The comment does not match the code down in line 516 - 521.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -    wait = true;
> +  return timerGuard(allClosed, 60000, "failed to close data channel pair");

Again I think you want to setup the timerGuard before you close the channels up in 617-621, so that the Promises are installed when closing the data channels.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> +                     this.registerSignalingCallback(messageType, resolve));

Is this promise going to resolve when a messsage of the requested type arrives?
Because that is what should really happen here (but was not easily possible when this got created). But I think this promise just resolves when the callback got registered.

::: dom/media/tests/mochitest/pc.js
(Diff revision 1)
> -      is(self._pc.iceConnectionState, self.next_ice_state, "iceConnectionState changed to '" +
> +    if (this.next_ice_state !== "") {

I think we should use the opportunity and remove this next_ice_state non-sense. Not a single test case uses it.

Comment 22

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/2239/#review1589

::: dom/media/tests/mochitest/test_getUserMedia_basicWindowshare.html
(Diff revision 2)
> -      playback.playMediaWithStreamStop(false, function () {
> +      return playback.playMedia(false);

Did you replace playMediaWithStreamStop() on purpose here with playMedia()?

::: dom/media/tests/mochitest/test_getUserMedia_callbacks.html
(Diff revision 2)
> +  <title>mozGetUserMedia Basic Audio Test</title>

Please update the title here as well.

::: dom/media/tests/mochitest/test_getUserMedia_basicWindowshare.html
(Diff revision 2)
> -        aStream.stop();
> +    }).then(() => SimpleTest.finish(), generateErrorCallback());

Did you dropped the aStream.stop() on purpose here (you keep using it in some other files)?

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStream.html
(Diff revision 2)
> -<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=822109">mozGetUserMedia Stop Video Stream</a>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=822109">mozGetUserMedia Stop Video Audio Stream</a>

This tests only Video, so remove the additional Audio from this link.

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStreamWithFollowupVideo.html
(Diff revision 2)
> -<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=822109">mozGetUserMedia Stop Video Stream With Followup Video</a>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=822109">mozGetUserMedia Stop Video+Audio Stream With Followup Video+Audio</a>

Remove "+Audio" from this link twice.

::: dom/media/tests/mochitest/test_getUserMedia_stopVideoStreamWithFollowupVideo.html
(Diff revision 2)
> -  <title>mozGetUserMedia Stop Video Stream With Followup Video</title>
> +  <title>mozGetUserMedia Stop Video+Audio Stream With Followup Video+Audio</title>

Remove "+Audio" twice from this title.

Comment 23

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/2239/#review1631

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
> -      test.next();
> +    if (this.steeplechase) {

this.steeplechase or test.steeplechase (which is used below)?

::: dom/media/tests/mochitest/templates.js
(Diff revision 2)
> -      }
> -    }
> -  ],
> -  [
> -    'PC_REMOTE_CHECK_GETSTATS_VIDEOTRACK_INBOUND',
> -    function (test) {
> -      var pc = test.pcRemote;
> -      var stream = pc._pc.getRemoteStreams()[0];
> -      var track = stream && stream.getVideoTracks()[0];
> -      if (track) {
> -        var msg = "pcRemote.HasStat inbound video rtp ";
> -        pc.getStats(track, function(stats) {
> -          ok(pc.hasStat(stats,
> -                        { type:"inboundrtp", isRemote:false, mediaType:"video" }),
> -             msg + "1");
> +  function PC_LOCAL_CHECK_GETSTATS_AUDIOTRACK_OUTBOUND(test) {
> +    return checkTrackStats(test, test.pcLocal, true, true);
> +  },
> +
> +  function PC_LOCAL_CHECK_GETSTATS_VIDEOTRACK_OUTBOUND(test) {
> +    return checkTrackStats(test, test.pcLocal, false, true);
> +  },
> +
> +  function PC_LOCAL_CHECK_GETSTATS_AUDIOTRACK_INBOUND(test) {
> +    return checkTrackStats(test, test.pcLocal, true, false);
> +  },
> +
> +  function PC_LOCAL_CHECK_GETSTATS_VIDEOTRACK_INBOUND(test) {
> +    return checkTrackStats(test, test.pcLocal, false, false);
> +  },

You are just iterating through all four possible combinations of your two boolean input types. Can we replace this with just one call and interate in checkTraveStats() instead?

Comment 24

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/2239/#review1637

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 2)
> -          is(targetChannel.readyState, "open", targetChannel + " is in state: 'open'");
> +        is(targetChannel.readyState, "open", targetChannel + " is in state: 'open'");
>  
> -          is(targetChannel.binaryType, "blob", targetChannel + " is of binary type 'blob'");
> +        is(targetChannel.binaryType, "blob", targetChannel + " is of binary type 'blob'");
> -          is(targetChannel.readyState, "open", targetChannel + " is in state: 'open'");
> +        is(targetChannel.readyState, "open", targetChannel + " is in state: 'open'");

Lest remove line 67. It appears to be a duplicate of line 64.

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 2)
> -        var message = "Lorem ipsum dolor sit amet";
> +      var message = "Lorem ipsum dolor sit amet";

Not good that this uses exactly the identical message as the first test message.

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 2)
> -          is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
> +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
>  
> -          is(targetChannel2.binaryType, "blob", targetChannel2 + " is of binary type 'blob'");
> +        is(targetChannel2.binaryType, "blob", targetChannel2 + " is of binary type 'blob'");
> -          is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
> +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");

Again 119 appers to be a duplicate of line 116.

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 2)
> -          if (options.id != undefined) {
> +        if (typeof options.id !== 'undefined') {
> -            is(sourceChannel2.id, options.id, sourceChannel2 + " id is:" + sourceChannel2.id);
> +          is(sourceChannel2.id, options.id, sourceChannel2 + " id is:" + sourceChannel2.id);
> -          }
> +        } else {
> -          else {
> -            options.id = sourceChannel2.id;
> +          options.id = sourceChannel2.id;
> -          }
> +        }

options is defined in line 110, so this if() appears to be pretty useless.

::: dom/media/tests/mochitest/dataChannel.js
(Diff revision 2)
> -        var message = "Lorem ipsum dolor sit amet";
> +      var message = "Lorem ipsum dolor sit amet";

And again the identical test message. Lets get some variants in here please.

::: dom/media/tests/mochitest/head.js
(Diff revision 2)
> +function waitUntil(func) {

Even if used sparingly can we make this take the interval and not always use a value of 200ms?

::: dom/media/tests/mochitest/head.js
(Diff revision 2)
> -function getBlobContent(blob, onSuccess) {
> +function getBlobContent(blob) {

Let's move this to dataChannel.js

Comment 25

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2239/#review1591

> The if suggests that there are cases where this is not part of promise chain. Is that the case? And if so how would this new Error behave then?

Yeah, :jib already identified this one.

> Why don't you throw the same Error here as you do up in line 253?

The unexpected event thing needed a little more work than that.  We need to somehow integrate the failure into the main chain of promises.

> What is this line good for?

Vestigal.

> Don't you need to call the timerGuard function after you set up the onclose callbacks below?

I've restored the timerGuard stuff in the last patch of the series.  I should probably squash those.

> Isn't this redundant to the onclose handler up in line 569 to 576?

The stuff in 569-576 is temporary stuff: you will note that in this patch, the data channels aren't actually closed before the PC is closed.

> Again I think you want to setup the timerGuard before you close the channels up in 617-621, so that the Promises are installed when closing the data channels.

This is a tricky one.  This is all down to the wonder that is JavaScript.

I'll make this a little clearer.  Though it shouldn't matter what order things happen here, I suspect that it could if we have recursive callbacks from data channel code (and that seems plausible, even if it would be wrong).

> Is this promise going to resolve when a messsage of the requested type arrives?
> Because that is what should really happen here (but was not easily possible when this got created). But I think this promise just resolves when the callback got registered.

This looks OK to me.  The resolve function will be called when a message of the requested type arrives.

> I think we should use the opportunity and remove this next_ice_state non-sense. Not a single test case uses it.

You're on.  Done.

Comment 26

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2239/#review1639

> Did you replace playMediaWithStreamStop() on purpose here with playMedia()?

No *good* reason :)  I'll fix it.  Both work equally well, it appears.

> Did you dropped the aStream.stop() on purpose here (you keep using it in some other files)?

It's actually called by `playMediaWithStreamStop()`

> Please update the title here as well.

I updated all the titles for the gUM tests!

Comment 27

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2239/#review1641

> this.steeplechase or test.steeplechase (which is used below)?

Nice.

Comment 28

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2239/#review1643

> Even if used sparingly can we make this take the interval and not always use a value of 200ms?

I'll make it default to 200, since this is tested.

Comment 29

[Martin Thomson [:mt:]]

/r/2241 - Bug 1118398 - Dispatch data channel onclose unconditionally on reset
/r/2243 - Bug 1119593 - Update WebRTC tests to use promises more consistently
/r/2245 - Bug 1119593 - Update WebRTC data channel tests
/r/2247 - Bug 1119593 - Update PeerConnection tests
/r/2249 - Bug 1119593 - Update identity tests
/r/2251 - Bug 1119593 - Update gUM tests to use promises consistently
/r/2253 - Bug 1119593 - Adding test for legacy PC callback functions
/r/2255 - Bug 1119593 - Adding test for legacy navigator.mozGetUserMedia
/r/2373 - Bug 1119593 - Re-enable per-data-channel close

Pull down these commits:

hg pull review -r dc73e53086df2f3fa3f48ce31ebf274b9a512fdd

Comment 30

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1553a66d78c9

Now with more commits and many more hundreds of lines deleted.

Comment 31

[Martin Thomson [:mt:]]

/r/2241 - Bug 1118398 - Dispatch data channel onclose unconditionally on reset
/r/2243 - Bug 1119593 - Update WebRTC tests to use promises more consistently
/r/2245 - Bug 1119593 - Update WebRTC data channel tests
/r/2247 - Bug 1119593 - Update PeerConnection tests
/r/2249 - Bug 1119593 - Update identity tests
/r/2251 - Bug 1119593 - Update gUM tests to use promises consistently
/r/2253 - Bug 1119593 - Adding test for legacy PC callback functions
/r/2255 - Bug 1119593 - Adding test for legacy navigator.mozGetUserMedia
/r/2373 - Bug 1119593 - Re-enable per-data-channel close
/r/2499 - Bug 1119593 - Aggressively removing boilerplate on tests

Pull down these commits:

hg pull review -r 57c96523b60add75443b154fbd11b8969e6e0e49

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/2239/#review1647

Good grief, another revision and I was half way through. Let me publish what I have and start over. Stop working! ;-)

::: dom/media/tests/mochitest/dataChannel.js
(Diff revisions 1 - 3)
> +    reader.onloadend = function (event) {
> +      resolve(event.target.result);
> +    };

=> ?

::: dom/media/tests/mochitest/dataChannel.js
(Diff revisions 1 - 3)
> -      var message = "Lorem ipsum dolor sit amet";
> +      var message = "I am the walrus; Goo goo g' joob";

Extraneous space in g'joob

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> +        return Promise.race([
> +          next(this._framework),
> +          unexpectedEventArrivedPromise
> +        ]);

Fits on one line with previously suggested rename. :-)

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> +    this._insertHelper(id, commands);
> +  },
> +
> +  _insertHelper: function(id, commands, delta) {
> +    delta = delta || 0;

I would pass in 0 and remove the ||= 0 line.

::: dom/media/tests/mochitest/pc.js
(Diff revisions 1 - 3)
> -
> +  var allChannels = (this.pcLocal ? this.pcLocal : this.pcRemote).dataChannels;

why not (this.pcLocal || this.pcRemote) ?

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> +function guardEvent(wrapper, obj, event, redirect) {

This function took me longer than it should to wrap my head around.

- Maybe rename to guardSingleFireEventHandler? Ooh, or perhaps thisSingleFireEventShouldHaveBeenAPromiseSpecPeople?

- Since the wrapper arg is always this, could we make the method a property of this instead? e.g. this.guardEventHandler(obj, event, redirect)?

I think that might help.

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> +var unexpectedEventArrived;
> +var unexpectedEventArrivedPromise = new Promise((x, reject) => {
> +  unexpectedEventArrived = reject;
> +});

How about
s/unexpectedEventArrived/rejectOnUnexpectedEvent/
s/unexpectedEventArrivedPromise/unexpectedEventArrived/
?

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> + * Once but that handler is used exactly once, and subsequent events will also

Rephrase please

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> + * @param {function} redirect
> + *        (Optional) a function that determines what is passed to the event
> + *        handler. By default, the handler is passed the wrapper (as opposed to
> + *        the normal cases where they receive an event object).  This redirect
> + *        function is passed the event.

Here's another example of things in the framework that concerns me (not your patch). Here the harness wrapper is returned to the eventhandler instead of the event (!) This appears to be an artifact of the harness trying to emulate peerConnection, altering the API for no great reason. I don't have an immediate alternative solution, but would love to explore different models here that doesn't mutate the API.

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> +   * @param {function|string} id
> +   *        Command function or name
> +   * @returns {number} Index of the command
> +   */
> +  indexOf: function (id) {
> +    if (typeof id === 'string') {

How about s/id/command/ or s/id/functionOrName/?

::: dom/media/tests/mochitest/head.js
(Diff revisions 1 - 3)
> +    var index = this.indexOf(id);
> +
> +    if (index >= 0) {

To make it shorter you could do:

var index = this.indeOf(id) + delta;
if (index >= delta) {

just saying.

::: dom/media/tests/mochitest/pc.js
(Diff revisions 1 - 3)
> +  guardEvent(this, this._channel, 'message', e => e.data);

Can message only fire once? That seems like an artificial limit.

Also, why are we modifying the API to return e.data instead of e? (predates your patch).

::: dom/media/tests/mochitest/pc.js
(Diff revisions 1 - 3)
>      Object.keys(this.signalingStateCallbacks).forEach(name => {
> -      this.signalingStateCallbacks[name](anEvent);
> +      this.signalingStateCallbacks[name](e);
> +    });

Seeing signalingStateCallbacks for the first time, I find it a bit elaborate for its single use to produce logs elsewhere in the same code:

http://mxr.mozilla.org/mozilla-central/search?string=signalingStateCallbacks

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Martin Thomson [:mt] from comment #31)
> /r/2499 - Bug 1119593 - Aggressively removing boilerplate on tests

OK I'm a bit lost in Review Board. Does this latest push (Revision 4) only contain changes for drno in /r/2499 ?

Comment 34

[Nils Ohlmeier [:drno]]

(In reply to Martin Thomson [:mt] from comment #29)
> hg pull review -r dc73e53086df2f3fa3f48ce31ebf274b9a512fdd

Unfortunately already this second revision is broken on steeplechase :-(

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Martin Thomson [:mt] from comment #30)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1553a66d78c9

Once everything succeeds, you should probably induce some errors on try to make sure the CommandChain failure paths work right.

Comment 36

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #33)
> OK I'm a bit lost in Review Board. Does this latest push (Revision 4) only
> contain changes for drno in /r/2499 ?

My understanding is that this revision was triggered by Ekr's feedback (although I certainly dislike the lengthy script includes and that they "prevent" you from splitting code into more sane chunks, because you would have to touch N files).

Comment 37

[Martin Thomson [:mt:]]

2499 is pretty much limited to boilerplate reduction.  It should enable easier refactoring and restructuring of the code.  I'd recommend finishing the review of the earlier revisions first.  Then look at the interdiff from 3-4 (or likely 3-5) to see where the fixes come in.

Comment 38

[Martin Thomson [:mt:]]

BTW, I'm really appreciating the aggressive approach you've both taken here: I fix code a little bit, and I get suggestions that fix it a lot.  Maybe we'll get some good code out of this.

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/2239/#review1663

I decided to finish reviewing Revision 3. Holler if there's anything for me in Revision 4.

::: dom/media/tests/mochitest/templates.js
(Diff revisions 1 - 3)
> +function checkAllTrackStats(pc) {
> +  var checks = [];
> +  for (var i = 0; i < 4; ++i) {
> +    // check all combinations
> +    checks.push(checkTrackStats(pc, (i & 1) === 1, (i & 2) === 2));
> +  }
> +  return Promise.all(checks);
> +}

Code-golf:

var checkAllTrackStats = pc =>
  Promise.all([0, 1, 2, 3].map(i => checkTrackStats(pc, i & 1, i & 2)));

::: dom/media/tests/mochitest/pc.js
(Diff revisions 1 - 3)
>      var happy = new Promise(resolve => {
>        this.addStreamCallbacks.checkMediaTracks = resolve;
>      }).then(() => {
> -      if (thandle) {
> -        clearTimeout(thandle);
> -      }
>        _checkMediaTracks(constraintsRemote);
>      });
> -    var sad = new Promise(resolve => (thandle = setTimeout(resolve, 60000)))
> -        .then(() => {
> -          ok(this.onAddStreamFired, this + " checkMediaTracks() timed out waiting" +
> +    var sad = wait(60000).then(() => {
> +      if (!this.onAddStreamFired) {
> +        // throw rather than call ok(false) because we only want this to be
> +        // caught if the sad path fails the promise race with the happy path
> +        throw new Error(this + " checkMediaTracks() timed out waiting" +
> -             " for onaddstream event to fire");
> +                        " for onaddstream event to fire");
> +      }
> -        });
> +    });
>      return Promise.race([ happy, sad ]);

In the sad promise, the only time this.onAddStreamFired != false is when happy promise beats it, so it can throw all it wants at that point and it wont matter since the race is over. That means this should reduce to:

return timerGuard(new Promise(r => this.addStreamCallbacks.checkMediaTracks = r),
                  60000, "onaddstream never fired");

Comment 40

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Martin Thomson [:mt] from comment #38)
> BTW, I'm really appreciating the aggressive approach you've both taken here:
> I fix code a little bit, and I get suggestions that fix it a lot.  Maybe
> we'll get some good code out of this.

You picked up a sword I think we were excited about. Easy for us to say "slash here, and there!" Thanks for taking up this monumental task.

Comment 41

[Nils Ohlmeier [:drno]]

(In reply to Nils Ohlmeier [:drno] from comment #34)
> (In reply to Martin Thomson [:mt] from comment #29)
> > hg pull review -r dc73e53086df2f3fa3f48ce31ebf274b9a512fdd
> 
> Unfortunately already this second revision is broken on steeplechase :-(

Identified the two lines at fault here. Because of some "interesting" design decision you can't call SimpleTest.X() in steeplechase. Instead steeplechase overloads the used functions, but not in the SimpleTest name space. I can show you the short term fixes tomorrow.

Comment 42

[Nils Ohlmeier [:drno]]

In case of steeplechase the Promise for the SDP Answer seems to return to early an empty SDP (but it works for the offer - which might be just timing luck).

Comment 43

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/2239/#review1675

::: dom/media/tests/mochitest/templates.js
(Diff revision 4)
> +    test.getSignalingMessage("answer").then(message => {

Missing "return" before test.getSignaling...

::: dom/media/tests/mochitest/mediaStreamPlayback.js
(Diff revision 4)
> +    SimpleTest.waitForExplicitFinish();

I removed this in my steeplechase setup as SimpleTest is not available on steeplechase. I think it should be ok, as waitForExplicitFinish() gets called for non-steeplechase runs in heads.js.

::: dom/media/tests/mochitest/templates.js
(Diff revision 4)
> +    return checkAllTrackStats(test.pcLocal);

test.pcRemote !

::: dom/media/tests/mochitest/pc.js
(Diff revision 4)
> +    SimpleTest.waitForExplicitFinish();

Commented out this line for steeplechase as SimpleTest is not available on steeplechase. I think it is save as this gets called in head.js already.

::: dom/media/tests/mochitest/network.js
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

network.js needs to be added to steeplechase.ini

Comment 44

[Nils Ohlmeier [:drno]]

I had the 2nd revision working in steeplechase. But there is still problems with the latest revision.

Comment 45

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2239/#review1669

I think that I'll rely on drno for that one.  We need to go through it to fix a steeplechase breakage tomorrow anyway.

> In the sad promise, the only time this.onAddStreamFired != false is when happy promise beats it, so it can throw all it wants at that point and it wont matter since the race is over. That means this should reduce to:
> 
> return timerGuard(new Promise(r => this.addStreamCallbacks.checkMediaTracks = r),
>                   60000, "onaddstream never fired");

See, I need to pay more attention.  I was trying to maintain existing behaviour, but it looks like that had layers of confusion.

BTW, I don't like the timerGuard thing, but I think we're stuck with it.

> Code-golf:
> 
> var checkAllTrackStats = pc =>
>   Promise.all([0, 1, 2, 3].map(i => checkTrackStats(pc, i & 1, i & 2)));

Why the hell not.

Comment 46

[Martin Thomson [:mt:]]

https://reviewboard.mozilla.org/r/2239/#review1667

> Here's another example of things in the framework that concerns me (not your patch). Here the harness wrapper is returned to the eventhandler instead of the event (!) This appears to be an artifact of the harness trying to emulate peerConnection, altering the API for no great reason. I don't have an immediate alternative solution, but would love to explore different models here that doesn't mutate the API.

Yes, and given the limited number of cases that this guard is actually used, I think that it would be relatively easy to remove the redirect (and retroactively fix the depedencies on it).

> This function took me longer than it should to wrap my head around.
> 
> - Maybe rename to guardSingleFireEventHandler? Ooh, or perhaps thisSingleFireEventShouldHaveBeenAPromiseSpecPeople?
> 
> - Since the wrapper arg is always this, could we make the method a property of this instead? e.g. this.guardEventHandler(obj, event, redirect)?
> 
> I think that might help.

The `this` is a different object though, so this would have to be defined once, then patched onto the different objects separately:
```
function wrapSingleShotEvent() {...}
PeerConnectionWrapper.prototype.wrapSingleShotEvent = wrapSingleShotEvent;
DataChannelWrapper.prototype.wrapSingleShotEvent = wrapSingleShotEvent;
```
etc...  That seemed less good than this.  The rename is good.

> To make it shorter you could do:
> 
> var index = this.indeOf(id) + delta;
> if (index >= delta) {
> 
> just saying.

Let the compiler make that optimization.

> Can message only fire once? That seems like an artificial limit.
> 
> Also, why are we modifying the API to return e.data instead of e? (predates your patch).

Currently, tests need to setup an onmessage for each message they expect.  I think that is a reasonable thing to do.

As for the e vs. e.data, I've fixed this throughout.

> Seeing signalingStateCallbacks for the first time, I find it a bit elaborate for its single use to produce logs elsewhere in the same code:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=signalingStateCallbacks

I've replaced this with addEventListener, which works nicely.

Comment 47

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Martin Thomson [:mt] from comment #45)
> BTW, I don't like the timerGuard thing, but I think we're stuck with it.

My only confusion here is why bother with timeouts. Assuming the mochitest harness enforces an overall timeout per test, what's gained by additional ones? Do we do anything useful when onaddstream fails to fire?

(In reply to Martin Thomson [:mt] from comment #46)
> The `this` is a different object though,

OK nm.

> > Can message only fire once? That seems like an artificial limit.
> 
> Currently, tests need to setup an onmessage for each message they expect.  I
> think that is a reasonable thing to do.

Why? That's a stilted subset of the real API, which lets us send tons of messages in a row. We should be testing that imho.

> > Seeing signalingStateCallbacks for the first time, I find it a bit elaborate for its single use to produce logs 
> > elsewhere in the same code:
> 
> I've replaced this with addEventListener, which works nicely.

Nice! Would that work for signalingCallbacks and addStreamCallbacks as well?

Comment 48

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #47)
> (In reply to Martin Thomson [:mt] from comment #45)
> > BTW, I don't like the timerGuard thing, but I think we're stuck with it.
> 
> My only confusion here is why bother with timeouts. Assuming the mochitest
> harness enforces an overall timeout per test, what's gained by additional
> ones? Do we do anything useful when onaddstream fails to fire?

I've kept these at Nils' request, despite wanting to delete them all.  The problem is that the general timeout doesn't capture what was left hanging.  I think that I could monkey-patch SimpleTest to work around the issue, but I'd rather not do all the changes at once.

> Why? That's a stilted subset of the real API, which lets us send tons of
> messages in a row. We should be testing that imho.

Ahh, the question of coverage.  Yes, I agree.  I think that the pattern is still OK for now, though probably not eventually for `onmessage`.  When someone adds a test to check ordered delivery of multiple messages, then we can (and should) deal with that then.

> > I've replaced this with addEventListener, which works nicely.
> 
> Nice! Would that work for signalingCallbacks and addStreamCallbacks as well?

Already done.  Except that now I have to rebase against Byron's latest explosion, so you won't see this soon.

Comment 49

[Martin Thomson [:mt:]]

/r/2241 - Bug 1118398 - Dispatch data channel onclose unconditionally on reset
/r/2243 - Bug 1119593 - Update WebRTC tests to use promises more consistently
/r/2245 - Bug 1119593 - Update WebRTC data channel tests
/r/2247 - Bug 1119593 - Update PeerConnection tests
/r/2249 - Bug 1119593 - Update identity tests
/r/2251 - Bug 1119593 - Update gUM tests to use promises consistently
/r/2253 - Bug 1119593 - Adding test for legacy PC callback functions
/r/2255 - Bug 1119593 - Adding test for legacy navigator.mozGetUserMedia
/r/2373 - Bug 1119593 - Re-enable per-data-channel close
/r/2499 - Bug 1119593 - Aggressively removing boilerplate on tests
/r/2627 - Bug 1119593 - Fixing test preconditions for steeplechase
/r/2629 - Bug 1119593 - Dealing with multiple streams

Pull down these commits:

hg pull review -r 4f6cb7677f37c04b198e8c403242dc9ae658075c

Comment 50

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Can you help me with which part of Revision 5 is for me? I see a big diff.

Comment 51

[Martin Thomson [:mt:]]

I'm happy with your existing review.  I just can't stop reviewboard from blasting out r? to all involved.  It's a blunt implement.  Add that to the fact that you can't see what specific patches your name is on as a reviewer.  If you look at all the patches, you will see that your name only appears on one of them.

Anyhow, enough RB-bashing.  I'll clear your r? and try to remember to do so if I need another review round with Nils.

Comment 52

[Nils Ohlmeier [:drno]]

Comment on attachment 8546282 [details]
MozReview Request: bz://1119593/mt

https://reviewboard.mozilla.org/r/2239/#review1937

::: dom/media/tests/mochitest/pc.js
(Diff revisions 4 - 5)
>      // TODO: remove this once Bugs 998552 and 998546 are closed

Remove this comment please, as it refers to an empty line now.

Comment 53

[Nils Ohlmeier [:drno]]

Comment on attachment 8546282 [details]
MozReview Request: bz://1119593/mt

https://reviewboard.mozilla.org/r/2239/#review1939

Ship It!

Comment 54

[Martin Thomson [:mt:]]

One last test run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb66c4b39654
because inbound is closed.

Comment 55

[Martin Thomson [:mt:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33b1f11c8784
https://hg.mozilla.org/integration/mozilla-inbound/rev/288e9b7efccc
https://hg.mozilla.org/integration/mozilla-inbound/rev/49e681140796
https://hg.mozilla.org/integration/mozilla-inbound/rev/2198a2cd71a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc930e78d1b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9ee98fcce6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b17ccc1d9c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e61d3076385
https://hg.mozilla.org/integration/mozilla-inbound/rev/544d8d52bbaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2f5bf9e656
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a3067465821

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a75426ed6ab0 (along with bug 1118398) for zmedia failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=5731521&repo=mozilla-inbound

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

See also bug 1091322.

Comment 58

[Martin Thomson [:mt:]]

(In reply to Wes Kocher (:KWierso) from comment #56)
> All backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a75426ed6ab0 (along
> with bug 1118398) for zmedia failures:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=5731521&repo=mozilla-
> inbound

Gah, why didn't that turn up on try?  It's an obvious (and simple) screw-up.  Easy to fix too.

(In reply to Wes Kocher (:KWierso) from comment #57)
> See also bug 1091322.

That is definitely not caused by this.  I know that :drno is looking into it.

Comment 59

[Martin Thomson [:mt:]]

I'm seeing a couple of crashes on try with Android.  Nothing that this patch could possibly cause though.  If changing some JS can cause this, we're worse off than I thought.

Comment 60

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=258efa7bcea9

Comment 61

[Martin Thomson [:mt:]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=db7c4ffd5a53

Comment 62

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And out again for zmedia failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/a2984f1964f6

https://treeherder.mozilla.org/logviewer.html#?job_id=5994560&repo=mozilla-inbound

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Bug 1122722 needs to land before this can, no?

Comment 64

[Martin Thomson [:mt:]]

OK, I have got to be more careful.  The files that went to try are somehow different to what I landed.  This has happened before.

Comment 65

[Martin Thomson [:mt:]]

https://bug1119593.bugzilla.mozilla.org/attachment.cgi?id=8546282

Comment 66

[Martin Thomson [:mt:]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65f98401aa35

Comment 67

[Martin Thomson [:mt:]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7f03823a577e

Comment 68

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9be9159b75fd
https://hg.mozilla.org/mozilla-central/rev/9268d8d19a26
https://hg.mozilla.org/mozilla-central/rev/f6a637428a85
https://hg.mozilla.org/mozilla-central/rev/2262a8daf32c
https://hg.mozilla.org/mozilla-central/rev/5d34ef3c29ec
https://hg.mozilla.org/mozilla-central/rev/599e8f39bbcc
https://hg.mozilla.org/mozilla-central/rev/d7d594b94768
https://hg.mozilla.org/mozilla-central/rev/1c44eb512e3f
https://hg.mozilla.org/mozilla-central/rev/75f409f3a7b1
https://hg.mozilla.org/mozilla-central/rev/a18af75beb27
https://hg.mozilla.org/mozilla-central/rev/7f03823a577e

Comment 69

[Martin Thomson [:mt:]]

Attachment: 02 Bug_1119593___Update_WebRTC_tests_to_use_promises_more_consistently__r_drno_jib.patch

Approval Request Comment
[Feature/regressing bug #]: 1119593
[User impact if declined]: none
[Describe test coverage new/current, TreeHerder]: see comment history for lots of runs
[Risks and why]: This affects webrtc test code only, reducing intermittent test failures significantly.
[String/UUID change made/needed]: none

As landed.  I recommend pulling from inbound directly (that's where this came from).  Don't forget all 12 patches:  -r 47f17e4219ea::7f03823a577e

Comment 70

[Martin Thomson [:mt:]]

Attachment: 03 Bug_1119593___Update_WebRTC_data_channel_tests__r_drno.patch

Comment 71

[Martin Thomson [:mt:]]

Attachment: 04 Bug_1119593___Update_PeerConnection_tests__r_drno_jib.patch

Comment 72

[Martin Thomson [:mt:]]

Attachment: 05 Bug_1119593___Update_identity_tests__r_drno.patch

Comment 73

[Martin Thomson [:mt:]]

Attachment: 06 Bug_1119593___Update_gUM_tests_to_use_promises_consistently__r_drno_jib.patch

Comment 74

[Martin Thomson [:mt:]]

Attachment: 07 Bug_1119593___Adding_test_for_legacy_PC_callback_functions__r_drno_jib.patch

Comment 75

[Martin Thomson [:mt:]]

Attachment: 08 Bug_1119593___Adding_test_for_legacy_navigator_mozGetUserMedia__r_drno.patch

Comment 76

[Martin Thomson [:mt:]]

Attachment: 09 Bug_1119593___Re_enable_per_data_channel_close__r_drno.patch

Comment 77

[Martin Thomson [:mt:]]

Attachment: 10 Bug_1119593___Aggressively_removing_boilerplate_on_tests__r_drno.patch

Comment 78

[Martin Thomson [:mt:]]

Attachment: 11 Bug_1119593___Fixing_test_preconditions_for_steeplechase__r_drno.patch

Comment 79

[Martin Thomson [:mt:]]

Attachment: 12 Bug_1119593___Dealing_with_multiple_streams__r_drno.patch

Comment 80

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8556878 [details] [diff] [review]
02 Bug_1119593___Update_WebRTC_tests_to_use_promises_more_consistently__r_drno_jib.patch

All 12 patches in this bug contain test only changes. I'm approving all for Aurora. In future, you don't need to request approval to land test only changes. These changes can land with a=test-only.

Aurora+

Comment 81

[Ryan VanderMeulen [:RyanVM]]

Ugh, this has conflicts a-plenty on Aurora :(

Comment 82

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #81)
> Ugh, this has conflicts a-plenty on Aurora :(

Gah, I thought I needinfo'd Martin on this.

Comment 83

[Martin Thomson [:mt:]]

Indeed the conflicts are plentiful.  I am building a patch series for you.

Comment 84

[Martin Thomson [:mt:]]

For my own peace of mind: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a2b19f3a0f2

Comment 85

[Martin Thomson [:mt:]]

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=5c0cf399911c

Comment 86

[Ryan VanderMeulen [:RyanVM]]

I <3 you so much.

Comment 88

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1118398 - Dispatch data channel onclose unconditionally on reset

Comment 89

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Update WebRTC data channel tests

Comment 90

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Fixing test preconditions for steeplechase

Comment 91

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Dealing with multiple streams

Comment 92

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Update WebRTC tests to use promises more consistently

Comment 93

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Update PeerConnection tests

Comment 94

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Update identity tests

Comment 95

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Update gUM tests to use promises consistently

Comment 96

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Adding test for legacy PC callback functions

Comment 97

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Adding test for legacy navigator.mozGetUserMedia

Comment 98

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Aggressively removing boilerplate on tests

Comment 99

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1119593 - Re-enable per-data-channel close