webrtc: Test legacy offerToReceiveAudio/Video options #8450

adam-be · 2017-11-27T15:00:21Z

No description provided.

ghost · 2017-11-27T15:09:03Z

Build PASSED

Started: 2017-12-15 09:24:52
Finished: 2017-12-15 09:28:23

View more information about this build on:

alvestrand · 2017-11-27T15:21:44Z

I think this won't pass on Chrome (no transceivers), but does it "work" at least to the point where the test is parsed and calls the functions that it should?
What are the results on other browsers?

snuggs · 2017-11-27T18:02:31Z

@alvestrand ref to no transceivers? Or is this wpt related? Would like to get versed.

adam-be · 2017-11-28T06:02:13Z

It won't pass on Chrome (nor Firefox), but from what I see that's the case for all transceiver related tests.

@snuggs, transceivers are part of the spec, but not yet widely implemented.

snuggs · 2017-11-28T06:17:43Z

Copy that @adam-be! That's all I needed to know to start digging.

snuggs

Overall well done. Others may want to chime in on this but I encourage a help link to the appropriate spec. This allows contributors who may not be explicitly versed in the particular area of the spec.
This removes the need for questions like following to be answered after PR is merged. #8450 (comment)

My 2 Satoshi.

snuggs · 2017-11-28T06:28:36Z

webrtc/RTCPeerConnection-createOffer-offerToReceive.html

@@ -0,0 +1,85 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Test legacy offerToReceiveAudio/Video options</title>


@alvestrand what are our conventions for adding author & help meta tags? I have noticed this convention sprinkled throughout the wpt repo (primarily within css subset). I actually wish there were more references especially to the respective spec.

Examples

Spec help example: https://github.com/w3c/web-platform-tests/blob/56ea52b8668ceee63ec196c59dd11ad7797e4f0d/html/semantics/embedded-content/the-video-element/video_initially_paused-ref.html#L5

author example: https://github.com/w3c/web-platform-tests/blob/95eb7be62d2b5a3e9ef1165fd333d353fe3d4b30/html/rendering/bindings/the-button-element/button-type-menu-historical.html#L4

No idea about the convention for meta tags, I've not used them; this should have recommendations at the WPT project level - @foolip may want to comment, but that shouldn't block this PR.

that shouldn't block this PR @alvestrand
Agreed

You can add it if you find it useful. Myself, I never add author info because it's in git history, and add spec links only if the path and filename doesn't have all the info you need, which is often the case.

adam-be · 2017-11-28T08:37:24Z

Thanks for reviewing. I've added a help link.

I guess with our new "test as you commit" approach there will be situations where a test can't be linked to a section in a publicly available spec since the only place the correct section exists, is in pending PR.

alvestrand

This looks good to me. I think this tests the existing spec, so should be submitted without waiting for anything. In my opinion, tests that test changes not committed to the spec should be committed when the change to the spec is committed - there may be more opinions on that.

alvestrand · 2017-11-28T09:27:51Z

webrtc/RTCPeerConnection-createOffer-offerToReceive.html

@@ -0,0 +1,85 @@
+<!doctype html>
+<meta charset=utf-8>
+<title>Test legacy offerToReceiveAudio/Video options</title>


No idea about the convention for meta tags, I've not used them; this should have recommendations at the WPT project level - @foolip may want to comment, but that shouldn't block this PR.

adam-be · 2017-11-28T10:13:14Z

In my opinion, tests that test changes not committed to the spec should be committed when the change to the spec is committed - there may be more opinions on that.

Sounds reasonable to me.

jan-ivar · 2017-11-30T20:51:02Z

So I ran this test in Firefox Nightly (which supports transceivers now), and found a problem: The logic around "stopped" looks flipped.

offerToReceiveAudio option should be ignored if a stopped "sendrecv" transceiver exists

Expected PASS, got FAIL
assert_equals: Expect pc to still have one transceiver expected 1 but got 2
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:60:9
promise callback*@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:58:14
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
promise callback*promise_test@http://web-platform.test:8000/resources/testharness.js:539:31
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:46:5
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:11:3

offerToReceiveAudio option should be ignored if a stopped "recvonly" transceiver exists

Expected PASS, got FAIL
assert_equals: Expect pc to still have one transceiver expected 1 but got 2
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:80:9
promise callback*@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:78:14
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
promise callback*promise_test@http://web-platform.test:8000/resources/testharness.js:539:31
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:65:5
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:11:3

offerToReceiveVideo option should be ignored if a stopped "sendrecv" transceiver exists

Expected PASS, got FAIL
assert_equals: Expect pc to still have one transceiver expected 1 but got 2
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:60:9
promise callback*@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:58:14
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
promise_test/tests.promise_tests<@http://web-platform.test:8000/resources/testharness.js:543:27
promise callback*promise_test@http://web-platform.test:8000/resources/testharness.js:539:31
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:46:5
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:11:3

offerToReceiveVideo option should be ignored if a stopped "recvonly" transceiver exists

Expected PASS, got FAIL
assert_equals: Expect pc to still have one transceiver expected 1 but got 2
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:80:9
promise callback*@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:78:14
Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1489:20
promise_test/tests.promise_tests<@http://web-platform.test:8000/resources/testharness.js:543:27
promise callback*promise_test@http://web-platform.test:8000/resources/testharness.js:539:31
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:65:5
@http://web-platform.test:8000/webrtc/RTCPeerConnection-createOffer-offerToReceive.html:11:3

