From 709684968f0cacfaa5b6215e649f20ab4ea27436 Mon Sep 17 00:00:00 2001 From: Brian Egge Date: Fri, 21 Aug 2026 11:24:44 -0400 Subject: [PATCH 1/2] Fix macOS psutil failures, add macOS CI runner. Covered by brianegge.rst Two psutil calls assumed capabilities that macOS does not provide, and both were reachable from production code rather than only from tests. `TestRunner._is_remote_process_alive` calls `psutil.net_connections`, which needs root on macOS and raises `AccessDenied` otherwise. The exception propagated out through `_check_pidfile` / `make_runpath_dirs`, so an unprivileged run would abort outright whenever the runpath held a pidfile in the `{host}:{port};{pid}` form. It is now caught and logged, and the check falls through to the local PID check that already runs when the remote instance cannot be confirmed. The resource monitor asked `Process.as_dict()` for `io_counters`, an attribute psutil simply does not define on macOS; requesting it raises `ValueError: invalid attr name 'io_counters'` and killed the collector subprocess on every poll. The attribute is now only requested where psutil implements it, and the existing `except (AttributeError, KeyError)` path reports the per-process IO metrics as zero elsewhere. The two pidfile tests covering this drove the real system connection table, which is privileged on macOS and depends on host state everywhere else -- `test_check_pidfile_active_remote_process` skipped entirely on an idle machine, including on Linux CI. Both now stub `psutil.net_connections`, so they are deterministic and run on every platform. Adds a macOS job to the PR workflow, following the separate macos-build job in morganstanley/hobbes. It is scoped to `tests/unit` on 3.10 and 3.13: the functional suite needs Zookeeper/Kafka and the built UI bundle, and parts of it depend on the host resolving its own FQDN, which macOS does not do reliably. Verified: unit suite green on macOS 3.10/3.12/3.13/3.14 (1026 passed, was 1024 passed / 2 failed) and on Linux 3.14 (1026 passed, was 1025 passed / 14 skipped -- the formerly skipped test now runs). Resource monitor functional test passes on both macOS and Linux. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GWfe48Svf5j2TtgrryJfBK --- .github/workflows/test_pr.yml | 35 +++++++- dco/brianegge.rst | 81 +++++++++++++++++++ .../3952_changed.macos_psutil_support.rst | 4 + testplan/monitor/resource.py | 27 ++++--- testplan/runnable/base.py | 18 ++++- tests/unit/testplan/runnable/test_base.py | 55 +++++++------ 6 files changed, 184 insertions(+), 36 deletions(-) create mode 100644 dco/brianegge.rst create mode 100644 doc/newsfragments/3952_changed.macos_psutil_support.rst diff --git a/.github/workflows/test_pr.yml b/.github/workflows/test_pr.yml index 38cb4e399..13aeb3779 100644 --- a/.github/workflows/test_pr.yml +++ b/.github/workflows/test_pr.yml @@ -30,7 +30,7 @@ jobs: success: name: PR Test Completed - needs: [is_source_changed, test] + needs: [is_source_changed, test, test_macos] runs-on: ubuntu-22.04 if: always() && needs.is_source_changed.result == 'success' && (needs.is_source_changed.outputs.is_source_changed == 'false' || needs.Test.result == 'success') steps: @@ -164,3 +164,36 @@ jobs: run: uv pip install --system .${{ matrix.package-extras }} --group test -c constraints.txt - name: Test Python run: doit test + + test_macos: + name: Test (macOS) + needs: [lint_python] + runs-on: macos-latest + env: + CI: "" + strategy: + matrix: + python-version: ['3.10', '3.13'] + fail-fast: false + + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + - name: Set up Python + uses: actions/setup-python@a309ff8b426b58ec0e2a45f0f869d46889d02405 # v6.2.0 + with: + python-version: ${{ matrix.python-version }} + - name: Set up uv & Python + uses: astral-sh/setup-uv@37802adc94f370d6bfd71619e3f0bf239e1f3b78 # v7.6.0 + with: + enable-cache: true + cache-dependency-glob: "**/pyproject.toml" + python-version: ${{ matrix.python-version }} + - name: Install Python dependencies + run: uv pip install --system ".[all]" --group test -c constraints.txt + # Unit tests only for now. The functional suite needs Zookeeper/Kafka and + # the built UI bundle, and parts of it additionally depend on the host + # resolving its own FQDN, which is not reliable on macOS. Widening this + # to `doit test` is follow-up work. + - name: Test Python + run: pytest tests/unit --verbose diff --git a/dco/brianegge.rst b/dco/brianegge.rst new file mode 100644 index 000000000..42c82bafe --- /dev/null +++ b/dco/brianegge.rst @@ -0,0 +1,81 @@ +DEVELOPER CERTIFICATE OF ORIGIN +=============================== + +Thank you for your interest in the open source software project(s) (the “Project”) maintained or managed by +Morgan Stanley Services Group Inc. (“us” or “we”). The purpose of this Developer Certificate of Origin (the “DCO” or +“Agreement”) is to define the intellectual property license granted by persons or entities that make Contributions +(defined below) to the Project. You, **Brian Egge**, agree and certify as set forth in this Agreement. You may be +contacted at **brianegge@gmail.com**. + +By submitting a Contribution, including but not limited to by pull request, you agree that you have read and +understood this Agreement and you will be legally bound thereby. In consideration of the opportunity to +participate in the community of Project contributors, you hereby agree to the following terms and conditions in +connection with your present and future Contributions: + +1. **Contributions:** + + * The term “Contribution” means any source code, object code, patch, tool, sample, graphic, specification, manual, + documentation, or any other material submitted by you to the Project. + * A Contribution is “submitted” when any form of electronic, verbal, or written communication is sent to the Project, + including but not limited to communication on electronic mailing lists, source code control systems,and issue + tracking systems that are managed by, or on behalf of, the Project for the purpose of discussing or improving + software or documentation of the Project (“Communication”). + * Each Communication that is conspicuously marked or designated by you in writing as “Not a Contribution” will + not be considered a Contribution. + * Any Contribution submitted by you to the Project will be under the terms and conditions of this Agreement + without any additional terms or conditions. + +2. **Grant of License:** You hereby grant us and our affiliates, for purposes of the Project, and to recipients of +software distributed by the Project: + + * a perpetual, irrevocable, non-exclusive, worldwide, fully paid-up, royalty-free, unrestricted license to + exercise all rights (including sublicensing and commercial exploitation) under all worldwide copyrights, + copyright applications and registrations in the Contribution; and + * a perpetual, irrevocable, non-exclusive, worldwide, full paid-up, royalty-free patent license to make, have + made, use, offer to sell, sell, import, and otherwise transfer your Contribution and derivative works thereof, + where such license applies only to those patent claims licensable by you or your affiliates that are necessarily + infringed by your Contribution alone or by combination of your Contribution with the Project to which you + submitted the Contribution. + +3. **Ownership:** + + * Except as set out above, you keep all right, title and interest in your Contribution. + * You represent that: + + * you are the owner of the Contribution or are otherwise legally entitled to grant the above licenses; + * if any third party, including your employer and/or its affiliates, has rights to any intellectual property + included in your Contribution, then (i) each such third party has provided you written permission to make the + Contribution as specified herein or a written waiver of such rights in and to your + Contribution, **[and (ii) such third party(ies) is/are ]**; + * your Contribution is an original work created by you, and except for third parties who have given permission + to make the Contribution as set forth above, to your knowledge, no other person or entity has claimed, + claims, or has the right to claim any right whatsoever in the Contribution; and + * your Contribution includes complete details of any third party license(s) or other restriction(s) + (including, but not limited to, related copyrights, patents and trademarks) of which you are aware and + which are associated with any part of your Contribution, and of all matters required to be disclosed under + such third party licenses (such as all applicable copyright, patent, trademark and attribution notices, + and all modifications made to certain open source software). + +4. **Notice; Inclusion; No Confidentiality:** + + * You agree to notify us of any facts or circumstances of which you become aware that would make these + representations inaccurate in any respect. Notices and other communications to be sent as directed in the + applicable Project + * Neither we nor the Project is under any obligation to accept and include your Contribution, or to return it to you. + * You will not, absent a separate written agreement signed by us, impose any confidentiality obligations on us, + and we have not undertaken any obligation to treat any Contributions or other information you have or will + give us as confidential or proprietary information. + * You understand and agree that all Contributions including all personal information you submit with it may be + maintained indefinitely and may be redistributed consistent with the applicable open source license(s). + +5. **Effective Date:** The rights that you grant to us under these terms are effective on the date you first +submit a Contribution to us, even if your submission took place before the date you accept the terms of this Agreement. + +6. **Governing Law; Entire Agreement:** This Agreement is governed by the laws of the State of New York, +without regard to its choice of law provisions, and by the laws of the United States. This Agreement sets +forth the entire understanding and agreement between the parties, and supersedes any previous communications, +representations or agreements, whether oral or written, regarding the subject matter herein. No alteration, waiver, +amendment, change or supplement hereto shall be binding or effective unless the same is set forth in writing +signed by both parties. We may freely assign our rights or obligations under this Agreement. + + diff --git a/doc/newsfragments/3952_changed.macos_psutil_support.rst b/doc/newsfragments/3952_changed.macos_psutil_support.rst new file mode 100644 index 000000000..14c7cffec --- /dev/null +++ b/doc/newsfragments/3952_changed.macos_psutil_support.rst @@ -0,0 +1,4 @@ +Fix two macOS-only ``psutil`` failures and run the unit suite on a macOS CI runner. +``TestRunner._is_remote_process_alive`` no longer propagates ``psutil.AccessDenied`` when it +cannot enumerate TCP connections, and the resource monitor only requests ``io_counters`` on +platforms where ``psutil`` implements it. diff --git a/testplan/monitor/resource.py b/testplan/monitor/resource.py index f17496492..9a268f336 100755 --- a/testplan/monitor/resource.py +++ b/testplan/monitor/resource.py @@ -23,6 +23,21 @@ from testplan.runners.pools import communication from testplan.common.serialization.base import serialize, deserialize +# psutil does not implement Process.io_counters on every platform - notably it +# is absent on macOS. Asking as_dict() for an attribute psutil does not define +# raises ValueError, so only request it where it actually exists; the per- +# process IO metrics are then simply reported as zero. +_HAS_PROC_IO_COUNTERS = hasattr(psutil.Process, "io_counters") + +_PROC_ATTRS = [ + "pid", + "name", + "memory_info", + "cpu_percent", + "cmdline", + "create_time", +] + (["io_counters"] if _HAS_PROC_IO_COUNTERS else []) + @dataclasses.dataclass class ResourceData: @@ -250,17 +265,7 @@ def collect_process_data(self) -> None: self.last_process_resource = {} for proc in processes: try: - raw_data = proc.as_dict( - attrs=[ - "pid", - "name", - "memory_info", - "cpu_percent", - "cmdline", - "io_counters", - "create_time", - ] - ) + raw_data = proc.as_dict(attrs=_PROC_ATTRS) except psutil.NoSuchProcess: continue cpu_percent = float(raw_data["cpu_percent"]) diff --git a/testplan/runnable/base.py b/testplan/runnable/base.py index 8f898e900..eefad2e3c 100644 --- a/testplan/runnable/base.py +++ b/testplan/runnable/base.py @@ -1405,7 +1405,23 @@ def _is_remote_process_alive(self, host: str, port: int) -> bool: except ValueError: return False - for conn in psutil.net_connections(kind="tcp"): + try: + connections = psutil.net_connections(kind="tcp") + except psutil.AccessDenied: + # Enumerating system-wide sockets is privileged on some platforms + # (notably macOS, where it requires root). We cannot confirm the + # remote instance either way, so fall through to the local PID + # check rather than blocking the run outright. + self.logger.debug( + "Cannot enumerate TCP connections to check whether a remote " + "testplan instance on %s:%s is still alive - insufficient " + "privileges. Falling back to the local PID check.", + host, + port, + ) + return False + + for conn in connections: if ( conn.status == psutil.CONN_ESTABLISHED and conn.raddr diff --git a/tests/unit/testplan/runnable/test_base.py b/tests/unit/testplan/runnable/test_base.py index 0368aecf8..af6ad43a6 100644 --- a/tests/unit/testplan/runnable/test_base.py +++ b/tests/unit/testplan/runnable/test_base.py @@ -1,6 +1,8 @@ -import pytest +import collections import os + import psutil +import pytest from testplan.base import TestplanMock from testplan.common.report import ReportCategories @@ -16,6 +18,20 @@ from testplan.testing.multitest import MultiTest, suite +# Only the fields _is_remote_process_alive() looks at. Declared here rather +# than reusing psutil's own namedtuple, whose location is a private detail +# that has moved between psutil releases. +_Addr = collections.namedtuple("_Addr", ["ip", "port"]) +_Conn = collections.namedtuple("_Conn", ["status", "raddr"]) + + +def _established_conn(host: str, port: int) -> _Conn: + """Build an ESTABLISHED connection record pointing at host:port.""" + return _Conn( + status=psutil.CONN_ESTABLISHED, raddr=_Addr(ip=host, port=port) + ) + + def test_result_for_failed_task(): task = Task( target="dummy_target", @@ -122,7 +138,7 @@ def test_check_pidfile_another_process(self, tmpdir): ): plan._check_pidfile() - def test_check_pidfile_stale_remote_process(self, tmpdir): + def test_check_pidfile_stale_remote_process(self, tmpdir, mocker): """ Test PID file with stale remote process {host}:{port};{pid} format. @@ -134,17 +150,15 @@ def test_check_pidfile_stale_remote_process(self, tmpdir): plan._runpath = str(tmpdir) plan._pidfile_path = os.path.join(plan._runpath, "testplan.pid") - existing_connections = set() - for conn in psutil.net_connections(kind="tcp"): - if conn.status == psutil.CONN_ESTABLISHED and conn.raddr: - existing_connections.add((conn.raddr.ip, conn.raddr.port)) + # Stub out the connection table rather than reading the real one: + # enumerating system-wide sockets needs root on macOS, and on any + # platform the result depends on whatever the host happens to be + # doing at the time. + mocker.patch.object(psutil, "net_connections", return_value=[]) fake_host = "192.0.2.1" fake_port = 65432 - while (fake_host, fake_port) in existing_connections: - fake_port += 1 - fake_pid = 999999 while psutil.pid_exists(fake_pid): fake_pid += 1 @@ -154,7 +168,7 @@ def test_check_pidfile_stale_remote_process(self, tmpdir): plan._check_pidfile() - def test_check_pidfile_active_remote_process(self, tmpdir): + def test_check_pidfile_active_remote_process(self, tmpdir, mocker): """ Test PID file with active remote process {host}:{port};{pid} format. @@ -167,19 +181,14 @@ def test_check_pidfile_active_remote_process(self, tmpdir): plan._runpath = str(tmpdir) plan._pidfile_path = os.path.join(plan._runpath, "testplan.pid") - active_conn = None - for conn in psutil.net_connections(kind="tcp"): - if ( - conn.status == psutil.CONN_ESTABLISHED - and conn.raddr - and conn.raddr.ip - and conn.raddr.port - ): - active_conn = conn - break - - if active_conn is None: - pytest.skip("No active TCP connection found for testing") + # Present a single ESTABLISHED connection to the address recorded in + # the PID file. Using the real connection table would need root on + # macOS and would skip entirely on an otherwise idle host. + active_host, active_port = "192.0.2.1", 65432 + active_conn = _established_conn(active_host, active_port) + mocker.patch.object( + psutil, "net_connections", return_value=[active_conn] + ) fake_pid = 999999 while psutil.pid_exists(fake_pid): From 2f4e6dcd708b8468fd14d047c620f3612dd1ece3 Mon Sep 17 00:00:00 2001 From: Brian Egge Date: Fri, 21 Aug 2026 12:35:25 -0400 Subject: [PATCH 2/2] Widen driver timing margins for slow runners. Covered by brianegge.rst `TestDriverTiming::test_driver_timings` allowed only 100ms of headroom over the DummyDriver's own sleeps: the teardown sleeps 100ms and the assertion required the recorded interval to be under 200ms, and setup sleeps 200ms against a 300ms ceiling. That is too tight for a loaded GitHub macOS runner. The macOS 3.13 job recorded a 243ms teardown and failed, while every other job in the same run passed, including macOS 3.10 with identical bounds. The test checks that an interval was recorded around the sleep, not how fast the host is, so the upper bounds move to 600ms for setup and 500ms for teardown. The lower bounds are unchanged and still catch a timer that never measured the sleep. This mirrors the widening already done for the driver report tests in #1287 and for test_scheduling_2 in #1295. Verified on macOS 3.10, 3.13 and 3.14: 6 passed in each. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GWfe48Svf5j2TtgrryJfBK --- doc/newsfragments/3954_changed.driver_timing_margin.rst | 2 ++ .../testplan/testing/multitest/driver/test_driver.py | 9 ++++++--- 2 files changed, 8 insertions(+), 3 deletions(-) create mode 100644 doc/newsfragments/3954_changed.driver_timing_margin.rst diff --git a/doc/newsfragments/3954_changed.driver_timing_margin.rst b/doc/newsfragments/3954_changed.driver_timing_margin.rst new file mode 100644 index 000000000..cc8b8f1b8 --- /dev/null +++ b/doc/newsfragments/3954_changed.driver_timing_margin.rst @@ -0,0 +1,2 @@ +Widen the upper bounds in ``TestDriverTiming::test_driver_timings``. The assertions left only +100ms of headroom over the driver's own sleeps, which a slow CI host can exceed. diff --git a/tests/unit/testplan/testing/multitest/driver/test_driver.py b/tests/unit/testplan/testing/multitest/driver/test_driver.py index 158d2a2d5..e041426c9 100644 --- a/tests/unit/testplan/testing/multitest/driver/test_driver.py +++ b/tests/unit/testplan/testing/multitest/driver/test_driver.py @@ -195,9 +195,12 @@ def test_driver_timings(self): assert isinstance( driver.timer[ResourceTimings.RESOURCE_SETUP][0], Interval ) - # there is some UnicodeEncodeError when using pytest.approx + # there is some UnicodeEncodeError when using pytest.approx. + # Upper bounds are loose on purpose: this checks that an interval was + # recorded around the sleep, not how fast the runner is. A busy CI + # host can add a couple of hundred milliseconds of scheduling delay. assert ( - 0.1 < driver.timer[ResourceTimings.RESOURCE_SETUP][0].elapsed < 0.3 + 0.1 < driver.timer[ResourceTimings.RESOURCE_SETUP][0].elapsed < 0.6 ) driver.stop() @@ -208,5 +211,5 @@ def test_driver_timings(self): assert ( 0 < driver.timer[ResourceTimings.RESOURCE_TEARDOWN][0].elapsed - < 0.2 + < 0.5 )