Opened 6 years ago

Closed 6 years ago

#3487 enhancement closed fixed (fixed)

Add TestCase.flushWarnings

Reported by: exarkun Owned by:
Priority: high Milestone: Python-2.6
Component: trial Keywords:
Cc: z3p Branch: branches/flushwarnings-3487
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

TestCase.assertWarns is limited in that applications can only express a single warning using it. It is also a somewhat inconvenient API, even for the simplest warning tests, in that it requires filename and line number information which is somewhat confusing to developers.

TestCase.callDeprecated is similarly limited. Additionally, in trying to avoid the confusing API for assertWarns, it presents an interface which is very narrow. Instead of a warning type and message, it takes only a version number. Additionally, it only works with the deprecated decorator, not warnings in general.

This leaves trial without a general tool for making assertions about warnings. I suggest adding flushWarnings. Trial will collect warnings emitted during the execution of a test. flushWarnings will allow any number of warnings to be cleared from this collection mechanism and returned to the caller. Any unflushed warnings at the end of a test will be re-emitted so as to be visible to whatever is running trial. flushWarnings will support rudamentary filtering, similar to that offered by flushLoggedErrors.

This will work for the use case of multiple warnings. It removes the possibility for confusing intermediate stack levels to confuse the warning system or the developer. It gives all available information to the developer so that any assertion may be made. It preserves reporting of all unexpected or unhandled warnings. If the filtering supported is based on function objects, it removes the confusing filename/linenumber aspects present in the assertWarns API. Both assertWarns and callDeprecated can be implemented in terms of this mechanism, as can other higher-level test helpers.

See discussion on #3266 for more details (mostly historic).

Change History (25)

comment:1 Changed 6 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/flushwarnings-3487

(In [25012]) Branching to 'flushwarnings-3487'

comment:2 Changed 6 years ago by exarkun

(In [25013]) merge flushWarnings changes from #3266 branch

refs #3487
refs #3266

comment:3 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Ready for review. The branch should also resolve #3433 and #3427.

comment:4 Changed 6 years ago by z3p

And go towards fixing #2710.

comment:5 Changed 6 years ago by z3p

  • Cc z3p added

comment:6 Changed 6 years ago by exarkun

  • Milestone set to Python-2.6

comment:7 Changed 6 years ago by glyph

  • Keywords review removed
  • Owner set to glyph
  • Status changed from new to assigned

Reviewing.

comment:8 Changed 6 years ago by glyph

  • Keywords review added

Uh, put the review keyword back.

