classification
Title: Make test.script_helper more comprehensive, and use it in the test suite
Type: enhancement Stage: commit review
Components: Tests Versions: Python 3.5
process
Status: open Resolution:
Dependencies: 23981 24033 Superseder:
Assigned To: Nosy List: BreamoreBoy, Rodrigue.Alcazar, berker.peksag, bobcatfish, ezio.melotti, flipmcf, michael.foord, ncoghlan, pitrou, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: easy, patch

Created on 2010-08-04 22:50 by pitrou, last changed 2015-06-28 22:28 by bobcatfish.

Files
File name Uploaded Description Edit
script_helper_del_refcount.patch r.david.murray, 2010-12-08 13:14 review
iss9517_move_script_helpers_py.patch flipmcf, 2015-04-14 18:38 review
iss9517_move_script_helpers_py.patch flipmcf, 2015-04-15 15:17
iss9517_move_script_helpers.patch bobcatfish, 2015-04-23 00:20 review
issue9517.diff berker.peksag, 2015-04-23 04:41
iss9517_move_script_helpers_review1.patch bobcatfish, 2015-04-24 01:01 review
Messages (54)
msg112917 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-04 22:50
test.script_helper has a couple of dedicated functions to launch a Python interpreter instance in a subprocess. Unfortunately, it is little used and most test modules use their own ad hoc calls to subprocess instead.

Remedying the situation would require:
- adding functions to script_helper (currently, the available functions merge stdout and stderr together, which is clearly undesireable)
- perhaps improve the existing functions (kill_python() does a strange dance instead of calling communicate() on the subprocess.Popen object, is there a reason for that?)
- convert most uses of subprocess.<some_func>([sys.executable, ...]) in the test suite to use script_helper instead

This was suggested by Nick in issue477863.
msg112969 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-05 10:35
(The email daemon was not in a happy place, so posting directly)

On Thu, Aug 5, 2010 at 8:50 AM, Antoine Pitrou <report@bugs.python.org> wrote:
> - perhaps improve the existing functions (kill_python() does a strange dance instead of calling communicate() on the subprocess.Popen object, is there a reason for that?)

script_helper just factored out the old test_cmd_line approach which
was in turn based on a minimalistic change from a popen2 based
implementation (see
http://svn.python.org/view/python/trunk/Lib/test/test_cmd_line.py?r1=54386&r2=55245).
Doing something smarter probably isn't a bad idea.
msg113145 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-08-06 23:40
One other feature for the new-and-improved helpers: add a flag to allow "-E" to be omitted (as per the comment in test_cmd_line)
msg119517 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-10-24 14:05
I still think this is a good idea, I'm just not actively working on it. It might make a good project for someone wanting to get to know the process of working on CPython without having to deal with anything that is particularly tricky to understand.
msg119741 - (view) Author: Rodrigue Alcazar (Rodrigue.Alcazar) Date: 2010-10-27 21:37
> someone wanting to get to know the process of working on CPython without having to deal with anything that is particularly tricky to understand.

That sounds exactly like me :)

I can have a look at this ticket.
msg123503 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-06 20:53
I just tried using script_helper in a new test, so I have a couple of comments.

I don't see stdout and stderr being conflated, it looks to me like they are returned separately, at least by the assert methods.

The assert methods return results, which is unlike other assert methods.  This is very useful, even essential, and I wouldn't want to give it up.  That conflicts with the current unittest conventions, though.

It would be a big help if 'err' were returned with the refcount line removed if it is there, which would make tests using the methods return the same 'err' regardless of whether they are run under a debug build or not.

I think the names of the two assert functions should follow the current unit test conventions (assertPythonRunOK and asssertPythonRunNotOK, perhaps?)
msg123504 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-12-06 21:02
> I just tried using script_helper in a new test, so I have a couple of
> comments.
> 
> I don't see stdout and stderr being conflated, it looks to me like
> they are returned separately, at least by the assert methods.

That's because I wrote the assert methods since this issue was opened :)

> It would be a big help if 'err' were returned with the refcount line
> removed if it is there, which would make tests using the methods
> return the same 'err' regardless of whether they are run under a debug
> build or not.

Indeed.

> I think the names of the two assert functions should follow the
> current unit test conventions (assertPythonRunOK and
> asssertPythonRunNotOK, perhaps?)

