Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#7097 release blocker: regression closed fixed (fixed)

The warning emitted by _selectVerifyImplementation is missing some punctuation.

Reported by: Jean-Paul Calderone Owned by: Jean-Paul Calderone
Priority: normal Milestone: Twisted-14.0.0
Component: core Keywords:
Cc: Glyph Branch: branches/fix-si-warning-7097
branch-diff, diff-cov, branch-cov, buildbot
Author: hynek, exarkun

Description

The warning message is made up of several concatenated string literals. None of the string literals begin or end with a space, though, so words get smashed together in the resulting string.

Change History (19)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Also the warning points at twisted/internet/_sslverify.py:119, a random internal implementation detail. At best this is useless noise, at worst it's confusing to users who will probably think this is a bug in Twisted itself.

Pointing this warning at nothing would probably be better (something like warnings.showwarning(..., filename="", lineno="").

comment:2 Changed 8 years ago by Jean-Paul Calderone

The ImportError handling here also obscures other failures and makes it hard to actually get a working configuration.

If pyasn1_modules isn't installed or is too old then a warning saying "You do not have the service_identity module installed." is displayed. This is simply incorrect information.

comment:3 Changed 8 years ago by Hynek Schlawack

Author: hynek
Branch: branches/fix-si-warning-7097

(In [42562]) Branching to fix-si-warning-7097.

comment:4 Changed 8 years ago by Hynek Schlawack

Keywords: review added

comment:5 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Hynek Schlawack

Thanks.

This is fine as far as it goes although it might be nice to be consistent with the spacing style and use "... and make sure " instead of " all of its ...".

The issue of the warning pointing to a nonsense filename/lineno is still present. So is the issue that the user won't actually be told about a missing or too-old pyasn1_modules.

I suggested one possible solution to the former in a comment above.

A solution for the latter might be to include the string representation of the ImportError in the warning instead of trying to guess and summarize what the problem might be.

comment:6 Changed 8 years ago by Hynek Schlawack

Keywords: review added
Owner: changed from Hynek Schlawack to Jean-Paul Calderone

I believe I’ve addressed it all.

comment:7 Changed 8 years ago by Jean-Paul Calderone

Type: defectregression

comment:8 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Hynek Schlawack

Thanks.

  1. Somehow I overlooked the fact that the branch includes no unit test changes on my last review. This is disturbing. I suppose it means the warning was initially added with no unit tests. I guess that explains why it was a jumble of letters instead of correctly written sentences. :) Let's not compound this error by continuing to leave this code untested.
  2. The warning still points at twisted/internet/_sslverify.py:186:. It would be nice to have some input from other people about how bad this is, but lacking that I'd like to see the file and line number replaced with empty strings at least. It will look weird but it's the kind of line noise people can probably easily ignore?

Thanks again.

comment:9 Changed 8 years ago by Hynek Schlawack

Owner: Hynek Schlawack deleted

I’m afraid I’m under water until July (I know that sounds crass, but I’m leaving for a vacation in on 04/13 and have a string of conferences/vacations after that, and a bunch of work-related work that needs to get done until then).

So I’m removing myself as the owner. Feel free to take over anyone.

comment:10 Changed 8 years ago by Jean-Paul Calderone

(In [42587]) Add a test for the result and the warning when _selectVerifyImplementation is called and pyOpenSSL is older than 0.12.

refs #7097

comment:11 Changed 8 years ago by Jean-Paul Calderone

(In [42588]) Add a test for the result and the warning when _selectVerifyImplementation is called service_identity cannot be imported.

refs #7097

comment:12 Changed 8 years ago by Jean-Paul Calderone

Author: hynekhynek, exarkun
Cc: Glyph added
Keywords: review added

comment:13 Changed 8 years ago by Hynek Schlawack

Unless there's not some great reasons I'm missing, I'd like to suggest to make service_identity an argument too. Consistency & testability FTW.

comment:14 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

Thanks for fixing this. Tests look good, I think it's good to land.

