homepage

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.

classification
Title: Test harness unnecessarily disambiguating twice
Type: enhancement Stage: patch review
Components: Tests Versions: Python 3.4
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: chris.jerdonek, gmwils, loganasherjones, petri.lehtinen
Priority: normal Keywords: easy, patch

Created on 2012年07月09日 11:45 by chris.jerdonek, last changed 2022年04月11日 14:57 by admin.

Files
File name Uploaded Description Edit
Issue15305.diff gmwils, 2013年02月23日 15:44 review
issue15305-2.patch gmwils, 2013年02月24日 13:25 review
issue15305-3.patch chris.jerdonek, 2013年02月27日 18:54 review
Messages (12)
msg165077 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012年07月09日 11:45
It seems like our test harness is disambiguating more than it needs to for parallel testing.
In Lib/test/regrtest.py, we do this--
# Define a writable temp dir that will be used as cwd while running
# the tests. The name of the dir includes the pid to allow parallel
# testing (see the -j option).
TESTCWD = 'test_python_{}'.format(os.getpid())
...
with support.temp_cwd(TESTCWD, quiet=True):
 main()
And then in Lib/test/support.py, we are doing this--
# Disambiguate TESTFN for parallel testing, while letting it remain a valid
# module name.
TESTFN = "{}_{}_tmp".format(TESTFN, os.getpid())
with uses like--
with open(TESTFN, "wb") as f:
 # Do stuff with f.
It seems like only one of these measures should be necessary (a single working directory for all parallel tests using a disambiguated TESTFN, or one working directory for each process with a non-disambiguated TESTFN).
msg165078 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012年07月09日 11:51
The former option seems to make more sense to me (a single working directory for all parallel tests using a disambiguated TESTFN).
msg182752 - (view) Author: Geoff Wilson (gmwils) * Date: 2013年02月23日 15:44
Simplify to single build/test directory, and disambiguate by TEMPFN.
Test suite run on Mac OS X (./python.exe -m test -j3) without error. Some files created by tests do not use TESTFN, so may have build bots identify issues.
msg182775 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2013年02月23日 17:26
Looks good to me, and all tests also pass on my Ubuntu 12.10.
Chris: Would you be willing to commit this and watch the buildbots do their job? Or do the buildbots even use the -j option?
msg182853 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013年02月24日 04:28
I would be happy to commit and watch the buildbots, once I have confidence in the patch though. Question: I noticed that the following was changed in Lib/test/regrtest.py:
- with support.temp_cwd(TESTCWD, quiet=True):
+ with support.temp_cwd(quiet=True, path=TESTCWD):
But the corresponding change wasn't made in Lib/test/__main__.py (which I believe is the code path used by Geoff's `./python.exe -m test -j3` invocation):
http://hg.python.org/cpython/file/96f08a22f562/Lib/test/__main__.py#l12
Those two code chunks should really share code by the way (even the code comment is copied verbatim), which would help in not needing to update code in two places as in this issue/patch. Perhaps that should even be done first as part of a separate issue (to separate this into smaller changes).
msg182855 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013年02月24日 06:00
> Those two code chunks should really share code by the way
I created issue 17283 for this (it's okay to fix the current issue before this one).
msg182876 - (view) Author: Geoff Wilson (gmwils) * Date: 2013年02月24日 13:25
Both are called at different points when running './python.exe -m test -j3':
1. In __main__.py, this is called once at the start of the test run. By putting TESTCWD as the first/name arg, the directory is created.
2. In regrtest.py, the main file is called per test.
The support.temp_cwd command was raising a warning if the directory already existed. I've updated this to no longer raise a warning, although it will still try to create it. Passing it in explicitly as the path= argument would have it not try to create the directory.
msg182900 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013年02月24日 22:19
I don't think support.temp_cwd() should be changed for this issue (or needs to be). Also, changing it in the proposed way could mask errors in the test suite since tests were written against the current behavior.
regrtest.py and __main__.py should both behave the same with respect to creating the temp dir, etc. since both invocations should work from the command-line:
http://hg.python.org/cpython/file/96f08a22f562/Lib/test/regrtest.py#l9 
msg183163 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013年02月27日 17:12
The fix for issue 17283 has been committed now, which should make this slightly easier to fix (e.g. change one place instead of two).
msg183172 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013年02月27日 18:54
Here is a patch that updates Geoff's patch to the latest code, and addresses the directory creation issue.
msg223489 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014年07月19日 23:57
The patch is very short so can we have a formal review please.
msg341577 - (view) Author: Logan Jones (loganasherjones) * Date: 2019年05月06日 17:48
I'm working on this in the PyCon 2019 sprints.
Near as I can tell, while this issue still seems relevant, I think it might actually be for the best that this multiple disambiguation is left in the test suite. 
I removed the pid reference in the TESTFN and the tests passed in both the parallel and sequential cases. However, removing the pid results in multiprocessing tests having to be written more carefully if they choose to use the TESTFN.
Here is an explanation for why you would leave this code in. When running the tests in sequential mode, the tests will run in a CWD that includes the pid. When writing a multi-processing test using the TESTFN you would have to remember to add the os.getpid call to the actual TESTFN instead of it just being included by default.
Ultimately, this is really just extra information that is included in the temporary filenames. It might be worth just closing this issue.
History
Date User Action Args
2022年04月11日 14:57:32adminsetgithub: 59510
2019年05月06日 17:48:39loganasherjonessetnosy: + loganasherjones
messages: + msg341577
2019年04月26日 20:03:47BreamoreBoysetnosy: - BreamoreBoy
2014年07月19日 23:57:52BreamoreBoysetnosy: + BreamoreBoy
messages: + msg223489
2013年02月27日 19:14:06chris.jerdoneksetstage: patch review
type: enhancement
versions: + Python 3.4, - Python 3.3
2013年02月27日 18:54:26chris.jerdoneksetfiles: + issue15305-3.patch

messages: + msg183172
2013年02月27日 17:12:04chris.jerdoneksetmessages: + msg183163
2013年02月24日 22:19:05chris.jerdoneksetmessages: + msg182900
2013年02月24日 13:25:06gmwilssetfiles: + issue15305-2.patch

messages: + msg182876
2013年02月24日 06:00:22chris.jerdoneksetmessages: + msg182855
2013年02月24日 04:28:00chris.jerdoneksetmessages: + msg182853
2013年02月23日 17:26:30petri.lehtinensetnosy: + petri.lehtinen
messages: + msg182775
2013年02月23日 15:44:27gmwilssetfiles: + Issue15305.diff

nosy: + gmwils
messages: + msg182752

keywords: + patch
2012年07月09日 11:51:00chris.jerdoneksetmessages: + msg165078
2012年07月09日 11:45:51chris.jerdonekcreate

AltStyle によって変換されたページ (->オリジナル) /