893021 - Need DataChannel tests for non-default dictionary values

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

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

In particular, using the new names in the dictionary.

Bug 892441 is an example of the problem not having them can cause.  (The DataChannel test framework hadn't landed then.)

Comment 1

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

Attachment: add tests for DataChannel dictionary items

first cut at DataChannel dictionary tests.  We don't expose some properties, but I verified manually via NSPR_LOG_MODULES=datachannel:5 that they're being applied.

Comment 2

[Jason Smith [:jsmith]]

Comment on attachment 776721 [details] [diff] [review]
add tests for DataChannel dictionary items

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

review- mainly because I think this will intermittently fail in CI since we're not waiting for onopen to fire on the remote data channel. Other comments are mostly cleanup and nit picks.

::: dom/media/tests/mochitest/pc.js
@@ +526,4 @@
>        }
>  
>        // Register handlers for the remote peer
>        this.pcRemote.registerDataChannelOpenEvents(function (channel) {

In the negotiated case, we shouldn't follow this execution path. So we should only execute this logic if this not involving negotiated channels.

@@ +539,5 @@
> +          // externally negotiated - we need to open from both ends
> +          options.id = options.id || channel.id;  // allow for no id to let the impl choose
> +          self.pcRemote.createDataChannel(options, function (channel) {
> +            remoteChannel = channel;
> +            check_next_test();

This isn't going to always work. You need to wait for onopen to fire on the remote channel you have here before you call check_next_test.

::: dom/media/tests/mochitest/templates.js
@@ +305,5 @@
>        }, options);
>      }
>    ],
>    [
> +    'CREATE_THIRD_DATA_CHANNEL',

Nit - maybe call this CREATE_NEGOTIATED_CHANNEL?

@@ +309,5 @@
> +    'CREATE_THIRD_DATA_CHANNEL',
> +    function (test) {
> +      var options = {negotiated:true, id: 5, protocol:"foo/bar", ordered:false,
> +		     maxRetransmits:500};
> +      test.createDataChannel(options, function (sourceChannel2, targetChannel2) {

Somewhere in here we should test that the channels involved have the negotiated attribute set to true. Same thing for the ordered attribute as well.

@@ +311,5 @@
> +      var options = {negotiated:true, id: 5, protocol:"foo/bar", ordered:false,
> +		     maxRetransmits:500};
> +      test.createDataChannel(options, function (sourceChannel2, targetChannel2) {
> +        is(sourceChannel2.readyState, "open", sourceChannel2 + " is in state: 'open'");
> +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");

In the current implementation workflow this could potentially intermittently fail because you are not waiting for onopen to fire on the target channel. See the other comment in pc.js about this.

@@ +316,5 @@
> +
> +        is(targetChannel2.binaryType, "blob", targetChannel2 + " is of binary type 'blob'");
> +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
> +
> +        if (options.id != undefined) {

Why would this ever be undefined?

@@ +321,5 @@
> +          is(sourceChannel2.id, options.id, sourceChannel2 + " id is:" + sourceChannel2.id);
> +	} else {
> +	  options.id = sourceChannel2.id;
> +	}
> +	var reliable = !options.ordered ? false : (options.maxRetransmits || options.maxRetransmitTime);

In reality, the test options are hardcoded here, which means you should known ahead of time if this is reliable or not. So you shouldn't need this boolean evaluation - just use the expected reliable value you are expecting here.

@@ +325,5 @@
> +	var reliable = !options.ordered ? false : (options.maxRetransmits || options.maxRetransmitTime);
> +        is(sourceChannel2.protocol, options.protocol, sourceChannel2 + " protocol is:" + sourceChannel2.protocol);
> +        is(sourceChannel2.reliable, reliable, sourceChannel2 + " reliable is:" + sourceChannel2.reliable);
> +/*
> +  These aren't exposed by IDL yet

Might be better to make these todos instead of commenting these out.

Comment 3

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

(In reply to Jason Smith [:jsmith] from comment #2)
> Comment on attachment 776721 [details] [diff] [review]
> add tests for DataChannel dictionary items
> 
> Review of attachment 776721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> review- mainly because I think this will intermittently fail in CI since
> we're not waiting for onopen to fire on the remote data channel. Other
> comments are mostly cleanup and nit picks.

See below; I don't think we're failing to wait for onopen.
 
> ::: dom/media/tests/mochitest/pc.js
> @@ +526,4 @@
> >        }
> >  
> >        // Register handlers for the remote peer
> >        this.pcRemote.registerDataChannelOpenEvents(function (channel) {
> 
> In the negotiated case, we shouldn't follow this execution path. So we
> should only execute this logic if this not involving negotiated channels.

ok -- if (!options.negotiated) { .... }

> 
> @@ +539,5 @@
> > +          // externally negotiated - we need to open from both ends
> > +          options.id = options.id || channel.id;  // allow for no id to let the impl choose
> > +          self.pcRemote.createDataChannel(options, function (channel) {
> > +            remoteChannel = channel;
> > +            check_next_test();
> 
> This isn't going to always work. You need to wait for onopen to fire on the
> remote channel you have here before you call check_next_test. 

The callback isn't called until onopen fires.... so remoteChannel and check_next_test() are post-onopen.
See PCW_createDataChannel() - onCreation is called from onopen.

> ::: dom/media/tests/mochitest/templates.js
> @@ +305,5 @@
> >        }, options);
> >      }
> >    ],
> >    [
> > +    'CREATE_THIRD_DATA_CHANNEL',
> 
> Nit - maybe call this CREATE_NEGOTIATED_CHANNEL?
> 
> @@ +309,5 @@
> > +    'CREATE_THIRD_DATA_CHANNEL',
> > +    function (test) {
> > +      var options = {negotiated:true, id: 5, protocol:"foo/bar", ordered:false,
> > +		     maxRetransmits:500};
> > +      test.createDataChannel(options, function (sourceChannel2, targetChannel2) {
> 
> Somewhere in here we should test that the channels involved have the
> negotiated attribute set to true. Same thing for the ordered attribute as
> well.

The creation option 'negotiated' isn't exposed on a channel.
'ordered' isn't exposed in IDL yet (the test is there, but commented out).

> @@ +311,5 @@
> > +      var options = {negotiated:true, id: 5, protocol:"foo/bar", ordered:false,
> > +		     maxRetransmits:500};
> > +      test.createDataChannel(options, function (sourceChannel2, targetChannel2) {
> > +        is(sourceChannel2.readyState, "open", sourceChannel2 + " is in state: 'open'");
> > +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
> 
> In the current implementation workflow this could potentially intermittently
> fail because you are not waiting for onopen to fire on the target channel.
> See the other comment in pc.js about this.

See above.

> @@ +316,5 @@
> > +
> > +        is(targetChannel2.binaryType, "blob", targetChannel2 + " is of binary type 'blob'");
> > +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
> > +
> > +        if (options.id != undefined) {
> 
> Why would this ever be undefined?

You can set negotiated to true and not set id, and let the system select an id, which you then tell the other end (however you want) and the other side calls createDataChannel with that id.

> 
> @@ +321,5 @@
> > +          is(sourceChannel2.id, options.id, sourceChannel2 + " id is:" + sourceChannel2.id);
> > +	} else {
> > +	  options.id = sourceChannel2.id;
> > +	}
> > +	var reliable = !options.ordered ? false : (options.maxRetransmits || options.maxRetransmitTime);
> 
> In reality, the test options are hardcoded here, which means you should
> known ahead of time if this is reliable or not. So you shouldn't need this
> boolean evaluation - just use the expected reliable value you are expecting
> here.

I've coded it this way so we can permute the test as needed, or factor this out and run variant instances of it with different options.

One thing I didn't quite realize about this framework is that almost all the "tests" end up in templates, not in separate files.  Somewhat unfortunate, but maybe more efficient/easier.

> @@ +325,5 @@
> > +	var reliable = !options.ordered ? false : (options.maxRetransmits || options.maxRetransmitTime);
> > +        is(sourceChannel2.protocol, options.protocol, sourceChannel2 + " protocol is:" + sourceChannel2.protocol);
> > +        is(sourceChannel2.reliable, reliable, sourceChannel2 + " reliable is:" + sourceChannel2.reliable);
> > +/*
> > +  These aren't exposed by IDL yet
> 
> Might be better to make these todos instead of commenting these out.

I do plan to re-enable them, and had already coded them.

Comment 4

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #3)
> (In reply to Jason Smith [:jsmith] from comment #2)
> > Comment on attachment 776721 [details] [diff] [review]
> > add tests for DataChannel dictionary items
> > 
> > Review of attachment 776721 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > review- mainly because I think this will intermittently fail in CI since
> > we're not waiting for onopen to fire on the remote data channel. Other
> > comments are mostly cleanup and nit picks.
> 
> See below; I don't think we're failing to wait for onopen.
>  
> > ::: dom/media/tests/mochitest/pc.js
> > @@ +526,4 @@
> > >        }
> > >  
> > >        // Register handlers for the remote peer
> > >        this.pcRemote.registerDataChannelOpenEvents(function (channel) {
> > 
> > In the negotiated case, we shouldn't follow this execution path. So we
> > should only execute this logic if this not involving negotiated channels.
> 
> ok -- if (!options.negotiated) { .... }
> 
> > 
> > @@ +539,5 @@
> > > +          // externally negotiated - we need to open from both ends
> > > +          options.id = options.id || channel.id;  // allow for no id to let the impl choose
> > > +          self.pcRemote.createDataChannel(options, function (channel) {
> > > +            remoteChannel = channel;
> > > +            check_next_test();
> > 
> > This isn't going to always work. You need to wait for onopen to fire on the
> > remote channel you have here before you call check_next_test. 
> 
> The callback isn't called until onopen fires.... so remoteChannel and
> check_next_test() are post-onopen.
> See PCW_createDataChannel() - onCreation is called from onopen.

Ah, that's right. My bad.

> 
> > ::: dom/media/tests/mochitest/templates.js
> > @@ +305,5 @@
> > >        }, options);
> > >      }
> > >    ],
> > >    [
> > > +    'CREATE_THIRD_DATA_CHANNEL',
> > 
> > Nit - maybe call this CREATE_NEGOTIATED_CHANNEL?
> > 
> > @@ +309,5 @@
> > > +    'CREATE_THIRD_DATA_CHANNEL',
> > > +    function (test) {
> > > +      var options = {negotiated:true, id: 5, protocol:"foo/bar", ordered:false,
> > > +		     maxRetransmits:500};
> > > +      test.createDataChannel(options, function (sourceChannel2, targetChannel2) {
> > 
> > Somewhere in here we should test that the channels involved have the
> > negotiated attribute set to true. Same thing for the ordered attribute as
> > well.
> 
> The creation option 'negotiated' isn't exposed on a channel.
> 'ordered' isn't exposed in IDL yet (the test is there, but commented out).
> 
> > @@ +311,5 @@
> > > +      var options = {negotiated:true, id: 5, protocol:"foo/bar", ordered:false,
> > > +		     maxRetransmits:500};
> > > +      test.createDataChannel(options, function (sourceChannel2, targetChannel2) {
> > > +        is(sourceChannel2.readyState, "open", sourceChannel2 + " is in state: 'open'");
> > > +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
> > 
> > In the current implementation workflow this could potentially intermittently
> > fail because you are not waiting for onopen to fire on the target channel.
> > See the other comment in pc.js about this.
> 
> See above.
> 
> > @@ +316,5 @@
> > > +
> > > +        is(targetChannel2.binaryType, "blob", targetChannel2 + " is of binary type 'blob'");
> > > +        is(targetChannel2.readyState, "open", targetChannel2 + " is in state: 'open'");
> > > +
> > > +        if (options.id != undefined) {
> > 
> > Why would this ever be undefined?
> 
> You can set negotiated to true and not set id, and let the system select an
> id, which you then tell the other end (however you want) and the other side
> calls createDataChannel with that id.

That style of handling though should be handled in the framework, not a specific command. A specific command aiming to test a specific set of data, where as the framework would provide the harness for handling the situation you've described.

> 
> > 
> > @@ +321,5 @@
> > > +          is(sourceChannel2.id, options.id, sourceChannel2 + " id is:" + sourceChannel2.id);
> > > +	} else {
> > > +	  options.id = sourceChannel2.id;
> > > +	}
> > > +	var reliable = !options.ordered ? false : (options.maxRetransmits || options.maxRetransmitTime);
> > 
> > In reality, the test options are hardcoded here, which means you should
> > known ahead of time if this is reliable or not. So you shouldn't need this
> > boolean evaluation - just use the expected reliable value you are expecting
> > here.
> 
> I've coded it this way so we can permute the test as needed, or factor this
> out and run variant instances of it with different options.
> 
> One thing I didn't quite realize about this framework is that almost all the
> "tests" end up in templates, not in separate files.  Somewhat unfortunate,
> but maybe more efficient/easier.

The original design of the templates was intended that the initial templates built are intended to act as the happy path workflow. Then, specific tests change around a couple of commands or command list to meet their specific needs. 

What's probably happening here now is that we're turning the data channel template into a catch all, which will grow to be problematic, as the test will take longer & longer to execute. 

This is definitely possible to refactor here, so using the other approach here with multiple files should be possible. I won't block the landing of the patch on that, however.

Sounds like then the only blocking thing that needs to be fixed here is the if(!negotiated) comment. Other than, anything else that is possible to address here (framework cleanup) is a not a blocker for landing.

Comment 5

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

Attachment: add tests for DataChannel dictionary items

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f04c18dc0bd

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f04c18dc0bd