From 01ac999898b9cc01b0871fbf0f52d6593abf5226 Mon Sep 17 00:00:00 2001 From: Adam Gross Date: Tue, 13 Oct 2020 15:13:58 -0400 Subject: Fix occasional test failures when running multiple jobs 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. --- CHANGES.txt | 2 ++ runtest.py | 37 +++++++++++++++++++++++-------------- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index ae754ef..07aa4ca 100755 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -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` diff --git a/runtest.py b/runtest.py index 16c4f0f..f751212 100755 --- a/runtest.py +++ b/runtest.py @@ -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... @@ -310,7 +310,7 @@ else: # 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') @@ -318,7 +318,8 @@ else: cp = subprocess.run(command_args, stdout=tmp_stdout, stderr=tmp_stderr, - shell=False) + shell=False, + env=env) try: # Rewind to start of files @@ -348,11 +349,12 @@ else: # (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 @@ -380,8 +382,8 @@ class RuntestBase(ABC): 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) @@ -400,7 +402,7 @@ 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') @@ -408,7 +410,8 @@ class PopenExecutor(RuntestBase): cp = subprocess.run(self.command_str.split(), stdout=tmp_stdout, stderr=tmp_stderr, - shell=False) + shell=False, + env=env) self.status = cp.returncode try: @@ -423,11 +426,12 @@ class PopenExecutor(RuntestBase): 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 @@ -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) -- cgit v0.12