adam-be · 2017-12-04T10:58:01Z

@jan-ivar, that was my mistake. I've updated the test now and it's all green in Firefox nightly. :)

adam-be · 2017-12-04T11:03:15Z

@snuggs, do you believe that your comments above have been addressed?

fippo · 2017-12-14T15:49:19Z

webrtc/RTCPeerConnection-createOffer-offerToReceive.html

+    }, `offerToReceive${capsType} option should be ignored if a non-stopped "sendrecv" transceiver exists`);
+  });
+
+promise_test(t => {


whitespace nit: indent 2 spaces?
can fix when adding tests for w3c/webrtc-pc#1693

adam-be · 2017-12-14T15:58:53Z

It would be great to merge this PR since we want to base additional tests on it. @snuggs and @foolip, are you happy with the current state of the PR?

fippo · 2017-12-14T16:05:21Z

webrtc/RTCPeerConnection-createOffer-offerToReceive.html

+      return getTrackFromUserMedia(type)
+      .then(([track, stream]) => {
+        pc.addTrack(track, stream);
+        pc.createOffer();


missing return?

True; will fix.

alvestrand · 2017-12-18T14:05:11Z

@snuggs your review is blocking this merge, and was (I think) addressed ~20 days ago. Can you unblock?

Already addressed

webrtc: Test legacy offerToReceiveAudio/Video options

9dc57c8

wpt-pr-bot added webrtc wg-webrtc labels Nov 27, 2017

wpt-pr-bot requested review from agouaillard, alvestrand, dontcallmedom, guidou, henbos, phoglund, rwaldron, snuggs and youennf November 27, 2017 15:00

adam-be mentioned this pull request Nov 27, 2017

Set direction of transceiver created by offerToReceive* option w3c/webrtc-pc#1672

Merged

snuggs previously requested changes Nov 28, 2017

View reviewed changes

webrtc: Add help link

8da8d1b

alvestrand approved these changes Nov 28, 2017

View reviewed changes

webrtc: Align test with updated spec behavior

0e8cdd2

adam-be mentioned this pull request Dec 4, 2017

Fix offerToReceive* bug introduced by PR #1672 w3c/webrtc-pc#1679

Merged

adam-be mentioned this pull request Dec 5, 2017

Add testing guideline for naming test files and adding comments w3c/webrtc-pc#1664

Merged

webrtc: Extend test with two offerToReceive options

ece3211

This was referenced Dec 7, 2017

offerToReceive: Rewrite to handle two options to createOffer w3c/webrtc-pc#1686

Merged

Testing guidelines: Comments or help link? w3c/webrtc-pc#1687

Closed

alvestrand mentioned this pull request Dec 14, 2017

Specify legacy behavior when offerToReceive* option is set to false w3c/webrtc-pc#1693

Merged

fippo reviewed Dec 14, 2017

View reviewed changes

webrtc: Fix indentation

daedbcd

fippo reviewed Dec 14, 2017

View reviewed changes

webrtc: Add missing return at the end of 'then' handler

6db5091

fippo approved these changes Dec 18, 2017

View reviewed changes

alvestrand merged commit 07b32f0 into web-platform-tests:master Dec 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webrtc: Test legacy offerToReceiveAudio/Video options #8450

webrtc: Test legacy offerToReceiveAudio/Video options #8450

adam-be commented Nov 27, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Nov 27, 2017 •

edited by ghost

Loading

alvestrand commented Nov 27, 2017

snuggs commented Nov 27, 2017

adam-be commented Nov 28, 2017

snuggs commented Nov 28, 2017

snuggs left a comment

snuggs Nov 28, 2017

alvestrand Nov 28, 2017

snuggs Nov 28, 2017

foolip Nov 30, 2017

adam-be commented Nov 28, 2017

alvestrand left a comment

alvestrand Nov 28, 2017

adam-be commented Nov 28, 2017

jan-ivar commented Nov 30, 2017

adam-be commented Dec 4, 2017

adam-be commented Dec 4, 2017

fippo Dec 14, 2017

adam-be Dec 14, 2017

adam-be commented Dec 14, 2017

fippo Dec 14, 2017

adam-be Dec 15, 2017

alvestrand commented Dec 18, 2017

webrtc: Test legacy offerToReceiveAudio/Video options #8450

webrtc: Test legacy offerToReceiveAudio/Video options #8450

Conversation

adam-be commented Nov 27, 2017 • edited by wpt-pr-bot Loading

ghost commented Nov 27, 2017 • edited by ghost Loading

Build PASSED

alvestrand commented Nov 27, 2017

snuggs commented Nov 27, 2017

adam-be commented Nov 28, 2017

snuggs commented Nov 28, 2017

snuggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Examples

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-be commented Nov 28, 2017

alvestrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-be commented Nov 28, 2017

jan-ivar commented Nov 30, 2017

offerToReceiveAudio option should be ignored if a stopped "sendrecv" transceiver exists

offerToReceiveAudio option should be ignored if a stopped "recvonly" transceiver exists

offerToReceiveVideo option should be ignored if a stopped "sendrecv" transceiver exists

offerToReceiveVideo option should be ignored if a stopped "recvonly" transceiver exists

adam-be commented Dec 4, 2017

adam-be commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-be commented Dec 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvestrand commented Dec 18, 2017

adam-be commented Nov 27, 2017 •

edited by wpt-pr-bot

Loading

ghost commented Nov 27, 2017 •

edited by ghost

Loading