881658 - Fix handling of error and event callbacks in current WebRTC Mochitests

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

In the past couple of weeks some changes landed for how we handle error and event callbacks in the WebRTC mochitests, which make it kinda hard to read. And even with bug 784519 we broke our capability to extend the framework. All those kinds of failures should be fixed. Especially the before mentioned bug prevents us from getting the tests for datachannels landed.

Comment 1

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

Attachment: Patch v1

This patch makes it easier to use the various callbacks for error handlers and events. It also allows us to extend the framework in subclasses which have their own tests in the command steps and we do not force the next step in the signaling tests.

This patch is not completely finished yet, given that I want to check if we can make it simpler how the signaling event checks can be implemented during the pc setup steps. Therefore I just want to get your feedback.

Once finished by tomorrow I will submit a try server run. I can't as of now due to a very limited internet connection.

Comment 2

[Jason Smith [:jsmith]]

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

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

feedback- - I don't like the changes made in pc.js to remove the reusable state change expectation functions. Other changes in here look okay with some comments.

::: dom/media/tests/mochitest/head.js
@@ +176,3 @@
>   */
> +function unexpectedCallbackAndFinish(message) {
> +  var error = new Error();

Can show example output resulting from each error function here when you cause these functions to be called?

@@ +181,5 @@
>     * @param {object} aObj
>     *        The object fired back from the callback
>     */
> +  return function (aObj) {
> +    var where = error.stack.split("\n")[1];

I would rather aim to print the whole stack here, not just part of it.

::: dom/media/tests/mochitest/pc.js
@@ +265,5 @@
>    ],
>    [
>      'PC_LOCAL_SET_LOCAL_DESCRIPTION',
>      function (test) {
> +      var eventFired = true;

I don't like this. This is a regressing implementation here that pulls logic out of a function that promoted reusability.

@@ +614,5 @@
>  
>            self.attachMedia(stream, type, 'local');
>  
>            _getAllUserMedia(constraintsList, index + 1);
> +        }, unexpectedCallbackAndFinish());

Nit - couldn't you just do unexpectedCallbackAndFinish without any parameters included here?

::: dom/media/tests/mochitest/test_peerConnection_throwInCallbacks.html
@@ +17,5 @@
>    let error_count = 0;
>    let oldOnError = window.onerror;
>    window.onerror = function (errorMsg, url, lineNumber) {
>      if (errorMsg.indexOf("Expected") == -1) {
> +      getFail()(errorMsg);

Nit - Why couldn't you do getFail(errorMsg) here?

Comment 3

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

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

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

::: dom/media/tests/mochitest/head.js
@@ +176,3 @@
>   */
> +function unexpectedCallbackAndFinish(message) {
> +  var error = new Error();

Here the output for errors with the upcoming patch.

For unexpected callbacks:

Unexpected callback with message = 'unexpected success callback' at: ["PCW_setLocalDescription@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:715","PCT_setLocalDescription@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:422","commandsPeerConnection<@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:270","_executeNext@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:75",""]


For unexpected event:

Unexpected event 'onsignalingstatechange' fired with message = 'PeerConnectionWrapper (pcLocal)' at: ["PeerConnectionWrapper@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:538","PeerConnectionTest@http://mochi.test:8888/tests/dom/media/tests/mochitest/pc.js:341","@http://mochi.test:8888/tests/dom/media/tests/mochitest/test_peerConnection_addCandidateInHaveLocalOffer.html:20","@http://mochi.test:8888/tests/dom/media/tests/mochitest/head.js:122",""]

::: dom/media/tests/mochitest/pc.js
@@ +265,5 @@
>    ],
>    [
>      'PC_LOCAL_SET_LOCAL_DESCRIPTION',
>      function (test) {
> +      var eventFired = true;

Not sure if you have really read my comment beside the feedback request or not. But I have exactly mentioned that this is not final yet. So please take care about those things in the future.

@@ +614,5 @@
>  
>            self.attachMedia(stream, type, 'local');
>  
>            _getAllUserMedia(constraintsList, index + 1);
> +        }, unexpectedCallbackAndFinish());

No, this is a generator function which produces a new method used as callback. In most cases you want to add a description too.

::: dom/media/tests/mochitest/test_peerConnection_throwInCallbacks.html
@@ +17,5 @@
>    let error_count = 0;
>    let oldOnError = window.onerror;
>    window.onerror = function (errorMsg, url, lineNumber) {
>      if (errorMsg.indexOf("Expected") == -1) {
> +      getFail()(errorMsg);

Because that's how getFail is implemented and I really don't want to mess with it right now.

Comment 4

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

Attachment: Patch v2

This patch now moves everything from the template steps into individual methods attached to the test class. That way we have access to both peer connection instances, which is necessary for the upcoming data channel tests. A latest try run (https://tbpl.mozilla.org/?tree=Try&rev=b2dd926337b4) showed everything as green.

I ask for feedback here again because I have not updated my data channel patch, which would be necessary to confirm that everything is working. Reason is I don't want to have to do massive updates again and again. Once we agree on the implementation on this bug, I will do the update and ensure everything is working.

Comment 5

[Jason Smith [:jsmith]]

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

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

feedback- - this is missing almost all of the state change checks that was in the original patch. The rest looks okay.

::: dom/media/tests/mochitest/pc.js
@@ +266,5 @@
>    ],
>    [
>      'PC_LOCAL_SET_LOCAL_DESCRIPTION',
>      function (test) {
> +      test.setLocalDescription(test.pcLocal, test.pcLocal._last_offer, function () {

I don't see any checks here being made to ensure that the have the correct state change seen in the original diff - "have-local-offer."

@@ +274,5 @@
>    ],
>    [
>      'PC_REMOTE_SET_REMOTE_DESCRIPTION',
>      function (test) {
> +      test.setRemoteDescription(test.pcRemote, test.pcLocal._last_offer, function () {

Similar to the comment here, I don't see the check here to ensure "have-remote-offer" is gone to on the state change.

@@ +292,5 @@
>    ],
>    [
>      'PC_LOCAL_SET_REMOTE_DESCRIPTION',
>      function (test) {
> +      test.setRemoteDescription(test.pcLocal, test.pcRemote._last_answer, function () {

Missing state change analysis here for "stable" as well.

@@ +300,5 @@
>    ],
>    [
>      'PC_REMOTE_SET_LOCAL_DESCRIPTION',
>      function (test) {
> +      test.setLocalDescription(test.pcRemote, test.pcRemote._last_answer, function () {

Missing state change analysis here for "stable" as well.

@@ +395,5 @@
> + * and automatically handles the failure case.
> + *
> + * @param {PeerConnectionWrapper} peer
> +          The peer connection wrapper to run the command on
> + * @param {object} desc

Nit - object should be mozRTCSessionDescription

@@ +401,5 @@
> + * @param {function} onSuccess
> + *        Callback to execute if the local description was set successfully
> + */
> +PeerConnectionTest.prototype.setLocalDescription =
> +function PCT_setLocalDescription(peer, desc, onSuccess) {

Is it possible to reduce the duplication between setLocalDescription & setRemoteDescription? I'm seeing common code between the two functions here.

@@ +455,5 @@
> + * @param {PeerConnectionWrapper} peer
> +          The peer connection wrapper to run the command on
> + * @param {object} desc
> + *        mozRTCSessionDescription for the remote description request
> + * @param {Object} events

Nit - there isn't a parameter here called events.

@@ +544,5 @@
>      self.attachMedia(event.stream, 'video', 'remote');
>    };
>  
> +  /**
> +   * Callback for native peer connection 'ondatachannel' events. If no custom handler

Nit - comment appears to be referencing the incorrect event names.

Comment 6

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

Attachment: Patch v3

Fixed before mentioned issues. Please let me know if that works, so I can update the data channel patch. Thanks.

Comment 7

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

Comment on attachment 761668 [details] [diff] [review]
Patch v3

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

With the updated datachannel test applied the tests work fine and I can extend the framework for additional tests necessary for datachannels.

Jason, mind doing a full review of this patch?

Comment 8

[Jason Smith [:jsmith]]

Comment on attachment 761668 [details] [diff] [review]
Patch v3

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

::: dom/media/tests/mochitest/pc.js
@@ +412,5 @@
> + *        Callback to execute if the local description was set successfully
> + */
> +PeerConnectionTest.prototype.setLocalDescription =
> +function PCT_setLocalDescription(peer, desc, onSuccess) {
> +  var eventFired = false;

If there's a way to reduce the code duplication in this function and setRemoteDescription, then we should consider refactoring the common code out.

Not a blocker to land this however.

Comment 9

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

(In reply to Jason Smith [:jsmith] from comment #8)
> If there's a way to reduce the code duplication in this function and
> setRemoteDescription, then we should consider refactoring the common code
> out.

I would really like to wait with that. As mentioned earlier it's a complicated task and I want to see more use cases like that, e.g. the upcoming data channel tests, which might shed a light on a helper method/class which could simplify those things. But as of now I don't want to implement something which turns out to be totally wrong in the next couple of days. I will keep that in mind.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/03e03ea1a842

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/03e03ea1a842