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.

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

adds tests for the missing members of RTCRtpHeaderExtensionParameters #10282

Closed
wants to merge 1 commit into
base: master
from

Conversation

@kritisingh1
Member

kritisingh1 commented Apr 3, 2018

Closes #10253
@foolip please review and let me know further changes required, if any.
Thanks

@w3c-bots

This comment has been minimized.

w3c-bots commented Apr 3, 2018

Build PASSED

Started: 2018-04-03 16:09:40
Finished: 2018-04-03 16:23:02

View more information about this build on:

@kritisingh1

This comment has been minimized.

Member

kritisingh1 commented Apr 19, 2018

@jgraham could you please review this and tell me if any further changes are required? Thanks :)

@alvestrand

Sorry, I don't think that this proposal for more tests makes sense.
It's not possible to add RTP extensions that the browser doesn't support - they require code deep in the WebRTC RTP engine, which you can't add through the API.
See http://w3c.github.io/webrtc-pc/#rtcrtpparameters - the header extensions is a read-only parameter.
I don't have any idea how you can verify that these mandatory fields in a read-only parameter are mandatory, but this approach won't work.

return promise_rejects(t, 'InvalidModificationError',
sender.setParameters(param));
}, `setParameters() with modified headerExtensions should reject with InvalidModificationError, checks that id is a required field`);

This comment has been minimized.

@alvestrand

alvestrand Nov 3, 2018

Contributor

This test doesn't test anything at all. You can't add new header extensions, and the error code (InvalidModificationError) is the same for all cases.

@alvestrand

This comment has been minimized.

Contributor

alvestrand commented Nov 3, 2018

Closing this PR, since a different approach is required (and I don't know what it is).

@alvestrand alvestrand closed this Nov 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment