This issue tracker has been migrated to GitHub ,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2013年07月28日 12:32 by ncoghlan, last changed 2022年04月11日 14:57 by admin. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| script_helper-default.patch | Fotis.Koutoulakis, 2013年12月09日 01:14 | All changes needed to move and document script_helper.py | review | |
| issue_18576_second.patch | Fotis.Koutoulakis, 2013年12月09日 11:13 | The second patch with all changes required to script_helper.py and its documentation | review | |
| issue18576_third_try.patch | Fotis.Koutoulakis, 2013年12月09日 13:20 | The third patch, with changes to the documentation to fix the issues presented at the second one's code review. | review | |
| issue18576_move_and_document_script_helper.diff | ncoghlan, 2013年12月18日 12:34 | Moved without legacy alias + full docs | review | |
| issue18576.patch | bobcatfish, 2015年06月06日 22:01 | |||
| issue18576.patch | bobcatfish, 2015年06月28日 22:28 | review | ||
| Pull Requests | |||
|---|---|---|---|
| URL | Status | Linked | Edit |
| PR 1252 | closed | louielu, 2017年04月22日 06:18 | |
| Messages (24) | |||
|---|---|---|---|
| msg193819 - (view) | Author: Alyssa Coghlan (ncoghlan) * (Python committer) | Date: 2013年07月28日 12:32 | |
tests.script_helper provides various utilities for sensibly testing scripts in a subprocess. It isn't easy for CPython developers to discover, since it is isn't documented and the file is mixed in with actual tests in the main test directory. As discussed in #15494, it should be: 1. Moved from Lib/test/ to Lib/test/support (and imports in tests adjusted accordingly) 2. Documented in Doc/library/test.rst These changes should be made on both the 3.3 and default branches to avoid spurious merge conflicts. |
|||
| msg194908 - (view) | Author: Seydou Dia (seydou) * | Date: 2013年08月11日 19:15 | |
Since I am already on bugs.python.org/issue18578 I will tackle this issue, if you don't mind. |
|||
| msg205635 - (view) | Author: Fotis Koutoulakis (Fotis.Koutoulakis) * | Date: 2013年12月09日 01:14 | |
I have finished the changes needed to move script_helper from Lib/test to Lib/test/support, and I have also documented it. Tests run gracefully (but be sure to check out if it works as intended on your machines too) Two notes though: 1. I have only made the changes on the default branch. Will do 3.3 tomorrow. 2. When it came to the documentation, I had to break the 80 characters limit, as going by it was resulting in weird rendering or wrong information depicted from sphinx (for example, when showing that script_helper functions belonged to test.support instead of the correct location, test.support.script_helper). Last but not least, this is one of my first contributions to Python, so I would appreciate your input. |
|||
| msg205638 - (view) | Author: Alyssa Coghlan (ncoghlan) * (Python committer) | Date: 2013年12月09日 01:57 | |
I wouldn't worry about 3.3 at this point - the 3.3 test suite isn't going to see major changes for its final release, so the risk of merge conflicts is low. |
|||
| msg205656 - (view) | Author: Fotis Koutoulakis (Fotis.Koutoulakis) * | Date: 2013年12月09日 09:46 | |
Hello again. Is everything ok with the patch? Is there something not working as expected? Perhaps an omission or something? Do I need to do something more to get it accepted? Thanks for your time. |
|||
| msg205661 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2013年12月09日 10:07 | |
Please left test.script_helper as alias to test.support.script_helper. I.e. test/script_helper.py should contains something like: from test.support.script_helper import * And test this with unmodifiable other tests. |
|||
| msg205676 - (view) | Author: Fotis Koutoulakis (Fotis.Koutoulakis) * | Date: 2013年12月09日 11:13 | |
Ok, here is the second (modified patch) which contains a script so that no modifications are required to existing tests. I am uploading it as a second patch, so that the first one is left as a reference. As a sidenote, I fail to see convincing reasons for why we would want the second solution more than the first. The first one looks cleaner to my eyes. The only downside I can see to the original approach is breaking a couple of tests, which is something that can be fixed easily without even making it to the repo (running the tests in 2-3 could help find all possible broken tests beforehand). Could someone explain to me why the second solution is more desirable? |
|||
| msg205680 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2013年12月09日 11:44 | |
Third party code can use tests.script_helper. I prefer to do these changes in several steps: 1) Move script_helper.py to Lib/test/support/, document it, and create an alias (with deprecation warning). 2) Ensure that this doesn't break any buildbot. 3) Change tests to use test.support.script_helper instead of test.script_helper. 4) One or two versions later remove an alias. |
|||
| msg205681 - (view) | Author: Fotis Koutoulakis (Fotis.Koutoulakis) * | Date: 2013年12月09日 11:49 | |
Oh, I see. Does the last patch meet your expectations, or would you rather see all these changes implemented at once? |
|||
| msg205684 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) | Date: 2013年12月09日 12:17 | |
> Does the last patch meet your expectations, or would you rather see all these changes implemented at once? I left this for Nick. I have added comments on Rietveld. |
|||
| msg205689 - (view) | Author: Fotis Koutoulakis (Fotis.Koutoulakis) * | Date: 2013年12月09日 13:20 | |
Taking the feedback during code review, this is a patch that has the points raised by Serhiy Storchaka fixed. As always, please do note omission or mistakes from my part. Thanks for your help. |
|||
| msg205746 - (view) | Author: Alyssa Coghlan (ncoghlan) * (Python committer) | Date: 2013年12月09日 22:44 | |
Note that the test.* namespace has long been declared as unstable, so we don't worry about breaking third party code when rearranging the test suite (as far as I am aware, most Linux distros don't even include the test suite in their Python packages). I'll see how Mercurial goes tracking the file move when the alias is left behind, and decide which way to do the initial change based on whether or not the alias breaks the history tracking. |
|||
| msg206511 - (view) | Author: Alyssa Coghlan (ncoghlan) * (Python committer) | Date: 2013年12月18日 12:34 | |
The attached patch just moves the file without leaving an alias behind. It turns out we use this one in a fair few places (mostly for "assert_python_ok"). I'm going to sit on this change for the moment - after reading the docs, it's quite clear that the last couple of functions don't really belong in script_helper, but rather in the pkg_helper module discussed in issue 15376. So my current plan is to factor that helper out *first*, then when this one lands, the intent of grouping more of the support code that folks working on the test suite might be interested in using together will hopefully be clearer. |
|||
| msg242944 - (view) | Author: Alyssa Coghlan (ncoghlan) * (Python committer) | Date: 2015年05月12日 06:31 | |
script_helper was moved to the test.support package in issue 9517, so this issue is just about adding the docs now. |
|||
| msg242946 - (view) | Author: Alyssa Coghlan (ncoghlan) * (Python committer) | Date: 2015年05月12日 06:34 | |
Also noting that my draft docs are over a year old now, so they need to be reviewed against the current state of the helper module. |
|||
| msg243528 - (view) | Author: Christie (bobcatfish) * | Date: 2015年05月18日 19:46 | |
I'm going to take a look at bringing these docs up to date and including the changes from issue24033. |
|||
| msg244929 - (view) | Author: Christie (bobcatfish) * | Date: 2015年06月06日 22:01 | |
@ncoghlan I've taken the test.rst changes from the previous diff and reviewed them against the latest script_helper (including updates from issue24033). Nothing had fallen behind, but I've added what was missing. * I've realized that spawn_python and the helper run_python I added in issue24033 are basically the same so I will reconcile those * Is there some way to load the docstrings from script_helper.py in test.rst instead of writing explanations for each method twice? |
|||
| msg244936 - (view) | Author: Martin Panter (martin.panter) * (Python committer) | Date: 2015年06月07日 00:44 | |
If you diffed your patch from a public revision in the main repository, there should be a nice "Review" link. Anyway, here are some comments on issue18576.patch: +.. function:: assert_python_ok(*args, **env_vars) + + Runs the interpreter with *args* and optional environment + variables, asserting that *env_vars* succeeds (``rc == 0``). Perhaps you mean *env_vars* are the optional environment variables, and that the interpreter is asserted to have succeeded. Also maybe clarify where the *__cleanenv* and *__isolated* keywords come from? Function signatures? Also applies to assert_python_failure(), run_python/_until_end(). Maybe it is worth mentioning that assert_python_ok/failure() strip whitespace from the stderr stream. I seem to remember being tricked by this (writing a test to ensure stderr ended in a newline or something). + ``stdout`` are configured as binary pipes and ``stderr`` is merged with + ``stdout``.. There are two full stops above here.. I encourage you to start sentences with capital letters (although I admit sometimes other people do not agree with this). Suggestions: + *kw* is passed through to subprocess.Popen as additional keyword + arguments. => The *kw* arguments are passed through to subprocess.Popen. + *source* is a Unicode string which will be written to the file as UTF-8. => The *source* argument is a Unicode string which will be written to the file as UTF-8. + *depth* controls how many deeply nested the package is. => The *depth* argument controls how deeply nested the package is. + Creates a new zip archive in the given directory, containing the specified + Python script/module Needs one of those full stops I mentioned earlier :) |
|||
| msg244943 - (view) | Author: Alyssa Coghlan (ncoghlan) * (Python committer) | Date: 2015年06月07日 05:45 | |
As far as using docstrings for the online docs goes, there are some useful Sphinx add-ons that we've never enabled for the main Python docs (like autodoc, blockdiag, seqdiag) that could be good options to have available. For autodoc in particular, we prefer to avoid it for the main docs because "good docstring" and "good prose documentation" don't necessarily look alike. For ease of maintenance of test.support though, I think the "write a good docstring, get the online docs for free" rationale is a compelling one (since these docs are core contributor facing, rather than being aimed at Python users in general) docs@python.org would be the place to ask about the appropriateness of enabling some Sphinx add-ons. While enabling them seems like a reasonable idea to me, I don't personally know if there are actually technical issues with the idea, or if there just hasn't been a volunteer to add/enable them and update https://docs.python.org/devguide/documenting.html accordingly. |
|||
| msg244965 - (view) | Author: Christie (bobcatfish) * | Date: 2015年06月07日 18:25 | |
> If you diffed your patch from a public revision in the main repository, there should be a nice "Review" link. Ah, thanks @vadmium!!! I've been confused about why some patches get review links and others do not >.< I'll have a new patch up shortly which is diffed against a public revision and has your suggested changes. > For autodoc in particular, we prefer to avoid it for the main docs because "good docstring" and "good prose documentation" don't necessarily look alike. @ncoghlan for some reason I thought that the main docs were also auto generated from docstrings in the actual modules, but I can see now that that isn't the case. I'll reach out to docs@python.org to see how they feel about autogenerating the test.support docs. |
|||
| msg245921 - (view) | Author: Christie (bobcatfish) * | Date: 2015年06月28日 22:28 | |
Hey @vadmium! I've finally responded to your review feedback. I've attached a new patch, hopefully this time actually review-able. I've removed the docs for methods I added in @24033 so that this could be merged without it. Let me know if you have any more feedback, or if this could be merged. Thanks! |
|||
| msg245926 - (view) | Author: Martin Panter (martin.panter) * (Python committer) | Date: 2015年06月29日 08:19 | |
I left some more comments on Reitveld. The main one is I think stderr gets stripped with assert_python_ok(), but I don’t think stdout is touched. Correct me if I’m wrong though. |
|||
| msg292116 - (view) | Author: Louie Lu (louielu) * | Date: 2017年04月22日 12:20 | |
The PR on GitHub is based on bobcatfist's patch, addressed on Martin request and some minor change. |
|||
| msg351521 - (view) | Author: Julien Palard (mdk) * (Python committer) | Date: 2019年09月09日 16:28 | |
All those functions has already been documented. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022年04月11日 14:57:48 | admin | set | github: 62776 |
| 2019年09月09日 16:28:15 | mdk | set | status: open -> closed dependencies: - Make test.script_helper more comprehensive, and use it in the test suite nosy: + mdk messages: + msg351521 resolution: out of date stage: patch review -> resolved |
| 2017年04月22日 12:20:37 | louielu | set | nosy:
+ louielu messages: + msg292116 |
| 2017年04月22日 06:18:37 | louielu | set | pull_requests: + pull_request1367 |
| 2017年04月22日 05:15:27 | berker.peksag | link | issue30136 superseder |
| 2015年06月29日 08:19:05 | martin.panter | set | messages: + msg245926 |
| 2015年06月28日 22:28:59 | bobcatfish | set | files:
+ issue18576.patch messages: + msg245921 |
| 2015年06月28日 06:03:43 | ncoghlan | set | assignee: ncoghlan -> |
| 2015年06月07日 18:25:29 | bobcatfish | set | messages: + msg244965 |
| 2015年06月07日 05:45:02 | ncoghlan | set | messages: + msg244943 |
| 2015年06月07日 00:44:53 | martin.panter | set | nosy:
+ martin.panter messages: + msg244936 |
| 2015年06月06日 22:01:33 | bobcatfish | set | files:
+ issue18576.patch messages: + msg244929 |
| 2015年05月18日 19:46:35 | bobcatfish | set | nosy:
+ bobcatfish messages: + msg243528 |
| 2015年05月12日 06:34:27 | ncoghlan | set | messages: + msg242946 |
| 2015年05月12日 06:31:59 | ncoghlan | set | title: Rename and document test.script_helper as test.support.script_helper -> Document test.support.script_helper dependencies: + Make test.script_helper more comprehensive, and use it in the test suite, - Refactor the test_runpy walk_package support code into a common location messages: + msg242944 versions: + Python 3.5, - Python 3.4 |
| 2013年12月19日 18:59:41 | eric.snow | set | nosy:
+ eric.snow |
| 2013年12月18日 12:35:01 | ncoghlan | set | files:
+ issue18576_move_and_document_script_helper.diff dependencies: + Refactor the test_runpy walk_package support code into a common location messages: + msg206511 |
| 2013年12月09日 22:44:38 | ncoghlan | set | messages: + msg205746 |
| 2013年12月09日 13:20:44 | Fotis.Koutoulakis | set | files:
+ issue18576_third_try.patch messages: + msg205689 |
| 2013年12月09日 12:17:34 | serhiy.storchaka | set | messages:
+ msg205684 stage: needs patch -> patch review |
| 2013年12月09日 11:49:04 | Fotis.Koutoulakis | set | messages: + msg205681 |
| 2013年12月09日 11:44:45 | serhiy.storchaka | set | messages: + msg205680 |
| 2013年12月09日 11:13:13 | Fotis.Koutoulakis | set | files:
+ issue_18576_second.patch messages: + msg205676 |
| 2013年12月09日 10:07:16 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages: + msg205661 |
| 2013年12月09日 09:46:27 | Fotis.Koutoulakis | set | messages: + msg205656 |
| 2013年12月09日 02:04:40 | ncoghlan | link | issue15403 dependencies |
| 2013年12月09日 02:02:04 | ncoghlan | set | assignee: docs@python -> ncoghlan |
| 2013年12月09日 01:57:02 | ncoghlan | set | messages:
+ msg205638 versions: - Python 3.3 |
| 2013年12月09日 01:14:21 | Fotis.Koutoulakis | set | files:
+ script_helper-default.patch nosy: + Fotis.Koutoulakis messages: + msg205635 keywords: + patch |
| 2013年08月11日 19:15:44 | seydou | set | nosy:
+ seydou messages: + msg194908 |
| 2013年08月08日 13:37:21 | ezio.melotti | set | keywords:
+ easy nosy: + ezio.melotti stage: needs patch |
| 2013年07月28日 12:32:44 | ncoghlan | create | |