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/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/conftest.py b/tests/conftest.py index 41b737a..85486c3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,5 @@ import json +import os from argparse import Namespace from contextlib import contextmanager from pathlib import Path @@ -25,7 +26,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(...)``. + + 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") as killpg_mock, patch("tests.installer.os.getpgid", return_value=99999), diff --git a/tests/test_obs_install.py b/tests/test_obs_install.py index 604e7ea..a717359 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 @@ -66,7 +67,7 @@ def _stdout_side_effect(): @pytest.mark.integration def test_obs_existing_install_abort(obs_install_action, compose_path, stdout_mock): 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..d4d9b87 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 @@ -50,24 +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): - stdout_mock.side_effect = [ - [line.replace("", str(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_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..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 ------------------------------------------------- @@ -55,6 +64,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 +133,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 +164,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 +203,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() @@ -215,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 @@ -234,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 @@ -255,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.""" @@ -272,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 @@ -311,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 @@ -331,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() @@ -350,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()