classification
Title: Simplify PEP 409 command line test and move it to test_cmd_line_script
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: ethan.furman, ncoghlan, python-dev
Priority: normal Keywords: patch

Created on 2012-02-27 02:23 by ncoghlan, last changed 2012-05-21 13:03 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
raise_from_none_test_cleanup.diff ethan.furman, 2012-02-28 10:37 review
raise_from_none_test_cleanup.diff ethan.furman, 2012-02-28 15:20 review
raise_from_none_test_cleanup_3.diff ethan.furman, 2012-03-27 22:09 review
Messages (13)
msg154425 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-27 02:23
I stuffed up the review on one of the new tests in the PEP 409 patch - the one that checks if the exception context is suppressed correctly from the command line.

That test should be drastically simplified and moved from test_raise to test_cmd_line_script.
msg154542 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-02-28 10:37
Changed TestTraceback.test_traceback_verbiage to use test.support and test.script_helper (much simpler now, thanks Antoine!).

It is still in test_raise, however.  

I do not understand why we would put it in test_cmd_line_script -- it seems to me that test_cmd_line_script is to verify that python is working correctly when executing scripts, but for this test we are assuming that python does work correctly when executing scripts and we testing the verbiage from raising exceptions.
msg154544 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-28 10:44
It's about separating out the testing of the command line executable (i.e. test_cmd_line and test_cmd_line_script) from that of the interpreter's internal behaviour.

test_raise is about making sure the "raise" statement works correctly - the new test isn't testing that, it's testing the display code in the command line interface.

A non-CPython interpreter must conform exactly to test_raise, but conforming to test_cmd_line(_script) is less essential.
msg154545 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-02-28 11:00
Is there a better place, then, than test_cmd_line_script?

The entire purpose of PEP 409 is to allow the suppression of non-relevant information in the exception traceback.  As such I would expect every Python interpreter to adhere to it.

(Yes, I understand that custom display hooks can still do different things, but they are custom and not the default.)
msg154546 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-02-28 11:05
I guess the crux of the issue for me is that I'm trying to test interpreter output, and the fact that I am using a command-line script to do so is an implementation detail.
msg154547 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-28 11:30
test_cmd_line and test_cmd_line_script are both about testing the behaviour of the interpreter when run from the command line. Since this test *is* a behavioural test for the interpreter when invoked as an application, it should be in one of them.

The former is for testing the behaviour that doesn't involve invoking a script that does something, whereas the latter is for testing the way the interpreter handles various methods of script invocation, *including* what it prints to stderr and stdout in various scenarios.

Therefore, test_cmd_line_script *is* the right place for it. The fact that it happens to be testing the way an exception is displayed doesn't change the fact that it's a test of the way the interpreter handles a particular situation that may arise when running a script.
msg154554 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-02-28 15:20
Moved to test_cmd_line_script.
msg154558 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-02-28 17:13
Nick,

Thank you for your thorough answers.  I'm not trying to be difficult, 
just trying to learn and understand.
msg154578 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-02-28 23:00
Hah, I've been dealing with Python's regression test suite for 8+ years and there are still cases where I don't understand the rationale for testing things a particular way (beyond "it must have seemed like a good idea at the time"). It has a lot more historical cruft than the standard library does :)

The "proper" location for a particular test can be a bit of a coin toss in many cases, but one useful guide (which applies in this case) is to try to avoid adding *new* standard library dependencies to a test module if there's an alternate suitable location that already has those dependencies.
msg156955 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-03-27 21:05
Nick Coghlan wrote:
> Nick Coghlan <ncoghlan@gmail.com> added the comment:
> 
> Hah, I've been dealing with Python's regression test suite for 8+ years and there are still cases where I don't understand the rationale for testing things a particular way (beyond "it must have seemed like a good idea at the time"). It has a lot more historical cruft than the standard library does :)
> 
> The "proper" location for a particular test can be a bit of a coin toss in many cases, but one useful guide (which applies in this case) is to try to avoid adding *new* standard library dependencies to a test module if there's an alternate suitable location that already has those dependencies.

I just checked the patch and it still applies cleanly with the two 
effected tests passing.

Nick, any chance of getting this checked in and closed?
msg156958 - (view) Author: Ethan Furman (ethan.furman) * (Python committer) Date: 2012-03-27 22:09
Modified test name to test_pep_409_verbiage(); modified test body to use same pattern as other tests, thus eliminating the dangling test script files after the test was run.
msg156969 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-03-28 04:50
Yup, I plan to get a few things like this incorporated once I have broadband access at home again. Probably won't be early enough to get things into alpha 2, but they should make it for alpha 3.
msg161273 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-05-21 13:03
New changeset 02f13577b6fe by Nick Coghlan in branch 'default':
Close #14136 by cleaning up the PEP 409 command line test (patch by Ethan Furman)
http://hg.python.org/cpython/rev/02f13577b6fe
History
Date User Action Args
2012-05-21 13:03:46python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg161273

resolution: fixed
stage: needs patch -> resolved
2012-03-28 04:50:12ncoghlansetmessages: + msg156969
2012-03-27 22:09:34ethan.furmansetfiles: + raise_from_none_test_cleanup_3.diff

messages: + msg156958
2012-03-27 21:05:17ethan.furmansetmessages: + msg156955
2012-02-28 23:00:31ncoghlansetmessages: + msg154578
2012-02-28 17:13:16ethan.furmansetmessages: + msg154558
2012-02-28 15:20:13ethan.furmansetfiles: + raise_from_none_test_cleanup.diff

messages: + msg154554
2012-02-28 11:30:21ncoghlansetmessages: + msg154547
2012-02-28 11:05:29ethan.furmansetmessages: + msg154546
2012-02-28 11:00:52ethan.furmansetmessages: + msg154545
2012-02-28 10:44:02ncoghlansetmessages: + msg154544
2012-02-28 10:37:37ethan.furmansetfiles: + raise_from_none_test_cleanup.diff

nosy: + ethan.furman
messages: + msg154542

keywords: + patch
2012-02-27 02:23:19ncoghlancreate