classification
Title: test_curses should use unittest
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.5, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: chris.jerdonek, esc24, ezio.melotti, haypo, python-dev, zach.ware
Priority: normal Keywords: needs review, patch

Created on 2012-09-21 21:31 by chris.jerdonek, last changed 2014-10-17 19:12 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
issue-16000-1.patch chris.jerdonek, 2012-09-22 00:45 review
issue16000.v1.diff zach.ware, 2013-06-29 04:59 Full module transformation, version 1 review
issue16000.v1-cmp.diff zach.ware, 2013-06-29 05:00 For review purposes only (non-PEP-8 indention) review
issue16000.v2.diff zach.ware, 2013-12-20 05:13 review
Messages (10)
msg170925 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-21 21:31
This issue is to switch test_curses to using unittest.TestCase classes.

Currently, test_curses does not use unittest.TestCase.  It just calls a series of functions that exercise curses functionality and aborts if an exception occurs:

http://hg.python.org/cpython/file/3e677956eef4/Lib/test/test_curses.py#l314

Some consequences of this are that a single failure will cause remaining tests not to execute, there is no fine-grained reporting, and TestCase assertion methods are not available (e.g. assertRaisesRegexp()).
msg170926 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-09-21 21:33
Would like to work on such patch?
msg170931 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-21 21:49
Sure, I could start work on such a patch.  I may start by switching just a couple of the functions to TestCase though (to establish a model and to make sure everything is wired up correctly), rather than attempting to switch the entire module all at once.
msg170941 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-09-22 00:45
Here is an initial patch for discussion.  As I started to say in my previous comment, I think it would be worth settling on the wiring (e.g. setUp/tearDown/etc) before trying to do more.

Some comments on the current patch:

(1) Of the following methods, it's not clear to me which should go in setUpModule(), setUpClass(), and setUp(): curses.setupterm(), curses.initscr(), and curses.savetty().

(2) When running the tests using "__main__" or from regrtest in verbose mode, there is a slight rendering glitch in that the "ok" prints over the beginning of the output line instead of appending to the end:

$ ./python.exe Lib/test/test_curses.py
okst_issue10570 (__main__.CursesTestCase) ...
okst_issue6243 (__main__.CursesTestCase) ...

----------------------------------------------------------------------
Ran 2 tests in 0.005s

OK

I'm not sure how important it is to make that work perfectly.  Using regrtest in normal mode, there are no rendering defects that I see.
msg171707 - (view) Author: Ed Campbell (esc24) Date: 2012-10-01 11:44
I'd suggest using unittest.TestCase.assertRaises() as a context manager to remove some try-excepts. For example I think function test_userptr_without_set() on line 245 could use:

with self.assertRaises(curses.panel.error):
    p.userptr()

I could create a patch if it would help.
msg171732 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-10-01 16:40
Ed, yes, switching all of test_curses to using unittest patterns is the eventual goal of this issue, though this may be done in more than one stage.  As I said in my previous comment, I limited the first patch to focus on the proper setUp(), tearDown(), etc that we should be using.
msg192030 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-06-29 04:59
I had a chance to look at this today and took a stab at writing a patch (attached).  Since moving all of the test functions into a TestCase subclass necessarily means indenting everything another level, the patch is a huge ugly mix of - and + lines that don't really have any relation to each other.  As such, I'll also be attaching another patch which is for review purposes only: it differs from the real patch by only indenting the def statements by 2 spaces instead of the usual 4, allowing the bodies of the methods to not be indented, which makes the changes MUCH easier to keep track of.

As Chris mentioned previously, running the test in verbose mode (under regrtest) causes rendering problems.  I worked around this by adding a "print('')" to the setUp method; it doesn't make things perfect, but it makes it a bit easier to read.  I'm not terribly attached to this hack, though, so I'd be fine with removing it if anyone has a preference.

Also as Chris mentioned, there is an open question of how to do setup, whether to do things in TestCase.setUp, TestCase,setUpClass, or setUpModule.  This patch does initial curses.setupterm() in setUpModule, then initscr and savetty in setUp (with resetty and endwin in tearDown).  However, this is not how the module has been testing; it has been doing initscr/savetty and resetty/endwin only once throughout execution.  I've considered creating a subclass of the TestCurses test class which would do initscr/savetty in setUpClass, to do all the tests again on the same screen (as has been up to now), but I'd like others' opinions on that before I write it.

Also, this patch is against default; I'll make sure it works for 3.3 after a round or two of review.
msg192407 - (view) Author: Ed Campbell (esc24) Date: 2013-07-06 10:30
I think this is a real improvement. Thanks. I have a few comments:

I suspect you know this, but the rendering problem occurs because of the call to curses.endwin() in tearDown(). I experimented with delaying this until teadDownClass() but this led to even more undesirable side effects with the tests running in parallel. Your print() call seems like a simple workaround.

There is only a single test case so I don't see the advantage of using a setUpModule function over setUpClass. Is there one? I'd put the code in setUpClass (or possibly put the curses.setupterm() call in setUp and have neither setUpModule or setUpClass). If and when further test cases are added I'd consider factoring out common code into a setUpModule. I'd also consider putting the skip test if not sys.__stdout__.isatty() that's in your current setUpModule as a decorator with the other @unittest.skipIf class decorators. Is there a reason you singled this one out to go in setUpModule?

You have a typo on line 30: 'unkwown' should be 'unknown'
msg206664 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-12-20 05:13
It's only taken me 6 months, but I'm looking at this issue again :)

Ed, basically the only reason I used setUpModule was because it was a very direct translation from test_main to setUpModule--only the name and signature changed, the skip and initialization code stayed exactly the same.  It does make more sense for it to be in setUpClass, though, so that's done in the new patch.

Thanks for pointing out the typo, fixed.
msg229592 - (view) Author: Roundup Robot (python-dev) Date: 2014-10-17 19:11
New changeset f598d0014d07 by Zachary Ware in branch '3.4':
Issue #16000: Convert test_curses to use unittest
https://hg.python.org/cpython/rev/f598d0014d07

New changeset b7b49f26a87b by Zachary Ware in branch 'default':
Issue #16000: Convert test_curses to use unittest
https://hg.python.org/cpython/rev/b7b49f26a87b
History
Date User Action Args
2014-10-17 19:12:05zach.waresetstatus: open -> closed
assignee: zach.ware
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.5, - Python 3.3
2014-10-17 19:11:17python-devsetnosy: + python-dev
messages: + msg229592
2013-12-20 05:13:28zach.waresetfiles: + issue16000.v2.diff

messages: + msg206664
2013-07-06 10:30:24esc24setmessages: + msg192407
2013-06-29 05:00:45zach.waresetfiles: + issue16000.v1-cmp.diff
2013-06-29 04:59:59zach.waresetfiles: + issue16000.v1.diff

messages: + msg192030
2013-04-18 20:17:19ezio.melottisetnosy: + zach.ware

versions: + Python 3.3
2012-10-01 16:40:31chris.jerdoneksetmessages: + msg171732
2012-10-01 11:44:33esc24setnosy: + esc24
messages: + msg171707
2012-09-22 13:39:20ezio.melottisetnosy: + ezio.melotti
2012-09-22 00:45:14chris.jerdoneksetkeywords: + needs review, patch
files: + issue-16000-1.patch
messages: + msg170941

stage: patch review
2012-09-21 21:49:17chris.jerdoneksetmessages: + msg170931
2012-09-21 21:33:01hayposetnosy: + haypo
messages: + msg170926
2012-09-21 21:31:26chris.jerdonekcreate