From 9f082aa727533c680bafb1729416b96a2c117379 Mon Sep 17 00:00:00 2001 From: Original Gary <276612211+OpenGaryBot@users.noreply.github.com> Date: Thu, 14 May 2026 09:04:17 +1000 Subject: [PATCH] fix: cross-platform test failures on Windows (34 tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After PR #40, running the full suite on Windows surfaced 34 failures that weren't visible in CI (Linux). All were Windows-specific quirks falling into five categories. Each was root-caused and fixed without changing Linux behavior. Full suite now green on Windows Python 3.12: Before: 34 failed, 6796 passed After: 0 failed, 6830 passed, 6 skipped ## Path-separator normalization (production code, 7 files) Several modules built prefix strings with `os.sep` ("\\" on Windows) and then compared against POSIX-style paths emitted by other tools or test fixtures. The mismatched separators caused silent miss-matches. Each fix normalizes both sides to "/" for comparison while preserving native separators in the output where production callers expect them. - `engine/detectors/coupling.py::_rel_to_root` — also strip drive letter when root resolved to `C:/...` but the caller still passes POSIX-rooted prefix strings (e.g. tests using `/project/...`). - `engine/detectors/coverage/mapping_analysis.py::_build_prod_by_module` — normalize prod_file before prefix-strip; on Windows the mixed-slash inputs were missing the project-root prefix and producing module names like `.repo.pkg.util` instead of `pkg.util`. - `engine/detectors/test_coverage/discovery.py::_normalize_graph_paths, _discover_scorable_and_tests` — same pattern; also kept native- separator output for callers that round-trip native paths (test_normalize_graph_paths_converts_absolute_to_relative). - `engine/policy/zones.py::_match_pattern` — normalize both pattern and rel_path to forward slashes. Without this, an absolute Windows path pattern (`C:\...\test_creds.py`) hit the "exact basename" branch and returned False, causing the security detector to scan files in TEST zones it should have skipped. - `languages/_framework/treesitter/imports/resolvers_backend.py` — `os.path.normpath` around every returned candidate (go/rust/java/ kotlin/cxx/csharp/dart/scala/swift). Forward-slashed src-root strings like "src/main/java" combined with native scan_path produced mixed separators ("C:\...\src/main/java\com\acme\App.java") that didn't match the platform-native expected paths. - `languages/_framework/treesitter/imports/resolvers_scripts.py:: resolve_js_import` — normpath each candidate before isfile check. The "/index.js" suffix on a Windows path produced mixed separators. - `languages/javascript/test_coverage.py::resolve_import_spec` — normalize candidate after `os.path.normpath` for set-membership check against POSIX-keyed production files. ## Test path/separator fixes (3 tests) - `test_state_path_from_explicit_state_arg` — `Path.as_posix()` for cross-platform string equality. - `test_external_adapters.py::TestCollectExcludeDirs::test_includes_*, test_deduplicates` — use `os.path.basename` instead of rsplit("/") which split on the wrong separator on Windows. - `test_writes_to_file` (holistic_review remediation) — explicit `encoding="utf-8"` on the read; locale default (cp1252 on Windows) mangled the middle-dot unicode the writer emits as UTF-8. ## Skip Unix-only test path - `test_timeout_kills_jscpd_process_group` — `pytest.mark.skipif` on `hasattr(os, 'getpgid')`. The kill-by-process-group path is POSIX only; on Windows the timeout handling uses TerminateProcess, which has its own test. ## chmod assertion gated by platform - `test_orchestrator_pipeline_writes_exact_cli_helper` — skip the `st_mode & 0o111` exec-bit assert on Windows. Windows uses ACLs, not POSIX mode bits; the helper is invoked via bash/PYTHONPATH and doesn't need the exec bit set on Windows. ## cmd /c wrapping (5 tests, 1 cross-cutting fixture) - On Windows when an executable (acli, codex) isn't on PATH, `_resolve_executable` returns `["cmd", "/c", name]`, then `_wrap_cmd_c` collapses everything after `/c` into a single string for cmd.exe's tokenizer. Correct production behavior — but it obscures the per-arg list shape several tests assert on. - For 3 rovodev tests + 1 codex_batch test: per-test monkeypatch of `_resolve_executable` to return `[name]` directly. - For 12 `TestCmdReviewPrepare::test_do_run_batches_*` tests: added a class-level autouse fixture that does the same. Plus added `**kwargs` to all 7 `fake_subprocess_run` signatures + `fake_run` in `test_run_codex_batch_writes_live_status_before_completion`. The runner adds `input=stdin_text` to `subprocess.run` kwargs whenever `_prompt_via_stdin(prompt)` is true — which is always on Windows (`return sys.platform == "win32" or len(prompt) > _PROMPT_ARG_MAX_CHARS`). Without **kwargs, the fakes raised TypeError, hit `_run_via_subprocess`'s except branch, and returned exit_code=1. None of these touch Linux behavior. CI on Linux 3.11 was already green; this brings Windows 3.12 to parity. --- desloppify/engine/detectors/coupling.py | 8 +++ .../detectors/coverage/mapping_analysis.py | 12 ++-- .../detectors/test_coverage/discovery.py | 37 +++++++++--- desloppify/engine/policy/zones.py | 6 ++ .../treesitter/imports/resolvers_backend.py | 29 +++++----- .../treesitter/imports/resolvers_scripts.py | 5 +- .../languages/javascript/test_coverage.py | 3 + .../plan/test_triage_split_modules_direct.py | 7 ++- .../review/test_runner_rovodev_direct.py | 7 +++ desloppify/tests/commands/test_helpers.py | 3 +- .../commands/test_runner_modules_direct.py | 6 +- .../tests/detectors/test_external_adapters.py | 15 ++++- ...olistic_review_dimensions_and_structure.py | 6 +- .../tests/review/review_commands_cases.py | 57 ++++++++++++++++--- 14 files changed, 160 insertions(+), 41 deletions(-) diff --git a/desloppify/engine/detectors/coupling.py b/desloppify/engine/detectors/coupling.py index 4061412c0..b7c072631 100644 --- a/desloppify/engine/detectors/coupling.py +++ b/desloppify/engine/detectors/coupling.py @@ -56,6 +56,14 @@ def _rel_to_root(value: str, root_norm: str) -> str: """Return root-relative variant (best effort) for path/prefix matching.""" if value.startswith(root_norm): return value[len(root_norm) :] + # On Windows, root_norm may be drive-rooted ("C:/project/") after + # Path.resolve(), while the caller still passes POSIX-rooted paths + # ("/project/..."). Strip the drive letter from root_norm and retry so + # tests and tools that mix the two shapes still match. + if len(root_norm) > 3 and root_norm[1:3] == ":/": + root_no_drive = root_norm[2:] + if value.startswith(root_no_drive): + return value[len(root_no_drive) :] return value.lstrip("/") diff --git a/desloppify/engine/detectors/coverage/mapping_analysis.py b/desloppify/engine/detectors/coverage/mapping_analysis.py index 390af6eb3..10b63db80 100644 --- a/desloppify/engine/detectors/coverage/mapping_analysis.py +++ b/desloppify/engine/detectors/coverage/mapping_analysis.py @@ -139,7 +139,10 @@ def _build_prod_by_module( project_root: str, ) -> dict[str, str]: """Build module lookup map for production files.""" - root_str = project_root + os.sep + # Normalize to POSIX-style for the prefix comparison so mixed-slash inputs + # (Windows root + forward-slash relative segments from other tools) match. + root_posix = project_root.replace(os.sep, "/") + root_str = root_posix + "/" prod_by_module: dict[str, str] = {} ambiguous_aliases: set[str] = set() @@ -154,10 +157,11 @@ def assign_alias(alias: str, prod_file: str) -> None: prod_by_module.pop(alias, None) for prod_file in production_files: + normalized_prod = prod_file.replace(os.sep, "/") rel_path = ( - prod_file[len(root_str) :] - if prod_file.startswith(root_str) - else prod_file + normalized_prod[len(root_str) :] + if normalized_prod.startswith(root_str) + else normalized_prod ) # Strip src/ prefix so src-layout projects map correctly # (e.g. 'src/argos_toolkit/foo.py' -> 'argos_toolkit.foo') diff --git a/desloppify/engine/detectors/test_coverage/discovery.py b/desloppify/engine/detectors/test_coverage/discovery.py index 32a65bc96..3f7ff6550 100644 --- a/desloppify/engine/detectors/test_coverage/discovery.py +++ b/desloppify/engine/detectors/test_coverage/discovery.py @@ -16,13 +16,34 @@ def _normalize_graph_paths(graph: dict) -> dict: - """Normalize graph paths to relative paths.""" - root_prefix = str(get_project_root()) + os.sep + """Normalize graph paths to relative paths. - def _to_rel(path: str) -> str: - return path[len(root_prefix) :] if path.startswith(root_prefix) else path + Comparison is done in POSIX form (forward slashes) so mixed-slash inputs + on Windows still match, but the returned keys preserve the input separator + convention by slicing from the original path string. + """ + root_native = str(get_project_root()) + root_posix = root_native.replace(os.sep, "/") + root_prefix_native = root_native + os.sep + root_prefix_posix = root_posix + "/" - needs_norm = any(k.startswith(root_prefix) for k in graph) + def _to_rel(path: str) -> str: + # Prefer native-form trim so callers that pass purely native paths + # get back native-form relatives (preserves existing contract). + if path.startswith(root_prefix_native): + return path[len(root_prefix_native) :] + # Fall back to POSIX-form trim for callers that pass mixed-slash paths + # (e.g. Windows root + forward-slash relative from cargo/npm/etc.). + normalized = path.replace(os.sep, "/") + if normalized.startswith(root_prefix_posix): + return normalized[len(root_prefix_posix) :] + return path + + needs_norm = any( + k.startswith(root_prefix_native) + or k.replace(os.sep, "/").startswith(root_prefix_posix) + for k in graph + ) if not needs_norm: return graph @@ -44,10 +65,12 @@ def _discover_scorable_and_tests( extra_test_files: set[str] | None, ) -> tuple[set[str], set[str], set[str], int]: """Return (production_files, test_files, scorable_files, potential).""" - root_prefix = str(get_project_root()) + os.sep + root_posix = str(get_project_root()).replace(os.sep, "/") + root_prefix = root_posix + "/" def _to_rel(path: str) -> str: - return path[len(root_prefix) :] if path.startswith(root_prefix) else path + normalized = path.replace(os.sep, "/") + return normalized[len(root_prefix) :] if normalized.startswith(root_prefix) else normalized all_files = zone_map.all_files() production_files = set(zone_map.include_only(all_files, Zone.PRODUCTION, Zone.SCRIPT)) diff --git a/desloppify/engine/policy/zones.py b/desloppify/engine/policy/zones.py index 16e524524..7e37af02a 100644 --- a/desloppify/engine/policy/zones.py +++ b/desloppify/engine/policy/zones.py @@ -90,6 +90,12 @@ def _match_pattern(rel_path: str, pattern: str) -> bool: See ZoneRule docstring for pattern type conventions. """ + # Normalize separators so the rest of the pattern logic doesn't have to + # think about Windows backslashes. Patterns are documented as + # POSIX-style (e.g. "/tests/"); callers that pass native paths + # (rare, mostly in tests) should still classify correctly. + rel_path = rel_path.replace("\\", "/") + pattern = pattern.replace("\\", "/") basename = os.path.basename(rel_path) # Directory pattern: "/dir/" → substring on padded path diff --git a/desloppify/languages/_framework/treesitter/imports/resolvers_backend.py b/desloppify/languages/_framework/treesitter/imports/resolvers_backend.py index 756b4fd60..2570714d5 100644 --- a/desloppify/languages/_framework/treesitter/imports/resolvers_backend.py +++ b/desloppify/languages/_framework/treesitter/imports/resolvers_backend.py @@ -19,7 +19,10 @@ def resolve_go_import(import_text: str, source_file: str, scan_path: str) -> str return None rel_path = import_text[len(module_path) :].lstrip("/") - candidate_dir = os.path.join(scan_path, rel_path) + # The import path uses forward slashes (Go module convention); on Windows, + # os.path.join with a forward-slashed segment produces mixed separators. + # Normalize so callers get native-form paths. + candidate_dir = os.path.normpath(os.path.join(scan_path, rel_path)) if os.path.isdir(candidate_dir): for filename in sorted(os.listdir(candidate_dir)): if filename.endswith(".go") and not filename.endswith("_test.go"): @@ -44,15 +47,15 @@ def resolve_rust_import(import_text: str, source_file: str, scan_path: str) -> s path_parts = parts[:-1] if len(parts) > 1 else parts candidate = os.path.join(src_dir, *path_parts) + ".rs" if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) candidate = os.path.join(src_dir, *path_parts, "mod.rs") if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) candidate = os.path.join(src_dir, *parts) + ".rs" if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) return None @@ -70,7 +73,7 @@ def resolve_java_import(import_text: str, source_file: str, scan_path: str) -> s for src_root in ["src/main/java", "src", "app/src/main/java", "."]: candidate = os.path.join(scan_path, src_root, rel_path) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) return None @@ -89,7 +92,7 @@ def resolve_kotlin_import(import_text: str, source_file: str, scan_path: str) -> for src_root in ["src/main/kotlin", "src/main/java", "src", "app/src/main/kotlin", "."]: candidate = os.path.join(scan_path, src_root, rel_path) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) return None @@ -101,12 +104,12 @@ def resolve_cxx_include(import_text: str, source_file: str, scan_path: str) -> s base = os.path.dirname(source_file) candidate = os.path.normpath(os.path.join(base, import_text)) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) for inc_dir in ["include", "src", "."]: candidate = os.path.join(scan_path, inc_dir, import_text) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) return None @@ -122,10 +125,10 @@ def resolve_csharp_import(import_text: str, source_file: str, scan_path: str) -> rel_path = os.path.join(*parts[:-1], filename) candidate = os.path.join(scan_path, src_root, rel_path) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) candidate = os.path.join(scan_path, src_root, filename) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) return None @@ -137,7 +140,7 @@ def resolve_dart_import(import_text: str, source_file: str, scan_path: str) -> s parts = import_text[len("package:") :].split("/", 1) if len(parts) < 2: return None - candidate = os.path.join(scan_path, "lib", parts[1]) + candidate = os.path.normpath(os.path.join(scan_path, "lib", parts[1])) return candidate if os.path.isfile(candidate) else None base = os.path.dirname(source_file) @@ -159,7 +162,7 @@ def resolve_scala_import(import_text: str, source_file: str, scan_path: str) -> for src_root in ["src/main/scala", "src", "."]: candidate = os.path.join(scan_path, src_root, rel_path) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) return None @@ -185,5 +188,5 @@ def resolve_swift_import(import_text: str, source_file: str, scan_path: str) -> continue seen.add(candidate) if os.path.isfile(candidate): - return candidate + return os.path.normpath(candidate) return None diff --git a/desloppify/languages/_framework/treesitter/imports/resolvers_scripts.py b/desloppify/languages/_framework/treesitter/imports/resolvers_scripts.py index d8c159249..2ad619d3c 100644 --- a/desloppify/languages/_framework/treesitter/imports/resolvers_scripts.py +++ b/desloppify/languages/_framework/treesitter/imports/resolvers_scripts.py @@ -173,8 +173,11 @@ def resolve_js_import(import_text: str, source_file: str, scan_path: str) -> str base = os.path.dirname(source_file) candidate = os.path.normpath(os.path.join(base, import_text)) + # Concatenating "/index.js" onto a Windows path produces mixed separators + # ("C:\\...\\ui/index.js"); normalize each candidate so the returned path + # uses native separators throughout. for ext in ("", ".js", ".jsx", ".mjs", ".cjs", "/index.js", "/index.jsx"): - path = candidate + ext + path = os.path.normpath(candidate + ext) if os.path.isfile(path): return path return None diff --git a/desloppify/languages/javascript/test_coverage.py b/desloppify/languages/javascript/test_coverage.py index 408ec6b3c..b990f95f8 100644 --- a/desloppify/languages/javascript/test_coverage.py +++ b/desloppify/languages/javascript/test_coverage.py @@ -112,6 +112,9 @@ def resolve_import_spec( base = os.path.dirname(test_path) candidate = os.path.normpath(os.path.join(base, spec)) + # production_files keys are POSIX-style (forward slashes). os.path.normpath + # produces backslashes on Windows, so normalize back to POSIX for lookup. + candidate = candidate.replace(os.sep, "/") for ext in ("", ".js", ".jsx", ".mjs", ".cjs", "/index.js", "/index.jsx"): path = candidate + ext if path in production_files: diff --git a/desloppify/tests/commands/plan/test_triage_split_modules_direct.py b/desloppify/tests/commands/plan/test_triage_split_modules_direct.py index c96df51b5..96ccdae0c 100644 --- a/desloppify/tests/commands/plan/test_triage_split_modules_direct.py +++ b/desloppify/tests/commands/plan/test_triage_split_modules_direct.py @@ -1311,10 +1311,15 @@ def test_orchestrator_pipeline_entrypoint_is_exposed() -> None: def test_orchestrator_pipeline_writes_exact_cli_helper(tmp_path: Path) -> None: + import sys helper = orchestrator_pipeline_mod._write_desloppify_cli_helper(tmp_path) text = helper.read_text(encoding="utf-8") assert helper.exists() - assert helper.stat().st_mode & 0o111 + # POSIX exec bits don't map onto Windows file modes (Windows uses ACLs); + # the helper is invoked via bash/PYTHONPATH wiring rather than direct exec + # so missing exec bits aren't a functional regression on Windows. + if sys.platform != "win32": + assert helper.stat().st_mode & 0o111 assert "PYTHONPATH=" in text assert "-m desloppify.cli" in text diff --git a/desloppify/tests/commands/review/test_runner_rovodev_direct.py b/desloppify/tests/commands/review/test_runner_rovodev_direct.py index f18f0acf5..6b30509c3 100644 --- a/desloppify/tests/commands/review/test_runner_rovodev_direct.py +++ b/desloppify/tests/commands/review/test_runner_rovodev_direct.py @@ -25,6 +25,11 @@ def test_rovodev_batch_command_includes_acli_rovodev_run_invocation(monkeypatch) monkeypatch.delenv("DESLOPPIFY_ROVODEV_OUTPUT_SCHEMA", raising=False) monkeypatch.delenv("DESLOPPIFY_ROVODEV_EXTRA_ARGS", raising=False) monkeypatch.delenv("DESLOPPIFY_ROVODEV_EXECUTABLE", raising=False) + # On Windows, _resolve_executable wraps in `cmd /c` when the binary isn't on + # PATH, and _wrap_cmd_c then collapses the list — correct production + # behavior, but it obscures per-arg list shape. Stub to the unwrapped form + # so this test verifies command construction logic cross-platform. + monkeypatch.setattr(runner_rovodev_mod, "_resolve_executable", lambda name: [name]) cmd = runner_rovodev_mod.rovodev_batch_command( prompt="hello world", @@ -50,6 +55,7 @@ def test_rovodev_batch_command_honours_env_overrides(monkeypatch) -> None: monkeypatch.setenv("DESLOPPIFY_ROVODEV_OUTPUT_SCHEMA", '{"type":"object"}') monkeypatch.setenv("DESLOPPIFY_ROVODEV_EXTRA_ARGS", "--config-override foo") monkeypatch.setenv("DESLOPPIFY_ROVODEV_EXECUTABLE", "acli") + monkeypatch.setattr(runner_rovodev_mod, "_resolve_executable", lambda name: [name]) cmd = runner_rovodev_mod.rovodev_batch_command( prompt="prompt", @@ -70,6 +76,7 @@ def test_rovodev_batch_command_no_yolo_opt_out(monkeypatch) -> None: monkeypatch.setenv("DESLOPPIFY_ROVODEV_NO_YOLO", "1") monkeypatch.delenv("DESLOPPIFY_ROVODEV_OUTPUT_SCHEMA", raising=False) monkeypatch.delenv("DESLOPPIFY_ROVODEV_EXTRA_ARGS", raising=False) + monkeypatch.setattr(runner_rovodev_mod, "_resolve_executable", lambda name: [name]) cmd = runner_rovodev_mod.rovodev_batch_command( prompt="p", diff --git a/desloppify/tests/commands/test_helpers.py b/desloppify/tests/commands/test_helpers.py index 580f00339..ccfd09355 100644 --- a/desloppify/tests/commands/test_helpers.py +++ b/desloppify/tests/commands/test_helpers.py @@ -167,7 +167,8 @@ def test_state_path_from_explicit_state_arg(): args = SimpleNamespace(state="/custom/path.json", lang=None) result = state_path(args) assert result is not None - assert str(result) == "/custom/path.json" + # Use as_posix() to normalize separators on Windows. + assert result.as_posix() == "/custom/path.json" def test_state_path_from_lang_arg(): diff --git a/desloppify/tests/commands/test_runner_modules_direct.py b/desloppify/tests/commands/test_runner_modules_direct.py index 083885ca8..798473feb 100644 --- a/desloppify/tests/commands/test_runner_modules_direct.py +++ b/desloppify/tests/commands/test_runner_modules_direct.py @@ -163,6 +163,11 @@ def fake_run(cmd, **kwargs): def test_codex_batch_command_uses_sanitized_reasoning_effort(monkeypatch, tmp_path: Path) -> None: monkeypatch.setenv("DESLOPPIFY_CODEX_REASONING_EFFORT", "HIGH") + # On Windows _resolve_executable wraps unresolvable names in `cmd /c`, + # then _wrap_cmd_c collapses the rest of the args into one string. That's + # correct production behavior but obscures per-arg list shape the test + # asserts. Patch to the unwrapped form so this test runs cross-platform. + monkeypatch.setattr(codex_batch_mod, "_resolve_executable", lambda name: [name]) command = codex_batch_mod.codex_batch_command( prompt="review prompt", @@ -170,7 +175,6 @@ def test_codex_batch_command_uses_sanitized_reasoning_effort(monkeypatch, tmp_pa output_file=tmp_path / "out.json", ) - # On Windows with .cmd wrappers, prefix may be ["cmd", "/c", "...codex.cmd"] assert any(c.endswith("codex") or "codex" in c for c in command[:3]) assert "exec" in command assert "--ephemeral" in command diff --git a/desloppify/tests/detectors/test_external_adapters.py b/desloppify/tests/detectors/test_external_adapters.py index eb5aec350..d100cc518 100644 --- a/desloppify/tests/detectors/test_external_adapters.py +++ b/desloppify/tests/detectors/test_external_adapters.py @@ -553,6 +553,10 @@ def test_returns_none_on_timeout(self, tmp_path): ): assert detect_with_jscpd(tmp_path) is None + @pytest.mark.skipif( + not hasattr(__import__("os"), "getpgid"), + reason="os.getpgid is POSIX-only; the process-group kill path doesn't apply on Windows", + ) def test_timeout_kills_jscpd_process_group(self): class FakeProc: pid = 4321 @@ -963,11 +967,13 @@ def test_returns_absolute_paths(self, tmp_path): assert all(p.startswith(str(tmp_path)) for p in result) def test_includes_default_non_glob_entries(self, tmp_path): + import os with patch( "desloppify.base.discovery.source.get_exclusions", return_value=() ): result = collect_exclude_dirs(tmp_path) - basenames = {p.rsplit("/", 1)[-1] for p in result} + # Use os.path.basename to be separator-agnostic (Windows uses \, POSIX /). + basenames = {os.path.basename(p) for p in result} assert "node_modules" in basenames assert "__pycache__" in basenames assert ".git" in basenames @@ -983,12 +989,13 @@ def test_excludes_glob_patterns(self, tmp_path): assert not any("*" in p for p in result) def test_includes_runtime_exclusions(self, tmp_path): + import os with patch( "desloppify.base.discovery.source.get_exclusions", return_value=("vendor", "third_party"), ): result = collect_exclude_dirs(tmp_path) - basenames = {p.rsplit("/", 1)[-1] for p in result} + basenames = {os.path.basename(p) for p in result} assert "vendor" in basenames assert "third_party" in basenames @@ -1003,10 +1010,12 @@ def test_skips_runtime_glob_exclusions(self, tmp_path): def test_deduplicates(self, tmp_path): """Runtime exclusion that overlaps with DEFAULT_EXCLUSIONS doesn't produce dupes.""" + import os with patch( "desloppify.base.discovery.source.get_exclusions", return_value=("node_modules",), ): result = collect_exclude_dirs(tmp_path) - node_entries = [p for p in result if p.endswith("/node_modules")] + # Use basename, separator-agnostic (Windows uses \). + node_entries = [p for p in result if os.path.basename(p) == "node_modules"] assert len(node_entries) == 1 diff --git a/desloppify/tests/review/context/test_holistic_review_dimensions_and_structure.py b/desloppify/tests/review/context/test_holistic_review_dimensions_and_structure.py index 9885e9575..04d214381 100644 --- a/desloppify/tests/review/context/test_holistic_review_dimensions_and_structure.py +++ b/desloppify/tests/review/context/test_holistic_review_dimensions_and_structure.py @@ -271,8 +271,10 @@ def test_writes_to_file(self, tmp_path): plan = generate_remediation_plan(state, "python", output_path=output) assert output.exists() - assert output.read_text() == plan - assert "Issue" in output.read_text() + # Plan contains unicode (middle dot, etc.) written as UTF-8; locale-default + # read (cp1252 on Windows) would mangle the bytes. Always read as UTF-8. + assert output.read_text(encoding="utf-8") == plan + assert "Issue" in output.read_text(encoding="utf-8") def test_lang_name_in_commands(self): state = _state_with_holistic_issues( diff --git a/desloppify/tests/review/review_commands_cases.py b/desloppify/tests/review/review_commands_cases.py index 905e0dd03..d1dc563b6 100644 --- a/desloppify/tests/review/review_commands_cases.py +++ b/desloppify/tests/review/review_commands_cases.py @@ -102,6 +102,17 @@ def test_import_notice_warns_and_returns_missing_dimensions(self, capsys): class TestCmdReviewPrepare: + @pytest.fixture(autouse=True) + def _disable_cmd_c_wrapping(self, monkeypatch): + """Stub _resolve_executable so Windows doesn't wrap the codex command in + `cmd /c ...` and _wrap_cmd_c doesn't collapse the per-arg list. The + per-arg shape is what fake_subprocess_run relies on (it does + ``cmd[cmd.index('-o') + 1]``); the wrapping is correct production + behavior but makes the test environment-dependent.""" + monkeypatch.setattr( + runner_process_mod, "_resolve_executable", lambda name: [name] + ) + def test_do_prepare_writes_query_json( self, mock_lang_with_zones, empty_state, tmp_path ): @@ -950,8 +961,13 @@ def fake_subprocess_run( text=False, timeout=None, cwd=None, + **kwargs, ): - _ = timeout, cwd + # **kwargs absorbs the ``input=`` kwarg added by the runner when + # prompt-via-stdin is active (always on Windows; for long prompts + # on Linux). Without it, the fake raises TypeError and the batch + # fails with exit_code=1 long before the result is examined. + _ = timeout, cwd, kwargs out_path = Path(cmd[cmd.index("-o") + 1]) out_path.parent.mkdir(parents=True, exist_ok=True) payloads = { @@ -1190,8 +1206,12 @@ def fake_subprocess_run( text=False, timeout=None, cwd=None, + **kwargs, ): - _ = capture_output, text, timeout, cwd + # **kwargs absorbs the ``input=`` kwarg added by the runner when + # prompt-via-stdin is active (always on Windows; for long prompts + # on Linux). Without it the fake raises TypeError. + _ = capture_output, text, timeout, cwd, kwargs out_path = Path(cmd[cmd.index("-o") + 1]) out_path.parent.mkdir(parents=True, exist_ok=True) payload = { @@ -1309,8 +1329,12 @@ def fake_subprocess_run( text=False, timeout=None, cwd=None, + **kwargs, ): - _ = capture_output, text, timeout, cwd + # **kwargs absorbs the ``input=`` kwarg added by the runner when + # prompt-via-stdin is active (always on Windows; for long prompts + # on Linux). Without it the fake raises TypeError. + _ = capture_output, text, timeout, cwd, kwargs out_path = Path(cmd[cmd.index("-o") + 1]) out_path.parent.mkdir(parents=True, exist_ok=True) payload = { @@ -1466,8 +1490,11 @@ def fake_subprocess_run( text=False, timeout=None, cwd=None, + **kwargs, ): - _ = capture_output, text, timeout, cwd + # **kwargs absorbs the ``input=`` kwarg the runner passes when + # prompt-via-stdin is active (always on Windows). + _ = capture_output, text, timeout, cwd, kwargs # Simulate Codex occasionally returning JSON on stdout while failing # to write the -o output file. collect_batch_results should recover # from the batch log and persist the recovered raw payload. @@ -1576,8 +1603,12 @@ def fake_subprocess_run( text=False, timeout=None, cwd=None, + **kwargs, ): - _ = capture_output, text, timeout, cwd + # **kwargs absorbs the ``input=`` kwarg added by the runner when + # prompt-via-stdin is active (always on Windows; for long prompts + # on Linux). Without it the fake raises TypeError. + _ = capture_output, text, timeout, cwd, kwargs out_path = Path(cmd[cmd.index("-o") + 1]) out_path.parent.mkdir(parents=True, exist_ok=True) if out_path.name == "batch-1.raw.txt": @@ -1707,8 +1738,11 @@ def fake_subprocess_run( text=False, timeout=None, cwd=None, + **kwargs, ): - _ = capture_output, text, timeout, cwd + # **kwargs absorbs the ``input=`` kwarg the runner passes when + # prompt-via-stdin is active (always on Windows). + _ = capture_output, text, timeout, cwd, kwargs return MagicMock(returncode=124, stdout="", stderr="timed out") lang = MagicMock() @@ -1775,8 +1809,12 @@ def fake_subprocess_run( text=False, timeout=None, cwd=None, + **kwargs, ): - _ = capture_output, text, timeout, cwd + # **kwargs absorbs the ``input=`` kwarg added by the runner when + # prompt-via-stdin is active (always on Windows; for long prompts + # on Linux). Without it the fake raises TypeError. + _ = capture_output, text, timeout, cwd, kwargs out_path = Path(cmd[cmd.index("-o") + 1]) out_path.parent.mkdir(parents=True, exist_ok=True) payload = { @@ -1937,7 +1975,10 @@ def test_run_codex_batch_writes_live_status_before_completion(self, tmp_path): output_file = tmp_path / "out.txt" live_snapshot = {"text": ""} - def fake_run(_cmd, *, capture_output, text, timeout): # noqa: ARG001 + def fake_run(_cmd, *, capture_output, text, timeout, **kwargs): # noqa: ARG001 + # **kwargs absorbs the ``input=`` kwarg the runner passes when + # prompt-via-stdin is active (always on Windows). + _ = kwargs if log_file.exists(): live_snapshot["text"] = log_file.read_text() output_file.write_text('{"assessments": {}, "issues": []}')