If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add mochitest for handling ICE failure cases

RESOLVED FIXED in Firefox 41

Status

()

Core
WebRTC
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: drno, Assigned: juanb)

Tracking

36 Branch
mozilla41
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [webrtc-mochitest])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

Comment hidden (empty)
(Reporter)

Comment 1

3 years ago
Created attachment 8509813 [details] [diff] [review]
bug_1087629_add_ice_failures_tests.patch
(Reporter)

Updated

3 years ago
Attachment #8509813 - Flags: feedback?(docfaraday)

Comment 2

3 years ago
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

3 years ago
Whiteboard: [webrtc-mochitest]
(Reporter)

Updated

2 years ago
Assignee: drno → nobody
(Assignee)

Comment 3

2 years ago
Created 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.
Attachment #8620716 - Flags: review?(docfaraday)

Comment 4

2 years ago
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

2 years ago
Assignee: nobody → jbecerra

Updated

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

Comment 5

2 years ago
Created attachment 8621870 [details]
MozReview Request: Bug 1087629: revising according to review.

Bug 1087629: revising according to review.
Attachment #8621870 - Flags: review?(docfaraday)

Comment 6

2 years ago
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

2 years ago
Created attachment 8623168 [details]
MozReview Request: Bug 1087629: indentation

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

Comment 8

2 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

2 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

2 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

2 years ago
Attachment #8621870 - Flags: review+
(Assignee)

Comment 14

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

Bug 1087629: revising according to review.
(Assignee)

Comment 15

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

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

Comment 16

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

Bug 1087629: Disabling on B2G due to test failurse on B2G emulator.
(Assignee)

Comment 17

2 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

2 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

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/42f7a05958f4
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/42f7a05958f4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.