Skip to content

Commit

Permalink
Fix occasional test failures when running multiple jobs
Browse files Browse the repository at this point in the history
The way runtest.py passes the list of fixture directories is racy because it
sets it in os.environ['FIXTURE_DIRS'] and then spawns the subprocess, counting
on Python to start the subprocess before that list is overwritten when spawning
the next directory. At least on Windows, the environment is not copied in
subprocess.run so runtest.py may overwrite the list of fixture directories
with the list for test #2 while the subprocess module is still kicking off
test #1. I was able to easily reproduce this by running the command:
`python runtest.py -j 2 test\MSVC\VSWHERE.py test\AS\ASPPFLAGS.py`
a few times in a row. However, with this fix, that command repeatedly succeeds.

To validate ths fix, I also ran that command with "--xml a.xml" and
"--xml a.xml --nopipefiles" to validate that those other executors worked
correctly.
  • Loading branch information
grossag committed Oct 13, 2020
1 parent fb03ae9 commit 01ac999
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 14 deletions.
2 changes: 2 additions & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER
From Adam Gross:
- Fix minor bug affecting SCons.Node.FS.File.get_csig()'s usage of the MD5 chunksize.
User-facing behavior does not change with this fix (GH Issue #3726).
- Fix occasional test failures caused by not being able to find a file or directory fixture
when running multiple tests with multiple jobs.

From Joachim Kuebart:
- Suppress missing SConscript deprecation warning if `must_exist=False`
Expand Down
37 changes: 23 additions & 14 deletions runtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,8 @@ def escape(s):
if not catch_output:
# Without any output suppressed, we let the subprocess
# write its stuff freely to stdout/stderr.
def spawn_it(command_args):
cp = subprocess.run(command_args, shell=False)
def spawn_it(command_args, env):
cp = subprocess.run(command_args, shell=False, env=env)
return cp.stdout, cp.stderr, cp.returncode
else:
# Else, we catch the output of both pipes...
Expand All @@ -310,15 +310,16 @@ def spawn_it(command_args):
# http://http://thraxil.org/users/anders/posts/2008/03/13/Subprocess-Hanging-PIPE-is-your-enemy/
# and pass temp file objects to Popen() instead of the ubiquitous
# subprocess.PIPE.
def spawn_it(command_args):
def spawn_it(command_args, env):
# Create temporary files
tmp_stdout = tempfile.TemporaryFile(mode='w+t')
tmp_stderr = tempfile.TemporaryFile(mode='w+t')
# Start subprocess...
cp = subprocess.run(command_args,
stdout=tmp_stdout,
stderr=tmp_stderr,
shell=False)
shell=False,
env=env)

try:
# Rewind to start of files
Expand Down Expand Up @@ -348,11 +349,12 @@ def spawn_it(command_args):
# (but the subprocess isn't writing anything there).
# Hence a deadlock.
# Be dragons here! Better don't use this!
def spawn_it(command_args):
def spawn_it(command_args, env):
cp = subprocess.run(command_args,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
shell=False)
shell=False,
env=env)
return cp.stdout, cp.stderr, cp.returncode


Expand Down Expand Up @@ -380,8 +382,8 @@ def execute(self):

class SystemExecutor(RuntestBase):
""" Test class for tests executed with spawn_it() """
def execute(self):
self.stderr, self.stdout, s = spawn_it(self.command_args)
def execute(self, env):
self.stderr, self.stdout, s = spawn_it(self.command_args, env)
self.status = s
if s < 0 or s > 2:
sys.stdout.write("Unexpected exit status %d\n" % s)
Expand All @@ -400,15 +402,16 @@ class PopenExecutor(RuntestBase):
# and the 'allow_pipe_files' option, please check out the
# definition of spawn_it() above.
if allow_pipe_files:
def execute(self):
def execute(self, env):
# Create temporary files
tmp_stdout = tempfile.TemporaryFile(mode='w+t')
tmp_stderr = tempfile.TemporaryFile(mode='w+t')
# Start subprocess...
cp = subprocess.run(self.command_str.split(),
stdout=tmp_stdout,
stderr=tmp_stderr,
shell=False)
shell=False,
env=env)
self.status = cp.returncode

try:
Expand All @@ -423,11 +426,12 @@ def execute(self):
tmp_stdout.close()
tmp_stderr.close()
else:
def execute(self):
def execute(self, env):
cp = subprocess.run(self.command_str.split(),
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
shell=False)
shell=False,
env=env)
self.status = cp.returncode
self.stdout = cp.stdout
self.stderr = cp.stderr
Expand Down Expand Up @@ -756,11 +760,16 @@ def run_test(t, io_lock=None, run_async=True):
if head:
fixture_dirs.append(head)
fixture_dirs.append(os.path.join(scriptpath, 'test', 'fixture'))
os.environ['FIXTURE_DIRS'] = os.pathsep.join(fixture_dirs)

# Set the list of fixture dirs directly in the environment. Just putting
# it in os.environ and spawning the process is racy. Make it reliable by
# overriding the environment passed to execute().
env = dict(os.environ)
env['FIXTURE_DIRS'] = os.pathsep.join(fixture_dirs)

test_start_time = time_func()
if execute_tests:
t.execute()
t.execute(env)

t.test_time = time_func() - test_start_time
log_result(t, io_lock=io_lock)
Expand Down

0 comments on commit 01ac999

Please sign in to comment.