Well, they are functions, not methods, so I don't think they have to
follow the other convention.
msg123516 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-07 02:51
OK, fine on the convention, but I'd still like a more memorable name for assert_python_failure.  I've been working on this issue off and on today, and I've had to look up that name at least four times.  I can remember assert_python_ok, but I can't remember whether its inverse is assert_python_fails, assert_python_bad, or what.  For some reason I haven't guessed 'failure' even once so far :)  (I know it's not assert_python_not_ok because I remember it isn't parallel...)
msg123601 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-08 13:14
Here is a patch that causes _assert_python to remove the refcount lines from stderr.
msg123602 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-08 13:22
Hmm.  Having posted that it occurs to me that it could be useful to have the _remove_refcount function in test.support as remove_refcount instead.
msg123633 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-12-08 18:54
> Having posted that it occurs to me that it could be useful to have the 
> _remove_refcount function in test.support

There's already strip_python_stderr() :)
msg123637 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-12-08 19:29
Oh, good, I'll use that then.  I could have sworn I looked for that functionality a couple weeks ago and couldn't find it.
msg173718 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-10-24 23:08
> There's already strip_python_stderr() :)

This is already used in Lib/test/script_helper.py:42, and a way to call _assert_python() without using -E has also been introduced[0].

The next step would be to check if these functions could be used elsewhere in the other tests.  Also note that these functions are not currently documented anywhere.

[0]: http://hg.python.org/cpython/diff/f4b7ecf8a5f8/Lib/test/script_helper.py
msg221971 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-30 17:54
@Rodrigue did you ever make any progress with this?
msg222027 - (view) Author: Rodrigue Alcazar (Rodrigue.Alcazar) Date: 2014-07-01 13:30
@Mark, I had a look at the time I commented on the ticket but didn't get a chance to progress much in the end.
msg240305 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-09 02:18
Script helpers has been "made more discoverable" by moving it into the test.support namespace, but it has not been documented in the test.support docs.  I can no longer remember if this is intentional or not.  Regardless, this issue is still valid insofar as there are probably a number of places in the test suite where script_helpers can be used that they aren't being used.  We probably don't want a multi-module changeset, though, so this could become a meta issue for new issues for converting particular test files to use script_helpers.
msg240524 - (view) Author: Christie (bobcatfish) * Date: 2015-04-12 02:49
Hello @r.david.murray!

> We probably don't want a multi-module changeset, though, so this could become a meta issue for new issues for converting particular test files to use script_helpers.

Does this mean you'd like to leave this issue open until all of the test files are updated in separate issues?

Or is it that you'd like separate changesets for each module, but all associated with this issue?

Thanks!
msg240555 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-12 13:49
It could be done either way, but I suspect that indovidual issues for test module changes, entered as dependencies on this issue, is probably the more effective in this case.
msg240664 - (view) Author: Christie (bobcatfish) * Date: 2015-04-13 17:10
Okay thanks @r.david.murray, I'll take a look at doing this. I'll open an issue when I figure out which module to start on.
msg240801 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-13 23:03
Apparently I was imaginig that script_helpers had been moved into test.support.  test.support was changed into a module in prep for doing so, but as far as I can see it was never moved.  So that does remain to be done as well, and should be done first.
msg240818 - (view) Author: Christie (bobcatfish) * Date: 2015-04-14 00:46
Okee dokee, I'll tackle that first!
msg240982 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-14 18:38
I have a patch for moving script_helper as R. David suggested.

Here it is.
msg240990 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-14 19:20
bobcatfish: Sorry, I didn't refresh and see your comment before submitting my patch.   It only moves script_helpers, but doesn't address the original OP.  

Hopefully I made your life easier, not harder.
msg241001 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-14 19:55
re: test.test_tools.py  - should this also move into test.support ?
msg241010 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-14 20:19
test_tools is a test suite that tests the tools in the scripts directory, it is not tools for testing.  So no, it doesn't belong in test.support.
msg241069 - (view) Author: Christie (bobcatfish) * Date: 2015-04-15 01:28
Hey there @flipmcf, is the change which adds `script_helpers` to test.support is missing from your patch?

>>> import test.support.script_helper
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ImportError: No module named 'test.support.script_helper'

I'm thinking also, maybe instead of putting script_helper into the test.support namespace, we could move the individual methods and helpers from script_helper into test.support, e.g.:

