classification
Title: test_logging failure in WarningsTest on buildbots
Type: behavior Stage: commit review
Components: Tests Versions: Python 3.3, Python 3.4, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: 18242 Superseder:
Assigned To: terry.reedy Nosy List: Arfrever, brett.cannon, kbk, python-dev, r.david.murray, roger.serwy, terry.reedy, vinay.sajip, vstinner
Priority: normal Keywords: buildbot, patch

Created on 2013-05-28 15:25 by r.david.murray, last changed 2013-06-29 04:05 by terry.reedy. This issue is now closed.

Files
File name Uploaded Description Edit
zdummy.diff terry.reedy, 2013-06-16 23:09 review
changes.diff vinay.sajip, 2013-06-17 13:01 Changes resulting in passing tests review
issue18081-warn.diff terry.reedy, 2013-06-27 01:15 review
issue18081-warn3.diff terry.reedy, 2013-06-28 04:31 review
Messages (25)
msg190220 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-05-28 15:25
The following failure is showing up frequently in the buildbots.  I haven't checked closely enough to be sure if it is a heisenberg, but I suspect it is, since it doesn't seem to be related to any of the recent changesets that would have been included in the builds that just failed and that show the failure:

======================================================================
FAIL: test_warnings (test.test_logging.WarningsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/home/db3l/buildarea/3.3.bolen-freebsd/build/Lib/test/test_logging.py", line 1796, in test_warnings
    "dummy.py:42: UserWarning: Explicit\n  Dummy line\n")
AssertionError: '\nWarning (from warnings module):\n  File "dummy.py", line 42\n    Dummy line\n [truncated]... != 'dummy.py:42: UserWarning: Explicit\n  Dummy line\n'
+ dummy.py:42: UserWarning: Explicit
- 
- Warning (from warnings module):
-   File "dummy.py", line 42
-     Dummy line
? --
+   Dummy line
- UserWarning: Explicit
- >>>
msg191266 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-06-16 13:48
Seems to be fairly consistent on Windows but more random on Linux. I have also triggered it on my OS X machine randomly.

