837458 - Cleanup PC Test Framework to Allow for Better Maintainability and Reusability for Future Tests

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jason Smith [:jsmith]]

Per talking with Eric and seeing it for myself, the peer connection tests need a lot of cleanup in order for us to maintain it. I've got a patch coming soon that cleans up a bunch of this.

Comment 1

[Jason Smith [:jsmith]]

Attachment: PC Test Framework Cleanup v1

Comment 2

[Jason Smith [:jsmith]]

Attachment: PC Test Framework Cleanup v1

Comment 3

[Jason Smith [:jsmith]]

Comment on attachment 709450 [details] [diff] [review]
PC Test Framework Cleanup v1

Asking for feedback first since I guarantee someone will have a strong opinion about what I did here.

Comment 4

[Adam Roach [:abr]]

Adding Jan-Ivar to the CC list, since he's been working in here also.

Comment 5

[Adam Roach [:abr]]

Comment on attachment 709450 [details] [diff] [review]
PC Test Framework Cleanup v1

Review of attachment 709450 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't conducted a thorough review, but I do like the direction that these changes are taking.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jason, before I want to give feedback on this patch I would like to know what was the outcome of your talk with Eric. Nothing of that has been mentioned here yet. So please be so kind and give at least some information which will give me a better start on this patch. Thanks.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> Jason, before I want to give feedback on this patch I would like to know
> what was the outcome of your talk with Eric. Nothing of that has been
> mentioned here yet. So please be so kind and give at least some information
> which will give me a better start on this patch. Thanks.

What talk? The only chat information relevant to this patch I've already stated - which was maintainability was a problem, when he was working on a different patch.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 709450 [details] [diff] [review]
PC Test Framework Cleanup v1

Review of attachment 709450 [details] [diff] [review]:
-----------------------------------------------------------------

I think that's a great start. I have made a couple of general comments inline which are mostly about the API, and which we will have to discuss further. Given that I would like to see some API changes I will give f- for now.

::: dom/media/tests/mochitest/Makefile.in
@@ -18,5 @@
>    test_peerConnection_basicAudio.html \
>    test_peerConnection_basicAudioVideo.html \
>    test_peerConnection_basicAudioVideoCombined.html \
>    test_peerConnection_basicVideo.html \
> -  test_peerConnection_bug827843.html \

Why is this file getting removed?

::: dom/media/tests/mochitest/peerConnectionWrapper.js
@@ +10,5 @@
> + * @param {List} remoteMediaElementList the list of media elements representing
> + *                                      remote media content
> + * @param {List} localMediaElementList (optional) the list of media elements
> + *                                                representing local media
> + *                                                content

What if we would let the wrapper itself let create those media elements? That would drop a lot of the extra setup steps.

@@ +11,5 @@
> + *                                      remote media content
> + * @param {List} localMediaElementList (optional) the list of media elements
> + *                                                representing local media
> + *                                                content
> + * @param {List} gUMConstriantList (optional) the list of gUM constriants to be

nit: constriants vs. constraints

