Skip to content

ci(tests): run the test suite on windows-latest - #95

Merged
rboni-dk merged 3 commits into
mainfrom
ci/run-tests-on-windows
Sep 2, 2026
Merged

ci(tests): run the test suite on windows-latest#95
rboni-dk merged 3 commits into
mainfrom
ci/run-tests-on-windows

Conversation

@rboni-dk

@rboni-dk rboni-dk commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

CI has never run a test on Windows, which is where the installer's pip path fails silently. This runs the existing suite on windows-latest for every PR — about 90 seconds, covering the bug classes that need no real install: encodings, path handling, and the platform branches themselves.

The suite could not run there at all. conftest's autouse _no_real_process_group_signals patches os.killpg and os.getpgid, neither of which exists on Windows, so all 255 tests errored before executing. That guard exists to stop a Popen-mocked test from signalling a real process group; where there are no process groups there is nothing to guard, so it now steps aside. Seven tests in test_tg_start.py patch os.killpg themselves to assert on the POSIX stop path — those are marked posix_only and skipped, since supports_graceful_stop() keeps the installer off that branch on Windows entirely. 248 of 255 run there.

Shimming them instead would have been an active trap: the natural refactor of supports_graceful_stop() is a capability check, and a synthetic signal.SIGKILL would make it report True on Windows and send those tests quietly down the POSIX branch.

It found a real defect

stream_iterator wrapped its buffer in a TextIOWrapper with no encoding, so command output was decoded with locale.getpreferredencoding() — cp1252 on US Windows. That mangles the box-drawing characters and check marks docker compose and uv draw progress with, and silently defeats the partial-character handling immediately below it: the loop breaks on UnicodeDecodeError to wait for the rest of an incomplete UTF-8 sequence, but cp1252 maps almost any byte to something, so the sequence decodes to garbage instead of raising.

test_stream_iterator has covered this all along — it had just never run anywhere the locale encoding was not UTF-8. Reproducible on any host:

LC_ALL=en_NZ.ISO8859-1 pytest tests/test_stream_iterator.py

Tests that were platform-dependent by accident

Five were found locally by forcing platform.system() to "Windows", two more by the first real run. Each now pins the platform it actually tests rather than inheriting the host's.

The notable pair is test_obs_existing_install_abort and test_tg_existing_install_abort, which built a JSON payload by interpolating a path. On Windows that path carries backslashes — invalid escapes — so the payload failed to parse, the step found no existing install, and never aborted. Both now build it through json.dumps, as docker compose ls --format json emits it.

test_stop_app_tree_no_op_when_proc_already_exited also gains a docstring: it encodes a shortcut Windows deliberately does not take, so nobody later "fixes" it by making Windows no-op again.

Notes

The 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 file holding the target path — the job copies over it when that is what it got, and leaves a real link alone.

Run tests on Windows needs adding to the required checks before it gates merges.

🤖 Generated with Claude Code

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) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown

@rboni-dk
rboni-dk marked this pull request as draft September 2, 2026 01:28
`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) <noreply@anthropic.com>

@aarthy-dk aarthy-dk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

… 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) <noreply@anthropic.com>
@rboni-dk
rboni-dk marked this pull request as ready for review September 2, 2026 21:57
@rboni-dk
rboni-dk merged commit 8f3cbbf into main Sep 2, 2026
4 checks passed
@rboni-dk
rboni-dk deleted the ci/run-tests-on-windows branch September 2, 2026 21:57
rboni-dk added a commit that referenced this pull request Sep 2, 2026
Brings in the Windows PR job from #95, so this branch's checks exercise
tests/test_smoke_exe_args.py on Windows too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants