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 bitsurprising, and since this is mostly an API useful for testing, it'd bemuch better if it had a well-defined, documented (ie, stable and likelyto continue working in the next release of Python) error mode.

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 beenrecorded. AttributeError goes with the idea that the attributes arejust not set since no warnings are there to set the attributes.LookupError doesn't seem quite right. TypeError is definitely wrongsince 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 IndexErroris raised because no warning is currently recorded.

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

I won't be able to get to this until tonight, but assuming no oneobjects, I will make it be an AttributeError and a release blocker sothat the API can be considered stable in rc1.

Why wouldn't w.message simply be None?

There is no specific reason why it would be, although that is an optionas well. Part of the problem with None is that it is a legitimatedefault value for some arguments to showwarning() so it doesn'tnecessarily reflect that no exception was raised if you don't look atkey attributes.

The attached patch has WarningsRecorder raise AttributeError when thereis no recorded attribute and yet one tries to access warnings attributes.And just so you know, JP, make sure to use keyword arguments whencalling catch_warnings() (in case you didn't notice the note in thedocs). In Py3K they are keyword-only.

I hate to make API suggestions this late in the process, but this is thefirsttime I've looked at this.  I think the basic problem is that the contextmanager 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 historyhere,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'

The use of __getattr__ is a historical artifact. Originally there was noway to record multiple warnings as the object returned bycatch_warnings() represented only a single instance of a warning. Butthen the ability to record multiple warnings was added. To not breakexisting code (I am guessing), the setting of attributes on theWarningsRecorder was added. To tawdry life of catch_warnings(). =)While having the attributes of the last warning directly accessible ishandy, I am in no way married to the idea. Actually, if we run with thelist analogy we can just ditch WarningsRecorder and return a list itselfthat is directly appended to through a version of showwarning() that isa closure instead. That cuts out code and everyone knows how to workwith lists as sequential recording of events.OK, I'm sold. =) I will work on this today or tomorrow and get the patchdone and ready to go no later than Sunday evening for review.

And I forgot to mention that subclassing list is a new thing I tossed inwhen I moved the code and tweaked the API so that's another reason thatusing list's abilities was not pervasive.

Sounds good.  This needs to go into 2.6 and 3.0.  And while you're addit, can you add catch_warnings to the __all__?

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 forceskeyword-only arguments in 2.6 like 3.0.

The new patch ditches the WarningsRecorder class and just returns a listthat is directly mutated. I also removed all uses oftest.test_support.catch_warning() and moved them over. Docs have beenthoroughly updated to give example usage. And lastly, I fixed bsddb touse 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 tohave to search for all potential places where catch_warnings() wouldhave been needed.

The patch mostly looks good. However, all the w[-1] logic looks ratherverbose to me since its main use case in testing will be making sure*one* warning happened. Returning a list adds the extra step of checkingthe length and then indexing it for the warning validation. I'm notcompletely suggesting that you bring back the smart list, but maybe anoption on catch_warning to just yield the WarningMessage on __enter__.

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 fortesting, or for temporarily suppressing warnings. The stdlib is not anormal use-case in this regard since we have to be so careful withgiving deprecations.I honest don't fine the [-1] indexing that bad and I had to add all ofthem. =) Makes it explicit you are assuming there is at least onewarnings (and probably only one) and you should check that there wasnot an extra one.I will wait to see if Barry has anything to say on the matter since hepushed for the change.

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

r66322 has the fix in 3.0.

Reopening this because I disagree with the fix - I would prefer to seecatch_warnings() reverted back to the API and implementation* it usedprior 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 theimplementation of WarningsRecorder hadn't been changed then this bugwould 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 warningsrecorder is to test operations that are expected to raise a singlewarning. The list of warnings is retained solely for special cases whereone operation raises multiple warnings (e.g. see the py3k warnings testsfor __hash__ inheritance).*aside from the use of @contextmanager, obviously

