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 2019-05-06 17:48 by loganasherjones.

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
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