Create a mochitest that tests a basic flow using MediaConstriants during a PC handshake. This means you need test a basic flow that makes use of the third parameter to createOffer and createAnswer that's valid input: void createOffer (RTCSessionDescriptionCallback successCallback, RTCPeerConnectionErrorCallback failureCallback, optional MediaConstraints constraints); void createAnswer (RTCSessionDescriptionCallback successCallback, RTCPeerConnectionErrorCallback failureCallback, optional MediaConstraints constraints);

Comment on attachment 732206 [details] [diff] [review] Offer Media Constriant Tests v1 Here's something bare bones basic using offer constraints only. There's more tests that can definitely be added here, but let's start with something really simple and get that checked in. For this bug also - I'm keeping the data channel media constraint out of scope. Let's stick to OfferToReceiveAudio & OfferToReceiveVideo testing. More input for understanding some useful flows to watch out for media constraints using createOffer, addStream, and createAnswer would be greatly appreciated.

Comment on attachment 732206 [details] [diff] [review] Offer Media Constriant Tests v1 Review of attachment 732206 [details] [diff] [review]: ----------------------------------------------------------------- It's definitely a good addition I would certainly need for my datachannel tests now. So there are some things I would like to see clarified/fixed, especially on the pc module level. Please see the comments inline. ::: dom/media/tests/mochitest/pc.js @@ +359,5 @@ > this.pcRemote.constraints = constraintsRemote; > }; > > /** > + * Sets the media constriants used on a createOffer call in the test. Typo in 'constriants' across all the changed lines. @@ +365,5 @@ > + * @param {object} constriants the media constriants to use on createOffer > + */ > +PeerConnectionTest.prototype.setOfferConstriants = > +function PCT_setOfferConstriants(constriants) { > + this.pcLocal.offerConstriants = constriants; This is a simple pass-through. Why do you think it is necessary? I ask because it's confusing and you don't see to which pc it gets applied. Does it only apply to the local peer or what happens when it gets also set to the remote peer? @@ +408,5 @@ > this.configuration = configuration; > this.label = label; > > this.constraints = [ ]; > + this.offerConstriants = {}; I would appreciate if we could pass this through the configuration of the pc wrapper. Or does something prevent us from that?

Comment on attachment 732206 [details] [diff] [review] Offer Media Constriant Tests v1 Please don't steal reviews from others. Append on to the review, but don't steal.