@@ +18,5 @@
> + * @param {PeerConnection} (optional) the peer connection object to use
> + */
> +function PeerConnectionWrapper(remoteMediaElementList, localMediaElementList,
> +  gUMConstriantList, peerConnection) {
> +  this.remoteMediaElementList = remoteMediaElementList;

I didn't wanted to comment on this indentation style but here you can see how hard it is to read. You cannot see the difference between parameters and the actual code of this function.

@@ +20,5 @@
> +function PeerConnectionWrapper(remoteMediaElementList, localMediaElementList,
> +  gUMConstriantList, peerConnection) {
> +  this.remoteMediaElementList = remoteMediaElementList;
> +  this.remoteStreamIndex = 0;
> +  this.gUMConstriantList = gUMConstriantList || [];

Given that we will have to specify the constraints in nearly all the cases I would make it a required parameter and the test can pass in an empty array if necessary.

@@ +22,5 @@
> +  this.remoteMediaElementList = remoteMediaElementList;
> +  this.remoteStreamIndex = 0;
> +  this.gUMConstriantList = gUMConstriantList || [];
> +  this.localMediaElementList = localMediaElementList || [];
> +  this.peerConnection = peerConnection || new mozRTCPeerConnection();

Would there be cases when we have to create the peer connection in the test itself? I would rather go ahead and always use the wrapper.

@@ +39,5 @@
> +  containsRemoteStream : function PCW_containsRemoteStream(stream) {
> +    for(var i = 0; i < this.peerConnection.remoteStreams.length; i++) {
> +      if(stream === this.peerConnection.remoteStreams[i]) {
> +        return true;
> +      }

What about containsLocalStream? It should be added too.

@@ +55,5 @@
> +   * @param {Function} onSuccess the success callback when this handshake
> +   *                             completes
> +   */
> +  handShake : function PCW_handShake(remotePeerConnection, onSuccess) {
> +    var localOffer = null;

We should store a reference to the remote peer on this object too, and I think the handshake should get a PeerConnectionWrapper given that this is the type of object we want to work with. That way we would be able to update the remote peer with the same information when the hand shake was successful.

@@ +138,5 @@
> +   *
> +   * @param {Number} index the current index of the gUM constriants
> +   * @param {Function} onSuccess the success callback when gUM setup finishes
> +   */
> +  setupGUMStreams : function PCW_setupGUMStreams(index, onSuccess) {

We shouldn't expose the index to any outer code. As best we create an inner function which can be used for the recursion.

@@ +183,5 @@
> +      description = this.peerConnection.remoteDescription;
> +    } catch (e) {
> +      exception = e;
> +    }
> +    ok(exception, "Accessing remoteDescription after close throws exception");

I think both exception checks are better suited for a new mochitest which covers any kind of access after close.

@@ +195,5 @@
> + * @param {List} pcWrapperList The list of peer connection wrappers
> + * @param {Function} onSuccess The success callback when setup finishes
> + */
> +function setupPeerConnectionWrappers(index, pcWrapperList, onSuccess) {
> +  if(index < pcWrapperList.length) {

Same as for setupGUMStreams.

@@ +210,5 @@
> + *
> + * @param {List} The list of peer connection wrappers
> + */
> +function tearDownPeerConnectionWrappers(pcWrapperList) {
> +  for(var i = 0; i < pcWrapperList.length; i++) {

nit: applies to if, for, and other statements where you should add a blank before the opening bracket.

@@ +226,5 @@
> +function sessionDescriptionCheck(sessionDescription, type) {
> +  ok(sessionDescription, "sdp is non-null");
> +  is(sessionDescription.type, type, "sdp type is " + type);
> +  ok(sessionDescription.sdp.length > 10,
> +     "session description body length is plausible");

If the description is null you will get an exception with the last ok because you are trying to access a property of an undefined object.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Comment on attachment 709450 [details] [diff] [review]
> PC Test Framework Cleanup v1
> 
> Review of attachment 709450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think that's a great start. I have made a couple of general comments
> inline which are mostly about the API, and which we will have to discuss
> further. Given that I would like to see some API changes I will give f- for
> now.
> 
> ::: dom/media/tests/mochitest/Makefile.in
> @@ -18,5 @@
> >    test_peerConnection_basicAudio.html \
> >    test_peerConnection_basicAudioVideo.html \
> >    test_peerConnection_basicAudioVideoCombined.html \
> >    test_peerConnection_basicVideo.html \
> > -  test_peerConnection_bug827843.html \
> 
> Why is this file getting removed?

Because the refactoring done here removes the need to continue to have this test.

> 
> ::: dom/media/tests/mochitest/peerConnectionWrapper.js
> @@ +10,5 @@
> > + * @param {List} remoteMediaElementList the list of media elements representing
> > + *                                      remote media content
> > + * @param {List} localMediaElementList (optional) the list of media elements
> > + *                                                representing local media
> > + *                                                content
> 
> What if we would let the wrapper itself let create those media elements?
> That would drop a lot of the extra setup steps.

Perhaps. Let me think about this.

> 
> @@ +11,5 @@
> > + *                                      remote media content
> > + * @param {List} localMediaElementList (optional) the list of media elements
> > + *                                                representing local media
> > + *                                                content
> > + * @param {List} gUMConstriantList (optional) the list of gUM constriants to be
> 
> nit: constriants vs. constraints

For now, I'll backlog this, but I'll focus on the feedback level items (I know there's type-os like these in the patch).

> 
> @@ +18,5 @@
> > + * @param {PeerConnection} (optional) the peer connection object to use
> > + */
> > +function PeerConnectionWrapper(remoteMediaElementList, localMediaElementList,
> > +  gUMConstriantList, peerConnection) {
> > +  this.remoteMediaElementList = remoteMediaElementList;
> 
> I didn't wanted to comment on this indentation style but here you can see
> how hard it is to read. You cannot see the difference between parameters and
> the actual code of this function.

I'm holding comments on this until I hear back from others on the policy on this. But I'm backlogging this item to focus on the feedback items.

> 
> @@ +20,5 @@
> > +function PeerConnectionWrapper(remoteMediaElementList, localMediaElementList,
> > +  gUMConstriantList, peerConnection) {
> > +  this.remoteMediaElementList = remoteMediaElementList;
> > +  this.remoteStreamIndex = 0;
> > +  this.gUMConstriantList = gUMConstriantList || [];
> 
> Given that we will have to specify the constraints in nearly all the cases I
> would make it a required parameter and the test can pass in an empty array
> if necessary.

Yeah, I agree.

> 
> @@ +22,5 @@
> > +  this.remoteMediaElementList = remoteMediaElementList;
> > +  this.remoteStreamIndex = 0;
> > +  this.gUMConstriantList = gUMConstriantList || [];
> > +  this.localMediaElementList = localMediaElementList || [];
> > +  this.peerConnection = peerConnection || new mozRTCPeerConnection();
> 
> Would there be cases when we have to create the peer connection in the test
> itself? I would rather go ahead and always use the wrapper.

Yeah. I think the tests I was thinking about where this could be useful is if you wanted to make use of providing parameters to mozRTCPeerConnection (it accepts a configuration). By default, I don't think we care about it (which is why we create a mozRTCPeerConnnection with no parameters), but I left the door open in case a test ends up caring about it.

> 
> @@ +39,5 @@
> > +  containsRemoteStream : function PCW_containsRemoteStream(stream) {
> > +    for(var i = 0; i < this.peerConnection.remoteStreams.length; i++) {
> > +      if(stream === this.peerConnection.remoteStreams[i]) {
> > +        return true;
> > +      }
> 
> What about containsLocalStream? It should be added too.

Probably worth while to add, although no test was taking advantage of this yet. It was intended as a refactoring patch. Although I could quickly add it.

> 
> @@ +55,5 @@
> > +   * @param {Function} onSuccess the success callback when this handshake
> > +   *                             completes
> > +   */
> > +  handShake : function PCW_handShake(remotePeerConnection, onSuccess) {
> > +    var localOffer = null;
> 
> We should store a reference to the remote peer on this object too, and I
> think the handshake should get a PeerConnectionWrapper given that this is
> the type of object we want to work with. That way we would be able to update
> the remote peer with the same information when the hand shake was successful.

Ehh...I was trying to avoid having more than one peer connection object on this wrapper object instead have the OO design work something like:

I'm the local peer, what remote peer am I going to talk to?

Especially when we get to network topology tests, we definitely don't want tight binding between the local and remote peers. You can see an example of a test I demonstrated this on that one bug I filed recently where I had a local peer talk to two remote peers.

So I'd rather keep the binding between the local and remote peers separated as much as possible and allow for taking in a parameter to hook up to a remote peer.

The reason why I choose the parameter to be a "mozRTCPeerConnection" and not the "PeerConnectionWrapper" was cause there was no need to give the full object to this function. Only the peer connection object was necessary.

> 
> @@ +138,5 @@
> > +   *
> > +   * @param {Number} index the current index of the gUM constriants
> > +   * @param {Function} onSuccess the success callback when gUM setup finishes
> > +   */
> > +  setupGUMStreams : function PCW_setupGUMStreams(index, onSuccess) {
> 
> We shouldn't expose the index to any outer code. As best we create an inner
> function which can be used for the recursion.

Yeah, I'll clean that up.

> 
> @@ +183,5 @@
> > +      description = this.peerConnection.remoteDescription;
> > +    } catch (e) {
> > +      exception = e;
> > +    }
> > +    ok(exception, "Accessing remoteDescription after close throws exception");
> 
> I think both exception checks are better suited for a new mochitest which
> covers any kind of access after close.

Ehh...I don't really agree with that. The goal introducing checks within the object was for cases that we know will always be true for the tests we know about.

If so happens that we need to refactor this out for some other purpose in the future, that's fine. But getting the benefit of quickly being able to common checks across multiple tests (in the case of the basics) keeps this code cleaner IMO.

> 
> @@ +195,5 @@
> > + * @param {List} pcWrapperList The list of peer connection wrappers
> > + * @param {Function} onSuccess The success callback when setup finishes
> > + */
> > +function setupPeerConnectionWrappers(index, pcWrapperList, onSuccess) {
> > +  if(index < pcWrapperList.length) {
> 
> Same as for setupGUMStreams.

Yeah I'll clean that up.

> 
> @@ +210,5 @@
> > + *
> > + * @param {List} The list of peer connection wrappers
> > + */
> > +function tearDownPeerConnectionWrappers(pcWrapperList) {
> > +  for(var i = 0; i < pcWrapperList.length; i++) {
> 
> nit: applies to if, for, and other statements where you should add a blank
> before the opening bracket.

Will backlog style issues post feedback items being resolved (there's issues like this in the patch already known).

> 
> @@ +226,5 @@
> > +function sessionDescriptionCheck(sessionDescription, type) {
> > +  ok(sessionDescription, "sdp is non-null");
> > +  is(sessionDescription.type, type, "sdp type is " + type);
> > +  ok(sessionDescription.sdp.length > 10,
> > +     "session description body length is plausible");
> 
> If the description is null you will get an exception with the last ok
> because you are trying to access a property of an undefined object.

Yeah, I can add some quick if checks for undefined just for safety.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 709450 [details] [diff] [review]
PC Test Framework Cleanup v1

Review of attachment 709450 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/peerConnectionWrapper.js
@@ +18,5 @@
> + * @param {PeerConnection} (optional) the peer connection object to use
> + */
> +function PeerConnectionWrapper(remoteMediaElementList, localMediaElementList,
> +  gUMConstriantList, peerConnection) {
> +  this.remoteMediaElementList = remoteMediaElementList;

See my comments on another bug about this issue.  I'm ok with this if it's consistent, given the indent needs for function closures

@@ +39,5 @@
> +  containsRemoteStream : function PCW_containsRemoteStream(stream) {
> +    for(var i = 0; i < this.peerConnection.remoteStreams.length; i++) {
> +      if(stream === this.peerConnection.remoteStreams[i]) {
> +        return true;
> +      }

NOTE: you can't compare streams generally, and in particular the stream going into pc1 and coming out of pc2 can't be compared.

@@ +151,5 @@
> +        self.peerConnection.addStream(stream);
> +
> +        if(localMediaElement) {
> +          localMediaElement.mozSrcObject = stream;
> +          localMediaElement.play();

Local media normally should be muted, but probably doesn't matter in mochitests though may be surprising if it plays real audio

@@ +210,5 @@
> + *
> + * @param {List} The list of peer connection wrappers
> + */
> +function tearDownPeerConnectionWrappers(pcWrapperList) {
> +  for(var i = 0; i < pcWrapperList.length; i++) {

> nit: applies to if, for, and other statements where you should add a blank before the opening bracket.

Apply throughout

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ -1,4 @@
> -<!DOCTYPE HTML>
> -<html>
> -<!--
> -https://bugzilla.mozilla.org/show_bug.cgi?id=827843

Probably don't drop this one, and have it go back to being the place to test for post-close API issues

Comment 11

[cmtalbert]

(In reply to Randell Jesup [:jesup] from comment #10)

> @@ +151,5 @@
> > +        self.peerConnection.addStream(stream);
> > +
> > +        if(localMediaElement) {
> > +          localMediaElement.mozSrcObject = stream;
> > +          localMediaElement.play();
> 
> Local media normally should be muted, but probably doesn't matter in
> mochitests though may be surprising if it plays real audio
> 
Quick drive by comment.  If it's not important for the test, we'd prefer to have it muted. You can't imagine the noise of a couple hundred machines in a datacenter running mochitests.  It drives our DCops folks crazy.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #10)
> Comment on attachment 709450 [details] [diff] [review]
> PC Test Framework Cleanup v1
> 
> Review of attachment 709450 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/mochitest/peerConnectionWrapper.js
> @@ +18,5 @@
> > + * @param {PeerConnection} (optional) the peer connection object to use
> > + */
> > +function PeerConnectionWrapper(remoteMediaElementList, localMediaElementList,
> > +  gUMConstriantList, peerConnection) {
> > +  this.remoteMediaElementList = remoteMediaElementList;
> 
> See my comments on another bug about this issue.  I'm ok with this if it's
> consistent, given the indent needs for function closures
> 
> @@ +39,5 @@
> > +  containsRemoteStream : function PCW_containsRemoteStream(stream) {
> > +    for(var i = 0; i < this.peerConnection.remoteStreams.length; i++) {
> > +      if(stream === this.peerConnection.remoteStreams[i]) {
> > +        return true;
> > +      }
> 
> NOTE: you can't compare streams generally, and in particular the stream
> going into pc1 and coming out of pc2 can't be compared.

Okay, makes sense. Will look into this.

> 
> @@ +151,5 @@
> > +        self.peerConnection.addStream(stream);
> > +
> > +        if(localMediaElement) {
> > +          localMediaElement.mozSrcObject = stream;
> > +          localMediaElement.play();
> 
> Local media normally should be muted, but probably doesn't matter in
> mochitests though may be surprising if it plays real audio
> 
> @@ +210,5 @@
> > + *
> > + * @param {List} The list of peer connection wrappers
> > + */
> > +function tearDownPeerConnectionWrappers(pcWrapperList) {
> > +  for(var i = 0; i < pcWrapperList.length; i++) {
> 
> > nit: applies to if, for, and other statements where you should add a blank before the opening bracket.
> 
> Apply throughout

Will fix

> 
> ::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
> @@ -1,4 @@
> > -<!DOCTYPE HTML>
> > -<html>
> > -<!--
> > -https://bugzilla.mozilla.org/show_bug.cgi?id=827843
> 
> Probably don't drop this one, and have it go back to being the place to test
> for post-close API issues

Okay. So what I think the right idea then addressing Henrik and your comments is to possibility break out the consistent close checks into a helper away from close() and then reuse it for this test. We'll need to extend this test to not just test one type of stream though (which was the original intention for moving it into the framework - to get more coverage through different stream combinations).

Comment 13

[cmtalbert]

(In reply to Clint Talbert ( :ctalbert ) from comment #11)
> (In reply to Randell Jesup [:jesup] from comment #10)
> 
> > @@ +151,5 @@
> > > +        self.peerConnection.addStream(stream);
> > > +
> > > +        if(localMediaElement) {
> > > +          localMediaElement.mozSrcObject = stream;
> > > +          localMediaElement.play();
> > 
> > Local media normally should be muted, but probably doesn't matter in
> > mochitests though may be surprising if it plays real audio
> > 
> Quick drive by comment.  If it's not important for the test, we'd prefer to
> have it muted. You can't imagine the noise of a couple hundred machines in a
> datacenter running mochitests.  It drives our DCops folks crazy.
Actually, I thought about this while making lunch.  All our minis *should* have their volume physically turned down now (I believe we did that as part of the massive SCL1 migration last year). But, it's still hard to get 1700+ machines all configured the same way.  But either way the comment still stands -- if your test needs noise,by all means make noise.  But if you don't then muting by default is the way I'd prefer us to go.

Comment 14

[Eric Rescorla (:ekr)]

Attachment: Mochitest refactor

Comment 15

[Eric Rescorla (:ekr)]

Attachment: Mochitest refactor

Comment 16

[Eric Rescorla (:ekr)]

Attachment: Mochitest refactor

Comment 17

[Eric Rescorla (:ekr)]

Attachment: Mochitest refactor

Comment 18

[Eric Rescorla (:ekr)]

Attachment: Mochitest refactor

Comment 19

[Eric Rescorla (:ekr)]

Attached above is a refactor of the tests that dramatically reduces the code required to write the
basic sunny day tests.

This is a WIP because we need to also do the non sunny-day tests and also some cleanup
is needed.

The basic idiom is going to be to add/remove/replace entries in the list of operations to
perform. Someone will need to write accessors to manipulate the list, but I don't expect
that to be hard.

Comment 20

[Jason Smith [:jsmith]]

Comment on attachment 714940 [details] [diff] [review]
Mochitest refactor

Review of attachment 714940 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, I like the direction this is going. I avoided picking apart style issues here and focused on the overall framework structure.

Some major areas I'll note to work on includes:

- Improve separation of concerns of major areas and avoid duplicating existing framework logic - avoid tight binding
- We need to prove that the framework can do two different things - support a more than 2 PC case & a different command structure - can you create a PoC that proves that either of these work?

Probably needs another feedback pass though before heading to review though. Giving a feedback+ though for the direction being good.

::: dom/media/tests/mochitest/Makefile.in
@@ +33,5 @@
>    test_peerConnection_bug827843.html \
>    test_peerConnection_bug825703.html \
>    test_peerConnection_bug834153.html \
>    test_peerConnection_bug840344.html \
> +  pc_test_framework.js \

So there is some duplication now across pc.js and this. Can we figure out a way to avoid the duplication?

::: dom/media/tests/mochitest/pc_test_framework.js
@@ +1,1 @@
> +var IncludeScripts = function(scripts) {

This doesn't belong in pc_test_framework.js. This really belongs in head.js.

@@ +7,5 @@
> +		    });
> +};
> +
> +
> +var CreateTest = function(meta, run) {

All of this belongs in head.js, not this file.

@@ +41,5 @@
> +	content.setAttribute('id', 'content');
> +	document.body.appendChild(content);
> +    };
> +
> +    var Run = function(run) {

Uhh...don't do this. We shouldn't be duplicating the head.js logic here. Reuse the head.js logic and extend it as needed. And pull this logic out of pc_test_framework.js - this logic belongs in head.js.

@@ +70,5 @@
> +    CreateHTML(meta);
> +    Run(run);
> +};
> +
> +console.log("Including scripts");

Ehh...let's avoid globally defining this. Or define this within a consistent function call we use (run for example).

@@ +77,5 @@
> +		   'head.js',
> +		   'mediaStreamPlayback.js'
> +	       ]);
> +
> +var assert_ok = function(condition, message) {

This belongs in head.js, not here.

@@ +84,5 @@
> +        SimpleTest.finish()
> +};
> +
> +
> +var PeerConnectionInstance = function(framework, pc_label, pc_configuration) {

I'd rather split this out such that there's a clear distinction between the constructor and functions. See how MediaStreamPlayback is structured - let's stick to that style.

@@ +94,5 @@
> +    this.pc = new mozRTCPeerConnection(pc_configuration);
> +
> +    this.streams = [];
> +
> +    this.pc.onaddstream = function(event) {

I'm not sure I'd force this on setup always. I'd make this a step in the command structure to set this up. Some tests may or may not want this setup depending on what's intended to be tested.

@@ +103,5 @@
> +    assert_ok(this.pc, "Created PC");
> +
> +    // Make the element to put a given stream into.
> +    var makeElementForStream = function(type, side) {
> +        var stream_label = pc_label + '_' + side + '_' + type;

I feel like this logic should pulled out into a small helper function so that we know where we define this syntax clearly.

@@ +122,5 @@
> +    };
> +    
> +
> +    // Callback to call when we get media from either side
> +    this.attachMedia = function(stream, type, side) {

Note - this is kinda duplicating what media stream playback does. Can reuse some of the logic here?

@@ +124,5 @@
> +
> +    // Callback to call when we get media from either side
> +    this.attachMedia = function(stream, type, side) {
> +        info("Got media stream: " + type + ", " + side);
> +        // this.streams.append();

Why is this commented out?

@@ +132,5 @@
> +        element.play();
> +    };
> +
> +    // Get user media based on contraints.
> +    this.getUserMedia = function(constraints, success) {

Given that this already defined elsewhere, this gets a bit confusing to have this defined here as well. Can we choose a better name here? There's more than just gUM getting called here.

@@ +136,5 @@
> +    this.getUserMedia = function(constraints, success) {
> +        var self = this;
> +        var type = '';
> +
> +        if (constraints.audio) {

So why does this if statement logic have to be here?

@@ +153,5 @@
> +                     },
> +                     unexpectedCallbackAndFinish);
> +    };
> +
> +    this.getAllUserMediaInternal = function(constraints_list, index) {

Naming feels a bit confusing here - can you choose a better name for what's being done here? I think I originally called this setupGUM in the other patch.

Also - small nit that was mentioned in the other patch - don't expose the index parameter to the caller. Put an internal function in here that has that logic:

getAllUserMediaInternal = function(constriants_list) {
   function x(constriants_list, index) {
         // logic
   }
   
   x(constriants_list, 0);
}

@@ +166,5 @@
> +                                  );
> +                              }
> +                              );
> +        } else {
> +            self.framework.executeNext();

I feel like we shouldn't hard define this in this code and instead use a success callback here. Then, we would pass in self.framework.executeNext into this function.

@@ +171,5 @@
> +        }
> +    };
> +
> +
> +    this.getAllUserMedia = function(constraints_list) {

See the comment above - I'd do the internal function approach instead.

@@ +175,5 @@
> +    this.getAllUserMedia = function(constraints_list) {
> +        info("getAllUserMedia: " + constraints_list.length + " streams");
> +        this.getAllUserMediaInternal(constraints_list, 0);
> +
> +        return false;  // Asynchronous

Feels odd doing the return explicitly - maybe naming these style of functions might help improve clarity of why we return true/false for these.

@@ +183,5 @@
> +        var self = this;
> +
> +        this.pc.createOffer(function(offer) {
> +                           info("Got offer " + JSON.stringify(offer));
> +                           self.last_offer = offer;

Preferably, I'd always keep a running list of this to keep a clear understanding of history of data.

@@ +184,5 @@
> +
> +        this.pc.createOffer(function(offer) {
> +                           info("Got offer " + JSON.stringify(offer));
> +                           self.last_offer = offer;
> +                           self.framework.executeNext();

Same comment as above - I'd use a success callback and pass in self.framework.executeNext.

@@ +197,5 @@
> +
> +        this.pc.createAnswer(function(answer) {
> +                           info("Got answer " + JSON.stringify(answer));
> +                           self.last_answer = answer;
> +                           self.framework.executeNext();

Same comment as above - I'd use a success callback and pass in self.framework.executeNext.

@@ +209,5 @@
> +        var self = this;
> +
> +        this.pc.setLocalDescription(sdp, function() {
> +                                        info("Successfully setLocalDescription");
> +                                        self.framework.executeNext();

Same comment as above - I'd use a success callback and pass in self.framework.executeNext.

@@ +221,5 @@
> +        var self = this;
> +
> +        this.pc.setRemoteDescription(sdp, function() {
> +                                        info("Successfully setRemoteDescription");
> +                                        self.framework.executeNext();

Same comment as above - I'd use a success callback and pass in self.framework.executeNext.

@@ +229,5 @@
> +        return false;  // So we await callback.
> +    };
> +
> +    // Check that we are getting the media we expect
> +    this.checkMedia = function(local_constraints, remote_constraints) {

I'm wondering if it would be better off separating class logic by:

- General setup
- Commands
- Helper functions for checks

@@ +247,5 @@
> +    };
> +};
> +
> +
> +PeerConnectionTest = function(config1, config2) {

Similar comment as above - I'd use the constructor + prototype approach seen in media stream playback.

@@ +250,5 @@
> +
> +PeerConnectionTest = function(config1, config2) {
> +    var test = this; 
> +    var pc1 = new PeerConnectionInstance(this, 'PC1', config1);
> +    var pc2 = new PeerConnectionInstance(this, 'PC2', config2 || config1);

This really gets into defining the rules of what we executed under a certain set of tests. Knowing that, this might belong in a separate file that defines a "template" that a set of mochitests use. In this case, this would represent the template we are using for the basic pc smoke tests.

@@ +256,5 @@
> +    var pc2_constraints = null;
> +
> +    var execution_step = 0;  // The next step to execute.
> +
> +    this.cleanup = function() {

Hmm...I'm seeing tight binding between two concepts here:

- Framework management
- The specific PC configuration under test

Split these two concepts out into separate classes and have one use the other class.

For this function - this belongs as part of the "template" PC configuration under test.

@@ +269,5 @@
> +	}
> +    };
> +
> +    // The function to execute the next command in the list.
> +    this.executeNextInternal = function() {

This would be part of the framework management.

@@ +275,5 @@
> +            var this_step = to_execute[execution_step++];
> +
> +            info(this_step[0]);  // Print the label
> +            this_step[1]();
> +	    if (this_step[2] && this_step[2].async) {

I feel like we should aim to make the async vs. sync logic as transparent as possible. This feels like it exposes the transparency potentially.

@@ +288,5 @@
> +        SimpleTest.finish();
> +    };
> +
> +
> +    this.executeNext = function() {

This would be part of the framework management.

@@ +290,5 @@
> +
> +
> +    this.executeNext = function() {
> +	var self = this;
> +	setTimeout(function() {

Uhh...what is this setTimeout doing? It doesn't look necessary?

@@ +298,5 @@
> +    
> +    // The list of commands to execute.
> +    // Format is:
> +    // [ <string-label> ]
> +    var to_execute = [

This logic really belongs as part of the "template" for a set of tests, not the general framework.

@@ +375,5 @@
> +        ],
> +	[
> +	    'Wait 5 seconds',
> +	    function() {
> +		setTimeout(function() {

Why is this necessary? Do we really need to use setTimeout here? This might not play nicely on our CI machines.

@@ +384,5 @@
> +		async:true
> +	    }
> +	],
> +	[
> +	    'PC1_CHECKMEDIA',

I almost feel like there should be some separation of command logic between "execution of logic" commands and "check commands."

@@ +397,5 @@
> +	    }
> +	],
> +    ];
> +
> +    this.setGetUserMediaConstraints = function(constraints1, constraints2) {

This belongs as part of the "template" pc configuration.

@@ +402,5 @@
> +        pc1_constraints = constraints1;
> +        pc2_constraints = constraints2;
> +    };
> +
> +    this.runTests = function() {

This belongs as part of the framework.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Clint Talbert ( :ctalbert ) from comment #11)
> > > +        self.peerConnection.addStream(stream);
> > > +
> > > +        if(localMediaElement) {
> > > +          localMediaElement.mozSrcObject = stream;
> > > +          localMediaElement.play();
> > 
> > Local media normally should be muted, but probably doesn't matter in
> > mochitests though may be surprising if it plays real audio
> > 
> Quick drive by comment.  If it's not important for the test, we'd prefer to
> have it muted. You can't imagine the noise of a couple hundred machines in a
> datacenter running mochitests.  It drives our DCops folks crazy.

Clint, I run all the media mochitests lately and none of the media tests muted audio. Not sure why that is but lets ask Roc if we should follow-up on that separately.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Eric, it's hard for me to step in after you and Jason worked together on this new skeleton for WebRTC mochitests. Now seeing all those comments from Jason without a reply from you doesn't make it easier for me. Would you mind to answer at least the important ones so that I'm up2date of what you both agreed on? I will check the current patch but don't want to repeat the same questions. So my feedback this time will only be about the general implementation.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 714940 [details] [diff] [review]
Mochitest refactor

Review of attachment 714940 [details] [diff] [review]:
-----------------------------------------------------------------

In general I think it looks fine but I cannot comment on all those complains from Jason. So please help me with the outcome of the pair programming session.

Beside that something I would propose is that we make use of Task.jsm and promises if possible in mochitests. Those two fairly new common modules would make our life a lot easier when it comes to synchronizing async methods. Please see the following two links:

resource://gre/modules/commonjs/sdk/core/promise.js
resource://gre/modules/Task.jsm

How it works can be seen in the browser chrome test as attached here:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=820834&attachment=713629

If it sounds fine with you I can cook up a little test which would demonstrate this technology.

Some smaller comments inline...

::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html
@@ +11,5 @@
> +     },
> +     function() {
> +        var tests = new PeerConnectionTest();
> +        tests.setGetUserMediaConstraints([{audio:true}],[{audio:true}]);
> +        tests.runTests();

So I do not see how individual tests can execute special checks only appropriate to this single test. I don't think that putting all tasks into the framework is a good solution.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Henrik Skupin (:whimboo) from comment #21)
> Clint, I run all the media mochitests lately and none of the media tests
> muted audio. Not sure why that is but lets ask Roc if we should follow-up on
> that separately.

Yes I think we should.

Ideally the test machines would be muted at the system level somehow.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 709450 [details] [diff] [review]
PC Test Framework Cleanup v1

As discussed in the meeting this patch is obsolete now.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I will wait for Eric's feedback as requested in the meeting, and then will start updating the patch to incorporate the before mentioned changes.

Comment 27

[Eric Rescorla (:ekr)]

Henrik,

I think the major thing that I was supposed to weigh in on was how to adapt
the tests to allow other test sequences, e.g., multiple createOffers.

What we want to do to make this work is, I think, to allow external
access to the sequence of operations. Here is what I suggest:

1. Have the PeerConnectionTest take an optional "steps" argument
    which gets stuffed into the to_execute array.

2. Have a set of accessors that allow you to get/set the steps

    function getStepsToExecute() { return to_execute; }
    function setStepsToExecute(steps) { to_execute = steps; }
    function insertStepsAfter(step_name, steps) { ..; }  // Inserts steps after a named step
    function insertStepsBefore(step_name, steps) { ..; }  // Inserts steps before a named step
    function truncateStepsAfter(step_name) { ...}
    function truncateStepsAt(step_name)  { ...}

    Note that if you have the first two, the last 4 are just syntactic sugar, but I think
    nice sugar.

    So, for instance, if you wanted to verify that once stuff had set up,
    you could call createOffer twice, you would do:

    pctests.insertStepsAfter('PC1_CREATEOFFER',
                             [[  'PC1_CREATESECONDOFFER',
                                function() {
                                  this.previous_offer = last_offer;
				  pc1.createOffer();
                                },
				{ async:true }
			     ],
                             [  'PC1_CHECKOFFER',
                                function() {
				  assert_ok(this.previous_offer == pc1.last_offer);
                                };
			     ]]);


3. We haven't discussed negative tests, but I think the answer here is
   add an argument that means you expect failure. E.g.,

   pctests.insertStepsAfter('PC1_CREATEOFFER',
			    [[  'PC1_CLOSE',
                               function() { pc1.close(); },
			    ],
                            [  'PC1_CREATESECONDOFFER',
                                function() {
      				  pc1.createOffer();
                                },
				{ async:true, expect_fail:true }
			    ]]);
			      
 And this would invert the expected success/fail stuff.

How does that look to you?

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: WIP 0.1

First WIP after todays work which include:

* Convert objects into prototypes for re-usable classes
* Fix boilerplate generation so we are keeping elements as needed for mochitests
* Created Test class for Gum tests which will allow us to generate different boilerplates for gum/pc/datachannel tests
* Remove dynamic js file inclusion because it doesn't work due to async loading - we will most likely tackle that in a follow-up bug
* Add media elements under content node and not body so it's hidden by default for faster test execution
* Killed duplicated code for pc (gum stuff is still necessary to do) 
* Included more tests for verification
* other minor fixes of typos etc

Eric and Jason, what do you think? It would be good to also get some answers to Jason's review comments to get those things clarified. Otherwise I will have to ask tomorrow. Thanks.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Yes I think we should.
> 
> Ideally the test machines would be muted at the system level somehow.

Filed as bug 845452.

Comment 30

[Jason Smith [:jsmith]]

Comment on attachment 718520 [details] [diff] [review]
WIP 0.1

Review of attachment 718520 [details] [diff] [review]:
-----------------------------------------------------------------

feedback- only for the removal of the tests & modification of gUM automation - we should not be removing tests at all as a result of this patch. If it means that we have to duplicate temporarily for other tests, then do so. But I do not want to pull tests out of the tree as a result of the refactoring.

The rest of the patch was okay approach-wise. We're getting closer to getting the refactoring pieces done.

::: dom/media/tests/mochitest/Makefile.in
@@ -15,4 @@
>    test_getUserMedia_basicAudio.html \
>    test_getUserMedia_basicVideo.html \
>    test_getUserMedia_basicVideoAudio.html \
> -  test_getUserMedia_gumWithinGum.html \

Uhh...why are you removing all of these tests?

::: dom/media/tests/mochitest/head.js
@@ +18,2 @@
>   */
> +function GumTest(options) {

options...for what purpose?

@@ +23,2 @@
>  
> +GumTest.prototype = {

I thought we were straying away from doing this and instead hardcoding the typical HTML logic in the file. The naming feels a bit confusing too...it feels more like a HTML template generator. If we are still going with the generator, I'd pull out this into just a general helper function in head.js everyone else uses.

@@ +61,5 @@
> +    content.style.display = meta.visible ? 'block' : "none";
> +    document.body.appendChild(content);
> +  },
> +
> +  run: function GT_run(aCallback) {

Different patch please for the gUM refactoring. I'd rather not modify anything related to gUM automation in this patch. Keep this to the PC refactoring only.

@@ +70,5 @@
> +    }, this._desktopOnly);
> +  }
> +};
> +
> +function PeerConnectionTest(options) {

This definitely doesn't belong in head.js. Separate file.

@@ +72,5 @@
> +};
> +
> +function PeerConnectionTest(options) {
> +  options = options || { };
> +  GumTest.call(this, options);

Pull this out into a constructor. The flow of logic here seems...odd. Clarify?

@@ +80,5 @@
> +  this.pc1_constraints = null;
> +  this.pc2_constraints = null;
> +
> +  this.pc1 = new PeerConnectionWrapper(this, 'PC1', options.config1);
> +  this.pc2 = new PeerConnectionWrapper(this, 'PC2', options.config2 || options.config1);

I'm not sure we should out right force a 1:1 mapping. Can you allow for customization of the parameters and default to this if not specified?

@@ +87,5 @@
> +
> +  // The list of commands to execute.
> +  // Format is:
> +  // [ <string-label> ]
> +  this.to_execute = [

Doesn't belong in head.js. This should be up to the caller to specify in a template.

@@ +187,5 @@
> +    ]
> +  ];
> +}
> +
> +PeerConnectionTest.prototype = new GumTest();

I don't think there should be an inheritance chain between generating the boilerplate HTML and the framework logic.

@@ +190,5 @@
> +
> +PeerConnectionTest.prototype = new GumTest();
> +PeerConnectionTest.prototype.constructor = PeerConnectionTest;
> +
> +PeerConnectionTest.prototype.cleanup = function () {

Nit - Given that we are closing PCs here, I'd just call this closeAll.

@@ +208,5 @@
> +  var self = this;
> +
> +  function _executeNext() {
> +    while (self.execution_step < self.to_execute.length) {
> +      var this_step = self.to_execute[self.execution_step++];

I'd pull the increment of the execution step out to a separate line.

@@ +210,5 @@
> +  function _executeNext() {
> +    while (self.execution_step < self.to_execute.length) {
> +      var this_step = self.to_execute[self.execution_step++];
> +
> +      info(this_step[0]);  // Print the label

I think it might be worthwhile for readability to instead use an object for these "command objects" with appropriate naming attributes that follow suit. Something that falls in alignment with the Gang of Four command pattern would probably be appropriate. So something that has attributes like:

- label
- step
- etc...

@@ +220,5 @@
> +    }
> +
> +    info("Tests finished");
> +
> +    self.cleanup();

I think closing the PCs should really be done an execution step, not a forced step. There's cases where we might want to close the PCs as the first step, for example (such as the one test Adam wrote which is close the PCs and do stuff on each API function after close).

@@ +225,4 @@
>      SimpleTest.finish();
> +  };
> +
> +  window.setTimeout(_executeNext, 0);

Why are we calling setTimeout out right here?

@@ +228,5 @@
> +  window.setTimeout(_executeNext, 0);
> +};
> +
> +
> +PeerConnectionTest.prototype.setGetUserMediaConstraints = function (constraints1, constraints2) {

Shouldn't this happen as part of the constructor logic? And if the constraints are being modified after, does anything else need to be modified?

@@ +233,5 @@
> +  this.pc1_constraints = constraints1;
> +  this.pc2_constraints = constraints2;
> +};
> +
> +PeerConnectionTest.prototype.run = function (aCallback) {

I'm a bit confused by the gUM --> PC test framework hookup here. Clarify?

::: dom/media/tests/mochitest/pc.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  
> +function PeerConnectionWrapper(framework, label, configuration) {
> +  this._framework = framework;

So the entire use of the "framework" appears to be calling the next command in the list. So I don't think you need the entire framework object here.

I'd prefer if we had a way such that we remove the tight binding between calling the next command in the list vs. having a custom function if possible (in case we have a special custom need).

Can you accept a success function instead and have the execute command logic know how to handle the success functions?

I'd rather avoid doing a class obj <---> class obj style dependency if possible.

@@ +18,5 @@
> +    // Assume type is video until we get get{Audio,Video}Tracks.
> +    self.attachMedia(event.stream, 'audiovideo', 'remote');
> +  };
> +
> +  assert_ok(this._pc, "Created PC");

Do this before setting up onaddstream.

@@ +24,5 @@
> +
> +PeerConnectionWrapper.prototype = {
> +
> +  // Make the element to put a given stream into.
> +  makeElementForStream: function (type, side) {

Probably useful for gUM tests as well at some point in the future, so it might be worth to pull this into a global function both sets of tests can use.

@@ +53,5 @@
> +    element.play();
> +  },
> +
> +  // Get user media based on contraints.
> +  getUserMedia: function (constraints, success) {

I think we need a better naming convention here - we've got a bunch of functions all named getUserMedia now. Can we try a different name?

@@ +96,5 @@
> +  getAllUserMedia:function (constraints_list) {
> +    info("getAllUserMedia: " + constraints_list.length + " streams");
> +    this.getAllUserMediaInternal(constraints_list, 0);
> +
> +    return false;  // Asynchronous

The returns are a bit strange to indicate async. Why?

For anything that's a command, we should really take advantage of a command pattern to do this.

@@ +99,5 @@
> +
> +    return false;  // Asynchronous
> +  },
> +
> +  createOffer:function (constraints) {

See comment above about using a command pattern.

@@ +112,5 @@
> +
> +    return false;  // So we await callback.
> +  },
> +
> +  createAnswer:function (constraints) {

See comment above about using a command pattern.

@@ +125,5 @@
> +
> +    return false;  // So we await callback.
> +  },
> +
> +  setLocalDescription:function (sdp) {

See comment above about using a command pattern.

@@ +137,5 @@
> +
> +    return false;  // So we await callback.
> +  },
> +
> +  setRemoteDescription:function (sdp) {

See comment above about using a command pattern.

@@ +150,5 @@
> +    return false;  // So we await callback.
> +  },
> +
> +  // Check that we are getting the media we expect
> +  checkMedia:function (local_constraints, remote_constraints) {

See comment above about using a command pattern.

@@ +162,5 @@
> +    // This is synchronous.
> +    return true;
> +  },
> +
> +  close:function () {

What's the difference here between the cleanup function we had elsewhere and this general close function?

::: dom/media/tests/mochitest/test_getUserMedia_basicAudio.html
@@ +1,1 @@
>  <!DOCTYPE HTML>

Keep the gUM refactoring to a separate patch.

::: dom/media/tests/mochitest/test_getUserMedia_basicVideo.html
@@ +1,1 @@
>  <!DOCTYPE HTML>

Keep the gUM refactoring to a separate patch.

::: dom/media/tests/mochitest/test_getUserMedia_basicVideoAudio.html
@@ +1,1 @@
>  <!DOCTYPE HTML>

Keep the gUM refactoring to a separate patch.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jason Smith [:jsmith] from comment #30)
> feedback- only for the removal of the tests & modification of gUM automation
> - we should not be removing tests at all as a result of this patch. If it
> means that we have to duplicate temporarily for other tests, then do so. But
> I do not want to pull tests out of the tree as a result of the refactoring.

This is a WIP and as I have mentioned in my comment I just put in some more tests for verification that it works. That's all. None of them will be removed in a final patch.

In general I wanted to have feedback for the things I have changed, but not a full list of already known complains from Eric's last patch. I have mentioned that I haven't changed those yet because I need answers from him first. So below I will only answer those items which are appropriate.

> > +function GumTest(options) {
> 
> options...for what purpose?

This is optional (jsdoc strings still missing as you can see) and for now serves the desktop only support flag. It's flexible for the future.

> > +GumTest.prototype = {
> 
> I thought we were straying away from doing this and instead hardcoding the
> typical HTML logic in the file. The naming feels a bit confusing too...it

No, I checked back with Eric and Clint and we wanted to keep that: "leave the boilerplate generation as it is". I have no strong opinion but that makes it easier  for test creators.

> feels more like a HTML template generator. If we are still going with the
> generator, I'd pull out this into just a general helper function in head.js
> everyone else uses.

The benefit from having it in the appropriate test class is that you can generate the right boilerplate specific to the type of test. Especially when we want to include the external js files. But we could move out the general code which will always be the same to a generator method and only keep the dynamic js insertion code in the class.

> > +  run: function GT_run(aCallback) {
> 
> Different patch please for the gUM refactoring. I'd rather not modify
> anything related to gUM automation in this patch. Keep this to the PC
> refactoring only.

I see a benefit in establishing already a GumTest class which can be the base for PCTest. Otherwise we will have to do another large refactoring of code. Eric, shall I leave this out?

> > +function PeerConnectionTest(options) {
> 
> This definitely doesn't belong in head.js. Separate file.

It depends on the future and how classes are inherited. If we are never going to have a GumTest class then this is right. I can move all that if we want to go this way.

Comment 32

[cmtalbert]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> (In reply to Jason Smith [:jsmith] from comment #30)
> > > +GumTest.prototype = {
> > 
> > I thought we were straying away from doing this and instead hardcoding the
> > typical HTML logic in the file. The naming feels a bit confusing too...it
> 
> No, I checked back with Eric and Clint and we wanted to keep that: "leave
> the boilerplate generation as it is". I have no strong opinion but that
> makes it easier  for test creators.
> 
> > feels more like a HTML template generator. If we are still going with the
> > generator, I'd pull out this into just a general helper function in head.js
> > everyone else uses.
> 
> The benefit from having it in the appropriate test class is that you can
> generate the right boilerplate specific to the type of test. Especially when
> we want to include the external js files. But we could move out the general
> code which will always be the same to a generator method and only keep the
> dynamic js insertion code in the class.
I like having per test-type boilerplate fwiw. And yeah, as I understood it from the meeting yesterday, we are OK with the existing boilerplate required in the new style tests like you see here in Eric's patch: https://bugzilla.mozilla.org/attachment.cgi?id=714940 What we decided against was removing the rest of the boilerplate in those PC tests.  In my mind, we all agreed to keep the level of boilerplate to the same level that those refactored tests contain, and that still seems like a good compromise to me.
> 
> > > +  run: function GT_run(aCallback) {
> > 
> > Different patch please for the gUM refactoring. I'd rather not modify
> > anything related to gUM automation in this patch. Keep this to the PC
> > refactoring only.
> 
> I see a benefit in establishing already a GumTest class which can be the
> base for PCTest. Otherwise we will have to do another large refactoring of
> code. Eric, shall I leave this out?
> 
> > > +function PeerConnectionTest(options) {
> > 
> > This definitely doesn't belong in head.js. Separate file.
> 
> It depends on the future and how classes are inherited. If we are never
> going to have a GumTest class then this is right. I can move all that if we
> want to go this way.

Comment 33

[Eric Rescorla (:ekr)]

Comment on attachment 718520 [details] [diff] [review]
WIP 0.1

Review of attachment 718520 [details] [diff] [review]:
-----------------------------------------------------------------

This looks basically OK to me, modulo some comments below and the bit rot from where I changed the sync/async pattern.

::: dom/media/tests/mochitest/head.js
@@ +80,5 @@
> +  this.pc1_constraints = null;
> +  this.pc2_constraints = null;
> +
> +  this.pc1 = new PeerConnectionWrapper(this, 'PC1', options.config1);
> +  this.pc2 = new PeerConnectionWrapper(this, 'PC2', options.config2 || options.config1);

What do you mean a 1:1 mapping?

@@ +87,5 @@
> +
> +  // The list of commands to execute.
> +  // Format is:
> +  // [ <string-label> ]
> +  this.to_execute = [

I am indifferent to whether the "sunny day" template lives in PCTest or elsewhere, but it clearly needs to be somewhere where people don't need to recapitulate it.

@@ +210,5 @@
> +  function _executeNext() {
> +    while (self.execution_step < self.to_execute.length) {
> +      var this_step = self.to_execute[self.execution_step++];
> +
> +      info(this_step[0]);  // Print the label

This isn't a terrible idea, but the problem is that then it chews up a bunch of extra typing....

@@ +220,5 @@
> +    }
> +
> +    info("Tests finished");
> +
> +    self.cleanup();

I suggest a compromise. Let's do it as a forced step but have it check for double calling..

@@ +225,4 @@
>      SimpleTest.finish();
> +  };
> +
> +  window.setTimeout(_executeNext, 0);

I did it to avoid having the tests start running right away as part of the ctor.

@@ +230,5 @@
> +
> +
> +PeerConnectionTest.prototype.setGetUserMediaConstraints = function (constraints1, constraints2) {
> +  this.pc1_constraints = constraints1;
> +  this.pc2_constraints = constraints2;

You invoke this function before you start the tests and do gathering.

@@ +285,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  // If this is a desktop supported test and we're on android or b2g,
> +  // indicate that the test is not supported and skip the test
> +  if(desktopOnly && (navigator.userAgent.indexOf('Android') > -1 ||

I'm not following the relationship here to the gum desktop only options.... Are they distinct? should they be merged?

::: dom/media/tests/mochitest/pc.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  
> +function PeerConnectionWrapper(framework, label, configuration) {
> +  this._framework = framework;

I deliberately didn't do it this way because it makes every function require special success cb logic. My experience is
that this sort of chaining is hard to maintain.

@@ +96,5 @@
> +  getAllUserMedia:function (constraints_list) {
> +    info("getAllUserMedia: " + constraints_list.length + " streams");
> +    this.getAllUserMediaInternal(constraints_list, 0);
> +
> +    return false;  // Asynchronous

This actually is bit rot. You no longer need to retun these.

@@ +109,5 @@
> +        self._framework.executeNext();
> +      },
> +      unexpectedCallbackAndFinish);
> +
> +    return false;  // So we await callback.

And here.

@@ +162,5 @@
> +    // This is synchronous.
> +    return true;
> +  },
> +
> +  close:function () {

This applies to this PC and not to the test. The test has two PCs.

Comment 34

[Eric Rescorla (:ekr)]

Comment on attachment 718520 [details] [diff] [review]
WIP 0.1

Review of attachment 718520 [details] [diff] [review]:
-----------------------------------------------------------------

This looks basically OK to me, modulo some comments below and the bit rot from where I changed the sync/async pattern.

::: dom/media/tests/mochitest/head.js
@@ +80,5 @@
> +  this.pc1_constraints = null;
> +  this.pc2_constraints = null;
> +
> +  this.pc1 = new PeerConnectionWrapper(this, 'PC1', options.config1);
> +  this.pc2 = new PeerConnectionWrapper(this, 'PC2', options.config2 || options.config1);

What do you mean a 1:1 mapping?

@@ +87,5 @@
> +
> +  // The list of commands to execute.
> +  // Format is:
> +  // [ <string-label> ]
> +  this.to_execute = [

I am indifferent to whether the "sunny day" template lives in PCTest or elsewhere, but it clearly needs to be somewhere where people don't need to recapitulate it.

@@ +210,5 @@
> +  function _executeNext() {
> +    while (self.execution_step < self.to_execute.length) {
> +      var this_step = self.to_execute[self.execution_step++];
> +
> +      info(this_step[0]);  // Print the label

This isn't a terrible idea, but the problem is that then it chews up a bunch of extra typing....

@@ +220,5 @@
> +    }
> +
> +    info("Tests finished");
> +
> +    self.cleanup();

I suggest a compromise. Let's do it as a forced step but have it check for double calling..

@@ +225,4 @@
>      SimpleTest.finish();
> +  };
> +
> +  window.setTimeout(_executeNext, 0);

I did it to avoid having the tests start running right away as part of the ctor.

@@ +230,5 @@
> +
> +
> +PeerConnectionTest.prototype.setGetUserMediaConstraints = function (constraints1, constraints2) {
> +  this.pc1_constraints = constraints1;
> +  this.pc2_constraints = constraints2;

You invoke this function before you start the tests and do gathering.

@@ +285,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  // If this is a desktop supported test and we're on android or b2g,
> +  // indicate that the test is not supported and skip the test
> +  if(desktopOnly && (navigator.userAgent.indexOf('Android') > -1 ||

I'm not following the relationship here to the gum desktop only options.... Are they distinct? should they be merged?

::: dom/media/tests/mochitest/pc.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  
> +function PeerConnectionWrapper(framework, label, configuration) {
> +  this._framework = framework;

I deliberately didn't do it this way because it makes every function require special success cb logic. My experience is
that this sort of chaining is hard to maintain.

@@ +96,5 @@
> +  getAllUserMedia:function (constraints_list) {
> +    info("getAllUserMedia: " + constraints_list.length + " streams");
> +    this.getAllUserMediaInternal(constraints_list, 0);
> +
> +    return false;  // Asynchronous

This actually is bit rot. You no longer need to retun these.

@@ +109,5 @@
> +        self._framework.executeNext();
> +      },
> +      unexpectedCallbackAndFinish);
> +
> +    return false;  // So we await callback.

And here.

@@ +162,5 @@
> +    // This is synchronous.
> +    return true;
> +  },
> +
> +  close:function () {

This applies to this PC and not to the test. The test has two PCs.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given that I haven't heard back from Clint and Eric about the usefulness of the GumTest class which I have implemented in my first WIP, I will remove it in the next version and make the PeerConnectionClass the base class.

Jason, whenever you want to use the same for GUM tests you will have to implement it on your own later, as you have suggested. I will only concentrate on PC including data channel tests now.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 718520 [details] [diff] [review]
WIP 0.1

Review of attachment 718520 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +285,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  // If this is a desktop supported test and we're on android or b2g,
> +  // indicate that the test is not supported and skip the test
> +  if(desktopOnly && (navigator.userAgent.indexOf('Android') > -1 ||

If GUM tests do not work on Android, PC tests will also not work. So we have to depend on that.

::: dom/media/tests/mochitest/pc.js
@@ +18,5 @@
> +    // Assume type is video until we get get{Audio,Video}Tracks.
> +    self.attachMedia(event.stream, 'audiovideo', 'remote');
> +  };
> +
> +  assert_ok(this._pc, "Created PC");

We don't need this assertion at all. Given that the have the global try/catch we will fetch the error thrown by accessing a null object.

@@ +24,5 @@
> +
> +PeerConnectionWrapper.prototype = {
> +
> +  // Make the element to put a given stream into.
> +  makeElementForStream: function (type, side) {

Correct. I refactored this out to head.js.

@@ +53,5 @@
> +    element.play();
> +  },
> +
> +  // Get user media based on contraints.
> +  getUserMedia: function (constraints, success) {

This method is not necessary anymore. We can call the getUserMedia method from head.js directly.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: WIP 0.2

I want to push an interim update for my latest work. It's still not ready and the last peer connection test does not work because it will depend on how the chain will finally work. All the other tests have been transformed to use the new framework or the basic runTest() function if it's not worth to instantiate the framework.

This patch includes:
* The HTML boilerplate is getting constructed via the createHTML function located in head.js
* The base class is PeerConnectionTest and is located in pc.js (we might want to change its name)
* PeerConnectionWrapper has been updated to directly use the global getUserMedia function from head.js
* Updated tests for new framework, lesser duplicated code, and better readability
* A couple of smaller fixes and addtions

Keep in mind that this is still not ready for review and needs further work. If you have the time please have a look and let me know about the progress made.

Comment 38

[Jason Smith [:jsmith]]

Comment on attachment 719610 [details] [diff] [review]
WIP 0.2

Review of attachment 719610 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely getting there. Added only new comments on stuff I've noticed.

::: dom/media/tests/mochitest/test_peerConnection_bug825703.html
@@ +11,2 @@
>  <pre id="test">
> +<script type="application/javascript">

Just a random thought (but likely a different bug to work on this) - we should consider pushing for separating the tests that we expect exceptions to be thrown vs. tests we expect won't throw an exception thrown. That would result in reducing some of the duplication I see in these exception tests.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +25,2 @@
>  
> +      /* Check that the SDP is sane to start with */

A lot of these particular checks happening here with localDescription and remoteDescription apply as a general "check" command in our command structure to check our handshake was successful. So we might want to pull into one of our "command" objects as a check other tests can take advantage of.

@@ +48,2 @@
>  
> +      try { description = pcLocal.localDescription; } catch (e) { exception = e; }

Hmm...I'm wondering if it's worthwhile to add a helper function in head.js of something like "expect an exception to be thrown for this code" - that might reduce code duplication here.

::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
@@ +18,4 @@
>  
> +  test.to_execute = [[
> +    "TEST",
> +    function (pc1, pc2) {

We should really refactor this test into our command structure. That will prove if our command structure design can work with multiple different cases, not just the general handshake case we currently have specified.

::: dom/media/tests/mochitest/test_peerConnection_bug840344.html
@@ +55,2 @@
>  
> +  test.to_execute.push([

Similar comment to the other test - we should refactor this test to follow our command object design. The basic premise of what we are aiming for command structure design says that we should always be reuse command objects in different tests easily and add on new ones as needed.

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Jason Smith [:jsmith] from comment #38)
> Just a random thought (but likely a different bug to work on this) - we
> should consider pushing for separating the tests that we expect exceptions
> to be thrown vs. tests we expect won't throw an exception thrown. That would
> result in reducing some of the duplication I see in these exception tests.

Which duplication? I don't see anything were those tests check the same expected behaviors. Can you please be more specific? Something I like is to see better filenames if possible.

> A lot of these particular checks happening here with localDescription and
> remoteDescription apply as a general "check" command in our command
> structure to check our handshake was successful. So we might want to pull
> into one of our "command" objects as a check other tests can take advantage
> of.

I will re-assess once the command chain has been finished.

> > +      try { description = pcLocal.localDescription; } catch (e) { exception = e; }
> 
> Hmm...I'm wondering if it's worthwhile to add a helper function in head.js
> of something like "expect an exception to be thrown for this code" - that
> might reduce code duplication here.

Sounds like an idea which we can do in another bug.

> ::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
> @@ +18,4 @@
> >  
> > +  test.to_execute = [[
> > +    "TEST",
> > +    function (pc1, pc2) {
> 
> We should really refactor this test into our command structure. That will
> prove if our command structure design can work with multiple different
> cases, not just the general handshake case we currently have specified.

It's already refactored but not finished yet. It's a good example that a single task in the chain is getting handled correctly. Here we replace all the entries.

> > +  test.to_execute.push([
> 
> Similar comment to the other test - we should refactor this test to follow
> our command object design. The basic premise of what we are aiming for
> command structure design says that we should always be reuse command objects
> in different tests easily and add on new ones as needed.

Same as above. As long as we do not have accessors this is a simple workaround to push the test as the last item into the chain.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 719610 [details] [diff] [review]
WIP 0.2

Review of attachment 719610 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/pc.js
@@ +186,5 @@
> + * @param configuration
> + * @constructor
> + */
> +function PeerConnectionWrapper(framework, label, configuration) {
> +  this._framework = framework;

Given that the command chain should be a completely separate re-usable object we have to get rid of the framework parameter. As Jason mentioned the wrapper should never be aware about how it is used, so any back reference is not necessary. My next WIP moves all of this out for now. One thing which will remain is to find out how to improve the call to executeNext() so it doesn't have to be specified in each step.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: WIP 0.3

Sorry for the small update steps but I'm pulled into a lot of different work this week. But I still try to keep my focus on this patch.

Here the updates:
* Separation of the command chain from the test class, except the execution_step property. It will be moved next when the chain becomes a real object.
* Updated wrapper to be independent from the framework by using onSuccess callback methods.
* Renamed pc1/pc2 to pcLocal/pcRemote so it's clear what they stand for
* Updated/fixed tests for the command chain changes

Comment 42

[cmtalbert]

Just spoke with Henrik, this patch is corrupted. He'll attach a new one.  The main pc.js stuff is still legit, but you'll notice there are a bunch of GUM changes that got in there unexpectedly.

(In reply to Jason Smith [:jsmith] from comment #38)
> Comment on attachment 719610 [details] [diff] [review]
> ::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
> @@ +18,4 @@
> >  
> > +  test.to_execute = [[
> > +    "TEST",
> > +    function (pc1, pc2) {
> 
> We should really refactor this test into our command structure. That will
> prove if our command structure design can work with multiple different
> cases, not just the general handshake case we currently have specified.
> 

Jason, these are great comments, and I agree with them. But let's focus on getting this refactoring finished and landed. I would like to see the refactoring done and finished, the tests passing with the new refactored code.  Once we have that, we can then file follow on bugs to further refactor specific tests.

This refactoring is a nexus point in that we can't easily parallelize the work. But we can easily parallelize individual test refactoring after this work is done. So I'd like us to keep the focus on getting the pc.js framework right, and on just doing enough to get the tests to pass with the new framework. 

Let's be sure to file bugs for further refactoring like this, because it would be good to do and we can parallelize and prioritize that work appropriately.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 714940 [details] [diff] [review]
Mochitest refactor

Review of attachment 714940 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/pc_test_framework.js
@@ +94,5 @@
> +    this.pc = new mozRTCPeerConnection(pc_configuration);
> +
> +    this.streams = [];
> +
> +    this.pc.onaddstream = function(event) {

Erik, what is your take on it? Shall we modify it and move it into an execution step?

@@ +103,5 @@
> +    assert_ok(this.pc, "Created PC");
> +
> +    // Make the element to put a given stream into.
> +    var makeElementForStream = function(type, side) {
> +        var stream_label = pc_label + '_' + side + '_' + type;

Jason, what are you referring to here? It's not clear to me given that it's already a helper method in that object.

@@ +122,5 @@
> +    };
> +    
> +
> +    // Callback to call when we get media from either side
> +    this.attachMedia = function(stream, type, side) {

Erik, I would like to have your feedback here.

@@ +124,5 @@
> +
> +    // Callback to call when we get media from either side
> +    this.attachMedia = function(stream, type, side) {
> +        info("Got media stream: " + type + ", " + side);
> +        // this.streams.append();

Erik? Any comments? Was it just left out from testing? I reverted that for now.

@@ +275,5 @@
> +            var this_step = to_execute[execution_step++];
> +
> +            info(this_step[0]);  // Print the label
> +            this_step[1]();
> +	    if (this_step[2] && this_step[2].async) {

Jason, neither Eric nor myself are sure what that comment means. Can you please further elaborate that? Is it that tests should not have to set this async properties? Are you leaning towards the way which I have used in my last WIP with executeNext()?

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) from comment #43)
> > +            var this_step = to_execute[execution_step++];
> > +
> > +            info(this_step[0]);  // Print the label
> > +            this_step[1]();
> > +	    if (this_step[2] && this_step[2].async) {
> 
> Jason, neither Eric nor myself are sure what that comment means. Can you
> please further elaborate that? Is it that tests should not have to set this
> async properties? Are you leaning towards the way which I have used in my
> last WIP with executeNext()?

Looks like my last comment messed up. I only wanted to reply to the comment from Jason. So please only care about this one. Additionally you can find a skeleton of the CommandChain class here: http://pastebin.mozilla.org/2194905

Comment 45

[Jason Smith [:jsmith]]

Comment on attachment 714940 [details] [diff] [review]
Mochitest refactor

Review of attachment 714940 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/pc_test_framework.js
@@ +103,5 @@
> +    assert_ok(this.pc, "Created PC");
> +
> +    // Make the element to put a given stream into.
> +    var makeElementForStream = function(type, side) {
> +        var stream_label = pc_label + '_' + side + '_' + type;

I think I wrote this originally cause this is something that could be used for the gUM tests eventually as well. So I was thinking this helper function could be in head.js instead that both PC and gUM tests could use.

@@ +275,5 @@
> +            var this_step = to_execute[execution_step++];
> +
> +            info(this_step[0]);  // Print the label
> +            this_step[1]();
> +	    if (this_step[2] && this_step[2].async) {

I think when I originally wrote this I think I'd optimally want an approach that would make the test as deterministic as possible - whether it's async or sync. This might be okay though now that I take another look at it.

::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html
@@ +11,5 @@
> +     },
> +     function() {
> +        var tests = new PeerConnectionTest();
> +        tests.setGetUserMediaConstraints([{audio:true}],[{audio:true}]);
> +        tests.runTests();

Right. I'd only expect the "framework" checks to be the "this must be true" checks. Like the SDP checks on that happen in that one test after the handshake is established.

Comment 46

[Jason Smith [:jsmith]]

(In reply to Clint Talbert ( :ctalbert ) from comment #42)
> Just spoke with Henrik, this patch is corrupted. He'll attach a new one. 
> The main pc.js stuff is still legit, but you'll notice there are a bunch of
> GUM changes that got in there unexpectedly.
> 
> (In reply to Jason Smith [:jsmith] from comment #38)
> > Comment on attachment 719610 [details] [diff] [review]
> > ::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
> > @@ +18,4 @@
> > >  
> > > +  test.to_execute = [[
> > > +    "TEST",
> > > +    function (pc1, pc2) {
> > 
> > We should really refactor this test into our command structure. That will
> > prove if our command structure design can work with multiple different
> > cases, not just the general handshake case we currently have specified.
> > 
> 
> Jason, these are great comments, and I agree with them. But let's focus on
> getting this refactoring finished and landed. I would like to see the
> refactoring done and finished, the tests passing with the new refactored
> code.  Once we have that, we can then file follow on bugs to further
> refactor specific tests.
> 
> This refactoring is a nexus point in that we can't easily parallelize the
> work. But we can easily parallelize individual test refactoring after this
> work is done. So I'd like us to keep the focus on getting the pc.js
> framework right, and on just doing enough to get the tests to pass with the
> new framework. 
> 
> Let's be sure to file bugs for further refactoring like this, because it
> would be good to do and we can parallelize and prioritize that work
> appropriately.

True, I can agree on that. Although I do think there needs to a PoC that proves that we can more with the framework than the typical smoke tests seen to prove that the framework does allow flexibility.

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: WIP 0.4

This implements the command chain we want to use. I tried to make use of Array.splice() but wasn't successful given that it requires a list of arguments and not an array as input. I didn't wanted to waste more time for such internals we can change later.

Two questions are remaining:

1. How do we want to handle negative tests? We could do that by accessing the real underlying methods of the peer connection object and put the test.next() call into the onError handler. Otherwise we could add an optional parameter to the wrapper methods we could use for onError.

2. How do we want to access properties and methods of the peer connection object? Do we need wrapper entries for all of them? Or is it ok to use pcLocal._pc for special tests?

Once the above has been cleared I will update the patch appropriately so we can land it. Documentation etc. I will do instantly in follow-up patches.

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: WIP 0.5

Last patch still had the modified GUM tests included. Those have now been removed.

Comment 49

[Jason Smith [:jsmith]]

Comment on attachment 721281 [details] [diff] [review]
WIP 0.5

Review of attachment 721281 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, really close. The multiple PC comment I made below was for a future patch, but that's noting something we need to think about in the future. The big items I see left are:

1. I'd push for command class objects over using lists
2. The actual commands for a test need to:
   a. Be a part of a template that multiple mochitests use. So the actual commands for a test can't be within pc.js directly.
   b. Represent reuse of the existing command framework on a per mochitest basis and not attempt to rewrite multiple PC functions within a command (build off of the framework, don't rewrite and duplicate).

::: dom/media/tests/mochitest/Makefile.in
@@ -9,5 @@
>  relativesrcdir = @relativesrcdir@
>  
>  include $(DEPTH)/config/autoconf.mk
>  
>  MOCHITEST_FILES = \

Just a reminder to make sure to update your patch to not remove these tests.

::: dom/media/tests/mochitest/head.js
@@ +24,5 @@
> +    document.head.appendChild(title);
> +  }
> +
> +  // Create the body
> +  if (meta.title) {

You only need one if block in this case, not two for if(meta.title).

::: dom/media/tests/mochitest/pc.js
@@ +35,5 @@
> +    function _executeNext() {
> +      if (self._current < self._steps.length) {
> +        var step = self._steps[self._current++];
> +
> +        info(step[0]);  // Print the label

Still kinda find it a tad weird to have the command object defined as a list. I'd expect it to be a class object.

@@ +38,5 @@
> +
> +        info(step[0]);  // Print the label
> +
> +        // todo: assertion to not call it twice? especially at the end
> +        step[1](self._object);

What's happening here?

@@ +44,5 @@
> +      else {
> +        info("Tests finished");
> +
> +        // todo: check that we haven't run it yet
> +        // self.cleanup();

Why is this commented out?

@@ +129,5 @@
> +}
> +
> +
> +// The list of commands to execute.
> +var commandsPeerConnection = [

Note - this still needs to be pulled out of the framework code. These are commands as part of set of basic smoke tests. If multiple tests are using the same commands, then break that out into a separate template file.

@@ +220,5 @@
> +  // Allow us to individually enable peer connection tests for now
> +  this._desktopSupportedOnly = options.desktopSupportedOnly || false;
> +
> +  this.pcLocal = new PeerConnectionWrapper('pcLocal', options.config_pc1);
> +  this.pcRemote = new PeerConnectionWrapper('pcRemote', options.config_pc2 || options.config_pc1);

Note that there will be different styles of tests that involve multiple peer connections. So we can't necessarily make the assumption that there will always be one local and one remote PC.

That might just mean that we might want to have two "test framework classes" potentially - one for the general 1:1 tests and another for the multiple PC cases.

What do you think?

Note - I'd expect this to probably be a followup bug to this refactoring we are doing here.

@@ +338,5 @@
> +  /**
> +   *
> +   * @param {function} onSuccess
> +   */
> +  createOffer : function (onSuccess) {

Any function that represents a command should really be pulled out into a separate object in alignment with how the command pattern works. Otherwise, this class will become a blob quite quickly.

@@ +409,5 @@
> +   *
> +   */
> +  close : function () {
> +    if (this._pc) {
> +      this._pc = null;

? I don't follow this. The function is called close, but you aren't calling close here?

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +20,5 @@
>  
> +  var steps = [
> +    [
> +      "CHECK_FOR_SANE_SDP",
> +      function (test) {

Can you pull this function into the framework pc.js? This a function that other tests could easily take advantage of for post-handshake check.

@@ +53,5 @@
> +        test.next();
> +      }
> +    ], [
> +      "CHECK_SDP_ON_CLOSED_PC",
> +      function (test) {

This needs to be split into two commands - one for the close() call, another for the check properties logic. I think there was one command for close being worked on in the pc framework class, so take advantage of that here. As for the check logic that happens after the close, I'd add that as another command function within the PC framework referenced here.

The reasoning for building a laundry list of available commands in the PC framework, not always within the test, is that with many of these tests, you might repeat the same command logic in multiple mochitests. In this case, there might be a case for running the same "after close" tests for a basic audio, basic video, basic audio/video, etc cases with a local to remote PC.

::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
@@ +20,4 @@
>  
> +  var steps = [[
> +    "CHECK_QUEUE_CREATE_ANSWER",
> +    function (test) {

So this test isn't entirely following our framework design here. The framework advocates for a set of commands to call for each major area, but the entire test exists here. So this needs to be refactored to follow the command model.

::: dom/media/tests/mochitest/test_peerConnection_bug840344.html
@@ +59,2 @@
>  
> +  var steps = [

This test also isn't really following the framework design either.

Comment 50

[Eric Rescorla (:ekr)]

(In reply to Jason Smith [:jsmith] from comment #49)
> Comment on attachment 721281 [details] [diff] [review]
> WIP 0.5
> 
> Review of attachment 721281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall, really close. The multiple PC comment I made below was for a future
> patch, but that's noting something we need to think about in the future. The
> big items I see left are:
> 
> 1. I'd push for command class objects over using lists

FWIW, I prefer the existing idiom. The alternative seems like
unnecessary typing.


> 2. The actual commands for a test need to:
>    a. Be a part of a template that multiple mochitests use. So the actual
> commands for a test can't be within pc.js directly.
>    b. Represent reuse of the existing command framework on a per mochitest
> basis and not attempt to rewrite multiple PC functions within a command
> (build off of the framework, don't rewrite and duplicate).
> 
> ::: dom/media/tests/mochitest/Makefile.in
> @@ -9,5 @@
> >  relativesrcdir = @relativesrcdir@
> >  
> >  include $(DEPTH)/config/autoconf.mk
> >  
> >  MOCHITEST_FILES = \
> 
> Just a reminder to make sure to update your patch to not remove these tests.
> 
> ::: dom/media/tests/mochitest/head.js
> @@ +24,5 @@
> > +    document.head.appendChild(title);
> > +  }
> > +
> > +  // Create the body
> > +  if (meta.title) {
> 
> You only need one if block in this case, not two for if(meta.title).
> 
> ::: dom/media/tests/mochitest/pc.js
> @@ +35,5 @@
> > +    function _executeNext() {
> > +      if (self._current < self._steps.length) {
> > +        var step = self._steps[self._current++];
> > +
> > +        info(step[0]);  // Print the label
> 
> Still kinda find it a tad weird to have the command object defined as a
> list. I'd expect it to be a class object.
> 
> @@ +38,5 @@
> > +
> > +        info(step[0]);  // Print the label
> > +
> > +        // todo: assertion to not call it twice? especially at the end
> > +        step[1](self._object);
> 
> What's happening here?
> 
> @@ +44,5 @@
> > +      else {
> > +        info("Tests finished");
> > +
> > +        // todo: check that we haven't run it yet
> > +        // self.cleanup();
> 
> Why is this commented out?
> 
> @@ +129,5 @@
> > +}
> > +
> > +
> > +// The list of commands to execute.
> > +var commandsPeerConnection = [
> 
> Note - this still needs to be pulled out of the framework code. These are
> commands as part of set of basic smoke tests. If multiple tests are using
> the same commands, then break that out into a separate template file.
> 
> @@ +220,5 @@
> > +  // Allow us to individually enable peer connection tests for now
> > +  this._desktopSupportedOnly = options.desktopSupportedOnly || false;
> > +
> > +  this.pcLocal = new PeerConnectionWrapper('pcLocal', options.config_pc1);
> > +  this.pcRemote = new PeerConnectionWrapper('pcRemote', options.config_pc2 || options.config_pc1);
> 
> Note that there will be different styles of tests that involve multiple peer
> connections. So we can't necessarily make the assumption that there will
> always be one local and one remote PC.
> 
> That might just mean that we might want to have two "test framework classes"
> potentially - one for the general 1:1 tests and another for the multiple PC
> cases.
> 
> What do you think?
> 
> Note - I'd expect this to probably be a followup bug to this refactoring we
> are doing here.
> 
> @@ +338,5 @@
> > +  /**
> > +   *
> > +   * @param {function} onSuccess
> > +   */
> > +  createOffer : function (onSuccess) {
> 
> Any function that represents a command should really be pulled out into a
> separate object in alignment with how the command pattern works. Otherwise,
> this class will become a blob quite quickly.
> 
> @@ +409,5 @@
> > +   *
> > +   */
> > +  close : function () {
> > +    if (this._pc) {
> > +      this._pc = null;
> 
> ? I don't follow this. The function is called close, but you aren't calling
> close here?
> 
> ::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
> @@ +20,5 @@
> >  
> > +  var steps = [
> > +    [
> > +      "CHECK_FOR_SANE_SDP",
> > +      function (test) {
> 
> Can you pull this function into the framework pc.js? This a function that
> other tests could easily take advantage of for post-handshake check.
> 
> @@ +53,5 @@
> > +        test.next();
> > +      }
> > +    ], [
> > +      "CHECK_SDP_ON_CLOSED_PC",
> > +      function (test) {
> 
> This needs to be split into two commands - one for the close() call, another
> for the check properties logic. I think there was one command for close
> being worked on in the pc framework class, so take advantage of that here.
> As for the check logic that happens after the close, I'd add that as another
> command function within the PC framework referenced here.
> 
> The reasoning for building a laundry list of available commands in the PC
> framework, not always within the test, is that with many of these tests, you
> might repeat the same command logic in multiple mochitests. In this case,
> there might be a case for running the same "after close" tests for a basic
> audio, basic video, basic audio/video, etc cases with a local to remote PC.
> 
> ::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
> @@ +20,4 @@
> >  
> > +  var steps = [[
> > +    "CHECK_QUEUE_CREATE_ANSWER",
> > +    function (test) {
> 
> So this test isn't entirely following our framework design here. The
> framework advocates for a set of commands to call for each major area, but
> the entire test exists here. So this needs to be refactored to follow the
> command model.
> 
> ::: dom/media/tests/mochitest/test_peerConnection_bug840344.html
> @@ +59,2 @@
> >  
> > +  var steps = [
> 
> This test also isn't really following the framework design either.

Comment 51

[Jason Smith [:jsmith]]

Okay, fair enough. The list is fine if you guys are alright with it (I don't feel strongly on this one).

Comment 52

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 721281 [details] [diff] [review]
WIP 0.5

Review of attachment 721281 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +24,5 @@
> +    document.head.appendChild(title);
> +  }
> +
> +  // Create the body
> +  if (meta.title) {

Correct. Just missed to remove that. Thanks for catching it.

::: dom/media/tests/mochitest/pc.js
@@ +38,5 @@
> +
> +        info(step[0]);  // Print the label
> +
> +        // todo: assertion to not call it twice? especially at the end
> +        step[1](self._object);

Something Eric mentioned to me. After investigation and the update as given in my next comment, it doesn't apply anymore.

@@ +44,5 @@
> +      else {
> +        info("Tests finished");
> +
> +        // todo: check that we haven't run it yet
> +        // self.cleanup();

Because the cleanup has to happen on the PeerConnectionTest class and not inside the CommandChain object. That was still a todo item for me to get moved over. It will be part of the next patch.

@@ +129,5 @@
> +}
> +
> +
> +// The list of commands to execute.
> +var commandsPeerConnection = [

No, those tests have to live in this file. They are applied by default for all peer connection tests. Unless you have special PC tests which exercise a completely different path, you do not have to worry about setting up necessary steps.

@@ +220,5 @@
> +  // Allow us to individually enable peer connection tests for now
> +  this._desktopSupportedOnly = options.desktopSupportedOnly || false;
> +
> +  this.pcLocal = new PeerConnectionWrapper('pcLocal', options.config_pc1);
> +  this.pcRemote = new PeerConnectionWrapper('pcRemote', options.config_pc2 || options.config_pc1);

I thought about the same but I don't think we have to care about that at this time. It's easy later to stuff them into an array so that we can support any number of peer connections.

I know that it will break tests which access pcLocal and pcRemote but it's really a quick search and replace change. So I don't think we have to block on that.

@@ +409,5 @@
> +   *
> +   */
> +  close : function () {
> +    if (this._pc) {
> +      this._pc = null;

Forgot to revert this when I was testing. Done now.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +20,5 @@
>  
> +  var steps = [
> +    [
> +      "CHECK_FOR_SANE_SDP",
> +      function (test) {

I wouldn't do it now but we might want to consider doing that in a follow-up bug.

@@ +53,5 @@
> +        test.next();
> +      }
> +    ], [
> +      "CHECK_SDP_ON_CLOSED_PC",
> +      function (test) {

Those are checks we do not have to exercise for all of our tests. It's enough to have them covered once. Keep in mind that every check here adds a good portion of time to the duration our pc tests need to run.

Same for the steps. There is no special step in the general command chain which exercises the closing of the pc connections. All that gets done in the teardown logic. Generally PC tests will not face that logic anyway, so I don't think we should do that.

::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
@@ +20,4 @@
>  
> +  var steps = [[
> +    "CHECK_QUEUE_CREATE_ANSWER",
> +    function (test) {

This test is absolutely special and doesn't need any of the general steps for PC tests. But thinking more about it we should let is use the framework at all. It's unnecessary here. I will revert it.

::: dom/media/tests/mochitest/test_peerConnection_bug840344.html
@@ +59,2 @@
>  
> +  var steps = [

Why not? It's kinda unclear to me what you are referring to here. If you refer to the code in CHECK_MULTIPLE_ANSWERS, we can't split this up into different steps due to multiple callbacks. I checked back with Eric yesterday on that, and he agreed on it.

Comment 53

[Jason Smith [:jsmith]]

Comment on attachment 721281 [details] [diff] [review]
WIP 0.5

Review of attachment 721281 [details] [diff] [review]:
-----------------------------------------------------------------

Switching to feedback- now - The rationales I'm being given I generally don't agree with it overall, as the original direction I thought was agreed on was pushing dynamic command list design to allow easy reuse across tests. The rationales for the design decisions indicate a direction I don't agree with and believe it turns the tests into "let's make the tests more readable" rather than "readability" and "reuse."

We probably should take this offline off the bug.

::: dom/media/tests/mochitest/pc.js
@@ +129,5 @@
> +}
> +
> +
> +// The list of commands to execute.
> +var commandsPeerConnection = [

I don't follow. The original intention of doing this command list style design was to allow dynamic command setup, as each PC test might execute a different set of steps and expect certain states during it. I definitely don't expect us to apply those by default to all peer connection tests at all - which already is shown by other tests in the bug# PC tests that there are different flows.

So I'm more expecting different paths on a per set of tests by default, not one set always. There will be many tests that do something like:

- Call X, Y, Z ==> What's the right state?
- Call Y, X, Z ==> What's the right state?
- Call Z, Y, X ==> What's the right state?
- etc.

@@ +220,5 @@
> +  // Allow us to individually enable peer connection tests for now
> +  this._desktopSupportedOnly = options.desktopSupportedOnly || false;
> +
> +  this.pcLocal = new PeerConnectionWrapper('pcLocal', options.config_pc1);
> +  this.pcRemote = new PeerConnectionWrapper('pcRemote', options.config_pc2 || options.config_pc1);

Right. Let's think about this one later in a different bug when we focus on multiple PC tests.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +20,5 @@
>  
> +  var steps = [
> +    [
> +      "CHECK_FOR_SANE_SDP",
> +      function (test) {

I honestly don't see why you can't get this done in this patch. It's a simple move to the framework and you've already got this in a function.

@@ +53,5 @@
> +        test.next();
> +      }
> +    ], [
> +      "CHECK_SDP_ON_CLOSED_PC",
> +      function (test) {

I don't know if we should make the assumption that having them covered once is sufficient - blackbox testing of API should not make that assumption. And yes, there is a command for closing PCs in the global framework. If it isn't working as expected, then fix the problem of why that's happening.

::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
@@ +20,4 @@
>  
> +  var steps = [[
> +    "CHECK_QUEUE_CREATE_ANSWER",
> +    function (test) {

I don't understand. The whole intention of the work we've done on this refactoring was to allow a very easy to use framework to build tests off of. It was not turn this into "let's make the code look nice and OO" but not make it extensible. So I don't agree with your direction here.

::: dom/media/tests/mochitest/test_peerConnection_bug840344.html
@@ +59,2 @@
>  
> +  var steps = [

The PC's test framework's test design should push for allowing as much reuse as possible of the commands after the test builds it. The setRemoteDescription logic you have below is code duplication of an existing command. And we should aim to build off of the framework to build more commands, not repeat code that we already have in the framework if possible. So I don't agree with the direction going here.

Comment 54

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: WIP 0.6

Updated WIP with latest comments. I would consider this the last WIP and after our planned discussion later today I might be able to even upload a final patch.

I still want to get Eric's feedback. Most likely we can do that during the call.

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1

Final patch which removes the changes from the Makefile and skips the BOM change from mediaPlayback.js.

Submitted to try server:
https://tbpl.mozilla.org/?tree=Try&rev=fbebd2722ada

Comment 56

[Jason Smith [:jsmith]]

Comment on attachment 721913 [details] [diff] [review]
Patch v1

Review of attachment 721913 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine with some nits and noting that the style issues would be taken care of in a different patch.

::: dom/media/tests/mochitest/head.js
@@ +113,2 @@
>        try {
> +        aCallback(this);

Why is the this reference included as a parameter?

::: dom/media/tests/mochitest/pc.js
@@ +22,5 @@
> +    return this._current;
> +  },
> +
> +  get finished() {
> +    return this._current == this._steps.length;

Nit - triple equals

@@ +38,5 @@
> +    var self = this;
> +
> +    function _executeNext() {
> +      if (!self.finished) {
> +        var step = self._steps[self._current++];

Nit - I'd more advocate for the increment being on a different line than the list index access to define var step - two separate pieces of logic.

@@ +250,5 @@
> +};
> +
> +PeerConnectionTest.prototype.teardown = function () {
> +  if (this.pcLocal) {
> +    this.pcLocal.close();

Nit - For each close operation called, I'd include an info statement

@@ +328,5 @@
> +            type += 'video';
> +          }
> +
> +          self.attachMedia(stream, type, 'local');
> +          self._pc.addStream(stream);

Nit - I'd add an info statement when addStream is called

@@ +420,5 @@
> +      // It might be that a test has already closed the pc. In those cases
> +      // we should not fail.
> +      try {
> +        this._pc.close();
> +      } catch (e) {

Nit - we might want to add an info statement here if the catch flow is executed to indicate an exception was thrown during close.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +20,5 @@
>  
> +  var steps = [
> +    [
> +      "CHECK_FOR_SANE_SDP",
> +      function (test) {

Nit - Since this going to be fixed in some followup bug, I'd add a TODO here to indicate to refactor this out at some point.

@@ +53,5 @@
> +        test.next();
> +      }
> +    ], [
> +      "CHECK_SDP_ON_CLOSED_PC",
> +      function (test) {

Hmm...remind why this couldn't be broken into two steps - one calling close, the other doing the checks? That falls in alignment with how we've setup our tests - one does the logic, the other does the checks.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 721913 [details] [diff] [review]
Patch v1

Review of attachment 721913 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +113,2 @@
>        try {
> +        aCallback(this);

That was a left-over from a change made based on Eric's initial patch. No change is necessary here.

::: dom/media/tests/mochitest/pc.js
@@ +38,5 @@
> +    var self = this;
> +
> +    function _executeNext() {
> +      if (!self.finished) {
> +        var step = self._steps[self._current++];

That's something which is usually be done but I can change it. No problem.

@@ +250,5 @@
> +};
> +
> +PeerConnectionTest.prototype.teardown = function () {
> +  if (this.pcLocal) {
> +    this.pcLocal.close();

I would not do this at this level but in the PeerConnectionWrapper instead.

@@ +299,4 @@
>     */
> +  attachMedia : function PCW_attachMedia(stream, type, side) {
> +    info("Got media stream: " + type + " (" + side + ")");
> +    this.streams.push(stream);

We might get problems here given that we do not separate local streams from remote ones. We don't have to update that now but should care of once it's important for a test.

@@ +328,5 @@
> +            type += 'video';
> +          }
> +
> +          self.attachMedia(stream, type, 'local');
> +          self._pc.addStream(stream);

Given that adding the stream is tightly bound to attachMedia I move this call into that method. An info line is already present there, so we don't need another one.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +20,5 @@
>  
> +  var steps = [
> +    [
> +      "CHECK_FOR_SANE_SDP",
> +      function (test) {

Will do. Also it's good that you commented on this test. It reminded me to remove the extra '._pc' code by setting up additional getters/setters for most used properties.

@@ +53,5 @@
> +        test.next();
> +      }
> +    ], [
> +      "CHECK_SDP_ON_CLOSED_PC",
> +      function (test) {

We could separate the pcLocal vs. pcRemote steps but I wouldn't close both peer connections at the same time. Also I don't think we need 4 steps for all of them.

Comment 58

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v2

Updated patch with comments from Jason. Also I have added the localDescription and remoteDescription properties to the wrapper which simply forwards it to the real pc object. Also some other nits have been fixed, so I would like to see another review also from Jason.

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1 (documentation)

This patch brings the documentation into the framework. While working on it I noticed that some names were badly chosen. I have updated them accordingly, and to make it clearer to work with the framework. Sadly I also had to modify some tests for it.

I hope we can finally combine those two patches into a single one, and to land the changes in a single changeset.

Just a notice to the jsdoc string for objects as parameters. Geo and I were having a long discussion on that last year and agreed both on such a style because we haven't found another way to get sane documentation output.

Comment 60

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v1.1 (documentation)

A qref is welcome before attaching a patch. Sorry for that.

Comment 61

[Eric Rescorla (:ekr)]

Since I plan to review this primarily for bugs rather than architecture, I intend to wait for jsmith's r+ on the bigger picture issues. Just wanted to make clear it's on my radar.

Comment 62

[Jason Smith [:jsmith]]

Comment on attachment 722191 [details] [diff] [review]
Patch v1.1 (documentation)

Review of attachment 722191 [details] [diff] [review]:
-----------------------------------------------------------------

Two issues that cause the review-

1. Change of the if(meta.title) flow, even though this is style changes
2. The empty object for options, which I don't think is possible

The rest are nits.

::: dom/media/tests/mochitest/head.js
@@ +29,5 @@
>    var elem = document.createElement('meta');
>    elem.setAttribute('charset', 'utf-8');
>    document.head.appendChild(elem);
>  
> +  var title = document.createElement('title');

Why did you remove the if(meta.title)? We should only be doing style changes here, but that changes a flow.

@@ +36,5 @@
>  
> +  // Create the body content
> +  var anchor = document.createElement('a');
> +  anchor.setAttribute('target', '_blank');
> +  if (meta.bug) {

Nit - I'd add a blank line before the if statement

@@ +61,5 @@
> + * @param {string} type
> + *        Type of media element to create ('audio' or 'video')
> + * @param {string} label
> + *        Description to use for the element
> + * @return {Node} The created HTML media element

Wouldn't this be HTMLMediaElement as the object type?

::: dom/media/tests/mochitest/moz.build
@@ -1,4 @@
> -# vim: set filetype=python:
> -# This Source Code Form is subject to the terms of the Mozilla Public
> -# License, v. 2.0. If a copy of the MPL was not distributed with this
> -# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Why is this being removed?

::: dom/media/tests/mochitest/pc.js
@@ +43,5 @@
>  
> +  /**
> +   * Returns the assigned commands of the chain.
> +   *
> +   * @returns {object[]} Commands of the chain

Wouldn't this just be a List object?

@@ +104,3 @@
>    indexOf: function (id) {
> +    for (var i = 0; i < this._commands.length; i++) {
> +      if (this._commands[i][0] == id) {

Nit - triple equals (may have missed this in the other patch.

@@ +326,4 @@
>   */
>  function PeerConnectionTest(options) {
> +  // If no options are specified make it an empty object
> +  options = options || { };

Based on what I'm seeing below, this would have to be defined with values. Otherwise, you'll get an error when calling such calls such as options.config_pc1.

@@ +358,5 @@
> + *        Media constrains for the local peer connection instance
> + * @param constraintsRemote
> + */
> +PeerConnectionTest.prototype.setMediaConstraints =
> +function PCT_setMediaConstraints(constraintsLocal, constraintsRemote) {

Spacing feels off here

Comment 63

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 722191 [details] [diff] [review]
Patch v1.1 (documentation)

Review of attachment 722191 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +29,5 @@
>    var elem = document.createElement('meta');
>    elem.setAttribute('charset', 'utf-8');
>    document.head.appendChild(elem);
>  
> +  var title = document.createElement('title');

I wanted to mention that when I was uploading the patch, but forgot finally.

So a mochitest should always have a title assigned to it, while a bug number is still optional because there might be situations when we do not have a bug filed. We could change this too and make both mandatory. Having them optional is not a good idea!

@@ +61,5 @@
> + * @param {string} type
> + *        Type of media element to create ('audio' or 'video')
> + * @param {string} label
> + *        Description to use for the element
> + * @return {Node} The created HTML media element

Such an object type doesn't exist. It would be just a name. The real object behind is a DOM node.

::: dom/media/tests/mochitest/moz.build
@@ -1,4 @@
> -# vim: set filetype=python:
> -# This Source Code Form is subject to the terms of the Mozilla Public
> -# License, v. 2.0. If a copy of the MPL was not distributed with this
> -# file, You can obtain one at http://mozilla.org/MPL/2.0/.

Huh, I thought that came in from another editor I was trying out. I will redo that.

::: dom/media/tests/mochitest/pc.js
@@ +43,5 @@
>  
> +  /**
> +   * Returns the assigned commands of the chain.
> +   *
> +   * @returns {object[]} Commands of the chain

An Array, correct.

@@ +326,4 @@
>   */
>  function PeerConnectionTest(options) {
> +  // If no options are specified make it an empty object
> +  options = options || { };

No, we don't have to define individual values here. When we create the empty object if nothing has been specified for the optional parameter, a call for options.foobar will return undefined.

@@ +358,5 @@
> + *        Media constrains for the local peer connection instance
> + * @param constraintsRemote
> + */
> +PeerConnectionTest.prototype.setMediaConstraints =
> +function PCT_setMediaConstraints(constraintsLocal, constraintsRemote) {

Spacing? Not sure what you mean here.

Comment 64

[Jason Smith [:jsmith]]

Comment on attachment 722191 [details] [diff] [review]
Patch v1.1 (documentation)

Review of attachment 722191 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +29,5 @@
>    var elem = document.createElement('meta');
>    elem.setAttribute('charset', 'utf-8');
>    document.head.appendChild(elem);
>  
> +  var title = document.createElement('title');

Oh, in that case, this change is fine.

@@ +61,5 @@
> + * @param {string} type
> + *        Type of media element to create ('audio' or 'video')
> + * @param {string} label
> + *        Description to use for the element
> + * @return {Node} The created HTML media element

Then what about this then - https://developer.mozilla.org/en-US/docs/DOM/HTMLMediaElement?

::: dom/media/tests/mochitest/pc.js
@@ +326,4 @@
>   */
>  function PeerConnectionTest(options) {
> +  // If no options are specified make it an empty object
> +  options = options || { };

Ah, okay. Shouldn't we just make options required then? We might end up on an odd path without options specified.

@@ +358,5 @@
> + *        Media constrains for the local peer connection instance
> + * @param constraintsRemote
> + */
> +PeerConnectionTest.prototype.setMediaConstraints =
> +function PCT_setMediaConstraints(constraintsLocal, constraintsRemote) {

Thought it was odd initially that the function was started at the same indentation as the line above. I guess that's okay.

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 722191 [details] [diff] [review]
Patch v1.1 (documentation)

Review of attachment 722191 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +61,5 @@
> + * @param {string} type
> + *        Type of media element to create ('audio' or 'video')
> + * @param {string} label
> + *        Description to use for the element
> + * @return {Node} The created HTML media element

Interesting. So lets use this specific type then. Thanks.

::: dom/media/tests/mochitest/pc.js
@@ +326,4 @@
>   */
>  function PeerConnectionTest(options) {
> +  // If no options are specified make it an empty object
> +  options = options || { };

No, that wouldn't change anything. As you can see at the moment none of the tests use a config for pcLocal or pcRemote. Only desktopSupport has to be specified. But once that is gone we have a state where the parameter really has to be optional. This style we have used a lot for Mozmill related tests.

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v2 (documentation)

Updated patch with comments from Jason.

Comment 67

[Jason Smith [:jsmith]]

Comment on attachment 722411 [details] [diff] [review]
Patch v2 (documentation)

Review of attachment 722411 [details] [diff] [review]:
-----------------------------------------------------------------

review+ with one nit

::: dom/media/tests/mochitest/pc.js
@@ +98,5 @@
> +   * Returns the index of the specified command in the chain.
> +   *
> +   * @param {string} id
> +   *        Identifier of the command
> +   * @returns {number}

Missing a description of what's being returned.

Comment 68

[Eric Rescorla (:ekr)]

Comment on attachment 722136 [details] [diff] [review]
Patch v2

Review of attachment 722136 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with minor issues below.

::: dom/media/tests/mochitest/head.js
@@ +22,5 @@
> +    var title = document.createElement('title');
> +    title.textContent = meta.title;
> +    document.head.appendChild(title);
> +
> +    var anchor = document.createElement('a');

Is there a reason to have this anchor with no bug #?

@@ +45,5 @@
> +
> +/**
> + * Make the element to put a given stream into
> + *
> + * @param type 'audio' || 'video'

I thought I had "audiovideo" as an option here. If the only permitted values are "audio" and "video" then you don't need the ?: below.

Looking at the rest of the code, I think we need to emit audiovideo and this comment is what's wrong

@@ +46,5 @@
> +/**
> + * Make the element to put a given stream into
> + *
> + * @param type 'audio' || 'video'
> + * @param side

There appears to be an argument called "label", not side...

@@ +60,5 @@
> +
> +  element = document.createElement(type === 'audio' ? 'audio' : 'video');
> +  element.setAttribute('id', id);
> +  element.setAttribute('height', 100);
> +  element.setAttribute('width', 150);

Do we want different sizes for audio and video?

@@ +72,5 @@
> +/**
> + * Wrapper function for mozGetUserMedia to allow a singular area of control
> + * for determining whether we run this with fake devices or not.
> + *
> + * @param {Dictionary} constraints the constraints for this mozGetUserMedia

Nit: indentation is kinda screwy here.

@@ +91,3 @@
>  /**
>   * Setup any Mochitest for WebRTC by enabling the preference for
>   * peer connections. As by bug 797979 it will also enable mozGetUserMedia().

This should probably say "disable mozGetUserMedia permissions checking"

::: dom/media/tests/mochitest/pc.js
@@ +365,5 @@
> +  /**
> +   *
> +   * @param {function} onSuccess
> +   */
> +  createOffer : function (onSuccess) {

I'm not following your convention here. Shouldn't all of these be named as well? Or alternately, none of them.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +48,5 @@
> +           "test.pcRemote.remoteDescription is not null");
> +        is(test.pcRemote.remoteDescription.type, "offer",
> +           "test.pcRemote.remoteDescription type is offer");
> +        ok(test.pcRemote.remoteDescription.sdp.length > 10,
> +           "test.pcRemote.remoteDescription body length is plausible");

Suggest we move the relevant tests here to be in every test and remove the rest.

::: dom/media/tests/mochitest/test_peerConnection_bug840344.html
@@ +65,5 @@
> +        test.pcLocal.setRemoteDescription(osd, function () {
> +          test.next();
> +        });
> +      }
> +    ], [

Suggest newline between arrays.

Comment 69

[Eric Rescorla (:ekr)]

Comment on attachment 722136 [details] [diff] [review]
Patch v2

Review of attachment 722136 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with minor issues below.

::: dom/media/tests/mochitest/head.js
@@ +22,5 @@
> +    var title = document.createElement('title');
> +    title.textContent = meta.title;
> +    document.head.appendChild(title);
> +
> +    var anchor = document.createElement('a');

Is there a reason to have this anchor with no bug #?

@@ +45,5 @@
> +
> +/**
> + * Make the element to put a given stream into
> + *
> + * @param type 'audio' || 'video'

I thought I had "audiovideo" as an option here. If the only permitted values are "audio" and "video" then you don't need the ?: below.

Looking at the rest of the code, I think we need to emit audiovideo and this comment is what's wrong

@@ +46,5 @@
> +/**
> + * Make the element to put a given stream into
> + *
> + * @param type 'audio' || 'video'
> + * @param side

There appears to be an argument called "label", not side...

@@ +60,5 @@
> +
> +  element = document.createElement(type === 'audio' ? 'audio' : 'video');
> +  element.setAttribute('id', id);
> +  element.setAttribute('height', 100);
> +  element.setAttribute('width', 150);

Do we want different sizes for audio and video?

@@ +72,5 @@
> +/**
> + * Wrapper function for mozGetUserMedia to allow a singular area of control
> + * for determining whether we run this with fake devices or not.
> + *
> + * @param {Dictionary} constraints the constraints for this mozGetUserMedia

Nit: indentation is kinda screwy here.

@@ +91,3 @@
>  /**
>   * Setup any Mochitest for WebRTC by enabling the preference for
>   * peer connections. As by bug 797979 it will also enable mozGetUserMedia().

This should probably say "disable mozGetUserMedia permissions checking"

::: dom/media/tests/mochitest/pc.js
@@ +365,5 @@
> +  /**
> +   *
> +   * @param {function} onSuccess
> +   */
> +  createOffer : function (onSuccess) {

I'm not following your convention here. Shouldn't all of these be named as well? Or alternately, none of them.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +48,5 @@
> +           "test.pcRemote.remoteDescription is not null");
> +        is(test.pcRemote.remoteDescription.type, "offer",
> +           "test.pcRemote.remoteDescription type is offer");
> +        ok(test.pcRemote.remoteDescription.sdp.length > 10,
> +           "test.pcRemote.remoteDescription body length is plausible");

Suggest we move the relevant tests here to be in every test and remove the rest.

::: dom/media/tests/mochitest/test_peerConnection_bug840344.html
@@ +65,5 @@
> +        test.pcLocal.setRemoteDescription(osd, function () {
> +          test.next();
> +        });
> +      }
> +    ], [

Suggest newline between arrays.

Comment 70

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 722136 [details] [diff] [review]
Patch v2

Review of attachment 722136 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/head.js
@@ +22,5 @@
> +    var title = document.createElement('title');
> +    title.textContent = meta.title;
> +    document.head.appendChild(title);
> +
> +    var anchor = document.createElement('a');

In such a case we would have an anchor but no link. I think that's fine.

@@ +45,5 @@
> +
> +/**
> + * Make the element to put a given stream into
> + *
> + * @param type 'audio' || 'video'

Oh, yeah. I will update the comment.

@@ +46,5 @@
> +/**
> + * Make the element to put a given stream into
> + *
> + * @param type 'audio' || 'video'
> + * @param side

That has been corrected with the documentation patch.

@@ +60,5 @@
> +
> +  element = document.createElement(type === 'audio' ? 'audio' : 'video');
> +  element.setAttribute('id', id);
> +  element.setAttribute('height', 100);
> +  element.setAttribute('width', 150);

I we need this we should implement that later. Elements are hidden by default in anyway.

@@ +91,3 @@
>  /**
>   * Setup any Mochitest for WebRTC by enabling the preference for
>   * peer connections. As by bug 797979 it will also enable mozGetUserMedia().

Seems like it was forgotten by an earlier patch. I will update for clarity.

::: dom/media/tests/mochitest/pc.js
@@ +365,5 @@
> +  /**
> +   *
> +   * @param {function} onSuccess
> +   */
> +  createOffer : function (onSuccess) {

That's done by the documentation patch.

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +48,5 @@
> +           "test.pcRemote.remoteDescription is not null");
> +        is(test.pcRemote.remoteDescription.type, "offer",
> +           "test.pcRemote.remoteDescription type is offer");
> +        ok(test.pcRemote.remoteDescription.sdp.length > 10,
> +           "test.pcRemote.remoteDescription body length is plausible");

I would really like to do that in a follow-up bug, so we keep the logic of tests here and do not cause further breakage.

Comment 71

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v3

Updated the patch with the minor nits from Eric and combined both into a single patch.

Also pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=b6672076e6d1

When everything is green I will push it to inbound.

Comment 72

[Henrik Skupin [:whimboo][⌚️UTC+2]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4740975a1063

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Randell, and Eric, how is our strategy for the tests on the aurora branch? Do we have to backport all upcoming datachannel tests? If yes we might also want to land it for Firefox 21.

Comment 74

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android mochitest-4 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e80f634fc8b

https://tbpl.mozilla.org/php/getParsedLog.php?id=20469826&tree=Mozilla-Inbound

Comment 75

[Ryan VanderMeulen [:RyanVM]]

FYI, this was failing on B2G M1 as well.

Comment 76

[Jason Smith [:jsmith]]

The failures here might imply our "desktopSupportedOnly" flag might not be working.

Comment 77

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #76)
> The failures here might imply our "desktopSupportedOnly" flag might not be
> working.

Yup, I'm right. The tests that are failing apparently are any test that makes use of the test framework that tries to set desktopSupportedOnly to true. Any of the tests that don't go through the framework appear to still work as expected.

Comment 78

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The problem here is now, that we first create the peer connection instances and then start the testrun. That way we don't bail out early enough anymore and badly fail by calling mozRTCPeerConnection. Also if the peerconnection pref wouldn't be enabled by default we would fail everywhere.

We will have to do a small refactoring, which I hopefully be able to do tomorrow.

Comment 79

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So we have two options here:

1. Move runTest() back into the test so we run all the preparation steps necessary for WebRTC tests as early as possible. It's not only the desktoponly flag which can cause us trouble in the future but also e.g. newly introduced preferences for new features. If we don't initialize them at the beginning our logic can become quite difficult.

2. Delay the creation of the PC wrapper instances until the test gets started via test.run(). We would have to add one or two more steps to the chain and have would have to temporarily store the configuration and contraints data on the test.

Personally I feel strong on the first option which will ensure that we only have to update one piece of code to stay compatible with the WebRTC features. With option 2 I think we will get too many different code paths for features in the framework we might not be able to handle in the future.

Jason and Eric, what do you prefer?

Comment 80

[Eric Rescorla (:ekr)]

I would have expected #2. Can you explain #1.

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+2]]

#1 is simply what we have until now in our tests. Add test content inside the runTest() method's callback. I know it's not pretty nice but I don't think we should duplicate that logic to make it somewhat available in the PC test framework. With that we can be sure that whenever the constructor for PeerConnectionTest is being called we have a valid environment in terms of supported platforms, and necessary preferences set.

Comment 82

[Jason Smith [:jsmith]]

I'd prefer to go back to #1 - logic within runTest in the mochitest. It solves the problem we are hitting in a simple manner, follows in consistency with other existing tests, etc.

runTest(function() {
   // test logic
}, true);

Comment 83

[Eric Rescorla (:ekr)]

#1 sounds fine to me.

Comment 84

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Good to see that we all agree here. New patch is upcoming.

Comment 85

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Patch v4

Moved code as discussed. I left the .run() method for the framework for consistency. run() is more descriptive than next(), and the latter should only be used in the individual test steps. 

Try-server push:
https://tbpl.mozilla.org/?tree=Try&rev=6a4023faa015

Comment 86

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Jason, the left-in debug line will be removed for the final landing.

Comment 87

[Eric Rescorla (:ekr)]

Comment on attachment 723427 [details] [diff] [review]
Patch v4

Review of attachment 723427 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 88

[Jason Smith [:jsmith]]

Comment on attachment 723427 [details] [diff] [review]
Patch v4

Review of attachment 723427 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm as well

Comment 89

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Relanded as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/875c77fe1e79

Comment 90

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/875c77fe1e79