Add mochitest for handling ICE failure cases

RESOLVED FIXED in Firefox 41

Status

()

defect
P2
normal
Rank:
25
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: drno, Assigned: jbecerra)

Tracking

36 Branch
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [webrtc-mochitest])

Attachments

(5 attachments)

Reporter

Description

5 years ago
No description provided.
Reporter

Updated

5 years ago
Attachment #8509813 - Flags: feedback?(docfaraday)
Comment on attachment 8509813 [details] [diff] [review]
bug_1087629_add_ice_failures_tests.patch

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

I think I'm ok with the general idea/approach here.
Attachment #8509813 - Flags: feedback?(docfaraday) → feedback+
Reporter

Updated

4 years ago
Whiteboard: [webrtc-mochitest]
Reporter

Updated

4 years ago
Assignee: drno → nobody
Assignee

Comment 3

4 years ago
Bug 1087629: adding two new test cases for ICE connection states.
Attachment #8620716 - Flags: review?(docfaraday)
https://reviewboard.mozilla.org/r/10809/#review9509

::: dom/media/tests/mochitest/mochitest.ini:98
(Diff revision 1)
> +[test_peerConnection_closeDuringIce.html]
> +skip-if = toolkit == 'gonk' || buildapp == 'mulet' # b2g (Bug 1059867)
>  [test_peerConnection_errorCallbacks.html]
> +[test_peerConnection_iceFailure.html]
> +skip-if = toolkit == 'gonk' || buildapp == 'mulet' # b2g (Bug 1059867)

Let's try removing these skip-ifs and see if they work, since bug 1169338 landed.

::: dom/media/tests/mochitest/test_peerConnection_closeDuringIce.html:11
(Diff revision 1)
> +    title: "Close PC's during ICE connectivity check"

No apostrophe needed here.

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:14
(Diff revision 1)
> +// Test iceFailure 

Trailing whitespace

::: dom/media/tests/mochitest/test_peerConnection_closeDuringIce.html:31
(Diff revision 1)
> +  info("Stored fake candidate: " + JSON.stringify(cand));  

Trailing whitespace.

Also, let's prepend test.pcLocal to this log.

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:25
(Diff revision 1)
> +  info("Stored fake candidate: " + JSON.stringify(cand));

Let's prepend test.pcRemote here.

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:30
(Diff revision 1)
> +  info("Stored fake candidate: " + JSON.stringify(cand));  

Trailing whitespace.

Also, let's prepend test.pcLocal here.

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:40
(Diff revision 1)
> +  	});

Replace tab with spaces here.

::: dom/media/tests/mochitest/test_peerConnection_closeDuringIce.html:43
(Diff revision 1)
> +  	});

Replace tab with spaces here.

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:62
(Diff revision 1)
> +  var test = new PeerConnectionTest();

This test is going to take a really long time. It would be very nice if we could tweak this to be configurable from the test-case: https://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/head.js#138

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:68
(Diff revision 1)
> +  test.chain.insertBefore("PC_REMOTE_SET_REMOTE_DESCRIPTION", PC_REMOTE_ADD_FAKE_ICE_CANDIDATE);
> +  test.chain.insertBefore("PC_LOCAL_SET_REMOTE_DESCRIPTION", PC_LOCAL_ADD_FAKE_ICE_CANDIDATE);
> +  test.chain.removeAfter("PC_LOCAL_SET_REMOTE_DESCRIPTION");

I suggest the same readability change as with the other test.

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:34
(Diff revision 1)
> +    ok(true, "Local Ice Failure Reached.");

Let's prepend this.pcLocal to the log string here.

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:39
(Diff revision 1)
> +    ok(true, "Remote Ice Failure Reached.");

Let's prepend this.pcRemote to the log string here.

::: dom/media/tests/mochitest/templates.js:408
(Diff revision 1)
> -    test.setRemoteDescription(test.pcLocal, test._remote_answer, STABLE)
> +    return test.setRemoteDescription(test.pcLocal, test._remote_answer, STABLE)

Huh, wonder how this was missed.

::: dom/media/tests/mochitest/test_peerConnection_closeDuringIce.html:71
(Diff revision 1)
> +  test.chain.insertBefore("PC_REMOTE_SET_REMOTE_DESCRIPTION", PC_REMOTE_ADD_FAKE_ICE_CANDIDATE);
> +  test.chain.insertBefore("PC_LOCAL_SET_REMOTE_DESCRIPTION", PC_LOCAL_ADD_FAKE_ICE_CANDIDATE);
> +  test.chain.removeAfter("PC_LOCAL_SET_REMOTE_DESCRIPTION");