(In reply to Henrik Skupin (:whimboo) from comment #4) > Comment on attachment 732206 [details] [diff] [review] > Offer Media Constriant Tests v1 > > Review of attachment 732206 [details] [diff] [review]: > ----------------------------------------------------------------- > > It's definitely a good addition I would certainly need for my datachannel > tests now. So there are some things I would like to see clarified/fixed, > especially on the pc module level. Please see the comments inline. > > ::: dom/media/tests/mochitest/pc.js > @@ +359,5 @@ > > this.pcRemote.constraints = constraintsRemote; > > }; > > > > /** > > + * Sets the media constriants used on a createOffer call in the test. > > Typo in 'constriants' across all the changed lines. Will fix. > > @@ +365,5 @@ > > + * @param {object} constriants the media constriants to use on createOffer > > + */ > > +PeerConnectionTest.prototype.setOfferConstriants = > > +function PCT_setOfferConstriants(constriants) { > > + this.pcLocal.offerConstriants = constriants; > > This is a simple pass-through. Why do you think it is necessary? I ask > because it's confusing and you don't see to which pc it gets applied. Does > it only apply to the local peer or what happens when it gets also set to the > remote peer? This is needed for media constraint hook up for the offer. This would only apply to the local peer, as the local peer is making the offer. It doesn't make sense to set it to the remote peer in this case - each set of media constraints needs to be individualized by each function. That's also the whole point of the test, too. > > @@ +408,5 @@ > > this.configuration = configuration; > > this.label = label; > > > > this.constraints = [ ]; > > + this.offerConstriants = {}; > > I would appreciate if we could pass this through the configuration of the pc > wrapper. Or does something prevent us from that? This I don't agree with based on the design of the framework. Our design indicates that we should default to the behavior of the typical smoke test first and modify only if necessary. The typical default is {}. Use set when you need to change it.

Comment on attachment 732334 [details] [diff] [review] Part 1 - Offer Media Constriant Tests v2 This time, with correct spelling.

(In reply to Jason Smith [:jsmith] from comment #6) > > @@ +408,5 @@ > > > this.configuration = configuration; > > > this.label = label; > > > > > > this.constraints = [ ]; > > > + this.offerConstriants = {}; > > > > I would appreciate if we could pass this through the configuration of the pc > > wrapper. Or does something prevent us from that? > > This I don't agree with based on the design of the framework. Our design > indicates that we should default to the behavior of the typical smoke test > first and modify only if necessary. The typical default is {}. Use set when > you need to change it. I think you misunderstood my request. I haven't said that the values which you are passing in here should be the default ones. But they should be passed through the configuration parameter. Given that we will have different data blocks something like the following is in my mind: configuration = { sdp: {}, offer: {} } If a test has to change the defaults, it should specify it appropriately. Adding another method doesn't look optimal to me.

(In reply to Henrik Skupin (:whimboo) from comment #9) > (In reply to Jason Smith [:jsmith] from comment #6) > > > @@ +408,5 @@ > > > > this.configuration = configuration; > > > > this.label = label; > > > > > > > > this.constraints = [ ]; > > > > + this.offerConstriants = {}; > > > > > > I would appreciate if we could pass this through the configuration of the pc > > > wrapper. Or does something prevent us from that? > > > > This I don't agree with based on the design of the framework. Our design > > indicates that we should default to the behavior of the typical smoke test > > first and modify only if necessary. The typical default is {}. Use set when > > you need to change it. > > I think you misunderstood my request. I haven't said that the values which > you are passing in here should be the default ones. But they should be > passed through the configuration parameter. Given that we will have > different data blocks something like the following is in my mind: > > configuration = { sdp: {}, offer: {} } > > If a test has to change the defaults, it should specify it appropriately. > Adding another method doesn't look optimal to me. No, I don't agree with that. Configurations also per the spec can have custom constraints as well. AddStream, createOffer, and createAnswer constraints are specified in our tests will be a bit more rare (only when we absolutely are intending to test these). I don't want to pollute the constructor and would rather only indicate it when we need to post construction.

(In reply to Jason Smith [:jsmith] from comment #10) > No, I don't agree with that. Configurations also per the spec can have > custom constraints as well. AddStream, createOffer, and createAnswer > constraints are specified in our tests will be a bit more rare (only when we > absolutely are intending to test these). I don't want to pollute the > constructor and would rather only indicate it when we need to post > construction. Keep in mind that we have the framework to make it easier for test writers to create tests, even for those rare cases. Also whenever a test needs custom constraints for whatever method you pointed out above, you want to create separate class methods, which assign those constraints? This will pollute the whole class instead of only having some additional lines in the constructor. So you say that's better? So what I think will be best will show the following examples: Default with no custom constraints: var test = new PeerConnectionTest(); Custom constraints for local pc: var test = new PeerConnectionTest({config_pc1: {...}}); Custom constraints for createOffer: var test = new PeerConnectionTest({offer: {...}}); Custom constraints for createOffer and createAnswer: var test = new PeerConnectionTest({offer: {...}, answer: {...}}); In all those cases the constructor code will not change at all. Only the appropriate steps in the command chain will check if a custom constraint has been specified for the current action. [ 'PC_LOCAL_CREATE_OFFER', function (test) { test.pcLocal.createOffer(function () { test.next(); }, test.options.offer); } ], If no custom constraints have been set we will pass an undefined value to the createOffer() method of the real pc instance.

(In reply to Henrik Skupin (:whimboo) from comment #11) > (In reply to Jason Smith [:jsmith] from comment #10) > > No, I don't agree with that. Configurations also per the spec can have > > custom constraints as well. AddStream, createOffer, and createAnswer > > constraints are specified in our tests will be a bit more rare (only when we > > absolutely are intending to test these). I don't want to pollute the > > constructor and would rather only indicate it when we need to post > > construction. > > Keep in mind that we have the framework to make it easier for test writers > to create tests, even for those rare cases. Also whenever a test needs > custom constraints for whatever method you pointed out above, you want to > create separate class methods, which assign those constraints? This will > pollute the whole class instead of only having some additional lines in the > constructor. So you say that's better? Yeah, actually I do. Double dictionaries are going to look really ugly in the constructor if we go the route you suggest. I also think that will be really hard to read too from the class-level and caller-level. > > So what I think will be best will show the following examples: > > Default with no custom constraints: > var test = new PeerConnectionTest(); > > Custom constraints for local pc: > var test = new PeerConnectionTest({config_pc1: {...}}); > > Custom constraints for createOffer: > var test = new PeerConnectionTest({offer: {...}}); > > Custom constraints for createOffer and createAnswer: > var test = new PeerConnectionTest({offer: {...}, answer: {...}}); This is going to lead to double dictionaries, which is messy. As I said before, use of media constraints is not a typical value you will set in many tests, so one off set calls I still think is a better approach and cleaner, especially since it avoid double dictionaries which will impact readability. > > In all those cases the constructor code will not change at all. Only the > appropriate steps in the command chain will check if a custom constraint has > been specified for the current action. > > [ > 'PC_LOCAL_CREATE_OFFER', > function (test) { > test.pcLocal.createOffer(function () { > test.next(); > }, test.options.offer); This I don't think is a good idea. Double dictionaries as a value into an object is really confusing design-wise.

(In reply to Jason Smith [:jsmith] from comment #12) > Yeah, actually I do. Double dictionaries are going to look really ugly in > the constructor if we go the route you suggest. I also think that will be > really hard to read too from the class-level and caller-level. > > > > Default with no custom constraints: > > var test = new PeerConnectionTest(); > > > > Custom constraints for local pc: > > var test = new PeerConnectionTest({config_pc1: {...}}); > > > > Custom constraints for createOffer: > > var test = new PeerConnectionTest({offer: {...}}); > > > > Custom constraints for createOffer and createAnswer: > > var test = new PeerConnectionTest({offer: {...}, answer: {...}}); Not sure what is looking ugly for you, but for me this is fine: const constraints = { ... } new PeerConnectionTest({offer: constraints}); I don't say that you will have to declare the whole object inside the constructor call. And we currently already have this logic for the pc configuration. So nothing would be different for other custom constraints. The API stays the same. > > [ > > 'PC_LOCAL_CREATE_OFFER', > > function (test) { > > test.pcLocal.createOffer(function () { > > test.next(); > > }, test.options.offer); > > This I don't think is a good idea. Double dictionaries as a value into an > object is really confusing design-wise. Not sure I understand that one. Here we use a simple dictionary or undefined if custom constraints have not been specified. I really don't see a reason to add tons of helper methods on the test class which are getting used that rarely and blows up the framework. Given that Eric was also involved into the initial framework design lets pull him in, so that he can also give his opinions.

Randell is already marked as a reviewer on this patch to give his opinion, no need to flag Eric down on this. This is turning into an unnecessary stylistic argument that's really not worth debating too. At this point, I'm done arguing about this.

Comment on attachment 732334 [details] [diff] [review] Part 1 - Offer Media Constriant Tests v2 Changing patch title to clarify this only one part of the tests of this patch, not the whole thing.

Comment on attachment 732334 [details] [diff] [review] Part 1 - Offer Media Constriant Tests v2 Review of attachment 732334 [details] [diff] [review]: ----------------------------------------------------------------- On the general issue of contention: I find this pattern reasonable: const offerConstraints = { ... }; const answerConstraints = { ... }; var test = new PeerConnectionTest({offer: offerContraints, answer: answerConstraints...}; Are there other ways to do this? Certainly. This seems to have more constrained impact if you need to add a new call with constraints to test, or to write a test which uses constraints, and has no impact on code that doesn't use constraints. So the example in this code of: var test = new PeerConnectionTest(); test.setOfferConstraints({ mandatory: { OfferToReceiveAudio: true } }); becomes: const offerConstraints = { mandatory: { OfferToReceiveAudio: true } }; var test = new PeerConnectionTest({ offer: offerConstraints }); which I see as equivalent in readability. I'm going to r+ because both methods will work, and both patterns will not be confusing to use. I don't see adding a function for each new object that takes constraints as problematic either - the number of such functions is quite limited. If I am expressing a preference, I have a small preference for a data-driven configuration; the only thing that can't easily express would be something that requires ordering. But I'm good with either.

Okay. Here's what I'll do based on Randell's comments: 1. Let's land part 1 in the tree to get coverage given the r+. 2. For part 2 of the tests, I could add an enhancement that allows for both types - data driven initial definition vs. set directly. Note that we still end up needing the set in the end if we ever introduce tests that modify offer constraints after the fact. 3. On the part 2 piece, I'll move over the data driven model, but leave behind the set functions for convenience for later tests.

(In reply to Jason Smith [:jsmith] from comment #17) > 2. For part 2 of the tests, I could add an enhancement that allows for both > types - data driven initial definition vs. set directly. Note that we still > end up needing the set in the end if we ever introduce tests that modify > offer constraints after the fact. That's a conclusion I also like and which sounds fine to me. But one thing you should make sure is that both ways are updating the internal options.foobar member, so follow-up changes to the constraints will update the correct set of data. That way we can save the additionally added internal properties.

Don't know when I'll get back to this. Unassigning for now.

Mass change P3->P4 to align with new Mozilla triage process.