Skip to content

Fix mismatch of connection ID between SignalR C++ client and hub #7681

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

Merged
merged 1 commit into from
Feb 19, 2019

Conversation

wegylexy
Copy link
Contributor

Summary of the changes

  • Append negotiated connection ID to connection URL

Addresses #6980

@BrennanConroy
Copy link
Member

Thanks for fixing this!

Would it be possible for you to add a test or two?

@wegylexy
Copy link
Contributor Author

wegylexy commented Feb 18, 2019

I lack experience in writing automated tests for client. It'd require a method in the hub to return the actual connection ID so the client can compare, and it needs a server running to test the client.

@wegylexy wegylexy changed the title Fix mismatch of connection ID between SignalR C++ client and hub Fix mismatch of connection ID between SignalR C++ client and server context Feb 18, 2019
@wegylexy wegylexy changed the title Fix mismatch of connection ID between SignalR C++ client and server context Fix mismatch of connection ID between SignalR C++ client and hub Feb 18, 2019
@BrennanConroy BrennanConroy added the area-signalr Includes: SignalR clients and servers label Feb 18, 2019
@BrennanConroy
Copy link
Member

We currently only have unit tests for the C++ client. You could try it out if you'd like by looking at some of the current tests and copying one that's similar to the functionality we want to test. https://github.com/aspnet/AspNetCore/blob/master/src/SignalR/clients/cpp/test/signalrclienttests/connection_impl_tests.cpp#L53

Or I can do it later if you'd like 😄

@wegylexy
Copy link
Contributor Author

I'll let you do it.

@BrennanConroy BrennanConroy merged commit dffe9ab into dotnet:master Feb 19, 2019
@BrennanConroy
Copy link
Member

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-signalr Includes: SignalR clients and servers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants