Closed Bug 1017321 Opened 6 years ago Closed 6 years ago

Add ICE connection tests to data channel tests

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: drno, Assigned: drno)

References

Details

Attachments

(1 file, 1 obsolete file)

Our data channel tests currently just assume the ICE connection gets established by waiting for the data channel to open.
We should add the ICE connection verification checks between the last set*Description calls and the verification that the data channel got established.
Assignee: nobody → drno
Verify ICE connectivity before proceeding with the test.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=f74fb0fe0d89
Attachment 8430431 [details] [diff] really starts to make sense once the data channel patch for Bug 1013809 has landed.
Target Milestone: --- → mozilla33
Attachment #8433481 - Flags: review?(jib)
Comment on attachment 8433481 [details] [diff] [review]
bug_1017321_add_ice_to_data_channel_tests.patch

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

lgtm. Nit optional. A general framework comment at the end.

::: dom/media/tests/mochitest/templates.js
@@ +432,5 @@
> +        myTest.next();
> +      };
> +      function onIceConnectedFailed () {
> +        dumpSdp(myTest);
> +        ok(false, "pc_local: ICE failed to switch to 'connected' state: " + myPc.iceConnectionState);

Maybe I'm a a stickler, but shouldn't the text in the above two ok() calls be the same?

@@ +452,5 @@
> +  [
> +    'PC_REMOTE_WAIT_FOR_ICE_CONNECTED',
> +    function (test) {
> +      var myTest = test;
> +      var myPc = myTest.pcRemote;

Not a comment on this patch, but I wanted to add a general observation on our test framework that stands out here:

We're externally synchronizing the actions of pcLocal and pcRemote:

  PC_REMOTE_WAIT_FOR_ICE_CONNECTED only goes once
  PC_LOCAL_WAIT_FOR_ICE_CONNECTED above it has finished.

There are probably other steps where this matters more, but this is true throughout, that we have a lot of cross-peer waiting, causing things to pause for each side to complete, which isn't how things work in the real world.

I think it would be more natural for pcLocal and pcRemote to use independent queues.

Not saying we should fix that here, but perhaps open a bug on it.
Attachment #8433481 - Flags: review?(jib) → review+
(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> Maybe I'm a a stickler, but shouldn't the text in the above two ok() calls
> be the same?

I'm a big opponent of identical log/test statements, because it makes it so much harder to understand where your test failed just looking at your log file. And especially with intermittent test failures your log file is usually your only source of information. In these cases distinct log messages are extremely helpful.
 
> Not a comment on this patch, but I wanted to add a general observation on
> our test framework that stands out here:
> 
> We're externally synchronizing the actions of pcLocal and pcRemote:
> 
>   PC_REMOTE_WAIT_FOR_ICE_CONNECTED only goes once
>   PC_LOCAL_WAIT_FOR_ICE_CONNECTED above it has finished.
> 
> There are probably other steps where this matters more, but this is true
> throughout, that we have a lot of cross-peer waiting, causing things to
> pause for each side to complete, which isn't how things work in the real
> world.
> 
> I think it would be more natural for pcLocal and pcRemote to use independent
> queues.
> 
> Not saying we should fix that here, but perhaps open a bug on it.

Very good point. I made the same observation quite some time ago, but never created a bug for it. Here you go: Bug 1020436
(In reply to Nils Ohlmeier [:drno] from comment #5)
> I'm a big opponent of identical log/test statements, because it makes it so
> much harder to understand where your test failed just looking at your log
> file.

I'm not arguing for identical messages for different steps, but identical messages for different success/failure code-paths of the "same step", e.g. when we (ab)use the ok() function explicitly with ok(true)/ok(false).

Consider that the ok function only takes one statement, not two: ok(expression, "my expression is true");

Of which the outcome is either:
  TEST-PASS | my expression is true
or
  TEST-UNEXPECTED-FAIL | my expression is true

So there is no ambiguity, and there's also no ambiguity about what the expression is in the latter case (it is false).

I take this to be the "correct" use given the ok() function's semantics.

In contrast, I find  custom-tailored ok(false) statements hard to follow because I can't tell whether to inverse what I'm reading (e.g. what does "TEST-UNEXPECTED | my expression is false" mean?) 

> Here you go: Bug 1020436

Great! Seems like two queues should work quite nicely.
Just a warning to the sheriffs:
I'm expecting this to cause "new" oranges. But this code is not causing the problem. If this creates intermittent test failures, it is because it catches problems in our tests earlier (not waiting for test framework timeout or my new data channel connection error).
Or in other words: this should not cause more oranges then before, but the label/fingerprint on the oranges changes because of this code.
You need to log in before you can comment on or make changes to this bug.