From c6539e1e315ef9b6d2706dd775dd855b0362e719 Mon Sep 17 00:00:00 2001 From: Ricardo Boni Date: Tue, 1 Sep 2026 21:26:15 -0400 Subject: [PATCH 1/3] ci(tests): run the test suite on windows-latest Windows is where the installer's pip path fails silently, and CI has never run a single test there. The suite could not run on Windows at all: the autouse fixture in conftest patches os.killpg and os.getpgid, neither of which exists on Windows, so all 255 tests errored before executing. signal.SIGKILL is missing there too, and the tests that force the POSIX branch reach it through os.killpg(..., signal.SIGKILL). Five tests were platform-dependent by accident rather than by intent, and now pin the platform they actually test: - test_pip_delete_removes_installer_local_uv created bin/uv, but resolve_uv_path looks for uv.exe on Windows, so the test would have exercised the "uv not found" branch instead. - test_start_testgen_app_happy_path let its `finally` cleanup reach the real stop_app_tree, which on Windows shells out via subprocess.run -- straight through the Popen mock the test had just installed, counting as a second app launch. - The two grace-period message tests assert output gated on supports_graceful_stop(), which is False on Windows. - test_stop_app_tree_no_op_when_proc_already_exited encodes a shortcut Windows deliberately does not take: a console Ctrl+C kills the parent along with the installer, so a dead parent says nothing about its children. The new job is separate rather than a matrix leg on `test`: both legs would upload an artifact named pytest-results, which upload-artifact v4 rejects, and the coverage comment should come from one leg only. tests/installer.py is a symlink, which a Windows checkout may materialize as a plain text file holding the target path; the job copies over it when that is what it got. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/pull_request.yml | 14 ++++++++++++++ tests/conftest.py | 16 ++++++++++++++-- tests/test_tg_pip_delete.py | 5 ++++- tests/test_tg_start.py | 17 ++++++++++++++++- 4 files changed, 48 insertions(+), 4 deletions(-) diff --git a/.github/workflows/pull_request.yml b/.github/workflows/pull_request.yml index f167521..90a1464 100644 --- a/.github/workflows/pull_request.yml +++ b/.github/workflows/pull_request.yml @@ -29,6 +29,20 @@ jobs: artifact_path: reports/ secrets: inherit + test-windows: + name: Run tests on Windows + uses: ./.github/workflows/python-job.yml + with: + runner: windows-latest + run: | + # tests/installer.py is a symlink to ../dk-installer.py. A Windows checkout + # materializes it as a plain file holding the target path unless core.symlinks is + # enabled, which turns `from tests.installer import ...` into a SyntaxError. Copy + # over it when that is what we got; a real symlink is left alone. + if [ ! -L tests/installer.py ]; then cp dk-installer.py tests/installer.py; fi + pytest + secrets: inherit + report-coverage: name: Report tests coverage runs-on: ubuntu-latest diff --git a/tests/conftest.py b/tests/conftest.py index 41b737a..6846d0a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ import json +import signal from argparse import Namespace from contextlib import contextmanager from pathlib import Path @@ -9,6 +10,14 @@ from tests.installer import CONSOLE, Action, TESTGEN_DEFAULT_IMAGE +# Windows has no SIGKILL, but the tests that force the POSIX branch of ``stop_app_tree`` +# and ``stop_standalone_orphans`` still reach ``signal.SIGKILL`` through +# ``os.killpg(..., signal.SIGKILL)``. The value never leaves the process -- every kill on +# that branch is mocked, and the assertions compare against this same object -- so simply +# defining it lets the whole suite run on a Windows runner. 9 keeps log output recognizable. +if not hasattr(signal, "SIGKILL"): + signal.SIGKILL = 9 + @pytest.fixture(autouse=True) def _no_real_browser_launch(): @@ -25,10 +34,13 @@ def _no_real_process_group_signals(): pgid 1 (init) and ``os.killpg(1, SIGTERM)`` from a root container actually signals init → CI runner shutdown. Tests that need to assert on these explicitly override the patches inside their own ``with patch(...)``. + + ``create=True`` because neither function exists on Windows: without it this autouse + fixture errors out every test in the suite before it starts. """ with ( - patch("tests.installer.os.killpg") as killpg_mock, - patch("tests.installer.os.getpgid", return_value=99999), + patch("tests.installer.os.killpg", create=True) as killpg_mock, + patch("tests.installer.os.getpgid", return_value=99999, create=True), ): yield killpg_mock diff --git a/tests/test_tg_pip_delete.py b/tests/test_tg_pip_delete.py index f44e85d..ab7f0bf 100644 --- a/tests/test_tg_pip_delete.py +++ b/tests/test_tg_pip_delete.py @@ -1,3 +1,4 @@ +import platform from functools import partial from pathlib import Path from unittest.mock import MagicMock, patch @@ -72,7 +73,9 @@ def test_pip_delete_removes_uv_tool_and_home(pip_delete_action, start_cmd_mock, def test_pip_delete_removes_installer_local_uv(pip_delete_action, start_cmd_mock, tmp_data_folder, tmp_path): local_bin = Path(tmp_data_folder) / "bin" local_bin.mkdir() - local_uv = local_bin / "uv" + # ``resolve_uv_path`` looks for uv.exe on Windows: create the name this platform's + # installer will actually look for, so the test exercises the real branch on both. + local_uv = local_bin / ("uv.exe" if platform.system() == "Windows" else "uv") local_uv.write_bytes(b"#!/bin/sh\n") fake_tg_home = tmp_path / ".testgen" diff --git a/tests/test_tg_start.py b/tests/test_tg_start.py index 90795d7..011ee14 100644 --- a/tests/test_tg_start.py +++ b/tests/test_tg_start.py @@ -55,6 +55,10 @@ def test_start_testgen_app_happy_path(app_action, args_mock, proc_running_then_s patch("tests.installer.resolve_testgen_path", return_value="/bin/testgen"), patch("tests.installer.subprocess.Popen", return_value=proc_running_then_stops) as popen_mock, patch("tests.installer.wait_for_tcp_port", return_value=True) as port_mock, + # The ``finally`` cleanup is not this test's subject, and on Windows it shells out + # via ``subprocess.run`` -- which goes through the Popen patched right above and + # would count as a second app launch. + patch("tests.installer.stop_app_tree"), ): start_testgen_app(app_action, args_mock) @@ -120,6 +124,9 @@ def test_start_testgen_app_handles_keyboard_interrupt(app_action, args_mock, con patch("tests.installer.resolve_testgen_path", return_value="/bin/testgen"), patch("tests.installer.subprocess.Popen", return_value=proc), patch("tests.installer.wait_for_tcp_port", return_value=True), + # Pinned: the grace-period messages below are POSIX-only behaviour, and the + # Windows counterpart is test_start_testgen_app_makes_no_grace_promise_on_windows. + patch("tests.installer.supports_graceful_stop", return_value=True), patch("tests.installer.stop_app_tree", return_value=True) as stop_mock, ): start_testgen_app(app_action, args_mock) @@ -148,6 +155,8 @@ def test_start_testgen_app_warns_when_grace_period_expires(app_action, args_mock patch("tests.installer.resolve_testgen_path", return_value="/bin/testgen"), patch("tests.installer.subprocess.Popen", return_value=proc), patch("tests.installer.wait_for_tcp_port", return_value=True), + # Pinned: Windows never promises a grace period, so it never warns one expired. + patch("tests.installer.supports_graceful_stop", return_value=True), patch("tests.installer.stop_app_tree", return_value=False), ): start_testgen_app(app_action, args_mock) @@ -185,10 +194,16 @@ def test_start_testgen_app_makes_no_grace_promise_on_windows(app_action, args_mo @pytest.mark.unit def test_stop_app_tree_no_op_when_proc_already_exited(): + """POSIX only. Windows deliberately does not take this shortcut: a console Ctrl+C + kills the parent along with the installer, so a dead parent there says nothing about + its children -- see test_stop_app_tree_windows_uses_taskkill_tree.""" proc = MagicMock() proc.poll.return_value = 0 # already exited - with patch("tests.installer.subprocess.run") as run_mock: + with ( + patch("tests.installer.supports_graceful_stop", return_value=True), + patch("tests.installer.subprocess.run") as run_mock, + ): stop_app_tree(proc) run_mock.assert_not_called() From 76f78b9662d48ad9ca94de8cc63db840ed0975b7 Mon Sep 17 00:00:00 2001 From: Ricardo Boni Date: Tue, 1 Sep 2026 21:31:42 -0400 Subject: [PATCH 2/3] fix(installer): decode command output as utf-8, not the locale encoding `stream_iterator` wrapped its buffer in a TextIOWrapper without an encoding, so it decoded with `locale.getpreferredencoding()` -- cp1252 on US Windows. Every non-ASCII byte a command writes came out mojibake (docker compose and uv both draw progress with box-drawing characters and check marks), and the partial-character handling was silently defeated: cp1252 maps almost any byte to something, so an incomplete utf-8 sequence decoded to garbage instead of raising UnicodeDecodeError and waiting for the rest. Caught by the new Windows job. test_stream_iterator covers it, but only fails where the locale encoding is not utf-8 -- reproducible on any host with LC_ALL=en_NZ.ISO8859-1. Two test artefacts on the same run: test_obs_existing_install_abort and test_tg_existing_install_abort interpolated a raw path into a JSON string, which on Windows carries backslashes -- invalid escapes, so the payload failed to parse, the step found no existing install and never aborted. Both now build the payload through json.dumps, as `docker compose ls --format json` does. Co-Authored-By: Claude Opus 5 (1M context) --- dk-installer.py | 7 ++++++- tests/test_obs_install.py | 7 ++++++- tests/test_tg_install.py | 7 ++++++- 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/dk-installer.py b/dk-installer.py index f04768f..53c5782 100755 --- a/dk-installer.py +++ b/dk-installer.py @@ -343,7 +343,12 @@ def stream_iterator(proc: subprocess.Popen, stream_name: str, file_path: pathlib "stdout": (0, "output"), "stderr": (1, "stderr"), }[stream_name] - buffer = io.TextIOWrapper(io.BytesIO()) + # Explicit utf-8: the default is ``locale.getpreferredencoding()``, which is cp1252 on + # US Windows. That mangles every non-ASCII byte a command writes (docker compose and uv + # both draw progress with box-drawing and check marks) and, worse, silently defeats the + # partial-character handling below -- cp1252 maps almost any byte to *something*, so an + # incomplete utf-8 sequence decodes to garbage instead of raising UnicodeDecodeError. + buffer = io.TextIOWrapper(io.BytesIO(), encoding="utf-8") def _iter(): proc_exited = False diff --git a/tests/test_obs_install.py b/tests/test_obs_install.py index 604e7ea..99ac176 100644 --- a/tests/test_obs_install.py +++ b/tests/test_obs_install.py @@ -1,3 +1,4 @@ +import json from functools import partial from itertools import count from pathlib import Path @@ -65,8 +66,12 @@ def _stdout_side_effect(): @pytest.mark.integration def test_obs_existing_install_abort(obs_install_action, compose_path, stdout_mock): + # Built with json.dumps rather than interpolated: on Windows compose_path contains + # backslashes, which are invalid escapes in a raw JSON string -- the payload would fail + # to parse and the step would find no existing install. Real `docker compose ls + # --format json` escapes them the same way. stdout_mock.side_effect = [ - [f'[{{"Name":"test-project","Status":"running(4)","ConfigFiles":"{compose_path}"}}]'], + [json.dumps([{"Name": "test-project", "Status": "running(4)", "ConfigFiles": str(compose_path)}])], [], ] with patch.object(obs_install_action, "steps", new=[ComposeVerifyExistingInstallStep]): diff --git a/tests/test_tg_install.py b/tests/test_tg_install.py index 85228d8..ccd682c 100644 --- a/tests/test_tg_install.py +++ b/tests/test_tg_install.py @@ -1,3 +1,4 @@ +import json from functools import partial from pathlib import Path from unittest.mock import call, patch @@ -65,8 +66,12 @@ def test_tg_install(tg_install_action, start_cmd_mock, stdout_mock, tmp_data_fol ids=("container", "volume"), ) def test_tg_existing_install_abort(stdout_effect, tg_install_action, stdout_mock, compose_path): + # json.dumps, minus its quotes, to get the path escaped as it appears inside a JSON + # string: on Windows it contains backslashes, which are invalid escapes raw, so the + # payload would fail to parse and the step would find no existing install. + escaped_compose_path = json.dumps(str(compose_path))[1:-1] stdout_mock.side_effect = [ - [line.replace("", str(compose_path)) for line in output] for output in stdout_effect + [line.replace("", escaped_compose_path) for line in output] for output in stdout_effect ] compose_path.touch() From aa28f35d8a97ec2a41b65978f4042d126525ab91 Mon Sep 17 00:00:00 2001 From: Ricardo Boni Date: Wed, 2 Sep 2026 16:59:28 -0400 Subject: [PATCH 3/3] refactor(tests): skip the POSIX stop tests on Windows, don't shim the stdlib Seven tests in test_tg_start.py patch os.killpg themselves to assert on the POSIX stop path. Defining signal.SIGKILL and passing create=True made them run on Windows, but only indirectly: the autouse fixture's create=True synthesized the attribute so their own inner patches had something to find. Load-bearing in a way nothing in conftest suggested. They also gain nothing there. supports_graceful_stop() keeps the installer off that branch on Windows entirely, so the tests exercise code that cannot run on the host. And a synthetic signal.SIGKILL is an active trap: the natural refactor of supports_graceful_stop() is a capability check, which the shim would make report True on Windows -- sending the Windows tests quietly down the POSIX branch. So: no stdlib shim, no create=True, the guard fixture steps aside where there are no process groups to guard, and those seven are marked posix_only. Windows runs 248 of 255. The remaining POSIX-pinned tests still run there: they mock stop_app_tree or supports_graceful_stop and never reach a POSIX API, and they do carry signal on Windows -- console output goes through the same encoding path that the utf-8 fix in 76f78b9 was about. Also drops the comments explaining the json.dumps payloads. That is how they should have been written in the first place, so there is nothing to explain; the tg parametrize now builds each payload from the path instead of string-substituting into pre-escaped JSON. Co-Authored-By: Claude Opus 5 (1M context) --- tests/conftest.py | 21 ++++++++------------- tests/test_obs_install.py | 4 ---- tests/test_tg_install.py | 22 ++++++++++------------ tests/test_tg_start.py | 16 ++++++++++++++++ 4 files changed, 34 insertions(+), 29 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 6846d0a..85486c3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,5 @@ import json -import signal +import os from argparse import Namespace from contextlib import contextmanager from pathlib import Path @@ -10,14 +10,6 @@ from tests.installer import CONSOLE, Action, TESTGEN_DEFAULT_IMAGE -# Windows has no SIGKILL, but the tests that force the POSIX branch of ``stop_app_tree`` -# and ``stop_standalone_orphans`` still reach ``signal.SIGKILL`` through -# ``os.killpg(..., signal.SIGKILL)``. The value never leaves the process -- every kill on -# that branch is mocked, and the assertions compare against this same object -- so simply -# defining it lets the whole suite run on a Windows runner. 9 keeps log output recognizable. -if not hasattr(signal, "SIGKILL"): - signal.SIGKILL = 9 - @pytest.fixture(autouse=True) def _no_real_browser_launch(): @@ -35,12 +27,15 @@ def _no_real_process_group_signals(): signals init → CI runner shutdown. Tests that need to assert on these explicitly override the patches inside their own ``with patch(...)``. - ``create=True`` because neither function exists on Windows: without it this autouse - fixture errors out every test in the suite before it starts. + Nothing to guard where process groups do not exist: Windows has no ``os.killpg`` to + call by accident, and the tests that would reach it are skipped there. """ + if not hasattr(os, "killpg"): + yield Mock() + return with ( - patch("tests.installer.os.killpg", create=True) as killpg_mock, - patch("tests.installer.os.getpgid", return_value=99999, create=True), + patch("tests.installer.os.killpg") as killpg_mock, + patch("tests.installer.os.getpgid", return_value=99999), ): yield killpg_mock diff --git a/tests/test_obs_install.py b/tests/test_obs_install.py index 99ac176..a717359 100644 --- a/tests/test_obs_install.py +++ b/tests/test_obs_install.py @@ -66,10 +66,6 @@ def _stdout_side_effect(): @pytest.mark.integration def test_obs_existing_install_abort(obs_install_action, compose_path, stdout_mock): - # Built with json.dumps rather than interpolated: on Windows compose_path contains - # backslashes, which are invalid escapes in a raw JSON string -- the payload would fail - # to parse and the step would find no existing install. Real `docker compose ls - # --format json` escapes them the same way. stdout_mock.side_effect = [ [json.dumps([{"Name": "test-project", "Status": "running(4)", "ConfigFiles": str(compose_path)}])], [], diff --git a/tests/test_tg_install.py b/tests/test_tg_install.py index ccd682c..d4d9b87 100644 --- a/tests/test_tg_install.py +++ b/tests/test_tg_install.py @@ -51,28 +51,26 @@ def test_tg_install(tg_install_action, start_cmd_mock, stdout_mock, tmp_data_fol assert Path(tmp_data_folder).joinpath("dk-tg-credentials.txt").stat().st_size > 0 marker = Path(tmp_data_folder).joinpath("dk-tg-install.json") assert marker.exists() - import json as _json - - assert _json.loads(marker.read_text())["install_mode"] == "docker" + assert json.loads(marker.read_text())["install_mode"] == "docker" @pytest.mark.integration @pytest.mark.parametrize( "stdout_effect", ( - [['[{"Name":"test-project","Status":"running(2)","ConfigFiles":""}]'], []], - [[], ['{"Labels":"com.docker.compose.project=test-project,", "Status":"N/A"}']], + lambda compose_path: [ + [json.dumps([{"Name": "test-project", "Status": "running(2)", "ConfigFiles": str(compose_path)}])], + [], + ], + lambda compose_path: [ + [], + [json.dumps({"Labels": "com.docker.compose.project=test-project,", "Status": "N/A"})], + ], ), ids=("container", "volume"), ) def test_tg_existing_install_abort(stdout_effect, tg_install_action, stdout_mock, compose_path): - # json.dumps, minus its quotes, to get the path escaped as it appears inside a JSON - # string: on Windows it contains backslashes, which are invalid escapes raw, so the - # payload would fail to parse and the step would find no existing install. - escaped_compose_path = json.dumps(str(compose_path))[1:-1] - stdout_mock.side_effect = [ - [line.replace("", escaped_compose_path) for line in output] for output in stdout_effect - ] + stdout_mock.side_effect = stdout_effect(compose_path) compose_path.touch() with patch.object(tg_install_action, "steps", new=[ComposeVerifyExistingInstallStep]): diff --git a/tests/test_tg_start.py b/tests/test_tg_start.py index 011ee14..3379a88 100644 --- a/tests/test_tg_start.py +++ b/tests/test_tg_start.py @@ -1,3 +1,4 @@ +import os import signal from pathlib import Path from unittest.mock import MagicMock, patch @@ -18,6 +19,14 @@ ) +# The POSIX stop path signals a process group, which Windows has no equivalent of -- +# ``supports_graceful_stop`` keeps the installer off that branch there entirely. These are +# skipped rather than shimmed: synthesizing ``os.killpg``/``signal.SIGKILL`` would also make +# a future capability-based ``supports_graceful_stop`` report True on Windows and quietly +# send the Windows tests down the wrong branch. +posix_only = pytest.mark.skipif(not hasattr(os, "killpg"), reason="needs POSIX process groups") + + # --- start_testgen_app helper ------------------------------------------------- @@ -230,6 +239,7 @@ def test_stop_app_tree_windows_uses_taskkill_tree(): @pytest.mark.unit +@posix_only def test_stop_app_tree_posix_signals_process_group(): proc = MagicMock() proc.poll.return_value = None @@ -249,6 +259,7 @@ def test_stop_app_tree_posix_signals_process_group(): @pytest.mark.unit +@posix_only def test_stop_app_tree_falls_through_to_kill_on_timeout(): """If SIGTERM doesn't take, escalate to SIGKILL / proc.kill().""" import subprocess as sp @@ -270,6 +281,7 @@ def test_stop_app_tree_falls_through_to_kill_on_timeout(): @pytest.mark.unit +@posix_only def test_stop_app_tree_reports_graceful_stop(): """Exiting within the grace period is the signal the caller uses to decide whether a running job got to checkpoint.""" @@ -287,6 +299,7 @@ def test_stop_app_tree_reports_graceful_stop(): @pytest.mark.unit +@posix_only def test_stop_app_tree_swallows_second_interrupt(): """A second Ctrl+C while we wait means 'stop now'. It must force-kill here rather than escaping to the caller's ``finally``, which would re-signal the tree — TestGen @@ -326,6 +339,7 @@ def test_stop_app_tree_windows_stop_is_always_forced(): @pytest.mark.unit +@posix_only def test_force_kill_sweeps_orphans_outside_the_process_group(): """``run-app all`` starts its ui/scheduler children in their own sessions, so killpg on the parent leaves them holding the port and the data dir — breaking the next @@ -346,6 +360,7 @@ def test_force_kill_sweeps_orphans_outside_the_process_group(): @pytest.mark.unit +@posix_only def test_second_interrupt_still_sweeps_orphans(): """The second-Ctrl+C path force-kills, so it owes the same cleanup.""" proc = MagicMock() @@ -365,6 +380,7 @@ def test_second_interrupt_still_sweeps_orphans(): @pytest.mark.unit +@posix_only def test_graceful_stop_does_not_sweep_orphans(): """A tree that stopped on its own has nothing left behind — don't reach for pkill.""" proc = MagicMock()