As far as the use cases go, it was moved out of test.test_support towarnings SOLELY due to the requests from the twisted folks forassistance in testing their generation of warnings. The fact that it isalso useful for suppressing warnings in general without affecting theglobal state of the warnings module (aside from thread safety issues) isjust a bonus.

With a name like catch_warnings() in the warnings module, it willdefinitely get used in production code to suppress warnings.  If itsintended to be used only by tests, then it belongs somewhere else.  Ifnot test_support then maybe unittest.  If it were moved then I wouldn'tcare about the bug that all other warnings caught are inaccessible.You'd still have to fix the w.messages attribute to be None if therewere no warnings raised.

It turns out the warnings.catch_warnings version has re-entrancy issuesdue 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 linux2Type "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_filtersFalse>>> 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_warningsimplementation that was used in the beta releases, and leave thequestion of whether to expose this ability somewhere other than our ownregression test support module for 2.7/3.1. That version worked, and theattempt 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 inr66135)And on the py3k branch we would revert:r66139 (merger66135)r66322* (merger66322)*This commit actually appears to have missed the changes totest.test_support that were inr66321 - test.support was not modified byther66322 checkin (which strikes me as all the more reason to revertall of these changes and go back to the beta implementation)

I also have to comment on the "makes the API simpler to use" note in thecheckin message. No, no it doesn't. See all those "warning[-1]" calls inthe current unit tests? They're all unhelpful, because if a warningdoesn't get raised, you're going to get an IndexError instead of anAssertion error (i.e. exactly the problem complained about in theoriginal message in this thread).Losing the attributes from the WarningRecorder means that you have tocheck 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 theright warning, and only worry about the *number* of warnings in caseswhere that is likely to matter (usually because you expect multiplewarnings from one operation).These are all *solvable* problems, but I don't think right before arelease candidate is the time to be fiddling with it - so let's revertback to providing this feature only through the regression test suiteand deal with moving it into a more "official" part of the standardlibrary in a later release after this version has had a chance to bakefor a while (the twisted folks can always try to import it from our testsuite, and copy our implementation as a fallback if the test suite isn'tavailable for some reason).

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 wehave a higher-level API built on top of it.  Having this low-level APIthat doesn't try to make a specific application more convenient (at thecost of ambiguity) means anyone can write a better high-level API on topof it that makes a specific use-case convenient.For Twisted, we actually would have very little difficulty coming upwith our own version of catch_warnings without copying theimplementation from the test_support module.  What we are *really*interested in is a public API.  Copying the implementation fromtest_support doesn't give us that.I understand the concern with introducing changes like this into CPythonso close to a release.  I just want it to be clear that without a publicAPI for this feature, the issue isn't resolved for Twisted. That may nothave been clear by just looking at this ticket, but see alsoissue3780which I filed before filing this one which was also marked as a releaseblocker and which was resolved only because of the existence of`warnings.catch_warnings` (therefore removing the public API would meanre-opening that ticket).

In working on the reversion patch, I just noticed thatr66321 alsoincorrectly removed a whole pile of w.reset() calls.When using a single catch_warning() context to test two or moreoperations which may raise the same warning, you *must* call w.reset()between each operation, or the later operations can fail to raisewarnings, but the test will still pass because the most recent warningis still one which was correctly raised by an earlier operation.

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

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

I will put together two patches: one that reverts all the way back towhat we had in the betas, and another which just reverts the Python testsuite changes in the most recent checkin (instead modifyingtest_support.catch_warning to use the modified warnings.catch_warnings),then fixes the context manager in the warnings module (adding tests forboth bugs)

Fully reverting the relocation also required revertingr66197 (a belatedcheckin of test_py3kwarn for ther66135 changes).There was also a change to test_warnings that had to be reverted (itappeared to have been mistakenly checked in as part of the checkin thatadded the bsddb Py3k warnings).Running tests now for the full reversion patch. The major objection tothat approach (aside from the issue with external testing of warnings)is the problem that actually lead to catch_warnings() being relocated inthe 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'tgoing to be viable.It's past 1 am here, so I'll be working on the other (cleaner) patchtomorrow 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 isintended either to suppress warnings, or to serve as a building blockfor better warning testing tools. The patch will also fix there-entrancy problem by adding explicit self._entered state guards.A test_support.catch_warning() implementation which is tailored towardsthe needs of the Python regression test suite (probably thelist-with-attributes interface provided by the previous incarnation ofwarnings.catch_warning, but with __getattr__ adjusted to return Nonewhen there is no warning caught).

Reversion patch attached - it does indeed recreate some nastydependencies from the main areas of the standard library on to theregression test suite. That's enough to kill the idea of a completereversion as far as I'm concerned - I'll get the proper fix done thisevening.(That's 18-20 hours from the time of this post, for anyone trying tofigure out timezones)