test.support.run_python_until_end
vs.
test.support.script_helper.run_python_until_end
msg241101 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-15 14:18
Yes, I did miss that.

(Root cause:  `git diff` instead of `git diff --staging`)

Sorry.  I'm learning, and decided to take the git path - might not have been the best choice.

I'll submit a new patch shortly.
msg241103 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-15 14:28
> I'm thinking also, maybe instead of putting script_helper into the
> test.support namespace, we could move the individual methods and
> helpers from script_helper into test.support, e.g.:
>
> test.support.run_python_until_end
> vs.
> test.support.script_helper.run_python_until_end1

Yes, I also agree that this looks better.

But as this was first humble patch submission to CPython, I followed David Murry's instructions during my review - which was to move the entire module. 

I would be opposed to adding the methods directly into __init__.py, 

I would recommend still moving script_helper into test.support , and adding a list of symbols from script_helper.py into __init__

eg:

test/support/__init__.py:
  from script_helper import run_python_until_end

test/some_test.py:
  from test.support import run_python_until_end

I am currently in the PyCon CPython sprint room if you are also here.
msg241112 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-15 15:17
Uploading new patch that includes the creation of Lib/test/support/script_helper.py
msg241120 - (view) Author: Christie (bobcatfish) * Date: 2015-04-15 15:55
> I would recommend still moving script_helper into test.support , and adding a list of symbols from script_helper.py into __init__

Sounds like a great plan to me @flipmcf!
msg241122 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-15 16:10
I actually originally wanted to do just that (move the script helpers routine into the old support.py) but I was in the minority.  test.support was turned into a package exactly so that script_helpers could become a sub-namespace.  As I remember, it was Nick Coghlan who championed the sub-namespace position :)
msg241165 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-15 21:23
namespaces are a honkin' great idea

I don't have enough information to say that 'script_helper' is the right name for the namespace, tho.
msg241186 - (view) Author: Christie (bobcatfish) * Date: 2015-04-16 01:01
@r.david.murray and @flipmcf:

I've audited all test modules using sys.executable for a starting point, and these tests look like they could benefit from helper methods to invoke python (probably the ones in script_helper, but we may want to create others if the use cases don’t quite fit):
* _test_multiprocessing.py
* test_base64.py
* test_capi.py - some of this looks legitimate, but some of it could maybe benefit from the helpers
* test_cmd_line.py
* test_faulthandler.py
* test_file_eintr.py
* test_gc.py
* test_keyword.py
* test_pdb.py
* test_popen.py
* test_quopri.py
* test_site.py
* test_source_encoding.py
* test_sys.py
* test_sysconfig.py
* test_tcl.py - something complex is going on in here, not sure if it’s necessary or not
* test_threading.py
* test_pindent.py
* test_traceback.py
* test_unicodedata.py
* test_json/test_tool.py

@r.david.murray I'm going to start with test_unicodedata.py for a nice easy starting point so I'll create a separate issue for it.

@flipmcf should I start using your patch set as-is or is there more to come?
msg241221 - (view) Author: Michael McFadden (flipmcf) * Date: 2015-04-16 15:12
@Christie   - Nope.  This patch can stand on it's own, simply moving the package to where it belongs.

In fact, I'd recommend applying the patch first.
msg241238 - (view) Author: Christie (bobcatfish) * Date: 2015-04-16 16:57
So @flipmcf we're sticking with the test.support.script_helper namespace?
msg241240 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-04-16 17:10
Alternatively, you can import public functions from support/script_helper.py in support/__init__.py.

Then you can do

    from test.support import assert_python_ok
msg241244 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-04-16 17:18
Reviewing iss9517_move_script_helpers_py.patch:

* Can you move script_helper.py by using "hg mv"?
msg241295 - (view) Author: Christie (bobcatfish) * Date: 2015-04-17 00:01
@r.david.murray I've created issue23981, I don't think I have permissions to add it as a dependency to this issue.
msg241836 - (view) Author: Christie (bobcatfish) * Date: 2015-04-23 00:20
Hey @berker.peksag, @r.david.murray!

Here's another change set where script_helpers.py is moved with an hg mv (still looks in the patch like the file was deleted and re-added, not sure if that's expected).