Some optional and minor feedback about the changes here:

  1. flushWarnings should really always take an argument.
  2. I think I actually disagree with hynek's drive-by comment; since OpenSSL is a mandatory dependency it makes sense that it would always be passed (it can't be None in this function, after all) but it would be much more annoying to capture the ImportError if you had to move the service_identity import to module scope.
  3. Probably want to fix the "0,.12" in SelectVerifyImplementationTests.test_pyOpenSSLTooOldWarning's docstring.
  4. I thought that the suppress test attribute was more for places where you're invoking some library where it's emitting a warning you can't control and don't care about. Wouldn't it be better to use flushWarnings here and assert about them too?
  5. The [foo] = list(...) idiom is syntactically handy; I do it all the time. But it's also a place where you get an errror rather than a failure if something goes wrong. The error you get, "too many values to unpack", also doesn't say anything about the values you got. The usual alternative, asserting about the length, then unpacking, is kinda tedious and doesn't actually give you a better error. What do you think about adding a test utility like `[foo] = fixedLength(testCase, 1, value)` so we have a go-to way of unpacking things like that which tells us something useful?
  6. SetAsideModule should clearly be a public API.
  7. The twistedchecker warning about __version__ is dumb, we should really get rid of that. I think there's already bug, but can you check and file one if not?

Other related things I noticed:

  1. I'm not going to talk about all the coverage issues in this module, but the code that imports SSL_CB_HANDSHAKE_(START|DONE), _OpenSSLECCurve._getBinding, the code that inspects set_tlsext_host_name, _idnaBytes, and _idnaText all have similar coverage issues - conditional importing and implementation selection with no direct test coverage - and most of them should probably emit warnings as well.
  2. We should probably start using requireModule in more places so we don't get random coverage issues when we optionally import stuff.

comment:15 Changed 8 years ago by Jean-Paul Calderone

flushWarnings should really always take an argument.

Yep. Just about always. But I think not this time. There's no coherent function for this warning to blame for the problem: this is related to the change to use warnings.showwarning and pass nonsense values for filename and lineno.

Probably want to fix the "0,.12" in SelectVerifyImplementationTests.test_pyOpenSSLTooOldWarning's docstring.

Fixed.

I thought that the suppress test attribute was more for places where you're invoking some library where it's emitting a warning you can't control and don't care about. Wouldn't it be better to use flushWarnings here and assert about them too?

I think that in the tests where I used suppress are places where I don't care about the warning. So the only difference here is that the warning happens to be coming from code we do control rather than code we don't control. :)

I prefer the style in which I wrote these tests because it gives us different test methods that fail because of different problems with the implementation. A test that makes assertions about both warnings and behavior will only tell me about one failure.

I *would* like a better tool for writing tests like this. For example, Eliot offers a decorator to add logging assertions to your test methods so you focus on writing tests for your code's *behavior* and annotate those tests with extra checks to make relating to logging (with an implementation such that you see all of the failed assertions instead of only the first one that runs). This seems like a useful pattern to me.

Not going to address this here but maybe someone should file a ticket for this idea.

The [foo] = list(...) idiom is syntactically handy; I do it all the time. But it's also a place where you get an errror rather than a failure if something goes wrong. The error you get, "too many values to unpack", also doesn't say anything about the values you got. The usual alternative, asserting about the length, then unpacking, is kinda tedious and doesn't actually give you a better error. What do you think about adding a test utility like [foo] = fixedLength(testCase, 1, value) so we have a go-to way of unpacking things like that which tells us something useful?

I mostly agree with the premise. I'm not 100% sold on the benefits of the error vs failure distinction because I think it's impossible to write a test that will "fail" no matter *how* broken the implementation is (or if it's possible it's so incredibly tedious as to completely negate any benefit derived from doing so). But that doesn't mean that an unpacking helper wouldn't be an incremental improvement from the current situation.

However, for the case of warnings, I wonder if it would be better to improve flushWarnings instead. For example, perhaps it should take some arguments for doing warning filtering? Or an argument that says "I expect to see only one warning, please fail the test if there are 0 or >1". Or both of these?

SetAsideModule should clearly be a public API.

Yes. :) #5977. :)

The twistedchecker warning about __version__ is dumb, we should really get rid of that. I think there's already bug, but can you check and file one if not?

If the github issue tracker finished loading before I forget about this, I'll dothat.

I'm not going to talk about all the coverage issues in this module, but the code that imports SSL_CB_HANDSHAKE_(START|DONE), _OpenSSLECCurve._getBinding, the code that inspects set_tlsext_host_name, _idnaBytes, and _idnaText all have similar coverage issues - conditional importing and implementation selection with no direct test coverage - and most of them should probably emit warnings as well.

