classification
Title: Test cases in test_sys don't match the comments
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: Gareth.Rees, python-dev, serhiy.storchaka, zach.ware
Priority: normal Keywords: patch

Created on 2014-02-04 16:09 by Gareth.Rees, last changed 2014-02-19 16:46 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
exittest.patch Gareth.Rees, 2014-02-04 16:09 review
exittest-1.patch Gareth.Rees, 2014-02-04 18:29 review
Messages (9)
msg210246 - (view) Author: Gareth Rees (Gareth.Rees) * Date: 2014-02-04 16:09
Lib/test/test_sys.py contains test cases with incorrect comments -- or
comments with incorrect test cases, if you prefer:

    # call without argument
    try:
        sys.exit(0)
    except SystemExit as exc:
        self.assertEqual(exc.code, 0)
    ...

    # call with tuple argument with one entry
    # entry will be unpacked
    try:
        sys.exit(42)
    except SystemExit as exc:
        self.assertEqual(exc.code, 42)
    ...

    # call with integer argument
    try:
        sys.exit((42,))
    except SystemExit as exc:
        self.assertEqual(exc.code, 42)
    ...

(In the quote above I've edited out some inessential detail; see the
file if you really want to know.)

You can see that in the first test case sys.exit is called with an
argument (although the comment claims otherwise); in the second it is
called with an integer (not a tuple), and in the third it is called
with a tuple (not an integer).

These comments have been unchanged since the original commit by Walter
Dörwald <http://hg.python.org/cpython/rev/6a1394660270>. I've attached
a patch that corrects the first test case and swaps the comments for
the second and third test cases:

    # call without argument
    rc = subprocess.call([sys.executable, "-c",
                          "import sys; sys.exit()"])
    self.assertEqual(rc, 0)

    # call with integer argument
    try:
        sys.exit(42)
    except SystemExit as exc:
        self.assertEqual(exc.code, 42)
    ...

    # call with tuple argument with one entry
    # entry will be unpacked
    try:
        sys.exit((42,))
    except SystemExit as exc:
        self.assertEqual(exc.code, 42)
    ...

Note that in the first test case (without an argument) sys.exit() with
no argument actually raises SystemExit(None), so it's not sufficient
to catch the SystemExit and check exc.code; I need to check that it
actually gets translated to 0 on exit.
msg210252 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-02-04 17:08
Good catch!  See my review comments.

Also, if you haven't already, could you please sign a contributor's agreement?  See http://www.python.org/psf/contrib/.
msg210257 - (view) Author: Gareth Rees (Gareth.Rees) * Date: 2014-02-04 18:29
I normally try not to make changes "while we're in here" for fear of
introducing errors! But I guess the test cases are less critical, so
I've taken your review comments as a license to submit a revised patch
that:

* incorporates your suggestion to use assert_python_ok from
  test.script_helper, instead of subprocess.call;
* replaces the other uses of subprocess.call with
  assert_python_failure and adds a check on stdout;
* cleans up the assertion-testing code using the context manager form
  of unittest.TestCase.assertRaises.

I've signed and submitted a contributor agreement as requested.
msg211445 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-02-17 21:33
The newer patch looks good to me, I'll get it committed as soon as I can test it.  Thanks!
msg211513 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-18 14:42
New changeset 63f0a1e95d2b by Zachary Ware in branch '2.7':
Issue #20510: Rewrote test_exit in test_sys to match existing comments
http://hg.python.org/cpython/rev/63f0a1e95d2b

New changeset fa81f6ddd60e by Zachary Ware in branch '3.3':
Issue #20510: Rewrote test_exit in test_sys to match existing comments
http://hg.python.org/cpython/rev/fa81f6ddd60e

New changeset 2438fbf4ad9d by Zachary Ware in branch 'default':
Issue #20510: Merge with 3.3.
http://hg.python.org/cpython/rev/2438fbf4ad9d
msg211514 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-02-18 15:48
Fixed, thanks for the report and patch!

And btw, you are right to avoid "while we're in there" changes in general, but modernizing the test suite gets a little bit of leniency in that regard.  It wouldn't have been appropriate to venture outside of test_exit in this issue, but test_exit was terribly outdated and needed some fixing.
msg211561 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-18 21:40
What about following test?

        with self.assertRaises(SystemExit) as cm:
            sys.exit()
        self.assertIsNone(cm.exception.code)
msg211625 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-02-19 15:57
I considered doing a test like that, but figured it was pretty well covered by the assert_python_ok no-arg test.  On the other hand, it looks like we document that the default value of SystemExit().code is None, so it should be tested.  I'll add it in.

Thanks, Serhiy!
msg211630 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-19 16:46
New changeset 292cac3afca6 by Zachary Ware in branch '2.7':
Issue #20510: Confirm that the code attribute of the SystemExit
http://hg.python.org/cpython/rev/292cac3afca6

New changeset 71411d5f3c8b by Zachary Ware in branch '3.3':
Issue #20510: Confirm that the code attribute of the SystemExit
http://hg.python.org/cpython/rev/71411d5f3c8b

New changeset 24412574fe24 by Zachary Ware in branch 'default':
Issue #20510: Merge with 3.3
http://hg.python.org/cpython/rev/24412574fe24
History
Date User Action Args
2014-02-19 16:46:31python-devsetmessages: + msg211630
2014-02-19 15:57:08zach.waresetmessages: + msg211625
2014-02-18 21:40:04serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg211561
2014-02-18 15:48:55zach.waresetstatus: open -> closed
resolution: fixed
messages: + msg211514

stage: patch review -> resolved
2014-02-18 14:42:07python-devsetnosy: + python-dev
messages: + msg211513
2014-02-17 21:33:00zach.waresetassignee: zach.ware
messages: + msg211445
2014-02-04 18:29:18Gareth.Reessetfiles: + exittest-1.patch

messages: + msg210257
2014-02-04 17:08:35zach.waresetstage: patch review
messages: + msg210252
versions: + Python 2.7, Python 3.3
2014-02-04 17:05:10zach.waresetnosy: + zach.ware
2014-02-04 16:09:06Gareth.Reescreate