Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add test for constructing RTCSctpTransport #6147

Merged
merged 2 commits into from
Jun 7, 2017
Merged

Add test for constructing RTCSctpTransport #6147

merged 2 commits into from
Jun 7, 2017

Conversation

soareschen
Copy link
Contributor

@soareschen soareschen commented Jun 5, 2017 •
edited by wpt-pr-bot
Loading

Add basic test for indirect construction of RTCSctpTransport. According to the spec. The only time an RTCSctpTransport is constructed is during setLocalDescription or setRemoteDescription with answer containing a data media line.

@ghost
Copy link

ghost commented Jun 5, 2017 •
edited by ghost
Loading

View the complete job log.

Firefox (nightly)

Testing web-platform-tests at revision 7d06454
Using browser at version BuildID 20170605100313; SourceStamp 275588f4d852d7dc183a9dcc70a311413dc7a063
Starting 10 test iterations
All results were stable

All results

1 test ran
/webrtc/RTCSctpTransport-constructor.html
Subtest Results Messages
OK
setRemoteDescription() with answer containing data media should initialize pc.sctp FAIL assert_equals: expected (object) null but got (undefined) undefined
setLocalDescription() with answer containing data media should initialize pc.sctp FAIL assert_equals: expected (object) null but got (undefined) undefined

@ghost
Copy link

ghost commented Jun 5, 2017 •
edited by ghost
Loading

View the complete job log.

Sauce (safari)

Testing web-platform-tests at revision 7d06454
Using browser at version 10.0
Starting 10 test iterations
All results were stable

All results

1 test ran
/webrtc/RTCSctpTransport-constructor.html
Subtest Results Messages
OK
setRemoteDescription() with answer containing data media should initialize pc.sctp FAIL Can't find variable: RTCPeerConnection
setLocalDescription() with answer containing data media should initialize pc.sctp FAIL Can't find variable: RTCPeerConnection

@ghost
Copy link

ghost commented Jun 5, 2017 •
edited by ghost
Loading

View the complete job log.

Chrome (unstable)

Testing web-platform-tests at revision 7d06454
Using browser at version 60.0.3112.10 dev
Starting 10 test iterations
All results were stable

All results

1 test ran
/webrtc/RTCSctpTransport-constructor.html
Subtest Results Messages
OK
setRemoteDescription() with answer containing data media should initialize pc.sctp FAIL assert_equals: expected (object) null but got (undefined) undefined
setLocalDescription() with answer containing data media should initialize pc.sctp FAIL assert_equals: expected (object) null but got (undefined) undefined

@ghost
Copy link

ghost commented Jun 5, 2017 •
edited by ghost
Loading

View the complete job log.

Sauce (MicrosoftEdge)

Testing web-platform-tests at revision 7d06454
Using browser at version 14.14393
Starting 10 test iterations
All results were stable

All results

1 test ran
/webrtc/RTCSctpTransport-constructor.html
Subtest Results Messages
OK
setRemoteDescription() with answer containing data media should initialize pc.sctp FAIL 'RTCPeerConnection' is undefined
setLocalDescription() with answer containing data media should initialize pc.sctp FAIL 'RTCPeerConnection' is undefined

rwaldron
rwaldron requested changes Jun 5, 2017
View reviewed changes
Copy link
Contributor

@rwaldron rwaldron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two minor changes

webrtc/RTCSctpTransport-constructor.html Outdated
const pc = new RTCPeerConnection();
return pc.setRemoteDescription(offer)
.then(() => pc.createAnswer());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit pick: this function body has a 4 space indent!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the helper code now that I can directly import it from the merged helper.js

webrtc/RTCSctpTransport-constructor.html
@@ -0,0 +1,95 @@
<!doctype html>
<meta charset="utf-8">
<title>RTCSctpTransport constructor</title>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add this to interfaces.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This require quite a bit of refactoring on interfaces.html since it involves async initialization. I will put that in a separate PR together with other pending interface tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will put that in a separate PR together with other pending interface tests.

Sounds like a good plan!

@soareschen soareschen force-pushed the rtcsctptransport-constructor branch from 816643d to 789f46e Compare June 6, 2017 05:04
@soareschen
Copy link
Contributor Author

Rebased and ready to merge.

@sideshowbarker
Copy link
Contributor

w3c-test:mirror

@wpt-pr-bot wpt-pr-bot requested a review from youennf June 7, 2017 03:04
rwaldron
rwaldron approved these changes Jun 7, 2017
View reviewed changes
Copy link
Contributor

@rwaldron rwaldron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent

webrtc/RTCSctpTransport-constructor.html
@@ -0,0 +1,95 @@
<!doctype html>
<meta charset="utf-8">
<title>RTCSctpTransport constructor</title>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will put that in a separate PR together with other pending interface tests.

Sounds like a good plan!

webrtc/RTCSctpTransport-constructor.html

// The following helper functions are called from RTCPeerConnection-helper.js:
// generateOffer()
// generateAnswer()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome :)

If I have time, I will try to update all the other tests with this comment.

@rwaldron rwaldron merged commit f2211f3 into web-platform-tests:master Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwaldron rwaldron rwaldron approved these changes

@dontcallmedom dontcallmedom Awaiting requested review from dontcallmedom

@agouaillard agouaillard Awaiting requested review from agouaillard

@alvestrand alvestrand Awaiting requested review from alvestrand

@foolip foolip Awaiting requested review from foolip

@phoglund phoglund Awaiting requested review from phoglund

@henbos henbos Awaiting requested review from henbos

@guidou guidou Awaiting requested review from guidou

@youennf youennf Awaiting requested review from youennf

Assignees
No one assigned
Labels
webrtc wg-webrtc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@soareschen @sideshowbarker @rwaldron @wpt-pr-bot