Adding accept header to callsites within SignalR

[MattyLeslie]

Adding accept header to callsites within SignalR

Summary of the changes As mentioned in #47398, many of the HttpClient call sites within the SignalR library were missing their appropriate accept headers. This change aims to address that issue.

Description Changes made include:

1. Adding accept header to NegotiateAsync() within HttpConnection to ensure header is present for negotiation requests.
2. Adding accept header to SendMessage() within SendUtils for the send requests.
3. Adding accept header to Poll() within LongPollingTransport for the get request.
4. Test to ensure first request and subsequent polling requests for LongPollingTransport have the correct accept header
5. Test to ensure the request for ServerSentEventsTransport has the correct accept header
6. Test to ensure the negotiate in HttpConnection has the correct accept header.

Fixes #47398 by ensuring the negotiation requests and other HttpClient call sites of SignalR are made with the relevant accept header.

[BrennanConroy]

Are you actually changing anything, or just kicking the build? If it's kicking the build please stop. Also, could you please look into adding tests, see https://github.com/dotnet/aspnetcore/blob/42bc1a131e745d91a22030aea28af924b424e4e4/src/SignalR/common/Http.Connections/test/ServerSentEventsTests.cs#L34 as an example.

[BrennanConroy]

Looks like you're missing negotiate? Otherwise I think that covers all the spots.

[BrennanConroy]

I'm sorry to see this closed, anything I can help with? It looked really close to finished.

[MattyLeslie]

Hello @BrennanConroy,

I've reopened the PR and pushed some new commits.

I'm encountering a couple of challenges that I'm currently addressing:

1. For the ServerSentEventsTransport.cs call site, the test hangs if the accept header is missing from the request in the StartAsync() method.
2. The test I created for the NegotiateAsync() method in HttpConnection.cs isn't passing. It seems that not all dependencies are being adequately mocked.

To better test the NegotiateAsync() method, I modified the unit testing constructor to accept a nullable HttpMessageHandler. This change allows me to mock HttpMessageHandler in the CreateHttpClient() method, enabling me to verify the outgoing requests made by the HttpClient during NegotiateAsync(). I hope this change is appropriate.

I welcome any constructive feedback on the testing approach I've implemented for the various call sites. I'm actively working to resolve the issues mentioned above.

[BrennanConroy]

To better test the NegotiateAsync() method, I modified the unit testing constructor to accept a nullable HttpMessageHandler. This change allows me to mock HttpMessageHandler in the CreateHttpClient() method, enabling me to verify the outgoing requests made by the HttpClient during NegotiateAsync(). I hope this change is appropriate.

Why not use HttpConnectionOptions.HttpMessageHandlerFactory? https://github.com/dotnet/aspnetcore/blob/0bae45ebe88ad07f69e13a142c93a05b496dc9b4/src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs#L3272

[MattyLeslie]

Thank you very much for the insight @BrennanConroy. I believe I have completed all the necessary tests.

[MattyLeslie]

@BrennanConroy, it seems functional tests from MVC are failing due to httpClient.timeout. Can I assume this is network related ?

[BrennanConroy]

Ignore it, I'll rerun the tests if I don't have any more comments after reviewing later today.

[BrennanConroy]

Thanks @MattyLeslie! Sorry for all the nit-picky comments 😄