I can understand the amount of time it might take to revert this;merging was horrendous and stuff was partially combined into singlepatches as to get a review done for all related changes instead of doingmore atomic commits if a review was not required.And it took me days to make the transition and related changes, soundoing is obviously not easy. Sorry about that.

No worries - the only reason I suggested full reversion at all isbecause I had temporarily forgotten why the relocation had become sonecessary (i.e. we needed the feature ourselves in the main part of thestandard library to avoid emitting spurious warnings).J.P.'s suggestion (basic functionality in warnings, with each group ofusers providing their own convenience API tailored to their own usecases) makes an awful lot of sense, which is why it is the model I amgoing to adopt.

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 andrestores the filter? That might be enough and then let test.supporthave a subclass or another context manager that plugs in the wholeshowwarning() implementation. Then everyone should be happy.

Not quite that basic for the warnings.catch_warnings() part. I plan toleave the current warnings.catch_warnings() alone (aside from addingsome re-entrancy checks), and add back atest.test_support.check_warnings() that uses a WarningsRecorder objectto simplify the specific use cases in the Python regression test suite(i.e. at least adding back the easy attribute access, and possibly otherthings if there are other common features of the way we use it in thetests).The patch will make it clearer (working on that now).

Cleanup patch now attached. Details of changes:- warnings.catch_warnings() now has reentry guards (and associatedtests) 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 managerin the warnings module). This wrapper is no longer configurable - it isnow used solely for tests that want to record normal warnings and checkthe 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 wantwarnings 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 arerun before it (the lack of reinitialisation of extension modules stillcauses problems though)- tested with "./python -3 -m test.regrtest -uall -x test_ossaudiodev"(exclusion is needed because test_ossaudiodev hasn't worked properly onmy machine in a very long time - the audio file playback runs overtimeand I've never found the time to figure out why)Just needs a review and then should be good to go.

Code looks good.

Applied to trunk for 2.6 inr66386.

Blocked merge in the py3k branch since it requires some fiddling tohandle the change from test.test_support to test.support. I'll post anew patch here for the py3k forward port when I can (I may not make itbefore 3.0b4 though, so unassigning for the moment).

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 mergerevisionLib/test/test_support.py > diff.patch(py3k) $ patchLib/test/support.py < diff.patch>> ----------> assignee: ncoghlan ->> versions:  -Python 2.6>> _______________________________________> Python tracker <report@bugs.python.org>> <http://bugs.python.org/issue3781>> _______________________________________>

3.0 version of the patch attached (it turned that it wasn't so muchtest_support itself that caused the forward port problems, as the factthat most of the tests that use this feature in 2.x are testingspecifically for Py3k warnings, or for other deprecated features thataren't part of 3.0, so many of the changes either weren't needed, ortheir contexts had changed completely).

The 3.0 forward port ofr66386 still needs a once over before beingcommitted (there were enough differences that I don't think the reviewof the 2.6 version is enough to cover the 3.0 version as well).Once that is done, then this issue can be closed.

Applied inr66945.