Don't fall back to Apollo when not in subprotocols

[jaylett]

If the server doesn't return Sec-WebSocket-Protocol, we can't default to graphql-ws (ie Apollo) if the transport was configured without it as a subprotocol. In that situation, default to graphql-transport-ws (GRAPHQLWS).

[jaylett]

I've encountered a live server that doesn't seem to include Sec-WebSocket-Protocol during initialisation, and speaks graphql-transport-ws not graphql-ws. (At least, carrying this patch locally makes it possible to talk to the server.)

I'm not sure how to write a test for this, because I don't really understand how the (mocked?) servers work.

[jaylett]

Hmm, the lint failure suggests that something other than the normal control path can create the adapter, given the normal WebsocketsProtocolTransportBase always sets subprotocols on the adapter. I'll push a fix for this once the other tests have run.

[leszekhanusz]

For the tests, you have different server pytest fixtures in the conftest.py file

server for the old apollo server and graphqlws_server for the graphql-ws protocol.
If you check those fixtures, you notice that only graphqlws_server sets the protocol back in a header.

Then you have fixtures like client_and_server and client_and_graphqlws_server which are just fixtures made to avoid duplicating the creation of the client and transport in each test.

We have the test_websocket_*.py files using server and client_and_server fixtures
We have the test_graphqlws_*.py files using graphqlws_server and client_and_graphqlws_server fixtures

So, if we want to test that your modification is working, you could in the test_graphqlws_subscription.py file create a test similar to test_graphqlws_subscription, but using the server fixture instead of the graphqlws_server fixture, which will not return the subprotocol and creating the client and session yourself instead of using the client_and_graphqlws_server fixture (see below the test test_graphqlws_subscription_sync for inspiration as it uses the graphqlws_server fixture directly)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (7fb869a) to head (35938a6).
⚠️ Report is 52 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##            master      #586    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           38        40     +2     
  Lines         2908      3314   +406     
==========================================
+ Hits          2908      3314   +406     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[leszekhanusz]

I can add that test if you want.

[jaylett]

I can add that test if you want.

I think that would be better, please - I had a stab myself and the closest I could get was something that passed with the change, but hung (presumably on an async mismatch) without, and there are too many layers of things I'm not familiar with for me to easily start debugging through whatever's running at that point in the tests :)