comment:9 Changed 6 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to exarkun
  • Status changed from assigned to new
  • warnings no longer show up in the test output. For example:
    #test_warnings.py
    from twisted.trial.unittest import TestCase
    import warnings
    class WarningWarner(TestCase):
        def test_warning(self):
            warnings.warn("warning!", category=UserWarning)
    
    results in:
    $ trial test_warnings.py
    test_warnings
      WarningWarner
        test_warning ...                                                       [OK]
    
    -------------------------------------------------------------------------------
    Ran 1 tests in 0.002s
    
    PASSED (successes=1)
    
  • With your branch merged:
    glyph@nhuvasarim:~$ python -Werror /home/glyph/Projects/Twisted/trunk/bin/trial test_warnings.py 
    test_warnings
      WarningWarner
        test_warning ...                                                       [OK]
    Traceback (most recent call last):
      File "/home/glyph/Projects/Twisted/trunk/bin/trial", line 24, in <module>
        run()
      File "/home/glyph/Projects/Twisted/trunk/twisted/scripts/trial.py", line 361, in run
        test_result = trialRunner.run(suite)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 816, in run
        return self._runWithoutDecoration(test)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 842, in _runWithoutDecoration
        run()
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 837, in <lambda>
        run = lambda: suite.run(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 291, in run
        TestSuite.run(self, result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1443, in run
        test(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1431, in __call__
        return self.run(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 164, in run
        super(LoggedSuite, self).run(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1443, in run
        test(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1431, in __call__
        return self.run(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 137, in run
        test(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1431, in __call__
        return self.run(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 137, in run
        test(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1431, in __call__
        return self.run(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/runner.py", line 137, in run
        test(result)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 740, in __call__
        return self.run(*args, **kwargs)
      File "/home/glyph/Projects/Twisted/trunk/twisted/trial/unittest.py", line 1164, in run
        warnings.warn_explicit(**w)
      File "/usr/lib/python2.5/warnings.py", line 102, in warn_explicit
        raise message
    UserWarning: warning!
    
  • On the less serious side, think _Warning could use some documentation explaining what it is, not just what its attributes are.
  • Sadly, the branch otherwise looks great. After staring at it for a while I'm not really sure what the problems are, so I'm going to give it back to you for fixing rather than continue trying to provide more useful feedback.

comment:10 Changed 6 years ago by exarkun

(In [25135]) a bit more docs for _Warning

refs #3487

comment:11 Changed 6 years ago by exarkun

(In [25136]) better docs for _Warning

refs #3487

comment:12 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

warnings no longer show up in the test output. For example:

Warnings show up now. They're not printed to stdout, as they used to be, but written to the reporter's output stream by the reporter which also implements a simple form of duplicate removal.

With your branch merged:

If configuration dictates that a warning is actually an exception, an error is now added to the result for the test which didn't flush that warning. ie, Trial doesn't crash if Python is invoked with -Werror anymore.

On the less serious side, think _Warning could use some documentation explaining what it is, not just what its attributes are.

I improved the docs a bit.

Sadly, the branch otherwise looks great. After staring at it for a while I'm not really sure what the problems are, so I'm going to give it back to you for fixing rather than continue trying to provide more useful feedback.

I added 3 hidden bugs for the next reviewer, to keep it interesting. ;)

comment:13 Changed 6 years ago by exarkun

#3433 was a duplicate of #2820, by the way. I think the branch will also resolve #3506.

comment:14 Changed 6 years ago by glyph

  • Owner set to glyph
  • Status changed from new to assigned

Reviewing.

comment:15 Changed 6 years ago by glyph

  • Keywords review removed
  • Owner changed from glyph to exarkun
  • Status changed from assigned to new
  1. It's a bit weird that when warnings are configured as errors, a warning emitted at module scope will raise an immediate exception, but a warning emitted in the course of a test will report a test failure but not actually raise an exception. This would seem to strongly discourage the emission of warnings at module scope. Maybe that's a good thing. I don't have any specific feedback to this point, and I think the current strategy is acceptable. However, it should probably be accepted.
  2. Reporter._publisher is undocumented.
  3. The declaration of Reporter.__init__ is greater than 80 characters long.
  4. Reporter._observeWarnings should have a @param declaration describing event. It might be helpful to also describe it specifically as an argument to log.addObserver.
  5. Some unused imports:
    twisted/test/test_log.py:11: 'util' imported but unused
    twisted/test/test_log.py:13: 'reflect' imported but unused
    twisted/trial/test/test_log.py:10: 'defer' imported but unused
    twisted/trial/test/test_log.py:11: 'reflect' imported but unused
    twisted/trial/test/test_log.py:12: 'runner' imported but unused
    

Otherwise this is looking good; no functional issues that I can see, and I did a bunch of interactive testing this time. When you've cleaned up these minor issues, you can go ahead and merge.

I think you should probably file a ticket for documenting the behavior of warnings-as-errors though; although this is probably part of a larger task of documenting trial in general, since its documentation is pretty pathetic.

comment:16 Changed 6 years ago by exarkun

(In [25249]) Address most review feedback

refs #3487

comment:17 follow-up: Changed 6 years ago by exarkun

  1. ... However, it should probably be accepted.

I'm not sure what this means.

I think you should probably file a ticket for documenting the behavior of warnings-as-errors though;

Filed #3520.

comment:18 Changed 6 years ago by exarkun

(In [25250]) fix epytext markup

refs #3487

comment:19 Changed 6 years ago by exarkun

(In [25251]) Fix test_missingSource on Python 2.4 when PIL is installed

Due to bugs in linecache, having the missing source file be named __init__.py is
incredibly unlikely to work. Hack around this in the test by not testing that case.
If inspect.getsource actually works right, this functionality will work right. If
it doesn't, then it won't.

refs #3487

comment:20 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

In addition to the above fixes, I also needed to add path case normalization so that filtering worked properly on Windows.

Here's the full diff since the last review:
http://twistedmatrix.com/trac/changeset?new=branches%2Fflushwarnings-3487%4025262&old=branches%2Fflushwarnings-3487%4025157

comment:22 in reply to: ↑ 17 Changed 6 years ago by glyph

Replying to exarkun:

  1. ... However, it should probably be accepted.

I'm not sure what this means.

Sorry, that was a left-over sentence from a different edit of phrasing the other stuff. The point was, the code is OK as it is, but it should be documented.

comment:23 Changed 6 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

Thanks very much for the build results and inter-review patch.

No further problems I can see, although I wish I were more confident in my assessment. Merge away, and hopefully I won't look stupid as a result :).

comment:24 Changed 6 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [25279]) Merge flushwarnings-3487

Author: radix, itamar, exarkun
Reviewer: glyph
Fixes: #3487
Fixes: #3427
Fixes: #2820
Fixes: #3506

Introduce TestCase.flushWarnings, an API for interacting with the Python warnings
module in unit tests. By default, the default trial reporter will write warnings
emitted by tests to its output stream (no longer does the runner print them to
stdout). flushWarnings may be used to prevent this from happening in particular
tests, and its return value may be used to make assertions about what warnings have
been emitted. TestCase.assertWarns and TestCase.callDeprecated are both now
implemented in terms of this more flexible API.

The default reporter includes rudamentary support for duplicate suppression: each
warning will be reported only once per test, but if a warning is emitted by two or
more tests, it will be reported for each test (isolating tests from each other
further by making warning behavior the same regardless of what other tests have run).

There is also some support for failing tests based on warnings emitted. If the Python
warnings filters system is used to turn a warning into an exception, any test which
causes such a warning to be emitted will have an error attributed to it.

comment:25 Changed 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.