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.

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

Add RTCPeerConnection-close.html tests from PR13499 #16045

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Mar 24, 2019

No description provided.

@youennf
Copy link
Contributor Author

youennf commented Mar 24, 2019

PR not to be merged yet.
@lgrahl, I would like to understand why 24 channels is needed in that test, and the mix with negotiated as well.
If it is needed, I think we should add a very simple test with 1 data channel only and checking for closed state.

@lgrahl
Copy link

lgrahl commented Mar 24, 2019

The mix with negotiated makes sense because negotiated data channels that don't have a matching end on the remote side behave differently - they are open but send data into a void and can trigger potentially interesting bugs on stream resets (see the comment in line 67). Furthermore, negotiated data channels tend to be handled slightly different in implementations.

I guess they could be moved into separate tests.

Edit: It doesn't need to be 24 channels. Two should do. (Background: I think this was originally part of the ID tests.)

@youennf
Copy link
Contributor Author

youennf commented Mar 24, 2019

OK, it makes sense to me to make a very simple test checking closure of a given pc and state of its channels.

And add other tests for negotiated channels.
Maybe simplify to just 1 negotiated channel both side instead of 12?

Also, while the data channels of the pc being closed should get their state synchronously changed, this is probably asynchronously done for the channels of the other pc.
And this is probably what should be tested: negotiated channels without counterparts should get to closed readyState after closing the counterpart pc.

@lgrahl
Copy link

lgrahl commented Mar 24, 2019

Maybe simplify to just 1 negotiated channel both side instead of 12?

👌

Also, while the data channels of the pc being closed should get their state synchronously changed, this is probably asynchronously done for the channels of the other pc.

Good catch.

And this is probably what should be tested: negotiated channels without counterparts should get to closed readyState after closing the counterpart pc.

Not sure I can follow you. If you mean "All local channels should be closed immediately" and this is what we should test for, then, yes, I would agree.

@zcorpan
Copy link
Member

zcorpan commented Nov 22, 2019

@youennf

PR not to be merged yet.

Does this still apply? If yes, please add "do not merge yet" label and explain what the next step is.

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

5 participants