fix(tests): make Windows unit-test job green#1427
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes Windows CI unit test failures by addressing platform-specific assumptions in tests and fixing a real temp-file FD leak in atomic_write_text.
Changes:
- Fix
atomic_write_textcleanup so temp files aren’t left locked on Windows whenos.fdopenfails. - Make affected tests Windows-compatible via targeted
skipif,patch(..., create=True), and portable path assertions. - Stabilize Rich table rendering assertions by forcing a wider
Consolein color-formatter test helpers.
Show a summary per file
| File | Description |
|---|---|
src/apm_cli/utils/atomic_io.py |
Closes the raw mkstemp fd on error before unlinking, avoiding Windows temp-file lock/leak. |
tests/unit/utils/test_atomic_io.py |
Uses patch(..., create=True) so Windows can patch missing os.fchmod. |
tests/unit/utils/test_paths.py |
Updates absolute-path assertions to be Windows-compatible. |
tests/unit/test_reflink_phase3w5.py |
Skips Linux-only reflink tests on Windows to avoid fcntl import-time failure. |
tests/unit/test_reflink_filesystem_support.py |
Same Windows skip for Linux-only reflink tests. |
tests/unit/test_output_formatters_phase3.py |
Forces wide Rich Console to prevent truncation-driven assertion failures. |
tests/unit/test_output_formatters_rendering.py |
Same console-width forcing for deterministic Rich output in tests. |
Copilot's findings
- Files reviewed: 7/7 changed files
- Comments generated: 3
| # Must be absolute (starts with /) | ||
| assert result.startswith("/") | ||
| # Must be absolute (POSIX leading-slash, or Windows drive like "C:/") | ||
| assert Path(result).is_absolute() or (len(result) >= 2 and result[1] == ":") |
Comment on lines
+113
to
+114
| # Falls back to resolved absolute posix (POSIX leading-slash or Windows drive) | ||
| assert Path(result).is_absolute() or (len(result) >= 2 and result[1] == ":") |
Comment on lines
+1388
to
+1392
| # Force a wide console so Rich tables don't truncate column contents | ||
| # under narrow terminals (Windows CI runners default to ~80 cols). | ||
| from rich.console import Console | ||
|
|
||
| f.console = Console(width=200, force_terminal=False) |
bcc5c8b to
daf99e0
Compare
danielmeppiel
pushed a commit
that referenced
this pull request
May 21, 2026
Two findings from the Copilot reviewer, both accepted: 1. test_paths.py path assertions -- the previous fallback '(len(result) >= 2 and result[1] == ":")' would incorrectly accept drive-RELATIVE paths like 'C:foo' as absolute. Path(result).is_absolute() alone is correct on both POSIX and Windows because pathlib instantiates the platform's Path subclass; drop the redundant drive-letter clause. 2. _color_formatter helper duplicated across test_output_formatters_phase3.py and test_output_formatters_rendering.py -- extract the Console(width=200) construction into tests/unit/_formatter_helpers.py so the two modules cannot drift on Rich settings. Both call sites now delegate to make_color_formatter(). No behavior change in production code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address all unit-test failures observed on the windows-latest runner (run https://github.com/microsoft/apm/actions/runs/26188855664): - tests/unit/test_reflink_{filesystem_support,phase3w5}.py: skip TestCloneLinux on win32; it does 'import fcntl' which is Linux-only. - tests/unit/utils/test_atomic_io.py: patch os.fchmod with create=True so the patch works on Windows where os.fchmod does not exist. - src/apm_cli/utils/atomic_io.py: close the raw fd in the failure path before unlinking the tmp file, so Windows can release its lock and the cleanup succeeds (was leaking the tmp file when os.fdopen raised). - tests/unit/utils/test_paths.py: accept Windows drive-letter absolute paths (e.g. 'C:/...') in addition to POSIX leading slashes when asserting portable_relpath() fell back to an absolute path. - tests/unit/test_output_formatters_{phase3,rendering}.py: pin Rich Console width=200 in the _color_formatter test helper so table columns are not truncated under narrow Windows CI terminal widths (the 'constitution' assertion was failing because Rich shortened the cell to 'constitutio\u2026'). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ckage to base The codex and copilot client adapters carried identical copies of _select_remote_with_url and _select_best_package (priority order [npm, docker, pypi, homebrew]). After the recent #1277 merge the two chunks crossed the pylint R0801 --min-similarity-lines=10 threshold, breaking the 'Code duplication guardrail' lint step on every open PR: src/apm_cli/marketplace/version_check.py:1:0: R0801: Similar lines in 2 files ==apm_cli.adapters.client.codex:[545:573] ==apm_cli.adapters.client.copilot:[1003:1049] Move both helpers onto MCPClientAdapter (where _infer_registry_name they depend on already lives) and delete the duplicates from codex and copilot. VSCodeAdapter keeps its own _select_best_package because it uses a different priority order ([npm, pypi, docker]) with extra runtime_hint fallback, so leaving its override in place is correct. No behavior change: the moved bodies are byte-identical to the removed copies. 26 existing best_package / remote_with_url unit tests still pass; lint pipeline (ruff + pylint R0801) is green locally. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two findings from the Copilot reviewer, both accepted: 1. test_paths.py path assertions -- the previous fallback '(len(result) >= 2 and result[1] == ":")' would incorrectly accept drive-RELATIVE paths like 'C:foo' as absolute. Path(result).is_absolute() alone is correct on both POSIX and Windows because pathlib instantiates the platform's Path subclass; drop the redundant drive-letter clause. 2. _color_formatter helper duplicated across test_output_formatters_phase3.py and test_output_formatters_rendering.py -- extract the Console(width=200) construction into tests/unit/_formatter_helpers.py so the two modules cannot drift on Rich settings. Both call sites now delegate to make_color_formatter(). No behavior change in production code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
32991fb to
c6587a6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(tests): make Windows unit-test job green
TL;DR
The
Build & Test (windows-latest)job in run #26188855664 failed on 14 unit tests across 5 distinct root causes: a Linux-onlyfcntlimport,os.fchmodnot existing on Windows, a realatomic_write_textfd-leak that prevents tmp-file cleanup on Windows, POSIX-only path assertions, and RichConsole()truncating table columns under the narrow CI terminal width. This PR fixes each root cause at the right layer (one production fix inatomic_io.py, the rest in tests) so the Windows job goes green without weakening Linux/macOS coverage.Problem (WHY)
The Windows runner failed with five distinct, simultaneous symptoms in
tests/unit:fcntlimport errors inTestCloneLinux(test_reflink_filesystem_support.py,test_reflink_phase3w5.py) --fcntlis a POSIX-only stdlib module; on Windows the import itself raisesModuleNotFoundError, so the whole class is uncollectable.os.fchmodAttributeErrorintest_atomic_io.py::TestAtomicWriteText::test_fchmod_*--unittest.mock.patchrequires the target attribute to exist; on Windowsos.fchmoddoes not exist, so the patch itself raises before the test body runs.atomic_write_textleaks the tmp file on Windows whenos.fdopenraises -- the rawfdfrommkstempis never closed, so Windows file locking blocks the subsequentos.unlink(tmp_name).test_tmp_file_cleaned_up_on_write_failurecorrectly caught this; the bug is in the production code, not the test.result.startswith("/")assertions intest_paths.py::TestPortableRelpath-- absolute paths on Windows look likeC:/Users/..., not/Users/....Console()truncating "constitution" to "constitutio…" intest_output_formatters_{phase3,rendering}.py::test_constitution_row_colored-- the Windows CI runner detects a narrow terminal width (~80 cols), so Rich shrinks the 100+ char-wide table and the assertion"constitution" in textfails.Per the repo's contract that "agents pattern-match well against concrete structures", each failure deserves a targeted fix at the layer it actually lives in -- not a blanket
skipif win32over the whole file.Approach (WHAT)
TestCloneLinuximport-timefcntlfailure@pytest.mark.skipif(sys.platform == "win32", ...)on the classpatch("...os.fchmod")AttributeErrorcreate=Trueto the threepatchcallsatomic_write_texttmp-file leak on Windowsfdin the failure path before unlinkingresult.startswith("/")on Windows drive pathsPath(result).is_absolute() or drive-letterConsole(width=200)in the_color_formatterhelperImplementation (HOW)
src/apm_cli/utils/atomic_io.py-- trackfd_wrapped = Falseand flip it toTrueonly afteros.fdopen(fd, ...)returns; in theexceptbranch, close the fd first when it was never wrapped, then unlink. Preserves the exception-propagation contract; eliminates the Windows tmp-leak.tests/unit/test_reflink_filesystem_support.py,tests/unit/test_reflink_phase3w5.py--@pytest.mark.skipif(sys.platform == "win32", reason="Linux-only: fcntl module not available on Windows")onTestCloneLinux. Theimport fcntllives inside the class body, so a class-level skip is the minimum-scope fix.tests/unit/utils/test_atomic_io.py-- three call sites:patch("apm_cli.utils.atomic_io.os.fchmod", create=True). Behavior on POSIX is unchanged (the attribute exists;create=Trueis a no-op).tests/unit/utils/test_paths.py-- two assertions changed fromresult.startswith("/")toPath(result).is_absolute() or (len(result) >= 2 and result[1] == ":"). Forward-slash-only check is preserved.tests/unit/test_output_formatters_phase3.py,tests/unit/test_output_formatters_rendering.py--_color_formatter()helper now constructs aConsole(width=200, force_terminal=False)and assigns it tof.console, so Rich's auto-detect cannot shrink columns below content length.Diagram -- atomic_write_text failure path before/after
Legend: shows why closing the fd before
unlinkis required on Windows but is a no-op on POSIX.flowchart LR A[mkstemp returns fd, tmp_name] --> B{os.fdopen fd} B -- success --> C[write + os.replace] B -- raises --> D{"before: skip os.close(fd)"} D --> E["os.unlink(tmp_name)<br/>POSIX: succeeds<br/>Windows: blocked by open fd"] B -- raises --> F["after: os.close(fd) first"] F --> G["os.unlink(tmp_name)<br/>succeeds on every platform"]Trade-offs
TestCloneLinuxon Windows rather than mockfcntl. Mocking a missing stdlib module is fragile (sys.modules['fcntl'] = MagicMock()) and would test the mock, not the code. The reflink code path that usesfcntlis itself gated on Linux, so Windows has no production code to exercise here. "Add what the agent lacks, omit what it knows" -- omit the test on the platform that does not run the code.Console(width=200)in the test helper rather than parametrize production code. Production formatters intentionally honor the user's actual terminal width; threading a width throughCompilationFormatter.__init__just to satisfy tests would leak test concerns into the production API.Benefits
Build & Testjob goes from 14 failures to 0 on the targeted suite.atomic_write_text: tmp files no longer leak on Windows whenfdopenraises.tests/unitshows only 4 pre-existing failures onmain, none introduced here).Validation
Local run on macOS against the directly-affected files:
Lint contract (mirrors CI
Lintjob per.apm/instructions/linting.instructions.md):Pre-existing unrelated failures on main (verified by stashing this PR's diff)
These reproduce on
mainwith the PR diff stashed and are out of scope.Scenario evidence
atomic_write_textnever leaves a half-written or leaked tmp file whenfdopenfails (Windows + POSIX)tests/unit/utils/test_atomic_io.py::TestAtomicWriteText::test_tmp_file_cleaned_up_on_write_failureportable_relpathfalls back to a forward-slash absolute path on every platform when path is not under basetests/unit/utils/test_paths.py::TestPortableRelpath::test_path_not_under_base_returns_absoluteCompilationFormattercolor path renders the constitution row legibly regardless of detected terminal widthtests/unit/test_output_formatters_*.py::TestFormatOptimizationProgressColorBranches::test_constitution_row_coloredHow to test
git fetch origin danielmeppiel/stunning-meme && git checkout danielmeppiel/stunning-memeuv run --extra dev pytest tests/unit/utils/test_atomic_io.py tests/unit/utils/test_paths.py tests/unit/test_reflink_filesystem_support.py tests/unit/test_reflink_phase3w5.py tests/unit/test_output_formatters_phase3.py tests/unit/test_output_formatters_rendering.py-- expect green.Build & Testjob goes green on this PR's CI run.uv run --extra dev ruff check src/ tests/ && uv run --extra dev ruff format --check src/ tests/.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com