Raise warning in case a SSLStream method returns none because no handshake was established #743
Conversation
Codecov Report
@@ Coverage Diff @@
## master #743 +/- ##
==========================================
+ Coverage 99.34% 99.34% +<.01%
==========================================
Files 96 96
Lines 11577 11625 +48
Branches 827 828 +1
==========================================
+ Hits 11501 11549 +48
Misses 56 56
Partials 20 20
Continue to review full report at Codecov.
|
|
|
||
| with pytest.raises(NoHandshakeError): | ||
| client.selected_alpn_protocol() | ||
| server.selected_alpn_protocol() |
There was a problem hiding this comment.
In order to make the test actually cover both lines, you need to have two separate pytest.raises() blocks. Otherwise what happens is that either the block exits at the first line because it raises (and it never executes the second one); or the first line doesn't raise, but you won't see that because the second one does raise.
| server.selected_alpn_protocol() | |
| with pytest.raises(NoHandshakeError): | |
| server.selected_alpn_protocol() |
The same applies to the other similar tests. This is the reason why codecov is complaining, and if you fix it (assuming the tests pass) you will get full coverage again :)
There was a problem hiding this comment.
I see, I'll fix it. Thanks for your help with that!
|
@sorcio Just fixed the tests, and they seem to pass locally. Let's see what'll say codecov now :) |
|
@jxub fantastic, thank you! :) Since this change is affecting public API I need to bother you a bit more for a couple small tasks:
I'm not sure what the best strategy to document this is. It could go in the docstring for |
|
I'm gonna look at the failing checks asap. |
|
Failed build seems to just be a Travis backend hiccough, I restarted the job. |
|
Yay! 😍 Thanks. |
|
Thanks, and welcome! 🎉 🎂 And, no pressure, but if you'd like to keep contributing then we'd love to have you, so I'm sending you a github invite now. You can read more about this in our contributing documentation. |
- Move the exception from the main namespace into trio.ssl, since it's ssl-specific, and rename from NoHandshakeError → NeedHandshakeError - Add the exception to the docs - In the newsfragment, use the original issue number, and shorten - Smooth out the docs a bit - Add some more methods that require handshakes to the list
References the issue #735
The problem could be extended to the following methods in SSLStream which return none in case the handshake is not established:
get_channel_binding: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.get_channel_bindingselected_alpn_protocol: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.selected_alpn_protocolselected_npn_protocol: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.selected_npn_protocolThese methods raise now a
NoHandshakeErrorif the handshake wasn't established. Note that they still returnNonein the cases when:SSLContext.set_alpn_protocols()orSSLContext.set_npn_protocols()was not called.and in the case of
get_channel_bindingin the case when there is no connection.