I've ALSO removed some unused imports from script_helpers.py - temp_dir was being imported from script_helpers in a few places tho, so I had to fix that. If you guys would rather not include that change here, please let me know!
msg241839 - (view) Author: Christie (bobcatfish) * Date: 2015-04-23 00:27
Created issue24033. If it and issue23981 could be added as dependencies of this issue (or I could get permission to add them?) that would be swell!
msg241842 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-04-23 04:41
Thanks for the patch, Christie. Review comments: http://bugs.python.org/review/9517/

> (still looks in the patch like the file was deleted and re-added, not sure if that's expected).

I don't know if it's important. I'm using a very old version of Mercurial (2.0.2 :)), so it can be related to different Mercurial versions. Attaching a patch for reference.
msg241848 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-23 07:16
You should create the patch with "hg diff --git" to preserve moving. And then apply it with "hg import", not the patch command. Unfortunately Rietveld don't like patches in git format.
msg241906 - (view) Author: Christie (bobcatfish) * Date: 2015-04-24 01:01
Hey @berker.peksag, I've added a new patch in response to your review!

> You should create the patch with "hg diff --git" to preserve moving. And then apply it with "hg import", not the patch command. Unfortunately Rietveld don't like patches in git format.

Ick, I'm guessing it's okay to just use plain old "hg diff" then, if Rietveld doesn't like the patches in git format.
msg241913 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-04-24 05:55
The committer should be careful, and manually make "hg mv" and apply the patch with the patch command, not "hg import". We shouldn't lost the history.
msg242090 - (view) Author: Christie (bobcatfish) * Date: 2015-04-27 01:24
@serhiy.storchaka - just double checking, do you guys need me to make any more changes to the patch?

And is there any more review needed, or is it possible for this to be merged?

Thanks very much!
msg242641 - (view) Author: Roundup Robot (python-dev) Date: 2015-05-06 03:36
New changeset f65174aef9ea by Berker Peksag in branch 'default':
Issue #9517: Move script_helper to the support package.
https://hg.python.org/cpython/rev/f65174aef9ea
msg242690 - (view) Author: Christie (bobcatfish) * Date: 2015-05-06 17:01
Thanks very much @berker.peksag!
msg242934 - (view) Author: Christie (bobcatfish) * Date: 2015-05-12 02:47
Hey @berker.peksag, @r.david.murray, @serhiy.storchaka,

If you get a chance I've got some changes up for review at:
* http://bugs.python.org/issue24033
* http://bugs.python.org/issue23981

Thanks!
msg242945 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-12 06:33
Just noting that issue 18576 has a draft patch for test.support.script_helper documentation (the "move to the support module" part of the current patch there is obsolete)
msg243000 - (view) Author: Christie (bobcatfish) * Date: 2015-05-12 19:41
Cool, thanks @ncoghlan! Would you like someone to take on the work of updating the latest patch on issue 18576?
msg243030 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-05-13 01:46
Sure, that would be great.
msg244001 - (view) Author: Christie (bobcatfish) * Date: 2015-05-24 20:17
Created Issue24279 for updating test_base64.
msg244924 - (view) Author: Christie (bobcatfish) * Date: 2015-06-06 18:14
Created issue24398 for updating test_capi.
msg245920 - (view) Author: Christie (bobcatfish) * Date: 2015-06-28 22:28
Hello all!

So the following are waiting for review:
* #24033
* #23981
* #24279

I'm going to hold off on continuing to refactor any other modules for the moment, would be great to get the above wrapped up and eventually merged when possible.

Thanks!
- Christie
History
Date User Action Args
2015-06-28 22:28:13bobcatfishsetmessages: + msg245920
2015-06-06 18:14:29bobcatfishsetmessages: + msg244924
2015-05-24 20:17:46bobcatfishsetmessages: + msg244001
2015-05-13 01:46:56ncoghlansetmessages: + msg243030
2015-05-12 19:41:37bobcatfishsetmessages: + msg243000
2015-05-12 06:33:32ncoghlansetmessages: + msg242945
2015-05-12 06:31:59ncoghlanlinkissue18576 dependencies
2015-05-12 02:47:15bobcatfishsetmessages: + msg242934
2015-05-06 17:01:19bobcatfishsetmessages: + msg242690
2015-05-06 03:36:54python-devsetnosy: + python-dev
messages: + msg242641
2015-05-03 15:55:19berker.peksagsetstage: patch review -> commit review
2015-04-27 01:24:53bobcatfishsetmessages: + msg242090
2015-04-24 05:55:17serhiy.storchakasetmessages: + msg241913
2015-04-24 01:01:14bobcatfishsetfiles: + iss9517_move_script_helpers_review1.patch

messages: + msg241906
2015-04-23 07:16:09serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg241848
2015-04-23 04:41:04berker.peksagsetfiles: + issue9517.diff

dependencies: + Update test_unicodedata.py to use script_helpers, Update _test_multiprocessing.py to use script helpers
messages: + msg241842
2015-04-23 00:27:59bobcatfishsetmessages: + msg241839
2015-04-23 00:21:12bobcatfishsetfiles: + iss9517_move_script_helpers.patch

messages: + msg241836
2015-04-17 00:01:58bobcatfishsetmessages: + msg241295
2015-04-16 17:18:06berker.peksagsetmessages: + msg241244
2015-04-16 17:10:26berker.peksagsetversions: + Python 3.5, - Python 3.4
nosy: + berker.peksag

messages: + msg241240

stage: needs patch -> patch review
2015-04-16 16:57:44bobcatfishsetmessages: + msg241238
2015-04-16 15:12:06flipmcfsetmessages: + msg241221
2015-04-16 01:01:43bobcatfishsetmessages: + msg241186
2015-04-15 21:23:33flipmcfsetmessages: + msg241165
2015-04-15 16:10:05r.david.murraysetmessages: + msg241122
2015-04-15 15:55:56bobcatfishsetmessages: + msg241120
2015-04-15 15:17:54flipmcfsetfiles: + iss9517_move_script_helpers_py.patch

messages: + msg241112
2015-04-15 14:28:49flipmcfsetmessages: + msg241103
2015-04-15 14:18:41flipmcfsetmessages: + msg241101
2015-04-15 01:28:32bobcatfishsetmessages: + msg241069
2015-04-14 20:19:38r.david.murraysetmessages: + msg241010
2015-04-14 19:55:13flipmcfsetmessages: + msg241001
2015-04-14 19:20:26flipmcfsetmessages: + msg240990
2015-04-14 18:38:50flipmcfsetfiles: + iss9517_move_script_helpers_py.patch
nosy: + flipmcf
messages: + msg240982

2015-04-14 00:46:08bobcatfishsetmessages: + msg240818
2015-04-13 23:03:56r.david.murraysetmessages: + msg240801
2015-04-13 17:10:20bobcatfishsetmessages: + msg240664
2015-04-12 13:49:25r.david.murraysetmessages: + msg240555
2015-04-12 02:49:44bobcatfishsetnosy: + bobcatfish
messages: + msg240524
2015-04-09 02:18:18r.david.murraysetmessages: + msg240305
2014-07-01 13:30:01Rodrigue.Alcazarsetmessages: + msg222027
2014-06-30 17:54:14BreamoreBoysetnosy: + BreamoreBoy
messages: + msg221971
2012-10-24 23:08:07ezio.melottisetstage: needs patch
messages: + msg173718
versions: + Python 3.4, - Python 3.2
2010-12-08 19:29:29r.david.murraysetmessages: + msg123637
2010-12-08 18:54:59pitrousetmessages: + msg123633
2010-12-08 13:22:55r.david.murraysetmessages: + msg123602
2010-12-08 13:14:52r.david.murraysetfiles: + script_helper_del_refcount.patch
keywords: + patch
messages: + msg123601
2010-12-07 02:51:30r.david.murraysetmessages: + msg123516
2010-12-06 21:02:58pitrousetmessages: + msg123504
2010-12-06 20:59:14r.david.murraysetnosy: + michael.foord
2010-12-06 20:53:16r.david.murraysetnosy: + r.david.murray
messages: + msg123503
2010-10-27 21:37:25Rodrigue.Alcazarsetnosy: + Rodrigue.Alcazar
messages: + msg119741
2010-10-24 14:05:04ncoghlansetkeywords: + easy
assignee: ncoghlan ->
messages: + msg119517
2010-08-06 23:40:50ncoghlansetassignee: ncoghlan
messages: + msg113145
2010-08-05 10:35:16ncoghlansetmessages: + msg112969
2010-08-04 22:50:46pitroucreate