Can't tell if it's a timing issue or some other test doing something bad. I'm worried solving it is going to require taking one of the failing instance's list of tests and then slowly pruning it down to find the trigger (if it's not a timing thing).
msg191267 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-06-16 14:31
Found the test cases to cause this::

 ./python.exe -m test test_idle test_logging

Adding Roger and Terry to see if they happen to remember any of the IDLE tests using logging or warnings in a way that could cause this.

P.S.: I thought we had a script somewhere which helped do a binary search for what tests cause a failure but I can't find it. Am I imagining things?
msg191277 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-16 18:06
test_idle, which runs the tests in idlelib/idle_test, currently comprises 9 test methods and perhaps 100 lines that do not, as far as I know, use warnings or logging. However, perhaps this has an effect:

F:\Python\dev\cpython\PCbuild>python_d -m test test_idle
[1/1] test_idle
Warning -- os.environ was modified by test_idle
Warning -- locale was modified by test_idle
1 test altered the execution environment:
    test_idle

Of course, the idle test code itself does no such modification. So I have no idea what the warning is about or how to stop it.

F:\Python\dev\cpython\PCbuild>python_d -m test.test_idle
which runs under unittest, does not produce the warning.

The other test files that use tcl/tk/tkinter, test_tcl, test_tk, test_ttk_guionly, and test_ttk_textonly, produce the os.environ warning but not the locale warning.

I duplicated the failure with the command you gave. Substituting _tcl or _tk for _idle removes the error. So perhaps the locale alteration, however it happens, is the problem.
msg191283 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-06-16 19:36
I actually don't get the warnings when I run test_idle under regrtest. But what I did was followed a hunch and added a check for warnings.showwarning() and sure enough, it's been left modified by IDLE: PyShell replaces the function as a side-effect of import. Not sure how you want to fix it. Using warnings.catch_warnings() during import would solve it, but I don't know if that's how you want to go in case that showwarnings replacement is needed.
msg191286 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-16 20:13
I discovered the same thing a different way. Grepping Lib/*.py for
'Warning (from warnings module)' hits
F:\Python\dev\cpython\Lib\idlelib\PyShell.py: 67:
F:\Python\dev\cpython\Lib\idlelib\run.py: 34:
Both monkey-patch warnings, when imported, to show warnings 'the Idle way' ;-). 

I am not sure which test file indirectly imports PyShell, but one must. Idle was written to be run from the command line. It was not written to be imported but not run, as in a test environment, or to be run as part of another script.

Roger, can the monkey patching be put in a function that is only called when Idle runs? Perhaps in PyShell.main and run.main? There is no need to reformat warning sent to the console (if there is one) during import, before Idle starts. Can the monkey-patching be undone when Python exits (in case it is being run from within a script)?

In the meanwhile, I will try using support.import_fresh_module('warnings') in test_logging, or better, saving and restoring warnings in test_idle.

[Although the alteration warnings are red-herrings for this issue, I am sure silencing them, including in tkinter tests, would be a good idea.]
msg191291 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2013-06-16 21:34
> In the meanwhile, I will try using support.import_fresh_module('warnings') in test_logging, or better, saving and restoring warnings in test_idle.

I agree the second option (changing test_idle) is better. The failing logging test is checking to ensure that the original warnings implementation is used when an explicit file is passed to warnings.showwarning, but it assumes that original implementation is the one in warnings when comparing the output from that (supposed) original implementation.

+1 to restoring the environment and locale in test_idle, too.
msg191293 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-16 22:37
Vinay, please comment on this: on my Win7-64 system, as least, test_logging seems broken without running test_idle first.

F:\Python\dev\cpython\PCbuild>python_d -m test test_logging
[1/1] test_logging
dummy.py:42: UserWarning: Explicit
Warning -- warnings.filters was modified by test_logging
test test_logging failed -- Traceback (most recent call last):
  File "F:\Python\dev\cpython\lib\test\test_logging.py", line 1779, in test_warnings_no_handlers
    self.assertEqual(len(logger.handlers), 1)
AssertionError: 0 != 1

Relevant code:
with warnings.catch_warnings():
  logging.captureWarnings(True)
  self.addCleanup(logging.captureWarnings, False)

  # confirm our assumption: no loggers are set
  logger = logging.getLogger("py.warnings")
  self.assertEqual(logger.handlers, [])

  warnings.showwarning("Explicit", UserWarning, "dummy.py", 42)
  self.assertEqual(len(logger.handlers), 1)
  self.assertIsInstance(logger.handlers[0], logging.NullHandler)

Since the above lines have not been touched in 2 years, and presumably worked before, perhaps something changed in warnings.catchwarnings or logging.capturewarnings so that NullHandler is no longer added.

With import_fresh_module added to the test.support import (line 69) and warnings imported with "warnings = import_fresh_module('warnings')";
  cpython\PCbuild>python_d -m test test_logging
produces the same error as above, so it works to eliminate the effect of Idle running first, and could be used as a temporary fix.

Side note: on Win7, should I see this?
test_basic (test.test_logging.NTEventLogHandlerTest) ... skipped 'win32evtlog/win32evtlogutil required for this test.'
msg191296 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-16 23:09
I reverted the change to test_logging, reran, and it runs fine, at least twice. Before the revert, I checked that the only two changes were the intentional ones.

I tried adding idle_test/test_zdummy.py with a tearDownModule that deletes sys.modules['warnings']. This 'works' in that the original failure passes, as with the change to test_logging, and
'dummy.py:42: UserWarning: Explicit' is printed. But with test_logging unchanged from when it ran and passed, I see the same 0 != 1 error as in the last message plus

Traceback (most recent call last):
  File "F:\Python\dev\cpython\lib\test\test_logging.py", line 1757, in test_warnings
    self.assertGreater(s.find("UserWarning: I'm warning you...\n"), 0)
AssertionError: -1 not greater than 0
msg191298 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2013-06-16 23:42
I would change the reverted test_logging to add "print(logging._warnings_showwarning)" before the failing assertions. If this is bound to the original warnings.showwarning implementation, all should be well. If it points to e.g. idle_showwarning, you would expect to get a failure, as idle_showwarning formats warning messages differently from what warnings.showwarning is originally bound to.

I think the right fix is to take the approach that logging does: have PyShell.py grow a captureWarnings(bool) function which either captures the old values before setting idle_showwarning/idle_formatwarning or restores them, according to the Boolean passed in. The PyShell.captureWarnings(True) could be called from PyShell module-level code if a side-effect free import is unavoidable, but test_idle could call PyShell.captureWarnings(False) to put things back as they were. I would have thought you could avoid side-effects by having the PyShell.captureWarnings(True) called from somewhere in the code path followed from "if __name__ == '__main__'", or from test_idle code.

Re. Your side note - IIRC that skip of the NTEventLogHandler is expected, unless you install some win32 components which aren't shipped with stock Python (Mark Hammond's pywin32 project, available on SourceForge). They are shipped with other Python distributions, e.g. ActivePython.
msg191305 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-06-17 01:34
As a side note, the various warnings about the execution environment being modified do not appear when tests are run under unittest because those warnings are generated by regrtest itself.
msg191309 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-17 02:05
The only locale setting is IOBinding.py: 21
# Try setting the locale, so that we can find out what encoding to use
    locale.setlocale(locale.LC_CTYPE, "")
This section, up to line 65, might be wrapped either by a function or if statement.

There are only 2 os.environ accesses and no settings in Idle. This alteration might be inherited from tkinter. File tkinter/_fix.py sets TCL/TK/TIX_LIBRARY if they are not already. This seem innocuous as far as other tests go.
msg191328 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2013-06-17 13:01
With the attached patch,

python -m test_idle test_logging

passes (though test_idle still alters locale and environment).

Running python -m idelib.idle, and using IDLE itself for inspection, the warnings.formatwarning is set to the implementation in idlelib.run.

Perhaps the PyShell changes aren't needed, but I think it may avoid similar issues in the future to leave them in.
msg191339 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-06-17 14:10
Note that it is a known issue that tk (not python's bindings, tk itself) alters the locale.
msg191342 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-06-17 14:56
Another odd thing about this failure is that http://hg.python.org/cpython/rev/59a11c81dd3c should have fixed it as it should be setting warnings.showwarning() back to it's original value before the test began executing. So I checked PyShell again and found out it's being very naughty.

If you look at http://hg.python.org/cpython/file/ec4b0145f7e5/Lib/idlelib/PyShell.py#l76 you will find that it is replacing warnings.formatwarning, and that's a big no-no. While warnings.showwarning is designed to be replaced as a hooking point (http://docs.python.org/3/library/warnings.html#warnings.showwarning), formatwarning is not and is meant to be the default function showwarning calls to produce the string that is to be used in the warning (http://docs.python.org/3/library/warnings.html#warnings.formatwarning).

My hunch (which I currently can't verify since I'm at work) is that if you stop replacing warnings.formatwarning and instead just have IDLE's showwarning version call its formatting function directly then this failure will go away. The warning about test_idle mucking with warnings.showwarning will still be there until a way to optionally set showwarning is developed, but at least the buildbots should go back to green.
msg191343 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-06-17 14:57
And Lib/idlelib/run.py also does the substitution. I'll file a separate bug for all of this.
msg191816 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-06-24 22:17
New changeset 2a9e1eb3719c by Victor Stinner in branch 'default':
Issue #18081: Workaround "./python -m test_idle test_logging" failure
http://hg.python.org/cpython/rev/2a9e1eb3719c
msg191817 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-06-24 22:20
This issue is *really* annoying: it makes buildbots almost useless (it is no more possible to check if a commit introduces a regression). So until the best fix is decided, I applied a temporary fix (based on  changes.diff written by  Vinay Sajip):

New changeset 2a9e1eb3719c by Victor Stinner in branch 'default':
Issue #18081: Workaround "./python -m test_idle test_logging" failure
http://hg.python.org/cpython/rev/2a9e1eb3719c

I checked that "import warnings; warnings.warn('test')" does still use the custom warning hook in IDLE. I don't know how to test all hooks.
msg191864 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2013-06-25 15:49
I replied on python-checkins, I'll state here as well: the fix is still off
as warnings.formatwarning is still being replaced.
On Jun 24, 2013 6:20 PM, "STINNER Victor" <report@bugs.python.org> wrote:

>
> STINNER Victor added the comment:
>
> This issue is *really* annoying: it makes buildbots almost useless (it is
> no more possible to check if a commit introduces a regression). So until
> the best fix is decided, I applied a temporary fix (based on  changes.diff
> written by  Vinay Sajip):
>
> New changeset 2a9e1eb3719c by Victor Stinner in branch 'default':
> Issue #18081: Workaround "./python -m test_idle test_logging" failure
> http://hg.python.org/cpython/rev/2a9e1eb3719c
>
> I checked that "import warnings; warnings.warn('test')" does still use the
> custom warning hook in IDLE. I don't know how to test all hooks.
>
> ----------
> nosy: +haypo
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue18081>
> _______________________________________
>
msg191865 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2013-06-25 16:18
> I replied on python-checkins, I'll state here as well: the fix is still off
> as warnings.formatwarning is still being replaced.

My changeset only fixes the unit test (test_idle), it's just to fix
buildbots. As I wrote, it's just a workaround.
msg191918 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-26 19:52
I should have changed the Versions sooner, as soon as it became obvious that this was not just a 3.4 issue. The 'temporary' 3.4-only patch breaks forward merging of a better patch. I will back it out just before I commit a 3.3 patch to both eliminate the .formatwarning replacement (#18242) and move the .showwarning replacement (part of Victor's patch).

If we ever add tests of PyShell.main and run.main (which we should), the problem of this issue will recur unless we make *sure* that the monkey patch moved inside those functions is reverted. Will try: finally: do that? I will leave this for a second patch.
msg191925 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-27 01:15
Here is a patch that looks much bigger than it really is. Idle was written before the warnings module, so the warnings system was conditioned on a successful warnings import. The test is no longer needed, and neither Vinay nor Victor repeated the test at the new location for the warnings.showwarning patch, within PyShell.main and run.main. I just deleted the test and dedented the indented blocks, which were, except for three lines, otherwise unchanged.

For run.py, I copied warnings.showwarnings and changed it to call idle_formatwarnings_subproc instead of (patched) warnings.formatwarnings. I then put the replacement of warnings.showwarnings in main after the sanity checks.

For PyShell.py, I made a similar change in showwarnings and the warnings.showwarnings replacement. I also added AttributeError to the print exception. At least on Windows, warning_stream = sys.__stderr__  is None when Idle is started normally, from an icon. But otherwise changing Idle to stop expecting a writable console is another issue.
msg191976 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-28 04:31
Improved 3.3 patch with tests incorporates Vinay's capture_warnings function to uncapture at end of import and main exit. I plan to apply after checking other versions and close this issue.
msg192026 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-06-29 03:48
New changeset 2176c1867b34 by Terry Jan Reedy in branch 'default':
Issue #18081: Back out temporary changeset, 2a9e1eb3719c, to merge new patch.
http://hg.python.org/cpython/rev/2176c1867b34
msg192028 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-06-29 04:05
This was sent to #18242 but not here. I expected here also.

New changeset c15d0baac3d6 by Terry Jan Reedy in branch '3.3':
Issue *18081, #18242: Change Idle warnings capture in PyShell and run
http://hg.python.org/cpython/rev/c15d0baac3d6

New changeset 89e0d33cb978 by Terry Jan Reedy in branch '2.7':
Issue *18081, #18242: Change Idle warnings capture in PyShell and run 
http://hg.python.org/cpython/rev/89e0d33cb978

Brett's test case works on my machine, all 3 versions.
python_d -m test[.regrtest] test_idle test_logging
History
Date User Action Args
2013-06-29 04:05:43terry.reedysetstatus: open -> closed
resolution: fixed
messages: + msg192028
2013-06-29 03:48:04python-devsetmessages: + msg192026
2013-06-28 04:31:04terry.reedysetfiles: + issue18081-warn3.diff

messages: + msg191976
2013-06-27 01:15:31terry.reedysetfiles: + issue18081-warn.diff

messages: + msg191925
stage: needs patch -> commit review
2013-06-26 19:52:47terry.reedysetassignee: terry.reedy
messages: + msg191918
versions: + Python 2.7, Python 3.3
2013-06-25 16:18:32vstinnersetmessages: + msg191865
2013-06-25 15:49:03brett.cannonsetmessages: + msg191864
2013-06-24 22:20:33vstinnersetnosy: + vstinner
messages: + msg191817
2013-06-24 22:17:51python-devsetnosy: + python-dev
messages: + msg191816
2013-06-17 14:59:33brett.cannonsetdependencies: + IDLE should not be replacing warnings.formatwarning
2013-06-17 14:57:08brett.cannonsetmessages: + msg191343
2013-06-17 14:56:20brett.cannonsetmessages: + msg191342
2013-06-17 14:10:42r.david.murraysetmessages: + msg191339
2013-06-17 13:01:51vinay.sajipsetfiles: + changes.diff

messages: + msg191328
2013-06-17 02:05:53terry.reedysetmessages: + msg191309
2013-06-17 01:34:47r.david.murraysetmessages: + msg191305
2013-06-16 23:42:46vinay.sajipsetmessages: + msg191298
2013-06-16 23:09:04terry.reedysetfiles: + zdummy.diff
keywords: + patch
messages: + msg191296
2013-06-16 22:37:34terry.reedysetmessages: + msg191293
2013-06-16 21:34:25vinay.sajipsetmessages: + msg191291
2013-06-16 20:13:04terry.reedysetmessages: + msg191286
2013-06-16 19:36:11brett.cannonsetmessages: + msg191283
2013-06-16 18:06:19terry.reedysetmessages: + msg191277
2013-06-16 14:31:03brett.cannonsetnosy: + terry.reedy, kbk, roger.serwy
messages: + msg191267
2013-06-16 13:48:49brett.cannonsetnosy: + brett.cannon
messages: + msg191266
2013-06-09 04:46:20Arfreversetnosy: + Arfrever
2013-05-28 15:25:27r.david.murraycreate