msg154425 - (view) |
Author: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
Date: 2012-02-28 15:20 |
Moved to test_cmd_line_script.
|
msg154558 - (view) |
Author: Ethan Furman (ethan.furman) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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) *  |
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) *  |
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: Alyssa Coghlan (ncoghlan) *  |
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)  |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:27 | admin | set | github: 58344 |
2012-05-21 13:03:46 | python-dev | set | status: open -> closed
nosy:
+ python-dev messages:
+ msg161273
resolution: fixed stage: needs patch -> resolved |
2012-03-28 04:50:12 | ncoghlan | set | messages:
+ msg156969 |
2012-03-27 22:09:34 | ethan.furman | set | files:
+ raise_from_none_test_cleanup_3.diff
messages:
+ msg156958 |
2012-03-27 21:05:17 | ethan.furman | set | messages:
+ msg156955 |
2012-02-28 23:00:31 | ncoghlan | set | messages:
+ msg154578 |
2012-02-28 17:13:16 | ethan.furman | set | messages:
+ msg154558 |
2012-02-28 15:20:13 | ethan.furman | set | files:
+ raise_from_none_test_cleanup.diff
messages:
+ msg154554 |
2012-02-28 11:30:21 | ncoghlan | set | messages:
+ msg154547 |
2012-02-28 11:05:29 | ethan.furman | set | messages:
+ msg154546 |
2012-02-28 11:00:52 | ethan.furman | set | messages:
+ msg154545 |
2012-02-28 10:44:02 | ncoghlan | set | messages:
+ msg154544 |
2012-02-28 10:37:37 | ethan.furman | set | files:
+ raise_from_none_test_cleanup.diff
nosy:
+ ethan.furman messages:
+ msg154542
keywords:
+ patch |
2012-02-27 02:23:19 | ncoghlan | create | |