From a3603f0dcc17b95da87b35da7203357b5a5e8f9e Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 13 Jun 2022 17:41:11 +0200 Subject: [PATCH 1/4] gh-93353: regrtest checks for leaked temporary files When running tests with -jN, create a temporary directory per process and mark a test as "environment changed" if a test leaks a temporary file or directory. --- Lib/test/libregrtest/runtest.py | 5 +++++ Lib/test/libregrtest/runtest_mp.py | 34 ++++++++++++++++++++++++------ Lib/test/test_regrtest.py | 20 ++++++++++++++++++ 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/Lib/test/libregrtest/runtest.py b/Lib/test/libregrtest/runtest.py index 62cf1a3f1175ee..def304aaa34caa 100644 --- a/Lib/test/libregrtest/runtest.py +++ b/Lib/test/libregrtest/runtest.py @@ -80,6 +80,11 @@ class EnvChanged(Failed): def __str__(self) -> str: return f"{self.name} failed (env changed)" + @staticmethod + # Convert Passed to EnvChanged + def from_passed(other): + return EnvChanged(other.name, other.duration_sec, other.xml_data) + class RefLeak(Failed): def __str__(self) -> str: diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py index 39fab5566a089a..973f82ae9c143a 100644 --- a/Lib/test/libregrtest/runtest_mp.py +++ b/Lib/test/libregrtest/runtest_mp.py @@ -1,6 +1,6 @@ import faulthandler import json -import os +import os.path import queue import shlex import signal @@ -17,7 +17,8 @@ from test.libregrtest.cmdline import Namespace from test.libregrtest.main import Regrtest from test.libregrtest.runtest import ( - runtest, is_failed, TestResult, Interrupted, Timeout, ChildError, PROGRESS_MIN_TIME) + runtest, is_failed, TestResult, Interrupted, Timeout, ChildError, + PROGRESS_MIN_TIME, Passed, EnvChanged) from test.libregrtest.setup import setup_tests from test.libregrtest.utils import format_duration, print_warning @@ -52,7 +53,7 @@ def parse_worker_args(worker_args) -> tuple[Namespace, str]: return (ns, test_name) -def run_test_in_subprocess(testname: str, ns: Namespace) -> subprocess.Popen: +def run_test_in_subprocess(testname: str, ns: Namespace, tmp_dir: str) -> subprocess.Popen: ns_dict = vars(ns) worker_args = (ns_dict, testname) worker_args = json.dumps(worker_args) @@ -66,10 +67,14 @@ def run_test_in_subprocess(testname: str, ns: Namespace) -> subprocess.Popen: '-m', 'test.regrtest', '--worker-args', worker_args] + env = dict(os.environ) + env['TMPDIR'] = tmp_dir + env['TEMPDIR'] = tmp_dir + # Running the child from the same working directory as regrtest's original # invocation ensures that TEMPDIR for the child is the same when # sysconfig.is_python_build() is true. See issue 15300. - kw = {} + kw = {'env': env} if USE_PROCESS_GROUP: kw['start_new_session'] = True return subprocess.Popen(cmd, @@ -206,12 +211,12 @@ def mp_result_error( test_result.duration_sec = time.monotonic() - self.start_time return MultiprocessResult(test_result, stdout, err_msg) - def _run_process(self, test_name: str) -> tuple[int, str, str]: + def _run_process(self, test_name: str, tmp_dir: str) -> tuple[int, str, str]: self.start_time = time.monotonic() self.current_test_name = test_name try: - popen = run_test_in_subprocess(test_name, self.ns) + popen = run_test_in_subprocess(test_name, self.ns, tmp_dir) self._killed = False self._popen = popen @@ -266,7 +271,14 @@ def _run_process(self, test_name: str) -> tuple[int, str, str]: self.current_test_name = None def _runtest(self, test_name: str) -> MultiprocessResult: - retcode, stdout = self._run_process(test_name) + tmp_dir = os.getcwd() + '_tmpdir' + tmp_dir = os.path.abspath(tmp_dir) + try: + os.mkdir(tmp_dir) + retcode, stdout = self._run_process(test_name, tmp_dir) + finally: + tmp_files = os.listdir(tmp_dir) + os_helper.rmtree(tmp_dir) if retcode is None: return self.mp_result_error(Timeout(test_name), stdout) @@ -289,6 +301,14 @@ def _runtest(self, test_name: str) -> MultiprocessResult: if err_msg is not None: return self.mp_result_error(ChildError(test_name), stdout, err_msg) + if tmp_files: + msg = (f'\n\n' + f'Test leaked temporary files ({len(tmp_files)}): ' + f'{", ".join(sorted(tmp_files))}') + stdout += msg + if isinstance(result, Passed): + result = EnvChanged.from_passed(result) + return MultiprocessResult(result, stdout, err_msg) def run(self) -> None: diff --git a/Lib/test/test_regrtest.py b/Lib/test/test_regrtest.py index 133abf5044d424..4358034ea42ec0 100644 --- a/Lib/test/test_regrtest.py +++ b/Lib/test/test_regrtest.py @@ -1357,6 +1357,26 @@ def test_cleanup(self): for name in names: self.assertFalse(os.path.exists(name), name) + def test_leak_tmp_file(self): + code = textwrap.dedent(r""" + import os.path + import tempfile + import unittest + + class FileTests(unittest.TestCase): + def test_leak_tmp_file(self): + filename = os.path.join(tempfile.gettempdir(), 'mytmpfile') + with open(filename, "wb") as fp: + fp.write(b'content') + """) + testname = self.create_test(code=code) + + output = self.run_tests("--fail-env-changed", "-v", "-j1", testname, exitcode=3) + self.check_executed_tests(output, [testname], + env_changed=[testname], + fail_env_changed=True) + self.assertIn("Test leaked temporary files (1): mytmpfile", output) + class TestUtils(unittest.TestCase): def test_format_duration(self): From d973307070c9b05191864fdee44fca9c0ab81e6d Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 13 Jun 2022 22:12:24 +0200 Subject: [PATCH 2/4] Update comments --- Lib/test/libregrtest/runtest.py | 2 +- Lib/test/libregrtest/runtest_mp.py | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Lib/test/libregrtest/runtest.py b/Lib/test/libregrtest/runtest.py index def304aaa34caa..e9bb72a7d77ee1 100644 --- a/Lib/test/libregrtest/runtest.py +++ b/Lib/test/libregrtest/runtest.py @@ -80,8 +80,8 @@ class EnvChanged(Failed): def __str__(self) -> str: return f"{self.name} failed (env changed)" - @staticmethod # Convert Passed to EnvChanged + @staticmethod def from_passed(other): return EnvChanged(other.name, other.duration_sec, other.xml_data) diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py index 973f82ae9c143a..093377e433fab7 100644 --- a/Lib/test/libregrtest/runtest_mp.py +++ b/Lib/test/libregrtest/runtest_mp.py @@ -271,6 +271,9 @@ def _run_process(self, test_name: str, tmp_dir: str) -> tuple[int, str, str]: self.current_test_name = None def _runtest(self, test_name: str) -> MultiprocessResult: + # gh-93353: Check for leaked temporary files in the parent process, + # since the deletion of temporary files can happen late during + # Python finalization: too late for libregrtest. tmp_dir = os.getcwd() + '_tmpdir' tmp_dir = os.path.abspath(tmp_dir) try: From 1ff55a9c9bf2ffac5e815a2093a3a21097f14f80 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 14 Jun 2022 13:16:43 +0200 Subject: [PATCH 3/4] Use "Warning -- " format for buildbots --- Lib/test/libregrtest/runtest_mp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/libregrtest/runtest_mp.py b/Lib/test/libregrtest/runtest_mp.py index 093377e433fab7..c6190e5d22d503 100644 --- a/Lib/test/libregrtest/runtest_mp.py +++ b/Lib/test/libregrtest/runtest_mp.py @@ -306,7 +306,7 @@ def _runtest(self, test_name: str) -> MultiprocessResult: if tmp_files: msg = (f'\n\n' - f'Test leaked temporary files ({len(tmp_files)}): ' + f'Warning -- Test leaked temporary files ({len(tmp_files)}): ' f'{", ".join(sorted(tmp_files))}') stdout += msg if isinstance(result, Passed): From 916a00f72e9d50590513c39b7fed20d0882aa237 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 14 Jun 2022 13:19:19 +0200 Subject: [PATCH 4/4] Update test_regrtest --- Lib/test/test_regrtest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_regrtest.py b/Lib/test/test_regrtest.py index 4358034ea42ec0..5c072ea331d741 100644 --- a/Lib/test/test_regrtest.py +++ b/Lib/test/test_regrtest.py @@ -1375,7 +1375,7 @@ def test_leak_tmp_file(self): self.check_executed_tests(output, [testname], env_changed=[testname], fail_env_changed=True) - self.assertIn("Test leaked temporary files (1): mytmpfile", output) + self.assertIn("Warning -- Test leaked temporary files (1): mytmpfile", output) class TestUtils(unittest.TestCase):