This would probably read better if you did the removeAfter("PC_LOCAL_SET_REMOTE_DESCRIPTION") first, then appended the ADD_FAKE_ICE_CANDIDATE and CLOSE_DURING_ICE steps. (you can add the ICE candidates at any time, although adding them after you set the remote descriptions will make bug 1155856 a little easier.

::: dom/media/tests/mochitest/test_peerConnection_closeDuringIce.html:26
(Diff revision 1)
> +  info("Stored fake candidate: " + JSON.stringify(cand));

Let's prepend test.pcRemote to this log (ie; info(test.pcRemote + " Stored ..."))

Updated

4 years ago
Assignee: nobody → jbecerra

Updated

4 years ago
Attachment #8620716 - Flags: review?(docfaraday)
Assignee

Comment 5

4 years ago
Bug 1087629: revising according to review.
Attachment #8621870 - Flags: review?(docfaraday)
Comment on attachment 8621870 [details]
MozReview Request: Bug 1087629: revising according to review.

https://reviewboard.mozilla.org/r/11059/#review9661

::: dom/media/tests/mochitest/test_peerConnection_iceFailure.html:68
(Diff revision 1)
>    var test = new PeerConnectionTest();
>    test.setMediaConstraints([{audio: true}], [{audio: true}]);
>    test.chain.replace("PC_LOCAL_SETUP_ICE_HANDLER", PC_LOCAL_SETUP_NULL_ICE_HANDLER);
>    test.chain.replace("PC_REMOTE_SETUP_ICE_HANDLER", PC_REMOTE_SETUP_NULL_ICE_HANDLER);
>    test.chain.insertAfter("PC_REMOTE_SETUP_NULL_ICE_HANDLER", PC_LOCAL_WAIT_FOR_ICE_FAILED);
>    test.chain.insertAfter("PC_LOCAL_WAIT_FOR_ICE_FAILED", PC_REMOTE_WAIT_FOR_ICE_FAILED);
> -  test.chain.insertBefore("PC_REMOTE_SET_REMOTE_DESCRIPTION", PC_REMOTE_ADD_FAKE_ICE_CANDIDATE);
> -  test.chain.insertBefore("PC_LOCAL_SET_REMOTE_DESCRIPTION", PC_LOCAL_ADD_FAKE_ICE_CANDIDATE);
>    test.chain.removeAfter("PC_LOCAL_SET_REMOTE_DESCRIPTION");
> -  test.chain.append(PC_LOCAL_WAIT_FOR_ICE_FAILURE);
> -  test.chain.append(PC_REMOTE_WAIT_FOR_ICE_FAILURE);
> +  test.chain.append([PC_REMOTE_ADD_FAKE_ICE_CANDIDATE, PC_LOCAL_ADD_FAKE_ICE_CANDIDATE,
> +	PC_LOCAL_WAIT_FOR_ICE_FAILURE, PC_REMOTE_WAIT_FOR_ICE_FAILURE]);
>    test.run();

Let's indent this stuff.
Attachment #8621870 - Flags: review?(docfaraday) → review+
Assignee

Comment 7

4 years ago
Bug 1087629: indentation
Attachment #8623168 - Flags: review?(docfaraday)
Reporter

Comment 8

4 years ago
As Byron is on PTO it might be a good idea to find someone else for review. Maire can you help to identify an alternative reviewer please?
Flags: needinfo?(mreavy)
Given that Byron is on PTO, MT or ekr would be the best reviewers.  If they're unavailable to review please let me know.
Flags: needinfo?(mreavy)
Which patch should I review? The one that's r? is just whitespace.
Juan -- Please put your patches up for review to Ekr so we can get this bug resolved in Fx41.  Thanks!
Flags: needinfo?(jbecerra)
Assignee

Comment 12

4 years ago
Comment on attachment 8623168 [details]
MozReview Request: Bug 1087629: indentation

Byron gave the r+ to the earlier patch which only required final indentation edits. Plusing here to push it across the line.
Flags: needinfo?(jbecerra)
Attachment #8623168 - Flags: review?(docfaraday) → review+
Assignee

Comment 13

4 years ago
Comment on attachment 8620716 [details]
MozReview Request: Bug 1087629: adding two new test cases for ICE connection states.

Bug 1087629: adding two new test cases for ICE connection states.
Assignee

Updated

4 years ago
Attachment #8621870 - Flags: review+
Assignee

Comment 14

4 years ago
Comment on attachment 8621870 [details]
MozReview Request: Bug 1087629: revising according to review.

Bug 1087629: revising according to review.
Assignee

Comment 15

4 years ago
Comment on attachment 8623168 [details]
MozReview Request: Bug 1087629: indentation

Bug 1087629: indentation
Attachment #8623168 - Flags: review+
Assignee

Comment 16

4 years ago
Bug 1087629: Disabling on B2G due to test failurse on B2G emulator.
Assignee

Comment 17

4 years ago
Comment on attachment 8624535 [details]
MozReview Request: Bug 1087629: Disabling on B2G due to test failurse on B2G emulator.

Plused it after bwc r+ earlier patch. I edited mochitest.ini to disable one the tests on B2G due to failures on B2G emulator which appeared on try build.
Attachment #8624535 - Flags: review+
Assignee

Comment 18

4 years ago
I think we are good to go.

Try run with no issues related to this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2066a9b92e4f
Keywords: checkin-needed
backlog: --- → webRTC+
Rank: 25
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/42f7a05958f4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.