From 7c5f178976bfbdb0143b2299573275c4b8366891 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Fri, 15 Nov 2024 11:23:03 +0300 Subject: [PATCH 1/8] gh-109413: Enable `strict_optional = true` for `libregrtest/run_workers` --- Lib/test/libregrtest/mypy.ini | 4 --- Lib/test/libregrtest/run_workers.py | 48 ++++++++++++++++++++++------- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/Lib/test/libregrtest/mypy.ini b/Lib/test/libregrtest/mypy.ini index da75a27158a600..905341cc04b8f1 100644 --- a/Lib/test/libregrtest/mypy.ini +++ b/Lib/test/libregrtest/mypy.ini @@ -22,10 +22,6 @@ disallow_untyped_defs = False check_untyped_defs = False warn_return_any = False -# Enable --strict-optional for these ASAP: -[mypy-Lib.test.libregrtest.run_workers.*] -strict_optional = False - # Various internal modules that typeshed deliberately doesn't have stubs for: [mypy-_abc.*,_opcode.*,_overlapped.*,_testcapi.*,_testinternalcapi.*,test.*] ignore_missing_imports = True diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index dcc817ae9aceb6..ef9f84e8863665 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -111,12 +111,28 @@ def __init__(self, worker_id: int, runner: "RunWorkers") -> None: self.output = runner.output self.timeout = runner.worker_timeout self.log = runner.log - self.test_name: TestName | None = None - self.start_time: float | None = None + self._test_name: TestName | None = None + self._start_time: float | None = None self._popen: subprocess.Popen[str] | None = None self._killed = False self._stopped = False + @property + def test_name(self) -> TestName: + if self._test_name is None: + raise ValueError( + 'Should never call `.test_name` before calling `.run()`' + ) + return self._test_name + + @property + def start_time(self) -> float: + if self._start_time is None: + raise ValueError( + 'Should never call `.start_time` before calling `.run()`' + ) + return self._start_time + def __repr__(self) -> str: info = [f'WorkerThread #{self.worker_id}'] if self.is_alive(): @@ -129,7 +145,7 @@ def __repr__(self) -> str: popen = self._popen if popen is not None: dt = time.monotonic() - self.start_time - info.extend((f'pid={self._popen.pid}', + info.extend((f'pid={popen.pid}', f'time={format_duration(dt)}')) return '<%s>' % ' '.join(info) @@ -394,14 +410,14 @@ def run(self) -> None: except StopIteration: break - self.start_time = time.monotonic() - self.test_name = test_name + self._start_time = time.monotonic() + self._test_name = test_name try: mp_result = self._runtest(test_name) except WorkerError as exc: mp_result = exc.mp_result finally: - self.test_name = None + self._test_name = None mp_result.result.duration = time.monotonic() - self.start_time self.output.put((False, mp_result)) @@ -416,6 +432,8 @@ def run(self) -> None: def _wait_completed(self) -> None: popen = self._popen + # only needed for mypy: + assert popen is not None, "Should never call `._popen` before `.run()`" try: popen.wait(WAIT_COMPLETED_TIMEOUT) @@ -483,7 +501,7 @@ def __init__(self, num_workers: int, runtests: RunTests, self.worker_timeout: float | None = min(self.timeout * 1.5, self.timeout + 5 * 60) else: self.worker_timeout = None - self.workers: list[WorkerThread] | None = None + self._workers: list[WorkerThread] | None = None jobs = self.runtests.get_jobs() if jobs is not None: @@ -491,9 +509,17 @@ def __init__(self, num_workers: int, runtests: RunTests, # these worker threads would never get anything to do. self.num_workers = min(self.num_workers, jobs) + @property + def workers(self) -> list[WorkerThread]: + if self._workers is None: + raise ValueError( + 'Should never call `.workers` before `.start_workers()`', + ) + return self._workers + def start_workers(self) -> None: - self.workers = [WorkerThread(index, self) - for index in range(1, self.num_workers + 1)] + self._workers = [WorkerThread(index, self) + for index in range(1, self.num_workers + 1)] jobs = self.runtests.get_jobs() if jobs is not None: tests = count(jobs, 'test') @@ -503,7 +529,7 @@ def start_workers(self) -> None: processes = plural(nworkers, "process", "processes") msg = (f"Run {tests} in parallel using " f"{nworkers} worker {processes}") - if self.timeout: + if self.timeout and self.worker_timeout is not None: msg += (" (timeout: %s, worker timeout: %s)" % (format_duration(self.timeout), format_duration(self.worker_timeout))) @@ -555,7 +581,7 @@ def display_result(self, mp_result: MultiprocessResult) -> None: if mp_result.err_msg: # WORKER_BUG text += ' (%s)' % mp_result.err_msg - elif (result.duration >= PROGRESS_MIN_TIME and not pgo): + elif (result.duration and result.duration >= PROGRESS_MIN_TIME and not pgo): text += ' (%s)' % format_duration(result.duration) if not pgo: running = get_running(self.workers) From 21fd256b0e73239a978035625cee1727edd072b9 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Fri, 15 Nov 2024 11:32:20 +0300 Subject: [PATCH 2/8] Address review --- Lib/test/libregrtest/run_workers.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index ef9f84e8863665..1a14177e51ab7d 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -501,7 +501,7 @@ def __init__(self, num_workers: int, runtests: RunTests, self.worker_timeout: float | None = min(self.timeout * 1.5, self.timeout + 5 * 60) else: self.worker_timeout = None - self._workers: list[WorkerThread] | None = None + self.workers: list[WorkerThread] = [] jobs = self.runtests.get_jobs() if jobs is not None: @@ -509,17 +509,9 @@ def __init__(self, num_workers: int, runtests: RunTests, # these worker threads would never get anything to do. self.num_workers = min(self.num_workers, jobs) - @property - def workers(self) -> list[WorkerThread]: - if self._workers is None: - raise ValueError( - 'Should never call `.workers` before `.start_workers()`', - ) - return self._workers - def start_workers(self) -> None: - self._workers = [WorkerThread(index, self) - for index in range(1, self.num_workers + 1)] + self.workers = [WorkerThread(index, self) + for index in range(1, self.num_workers + 1)] jobs = self.runtests.get_jobs() if jobs is not None: tests = count(jobs, 'test') From 304065d9c3f4906ee997e514c9d0cac52c1d5ee7 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Fri, 15 Nov 2024 12:17:36 +0300 Subject: [PATCH 3/8] Address review --- Lib/test/libregrtest/run_workers.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index 1a14177e51ab7d..dff631f09c5abf 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -433,7 +433,8 @@ def run(self) -> None: def _wait_completed(self) -> None: popen = self._popen # only needed for mypy: - assert popen is not None, "Should never call `._popen` before `.run()`" + if popen is None: + raise ValueError("Should never call `._popen` before `.run()`") try: popen.wait(WAIT_COMPLETED_TIMEOUT) From 94754010c91b0c527d6ecda543a59578a9c2cfb7 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Fri, 15 Nov 2024 14:31:04 +0300 Subject: [PATCH 4/8] Fix CI --- Lib/test/libregrtest/run_workers.py | 44 ++++++++++++----------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index dff631f09c5abf..fc2676a148b649 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -111,27 +111,18 @@ def __init__(self, worker_id: int, runner: "RunWorkers") -> None: self.output = runner.output self.timeout = runner.worker_timeout self.log = runner.log - self._test_name: TestName | None = None - self._start_time: float | None = None + self.test_name: TestName | None = None + self.start_time: float | None = None self._popen: subprocess.Popen[str] | None = None self._killed = False self._stopped = False - @property - def test_name(self) -> TestName: - if self._test_name is None: + def current_test_name(self) -> TestName: + if self.test_name is None: raise ValueError( 'Should never call `.test_name` before calling `.run()`' ) - return self._test_name - - @property - def start_time(self) -> float: - if self._start_time is None: - raise ValueError( - 'Should never call `.start_time` before calling `.run()`' - ) - return self._start_time + return self.test_name def __repr__(self) -> str: info = [f'WorkerThread #{self.worker_id}'] @@ -143,7 +134,7 @@ def __repr__(self) -> str: if test: info.append(f'test={test}') popen = self._popen - if popen is not None: + if popen is not None and self.start_time is not None: dt = time.monotonic() - self.start_time info.extend((f'pid={popen.pid}', f'time={format_duration(dt)}')) @@ -327,13 +318,14 @@ def read_stdout(self, stdout_file: TextIO) -> str: except Exception as exc: # gh-101634: Catch UnicodeDecodeError if stdout cannot be # decoded from encoding - raise WorkerError(self.test_name, + raise WorkerError(self.current_test_name(), f"Cannot read process stdout: {exc}", stdout=None, state=State.WORKER_BUG) def read_json(self, json_file: JsonFile, json_tmpfile: TextIO | None, stdout: str) -> tuple[TestResult, str]: + test_name = self.current_test_name() try: if json_tmpfile is not None: json_tmpfile.seek(0) @@ -348,11 +340,11 @@ def read_json(self, json_file: JsonFile, json_tmpfile: TextIO | None, # gh-101634: Catch UnicodeDecodeError if stdout cannot be # decoded from encoding err_msg = f"Failed to read worker process JSON: {exc}" - raise WorkerError(self.test_name, err_msg, stdout, + raise WorkerError(test_name, err_msg, stdout, state=State.WORKER_BUG) if not worker_json: - raise WorkerError(self.test_name, "empty JSON", stdout, + raise WorkerError(test_name, "empty JSON", stdout, state=State.WORKER_BUG) try: @@ -361,7 +353,7 @@ def read_json(self, json_file: JsonFile, json_tmpfile: TextIO | None, # gh-101634: Catch UnicodeDecodeError if stdout cannot be # decoded from encoding err_msg = f"Failed to parse worker process JSON: {exc}" - raise WorkerError(self.test_name, err_msg, stdout, + raise WorkerError(test_name, err_msg, stdout, state=State.WORKER_BUG) return (result, stdout) @@ -379,14 +371,14 @@ def _runtest(self, test_name: TestName) -> MultiprocessResult: stdout = self.read_stdout(stdout_file) if retcode is None: - raise WorkerError(self.test_name, stdout=stdout, + raise WorkerError(test_name, stdout=stdout, err_msg=None, state=State.TIMEOUT) if retcode != 0: name = support.get_signal_name(retcode) if name: retcode = f"{retcode} ({name})" - raise WorkerError(self.test_name, f"Exit code {retcode}", stdout, + raise WorkerError(test_name, f"Exit code {retcode}", stdout, state=State.WORKER_FAILED) result, stdout = self.read_json(json_file, json_tmpfile, stdout) @@ -410,15 +402,15 @@ def run(self) -> None: except StopIteration: break - self._start_time = time.monotonic() - self._test_name = test_name + self.start_time = start_time = time.monotonic() + self.test_name = test_name try: mp_result = self._runtest(test_name) except WorkerError as exc: mp_result = exc.mp_result finally: - self._test_name = None - mp_result.result.duration = time.monotonic() - self.start_time + self.test_name = None + mp_result.result.duration = time.monotonic() - start_time self.output.put((False, mp_result)) if mp_result.result.must_stop(fail_fast, fail_env_changed): @@ -470,7 +462,7 @@ def get_running(workers: list[WorkerThread]) -> str | None: running: list[str] = [] for worker in workers: test_name = worker.test_name - if not test_name: + if not test_name or worker.start_time is None: continue dt = time.monotonic() - worker.start_time if dt >= PROGRESS_MIN_TIME: From a4e10713bbb73f50b97e9c4725abba59a1d9ce6c Mon Sep 17 00:00:00 2001 From: sobolevn Date: Fri, 15 Nov 2024 14:36:33 +0300 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Alex Waygood --- Lib/test/libregrtest/run_workers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index fc2676a148b649..eb202a6b9b5f4d 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -120,7 +120,7 @@ def __init__(self, worker_id: int, runner: "RunWorkers") -> None: def current_test_name(self) -> TestName: if self.test_name is None: raise ValueError( - 'Should never call `.test_name` before calling `.run()`' + 'Should never call `.current_test_name()` before calling `.run()`' ) return self.test_name @@ -426,7 +426,7 @@ def _wait_completed(self) -> None: popen = self._popen # only needed for mypy: if popen is None: - raise ValueError("Should never call `._popen` before `.run()`") + raise ValueError("Should never access `._popen` before calling `.run()`") try: popen.wait(WAIT_COMPLETED_TIMEOUT) From ed9a8e3077018f06093df8dddf93084d473b7472 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Fri, 15 Nov 2024 20:06:13 +0300 Subject: [PATCH 6/8] Address review --- Lib/test/libregrtest/run_workers.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index eb202a6b9b5f4d..8c2645114ef837 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -102,6 +102,9 @@ def __init__(self, super().__init__() +_NOT_RUNNING = "" + + class WorkerThread(threading.Thread): def __init__(self, worker_id: int, runner: "RunWorkers") -> None: super().__init__() @@ -111,19 +114,12 @@ def __init__(self, worker_id: int, runner: "RunWorkers") -> None: self.output = runner.output self.timeout = runner.worker_timeout self.log = runner.log - self.test_name: TestName | None = None - self.start_time: float | None = None + self.test_name = _NOT_RUNNING + self.start_time = 0.0 self._popen: subprocess.Popen[str] | None = None self._killed = False self._stopped = False - def current_test_name(self) -> TestName: - if self.test_name is None: - raise ValueError( - 'Should never call `.current_test_name()` before calling `.run()`' - ) - return self.test_name - def __repr__(self) -> str: info = [f'WorkerThread #{self.worker_id}'] if self.is_alive(): @@ -318,14 +314,14 @@ def read_stdout(self, stdout_file: TextIO) -> str: except Exception as exc: # gh-101634: Catch UnicodeDecodeError if stdout cannot be # decoded from encoding - raise WorkerError(self.current_test_name(), + raise WorkerError(self.test_name, f"Cannot read process stdout: {exc}", stdout=None, state=State.WORKER_BUG) def read_json(self, json_file: JsonFile, json_tmpfile: TextIO | None, stdout: str) -> tuple[TestResult, str]: - test_name = self.current_test_name() + test_name = self.test_name try: if json_tmpfile is not None: json_tmpfile.seek(0) @@ -402,15 +398,15 @@ def run(self) -> None: except StopIteration: break - self.start_time = start_time = time.monotonic() + self.start_time = time.monotonic() self.test_name = test_name try: mp_result = self._runtest(test_name) except WorkerError as exc: mp_result = exc.mp_result finally: - self.test_name = None - mp_result.result.duration = time.monotonic() - start_time + self.test_name = _NOT_RUNNING + mp_result.result.duration = time.monotonic() - self.start_time self.output.put((False, mp_result)) if mp_result.result.must_stop(fail_fast, fail_env_changed): From b2d6a89dbd48bfcd3dcd82223f8e6ddf7b66b69e Mon Sep 17 00:00:00 2001 From: sobolevn Date: Mon, 18 Nov 2024 16:15:57 +0300 Subject: [PATCH 7/8] Address review --- Lib/test/libregrtest/run_workers.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index 8c2645114ef837..71786ecbc1b217 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -115,7 +115,7 @@ def __init__(self, worker_id: int, runner: "RunWorkers") -> None: self.timeout = runner.worker_timeout self.log = runner.log self.test_name = _NOT_RUNNING - self.start_time = 0.0 + self.start_time = time.monotonic() self._popen: subprocess.Popen[str] | None = None self._killed = False self._stopped = False @@ -130,7 +130,7 @@ def __repr__(self) -> str: if test: info.append(f'test={test}') popen = self._popen - if popen is not None and self.start_time is not None: + if popen is not None: dt = time.monotonic() - self.start_time info.extend((f'pid={popen.pid}', f'time={format_duration(dt)}')) @@ -321,7 +321,6 @@ def read_stdout(self, stdout_file: TextIO) -> str: def read_json(self, json_file: JsonFile, json_tmpfile: TextIO | None, stdout: str) -> tuple[TestResult, str]: - test_name = self.test_name try: if json_tmpfile is not None: json_tmpfile.seek(0) @@ -336,11 +335,11 @@ def read_json(self, json_file: JsonFile, json_tmpfile: TextIO | None, # gh-101634: Catch UnicodeDecodeError if stdout cannot be # decoded from encoding err_msg = f"Failed to read worker process JSON: {exc}" - raise WorkerError(test_name, err_msg, stdout, + raise WorkerError(self.test_name, err_msg, stdout, state=State.WORKER_BUG) if not worker_json: - raise WorkerError(test_name, "empty JSON", stdout, + raise WorkerError(self.test_name, "empty JSON", stdout, state=State.WORKER_BUG) try: @@ -349,7 +348,7 @@ def read_json(self, json_file: JsonFile, json_tmpfile: TextIO | None, # gh-101634: Catch UnicodeDecodeError if stdout cannot be # decoded from encoding err_msg = f"Failed to parse worker process JSON: {exc}" - raise WorkerError(test_name, err_msg, stdout, + raise WorkerError(self.test_name, err_msg, stdout, state=State.WORKER_BUG) return (result, stdout) @@ -458,7 +457,7 @@ def get_running(workers: list[WorkerThread]) -> str | None: running: list[str] = [] for worker in workers: test_name = worker.test_name - if not test_name or worker.start_time is None: + if not test_name: continue dt = time.monotonic() - worker.start_time if dt >= PROGRESS_MIN_TIME: From 676e426b20b12a88073b367f2ea0ad25367792bc Mon Sep 17 00:00:00 2001 From: sobolevn Date: Mon, 18 Nov 2024 16:38:24 +0300 Subject: [PATCH 8/8] Address review --- Lib/test/libregrtest/run_workers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/libregrtest/run_workers.py b/Lib/test/libregrtest/run_workers.py index 71786ecbc1b217..0ca86a986ea436 100644 --- a/Lib/test/libregrtest/run_workers.py +++ b/Lib/test/libregrtest/run_workers.py @@ -366,14 +366,14 @@ def _runtest(self, test_name: TestName) -> MultiprocessResult: stdout = self.read_stdout(stdout_file) if retcode is None: - raise WorkerError(test_name, stdout=stdout, + raise WorkerError(self.test_name, stdout=stdout, err_msg=None, state=State.TIMEOUT) if retcode != 0: name = support.get_signal_name(retcode) if name: retcode = f"{retcode} ({name})" - raise WorkerError(test_name, f"Exit code {retcode}", stdout, + raise WorkerError(self.test_name, f"Exit code {retcode}", stdout, state=State.WORKER_FAILED) result, stdout = self.read_json(json_file, json_tmpfile, stdout)