Yep. Can you file these tickets? I already worked on this more than I planned to. :)

We should probably start using requireModule in more places so we don't get random coverage issues when we optionally import stuff.

Does requireModule help with this? I thought it just fixed pyflakes warnings with the ImportError-based approach.

comment:16 Changed 8 years ago by Jean-Paul Calderone

I think I actually disagree with hynek's drive-by comment; since OpenSSL is a mandatory dependency it makes sense that it would always be passed (it can't be None in this function, after all) but it would be much more annoying to capture the ImportError if you had to move the service_identity import to module scope.

I'll go along with this for now. Possibly at some later time service_identity will change in a way that we need to account for and that will require extra logic in the function being tested here and that will require extra tests. And then someone should definitely make service_identity an argument to make it easily testable. We can do that just in time, though.

comment:17 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [42594]) Merge fix-si-warning-7097

Author: hynek, exarkun Reviewer: exarkun, glyph Fixes: #7097

Add test coverage for the warning emitted when dependencies necessary for correct "service identity" checking in the TLS implementation are missing. Also fix a few minor issues with those warnings.

comment:18 Changed 8 years ago by Glyph

Replying to exarkun:

flushWarnings should really always take an argument.

I prefer the style in which I wrote these tests because it gives us different test methods that fail because of different problems with the implementation. A test that makes assertions about both warnings and behavior will only tell me about one failure.

I can see that benefit, yeah.

I *would* like a better tool for writing tests like this. For example, Eliot offers a decorator to add logging assertions to your test methods so you focus on writing tests for your code's *behavior* and annotate those tests with extra checks to make relating to logging (with an implementation such that you see all of the failed assertions instead of only the first one that runs). This seems like a useful pattern to me.

Not going to address this here but maybe someone should file a ticket for this idea.

Maybe after the new logging system lands ;).

The [foo] = list(...) idiom is syntactically handy; I do it all the time. But it's also a place where you get an errror rather than a failure if something goes wrong. The error you get, "too many values to unpack", also doesn't say anything about the values you got. The usual alternative, asserting about the length, then unpacking, is kinda tedious and doesn't actually give you a better error. What do you think about adding a test utility like [foo] = fixedLength(testCase, 1, value) so we have a go-to way of unpacking things like that which tells us something useful?

I mostly agree with the premise. I'm not 100% sold on the benefits of the error vs failure distinction because I think it's impossible to write a test that will "fail" no matter *how* broken the implementation is (or if it's possible it's so incredibly tedious as to completely negate any benefit derived from doing so). But that doesn't mean that an unpacking helper wouldn't be an incremental improvement from the current situation.

The case I'm concerned with is more the failure vs. error if the code is deleted entirely.

However, for the case of warnings, I wonder if it would be better to improve flushWarnings instead. For example, perhaps it should take some arguments for doing warning filtering? Or an argument that says "I expect to see only one warning, please fail the test if there are 0 or >1". Or both of these?

Composition! I think we should have such a function, and then a flushWarnings helper that composes it.

I'm not going to talk about all the coverage issues in this module, but the code that imports SSL_CB_HANDSHAKE_(START|DONE), _OpenSSLECCurve._getBinding, the code that inspects set_tlsext_host_name, _idnaBytes, and _idnaText all have similar coverage issues - conditional importing and implementation selection with no direct test coverage - and most of them should probably emit warnings as well.

Yep. Can you file these tickets? I already worked on this more than I planned to. :)

Filed: #7282, #7283, #7284, #7286

We should probably start using requireModule in more places so we don't get random coverage issues when we optionally import stuff.

Does requireModule help with this? I thought it just fixed pyflakes warnings with the ImportError-based approach.

Well, requireModule (or something like it - it itself might not really be expressive enough) would let you put the actual imports at the module level, and then pass the imported (or not) modules to functions which could then just be tested with different values. If you go with the ImportError based approach, unless all optional imports are in a function which is called twice, there will always be an uncovered couple of lines including the defaults.

comment:19 Changed 8 years ago by Glyph

Milestone: Twisted-14.0.0
Note: See TracTickets for help on using tickets.