| msg166392 - (view) |
Author: Anton Barkovsky (anton.barkovsky) * |
Date: 2012-07-25 13:15 |
| webbrowser._invoke opens /dev/null, never closes it and a warning is printed. I'm attaching a patch. The diff looks messy, but I'm just wrapping the code in a try-finally block, the rest is just indented. |
|
|
| msg166394 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2012-07-25 13:31 |
| Thanks. Is this warning printed by the webbrowser unit tests? If not can you see a way to add one that does? |
|
|
| msg166395 - (view) |
Author: Anton Barkovsky (anton.barkovsky) * |
Date: 2012-07-25 13:38 |
| The warning is printed by the file object when it closes itself in __del__: ResourceWarning: unclosed file <_io.TextIOWrapper name='/dev/null' mode='r+' encoding='UTF-8'> There isn't much to test, or is there? |
|
|
| msg166396 - (view) |
Author: Anton Barkovsky (anton.barkovsky) * |
Date: 2012-07-25 13:39 |
| To clarify, I discovered this when I was simply running webbrowser.open in REPL. |
|
|
| msg166397 - (view) |
Author: Ross Lagerwall (rosslagerwall) |
Date: 2012-07-25 13:43 |
| Are there any webbrowser unit tests? (this could probably use the new subprocess.DEVNULL constant in 3.3) |
|
|
| msg166399 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-07-25 13:48 |
| @Anton: That's what I was guessing. If we had a unit test in test_webbrowser that did the same thing, we'd have seen the resource warning when running the tests and fixed it. However, it looks like there aren't *any* tests for webbrowser, not even in test_sundry (which just makes sure modules without tests are importable). So adding a test that will trigger this resource warning requires setting up a test_webbrowser file first, even before we get to the problem of how to test something that wants to start up a web browser...(but that should be solvable with unittest.mock, I think). |
|
|
| msg166401 - (view) |
Author: Anton Barkovsky (anton.barkovsky) * |
Date: 2012-07-25 14:01 |
| Adding a patch that uses subprocess.DEVNULL instead. Writing tests for webbrowser should be a separate issue, right? |
|
|
| msg166404 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-07-25 14:27 |
| You could do it either way. Normally we prefer to have a test along with any fix; in this case adding a test involves adding the test module as well, but it is not different in principle. If you want to work on it and prefer to have it as a separate issue that's fine, we'll just make the test issue dependent on this one. |
|
|
| msg166900 - (view) |
Author: Anton Barkovsky (anton.barkovsky) * |
Date: 2012-07-30 14:07 |
| An updated patch with the same issue fixed in Konqueror class. |
|
|
| msg167424 - (view) |
Author: Anton Barkovsky (anton.barkovsky) * |
Date: 2012-08-04 17:54 |
| Added tests in #15557. |
|
|
| msg169774 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2012-09-03 16:53 |
| New changeset 901c790e4417 by R David Murray in branch 'default': #15447: Use subprocess.DEVNULL in webbrowser, instead of opening http://hg.python.org/cpython/rev/901c790e4417 New changeset 5da3b2df38b3 by R David Murray in branch 'default': #15557,#15447,#15509: webbrowser test suite added. http://hg.python.org/cpython/rev/5da3b2df38b3 |
|
|
| msg169779 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-09-03 16:55 |
| Thanks, Anton. |
|
|