Issue 3781: warnings.catch_warnings fails gracelessly when recording warnings but no warnings are emitted (original) (raw)
Created on 2008-09-04 22:10 by exarkun, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (41)
Author: Jean-Paul Calderone (exarkun) * 
Date: 2008-09-04 22:10
This example shows the behavior:
from warnings import catch_warnings
def test():
with catch_warnings(True) as w:
assert str(w.message) == "foo", "%r != %r" % (w.message, "foo")
test()This fails with an IndexError from the w.message. That's a bit
surprising, and since this is mostly an API useful for testing, it'd be
much better if it had a well-defined, documented (ie, stable and likely
to continue working in the next release of Python) error mode.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-04 22:22
On Thu, Sep 4, 2008 at 3:10 PM, Jean-Paul Calderone <report@bugs.python.org> wrote:
New submission from Jean-Paul Calderone <exarkun@divmod.com>:
This example shows the behavior:
from warnings import catch_warnings
def test(): with catch_warnings(True) as w: assert str(w.message) == "foo", "%r != %r" % (w.message, "foo")
test()
This fails with an IndexError from the
w.message. That's a bit surprising, and since this is mostly an API useful for testing, it'd be much better if it had a well-defined, documented (ie, stable and likely to continue working in the next release of Python) error mode.
The question is what exception to raise when no warning has been recorded. AttributeError goes with the idea that the attributes are just not set since no warnings are there to set the attributes. LookupError doesn't seem quite right. TypeError is definitely wrong since it has nothing to do with the type of anything.
So unless someone comes up with a better suggestion I will change getattr on catch_warnings to raise AttributeError when IndexError is raised because no warning is currently recorded.
Author: Jean-Paul Calderone (exarkun) *
Date: 2008-09-04 22:52
The specific exception type isn't that important to me, as long as I can rely on it being something in particular.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-04 22:56
I won't be able to get to this until tonight, but assuming no one objects, I will make it be an AttributeError and a release blocker so that the API can be considered stable in rc1.
Author: Antoine Pitrou (pitrou) *
Date: 2008-09-04 23:25
Why wouldn't w.message simply be None?
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-05 03:59
There is no specific reason why it would be, although that is an option as well. Part of the problem with None is that it is a legitimate default value for some arguments to showwarning() so it doesn't necessarily reflect that no exception was raised if you don't look at key attributes.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-05 04:11
The attached patch has WarningsRecorder raise AttributeError when there is no recorded attribute and yet one tries to access warnings attributes.
And just so you know, JP, make sure to use keyword arguments when calling catch_warnings() (in case you didn't notice the note in the docs). In Py3K they are keyword-only.
Author: Barry A. Warsaw (barry) *
Date: 2008-09-06 17:03
I hate to make API suggestions this late in the process, but this is the first time I've looked at this. I think the basic problem is that the context manager API is a bit weird. What I don't like is the fact that getattr() indexes the last item in the WarningsRecorder. I don't know the history here, but why wouldn't this be a better API?
with catch_warnings(True) as w:
assert len(w) > 0, 'No caught warnings'
assert str(w[-1].message) == 'foo', 'blah blah'Author: Brett Cannon (brett.cannon) *
Date: 2008-09-06 18:24
The use of getattr is a historical artifact. Originally there was no way to record multiple warnings as the object returned by catch_warnings() represented only a single instance of a warning. But then the ability to record multiple warnings was added. To not break existing code (I am guessing), the setting of attributes on the WarningsRecorder was added. To tawdry life of catch_warnings(). =)
While having the attributes of the last warning directly accessible is handy, I am in no way married to the idea. Actually, if we run with the list analogy we can just ditch WarningsRecorder and return a list itself that is directly appended to through a version of showwarning() that is a closure instead. That cuts out code and everyone knows how to work with lists as sequential recording of events.
OK, I'm sold. =) I will work on this today or tomorrow and get the patch done and ready to go no later than Sunday evening for review.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-06 18:26
And I forgot to mention that subclassing list is a new thing I tossed in when I moved the code and tweaked the API so that's another reason that using list's abilities was not pervasive.
Author: Barry A. Warsaw (barry) *
Date: 2008-09-06 18:31
Sounds good. This needs to go into 2.6 and 3.0. And while you're add it, can you add catch_warnings to the all?
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-06 22:20
On Sat, Sep 6, 2008 at 11:31 AM, Barry A. Warsaw <report@bugs.python.org> wrote:
Barry A. Warsaw <barry@python.org> added the comment:
Sounds good. This needs to go into 2.6 and 3.0.
Oh, definitely.
And while you're add it, can you add catch_warnings to the all?
Yep. And I will also change the init() so that it properly forces keyword-only arguments in 2.6 like 3.0.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-07 21:29
The new patch ditches the WarningsRecorder class and just returns a list that is directly mutated. I also removed all uses of test.test_support.catch_warning() and moved them over. Docs have been thoroughly updated to give example usage. And lastly, I fixed bsddb to use catch_warnings() where it was blindly resetting the warnings filter. It still sets a filter at the top of the module, but I didn't want to have to search for all potential places where catch_warnings() would have been needed.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-09-08 22:12
The patch mostly looks good. However, all the w[-1] logic looks rather verbose to me since its main use case in testing will be making sure one warning happened. Returning a list adds the extra step of checking the length and then indexing it for the warning validation. I'm not completely suggesting that you bring back the smart list, but maybe an option on catch_warning to just yield the WarningMessage on enter.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-08 23:17
On Mon, Sep 8, 2008 at 3:12 PM, Benjamin Peterson <report@bugs.python.org> wrote:
Benjamin Peterson <musiccomposition@gmail.com> added the comment:
The patch mostly looks good. However, all the w[-1] logic looks rather verbose to me since its main use case in testing will be making sure one warning happened. Returning a list adds the extra step of checking the length and then indexing it for the warning validation. I'm not completely suggesting that you bring back the smart list, but maybe an option on catch_warning to just yield the WarningMessage on enter.
Well, the real question is whether most users will use this for testing, or for temporarily suppressing warnings. The stdlib is not a normal use-case in this regard since we have to be so careful with giving deprecations.
I honest don't fine the [-1] indexing that bad and I had to add all of them. =) Makes it explicit you are assuming there is at least one warnings (and probably only one) and you should check that there was not an extra one.
I will wait to see if Barry has anything to say on the matter since he pushed for the change.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-09 00:50
Covered by r66321 in the trunk. Now I just need to merge into 3.0.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-09 01:52
r66322 has the fix in 3.0.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 10:38
Reopening this because I disagree with the fix - I would prefer to see catch_warnings() reverted back to the API and implementation* it used prior to the test_support->warnings migration.
That version had perfectly clear semantics when no warning was issued: w.message (and all of the other warning attributes) were None. If the implementation of WarningsRecorder hadn't been changed then this bug would have never arisen.
The attributes of the last warning are cached on the recorder because by far the most common intended use case that makes use of the warnings recorder is to test operations that are expected to raise a single warning. The list of warnings is retained solely for special cases where one operation raises multiple warnings (e.g. see the py3k warnings tests for hash inheritance).
*aside from the use of @contextmanager, obviously
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 10:40
As far as the use cases go, it was moved out of test.test_support to warnings SOLELY due to the requests from the twisted folks for assistance in testing their generation of warnings. The fact that it is also useful for suppressing warnings in general without affecting the global state of the warnings module (aside from thread safety issues) is just a bonus.
Author: Barry A. Warsaw (barry) *
Date: 2008-09-09 12:20
With a name like catch_warnings() in the warnings module, it will definitely get used in production code to suppress warnings. If its intended to be used only by tests, then it belongs somewhere else. If not test_support then maybe unittest. If it were moved then I wouldn't care about the bug that all other warnings caught are inaccessible.
You'd still have to fix the w.messages attribute to be None if there were no warnings raised.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 13:11
It turns out the warnings.catch_warnings version has re-entrancy issues due to the fact that it can't use @contextmanager:
Python 2.6b3+ (trunk:66143M, Sep 2 2008, 20:04:43) [GCC 4.2.3 (Ubuntu 4.2.3-2ubuntu7)] on linux2 Type "help", "copyright", "credits" or "license" for more information.
import warnings orig_filters = warnings.filters cw = warnings.catch_warnings() with cw: ... warnings.filters = [] ... with cw: ... pass ... warnings.filters is orig_filters False warnings.filters [] orig_filters [('ignore', None, <type 'exceptions.PendingDeprecationWarning'>, None, 0), ('ignore', None, <type 'exceptions.ImportWarning'>, None, 0), ('ignore', None, <type 'exceptions.BytesWarning'>, None, 0)]
I propose that we just revert to the test.test_support.catch_warnings implementation that was used in the beta releases, and leave the question of whether to expose this ability somewhere other than our own regression test support module for 2.7/3.1. That version worked, and the attempt to move it at the last minute has caused nothing but trouble.
So on trunk we would revert the following checkins: r66135 (relocate to warnings and change API) r66321 (change API again in attempt to fix bugs in r66135)
And on the py3k branch we would revert: r66139 (merge r66135) r66322* (merge r66322)
*This commit actually appears to have missed the changes to test.test_support that were in r66321 - test.support was not modified by the r66322 checkin (which strikes me as all the more reason to revert all of these changes and go back to the beta implementation)
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 13:33
I also have to comment on the "makes the API simpler to use" note in the checkin message. No, no it doesn't. See all those "warning[-1]" calls in the current unit tests? They're all unhelpful, because if a warning doesn't get raised, you're going to get an IndexError instead of an Assertion error (i.e. exactly the problem complained about in the original message in this thread).
Losing the attributes from the WarningRecorder means that you have to check if you got a warning first before you can check if you got the right warning. With the cached attributes, you can just check for the right warning, and only worry about the number of warnings in cases where that is likely to matter (usually because you expect multiple warnings from one operation).
These are all solvable problems, but I don't think right before a release candidate is the time to be fiddling with it - so let's revert back to providing this feature only through the regression test suite and deal with moving it into a more "official" part of the standard library in a later release after this version has had a chance to bake for a while (the twisted folks can always try to import it from our test suite, and copy our implementation as a fallback if the test suite isn't available for some reason).
Author: Jean-Paul Calderone (exarkun) *
Date: 2008-09-09 14:37
Exposing a list seems like a great low-level API to me. There are no [-1]s in the Twisted codebase as a result of using this API because we have a higher-level API built on top of it. Having this low-level API that doesn't try to make a specific application more convenient (at the cost of ambiguity) means anyone can write a better high-level API on top of it that makes a specific use-case convenient.
For Twisted, we actually would have very little difficulty coming up with our own version of catch_warnings without copying the implementation from the test_support module. What we are really interested in is a public API. Copying the implementation from test_support doesn't give us that.
I understand the concern with introducing changes like this into CPython
so close to a release. I just want it to be clear that without a public
API for this feature, the issue isn't resolved for Twisted. That may not
have been clear by just looking at this ticket, but see also
which I filed before filing this one which was also marked as a release
blocker and which was resolved only because of the existence of
warnings.catch_warnings (therefore removing the public API would mean
re-opening that ticket).
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 14:53
In working on the reversion patch, I just noticed that r66321 also incorrectly removed a whole pile of w.reset() calls.
When using a single catch_warning() context to test two or more operations which may raise the same warning, you must call w.reset() between each operation, or the later operations can fail to raise warnings, but the test will still pass because the most recent warning is still one which was correctly raised by an earlier operation.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 14:54
test.test_support is a public API (it's even documented).
Author: Jean-Paul Calderone (exarkun) *
Date: 2008-09-09 14:58
There was a discussion on python-dev about using things from the test
package from code outside the test package:
http://mail.python.org/pipermail/python-dev/2008-August/081860.html
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 15:02
I will put together two patches: one that reverts all the way back to what we had in the betas, and another which just reverts the Python test suite changes in the most recent checkin (instead modifying test_support.catch_warning to use the modified warnings.catch_warnings), then fixes the context manager in the warnings module (adding tests for both bugs)
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 15:29
Fully reverting the relocation also required reverting r66197 (a belated checkin of test_py3kwarn for the r66135 changes).
There was also a change to test_warnings that had to be reverted (it appeared to have been mistakenly checked in as part of the checkin that added the bsddb Py3k warnings).
Running tests now for the full reversion patch. The major objection to that approach (aside from the issue with external testing of warnings) is the problem that actually lead to catch_warnings() being relocated in the first place: suppressing spurious Py3k warnings in modules like cgi.py.
So as much as I was pushing that option earlier, it looks like it isn't going to be viable.
It's past 1 am here, so I'll be working on the other (cleaner) patch tomorrow evening.
The intended end result of that other patch:
A warnings.catch_warnings() implementation with the current interface (i.e. return a list on entry if record=True, None otherwise) that is intended either to suppress warnings, or to serve as a building block for better warning testing tools. The patch will also fix the re-entrancy problem by adding explicit self._entered state guards.
A test_support.catch_warning() implementation which is tailored towards the needs of the Python regression test suite (probably the list-with-attributes interface provided by the previous incarnation of warnings.catch_warning, but with getattr adjusted to return None when there is no warning caught).
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 15:40
Reversion patch attached - it does indeed recreate some nasty dependencies from the main areas of the standard library on to the regression test suite. That's enough to kill the idea of a complete reversion as far as I'm concerned - I'll get the proper fix done this evening.
(That's 18-20 hours from the time of this post, for anyone trying to figure out timezones)
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-09 19:27
I can understand the amount of time it might take to revert this; merging was horrendous and stuff was partially combined into single patches as to get a review done for all related changes instead of doing more atomic commits if a review was not required.
And it took me days to make the transition and related changes, so undoing is obviously not easy. Sorry about that.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-09 22:13
No worries - the only reason I suggested full reversion at all is because I had temporarily forgotten why the relocation had become so necessary (i.e. we needed the feature ourselves in the main part of the standard library to avoid emitting spurious warnings).
J.P.'s suggestion (basic functionality in warnings, with each group of users providing their own convenience API tailored to their own use cases) makes an awful lot of sense, which is why it is the model I am going to adopt.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-10 02:35
On Tue, Sep 9, 2008 at 3:13 PM, Nick Coghlan <report@bugs.python.org> wrote:
Nick Coghlan <ncoghlan@gmail.com> added the comment:
No worries - the only reason I suggested full reversion at all is because I had temporarily forgotten why the relocation had become so necessary (i.e. we needed the feature ourselves in the main part of the standard library to avoid emitting spurious warnings).
J.P.'s suggestion (basic functionality in warnings, with each group of users providing their own convenience API tailored to their own use cases) makes an awful lot of sense, which is why it is the model I am going to adopt.
Adopt how? As in just provide a context manager that copies and restores the filter? That might be enough and then let test.support have a subclass or another context manager that plugs in the whole showwarning() implementation. Then everyone should be happy.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-10 10:06
Not quite that basic for the warnings.catch_warnings() part. I plan to leave the current warnings.catch_warnings() alone (aside from adding some re-entrancy checks), and add back a test.test_support.check_warnings() that uses a WarningsRecorder object to simplify the specific use cases in the Python regression test suite (i.e. at least adding back the easy attribute access, and possibly other things if there are other common features of the way we use it in the tests).
The patch will make it clearer (working on that now).
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-10 15:19
Cleanup patch now attached. Details of changes:
warnings.catch_warnings() now has reentry guards (and associated tests) to prevent silent errors when misused
added back a test_support.check_warnings() convenience wrapper (deliberately changing the name to be different from the context manager in the warnings module). This wrapper is no longer configurable - it is now used solely for tests that want to record normal warnings and check the results.
restored the w.reset() calls that went away in the previous checkin
unit tests that want to test a different module, or don't want warnings recorded now consistently use warnings.catch_warnings() directly
cleanups up to the respective documentation
cleanups to test_py3kwarn so it is better behaved when other tests are run before it (the lack of reinitialisation of extension modules still causes problems though)
tested with "./python -3 -m test.regrtest -uall -x test_ossaudiodev" (exclusion is needed because test_ossaudiodev hasn't worked properly on my machine in a very long time - the audio file playback runs overtime and I've never found the time to figure out why)
Just needs a review and then should be good to go.
Author: Brett Cannon (brett.cannon) *
Date: 2008-09-10 22:48
Code looks good.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-11 12:27
Applied to trunk for 2.6 in r66386.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-11 14:02
Blocked merge in the py3k branch since it requires some fiddling to handle the change from test.test_support to test.support. I'll post a new patch here for the py3k forward port when I can (I may not make it before 3.0b4 though, so unassigning for the moment).
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-09-11 21:02
On Thu, Sep 11, 2008 at 9:03 AM, Nick Coghlan <report@bugs.python.org> wrote:
Nick Coghlan <ncoghlan@gmail.com> added the comment:
Blocked merge in the py3k branch since it requires some fiddling to handle the change from test.test_support to test.support. I'll post a new patch here for the py3k forward port when I can (I may not make it before 3.0b4 though, so unassigning for the moment).
The best way to do that is:
(trunk) $ svn diff -c mergerevision Lib/test/test_support.py > diff.patch (py3k) $ patch Lib/test/support.py < diff.patch
assignee: ncoghlan -> versions: -Python 2.6
Python tracker <report@bugs.python.org> <http://bugs.python.org/issue3781>
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-14 14:05
3.0 version of the patch attached (it turned that it wasn't so much test_support itself that caused the forward port problems, as the fact that most of the tests that use this feature in 2.x are testing specifically for Py3k warnings, or for other deprecated features that aren't part of 3.0, so many of the changes either weren't needed, or their contexts had changed completely).
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-24 20:52
The 3.0 forward port of r66386 still needs a once over before being committed (there were enough differences that I don't think the review of the 2.6 version is enough to cover the 3.0 version as well).
Once that is done, then this issue can be closed.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-10-16 23:24
Applied in r66945.
History
Date
User
Action
Args
2022-04-11 14:56:38
admin
set
github: 48031
2010-11-13 00:15:08
ned.deily
set
messages: -
2010-11-13 00:14:57
ned.deily
set
messages: -
2010-11-12 23:41:04
lkcl
set
messages: +
2010-11-12 23:39:20
lkcl
set
nosy: + lkcl
messages: +
2008-10-16 23:24:58
benjamin.peterson
set
status: open -> closed
resolution: fixed
messages: +
2008-10-02 12:54:58
barry
set
priority: deferred blocker -> release blocker
2008-09-26 22:20:23
barry
set
priority: release blocker -> deferred blocker
2008-09-24 20:52:28
ncoghlan
set
messages: +
2008-09-18 05:43:45
barry
set
priority: deferred blocker -> release blocker
2008-09-14 14:05:52
ncoghlan
set
keywords: + needs review
2008-09-14 14:05:35
ncoghlan
set
files: + issue3781_catch_warnings_cleanup_py3k.diff
messages: +
2008-09-11 21:02:27
benjamin.peterson
set
messages: +
2008-09-11 14:02:55
ncoghlan
set
assignee: ncoghlan ->
messages: +
versions: - Python 2.6
2008-09-11 12:27:23
ncoghlan
set
priority: release blocker -> deferred blocker
messages: +
2008-09-11 06:59:53
brett.cannon
set
keywords: + patch, - needs review
2008-09-10 22:48:33
brett.cannon
set
messages: +
2008-09-10 15:19:45
ncoghlan
set
keywords: + needs review, - patch
files: + issue3781_catch_warnings_cleanup.diff
messages: +
2008-09-10 10:06:10
ncoghlan
set
messages: +
2008-09-10 02:35:49
brett.cannon
set
messages: +
2008-09-09 22:13:38
ncoghlan
set
messages: +
2008-09-09 19:27:51
brett.cannon
set
messages: +
2008-09-09 15:40:50
ncoghlan
set
files: + issue3781_revert_to_beta_implementation_26.diff
messages: +
2008-09-09 15:29:39
ncoghlan
set
messages: +
2008-09-09 15:02:12
ncoghlan
set
messages: +
2008-09-09 14:58:46
exarkun
set
messages: +
2008-09-09 14:54:36
ncoghlan
set
messages: +
2008-09-09 14:53:20
ncoghlan
set
messages: +
2008-09-09 14:37:56
exarkun
set
messages: +
2008-09-09 14:21:09
ncoghlan
set
assignee: ncoghlan
2008-09-09 13:33:03
ncoghlan
set
messages: +
2008-09-09 13:11:40
ncoghlan
set
assignee: brett.cannon -> (no value)
messages: +
2008-09-09 12:20:44
barry
set
messages: +
2008-09-09 10:40:46
ncoghlan
set
messages: +
2008-09-09 10:38:04
ncoghlan
set
status: closed -> open
nosy: + ncoghlan
resolution: accepted -> (no value)
messages: +
2008-09-09 01:52:49
brett.cannon
set
status: open -> closed
resolution: accepted
messages: +
2008-09-09 00:50:00
brett.cannon
set
messages: +
2008-09-08 23:17:33
brett.cannon
set
messages: +
2008-09-08 22:12:41
benjamin.peterson
set
keywords: - needs review
nosy: + benjamin.peterson
messages: +
2008-09-07 21:29:26
brett.cannon
set
files: - catch_warnings_atts.diff
2008-09-07 21:29:16
brett.cannon
set
files: + clean_up_catch_warnings.diff
messages: +
2008-09-07 20:16:41
brett.cannon
set
assignee: brett.cannon
2008-09-06 22:20:58
brett.cannon
set
messages: +
2008-09-06 18:31:37
barry
set
messages: +
versions: + Python 3.0
2008-09-06 18:26:19
brett.cannon
set
messages: +
2008-09-06 18:24:21
brett.cannon
set
messages: +
2008-09-06 17:03:17
barry
set
nosy: + barry
messages: +
2008-09-05 04:11:59
brett.cannon
set
keywords: + patch, needs review
assignee: brett.cannon -> (no value)
messages: +
files: + catch_warnings_atts.diff
2008-09-05 03:59:14
brett.cannon
set
messages: +
2008-09-04 23:25:38
pitrou
set
nosy: + pitrou
messages: +
2008-09-04 22:56:49
brett.cannon
set
priority: release blocker
messages: +
2008-09-04 22:52:28
exarkun
set
messages: +
2008-09-04 22:22:32
brett.cannon
set
messages: +
2008-09-04 22:11:23
benjamin.peterson
set
assignee: brett.cannon
nosy: + brett.cannon
2008-09-04 22:10:08
exarkun
create