warnings.catch_warnings fails gracelessly when recording warnings but no warnings are emitted

[exarkun]

BPO	3781
Nosy	@warsaw, @brettcannon, @ncoghlan, @pitrou, @benjaminp
Files	clean_up_catch_warnings.diff issue3781_revert_to_beta_implementation_26.diff: Revert catch_warnings to the beta implementation issue3781_catch_warnings_cleanup.diff: Cleanup catch_warnings and test suite usage for rc1 issue3781_catch_warnings_cleanup_py3k.diff: Forward port of r66386 to py3k

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2008-10-16.23:24:58.875>
created_at = <Date 2008-09-04.22:10:08.782>
labels = ['type-bug', 'library', 'release-blocker']
title = 'warnings.catch_warnings fails gracelessly when recording warnings but no warnings are emitted'
updated_at = <Date 2010-11-13.00:15:08.527>
user = 'https://bugs.python.org/exarkun'

bugs.python.org fields:

activity = <Date 2010-11-13.00:15:08.527>
actor = 'ned.deily'
assignee = 'none'
closed = True
closed_date = <Date 2008-10-16.23:24:58.875>
closer = 'benjamin.peterson'
components = ['Library (Lib)']
creation = <Date 2008-09-04.22:10:08.782>
creator = 'exarkun'
dependencies = []
files = ['11416', '11444', '11456', '11489']
hgrepos = []
issue_num = 3781
keywords = ['patch', 'needs review']
message_count = 41.0
messages = ['72533', '72534', '72544', '72547', '72555', '72563', '72564', '72672', '72678', '72679', '72680', '72715', '72754', '72795', '72805', '72813', '72817', '72838', '72839', '72851', '72853', '72855', '72864', '72865', '72866', '72867', '72868', '72872', '72873', '72898', '72926', '72937', '72952', '72967', '72995', '73017', '73029', '73057', '73223', '73753', '74880']
nosy_count = 7.0
nosy_names = ['barry', 'brett.cannon', 'lkcl', 'exarkun', 'ncoghlan', 'pitrou', 'benjamin.peterson']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue3781'
versions = ['Python 3.0']

[exarkun]

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.

[brettcannon]

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.

[exarkun]

The specific exception type isn't that important to me, as long as I can
rely on it being something in particular.

[brettcannon]

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.

[pitrou]

Why wouldn't w.message simply be None?

[brettcannon]

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.

[brettcannon]

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.

[warsaw]

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'

[brettcannon]

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.

[brettcannon]

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.

[warsaw]

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__?

[brettcannon]

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.

[brettcannon]

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.

[benjaminp]

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__.

[brettcannon]

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.

[brettcannon]

Covered by r66321 in the trunk. Now I just need to merge into 3.0.

[brettcannon]

r66322 has the fix in 3.0.

[ncoghlan]

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

[ncoghlan]

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.

[warsaw]

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.

[ncoghlan]

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)

[exarkun]

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 bpo-3780
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).

[ncoghlan]

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.

[ncoghlan]

test.test_support *is* a public API (it's even documented).

[exarkun]

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

[ncoghlan]

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)

[ncoghlan]

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).

[ncoghlan]

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)

[brettcannon]

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.

[ncoghlan]

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.

[brettcannon]

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.

[ncoghlan]

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).

[ncoghlan]

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.

[brettcannon]

Code looks good.

[ncoghlan]

Applied to trunk for 2.6 in r66386.

[ncoghlan]

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).

[benjaminp]

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\>

---

[ncoghlan]

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).

[ncoghlan]

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.

[benjaminp]

Applied in r66945.