fix: cross-platform test failures on Windows (34 tests)#41
Merged
Conversation
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR executes a cross-platform path normalization campaign across detection, resolution, and test infrastructure. Engine detectors and language import resolvers normalize Windows backslashes to forward slashes during prefix matching and relative path computation. Tests are updated to skip platform-specific assertions (POSIX exec bits), use platform-agnostic basename lookups, and monkeypatch executable resolution to prevent command-wrapping variability. ChangesCross-platform path consistency
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
2 participants
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.
Summary
After #40, running the full test suite on Windows surfaced 34 failures that weren't visible in CI (Ubuntu Linux 3.11). All were Windows-specific quirks; none affect Linux. Full suite is now 0 failed, 6830 passed, 6 skipped on Windows Python 3.12.
Each failure was traced to root cause and fixed — no skipif-on-windows shortcuts except where the test genuinely targets a POSIX-only code path.
Categories of fix
Path-separator normalization (7 production files)
Several modules built prefix strings with
os.sep(\on Windows) then compared against POSIX-style paths emitted by other tools or test fixtures. Mismatched separators caused silent miss-matches. Each fix normalizes both sides to/for comparison while preserving native separators in output where production callers expect them.engine/detectors/coupling.py::_rel_to_rootPath.resolve()producedC:/...but caller still passes POSIX-rooted prefixengine/detectors/coverage/mapping_analysis.py::_build_prod_by_moduleprod_filebefore prefix-strip; was producing module names like.repo.pkg.utilinstead ofpkg.utilengine/detectors/test_coverage/discovery.py::_normalize_graph_paths,_discover_scorable_and_testsengine/policy/zones.py::_match_patternrel_pathto forward slashes. Without this, absolute Windows path patterns hit the "exact basename" branch and returned False, causing the security detector to scan files in TEST zones it should have skippedlanguages/_framework/treesitter/imports/resolvers_backend.pyos.path.normpatharound every returned candidate (go/rust/java/kotlin/cxx/csharp/dart/scala/swift)languages/_framework/treesitter/imports/resolvers_scripts.py::resolve_js_importnormpatheach candidate beforeisfilechecklanguages/javascript/test_coverage.py::resolve_import_specTest path/encoding fixes (3 tests)
test_state_path_from_explicit_state_arg—Path.as_posix()for cross-platform string equalitytest_external_adapters.py::TestCollectExcludeDirs::*—os.path.basenameinstead ofrsplit("/")which split on the wrong separatortest_writes_to_file(holistic_review remediation) — explicitencoding="utf-8"on the read; locale default (cp1252 on Windows) mangled the middle-dot unicodeSkip Unix-only test paths (1 test)
test_timeout_kills_jscpd_process_group—pytest.mark.skipif(not hasattr(os, 'getpgid')). The kill-by-process-group code path is POSIX only; Windows uses TerminateProcess (covered by separate test)Platform-gate chmod assert (1 test)
test_orchestrator_pipeline_writes_exact_cli_helper— skip thest_mode & 0o111exec-bit assert on Windows. Windows uses ACLs; the helper is invoked via bash/PYTHONPATH and doesn't need the exec bit set on Windowscmd /cwrapping (5 unit tests + 12 do_run_batches integration tests)On Windows when a CLI binary isn't on PATH,
_resolve_executablereturns["cmd", "/c", name], then_wrap_cmd_ccollapses everything after/cinto a single string for cmd.exe's tokenizer. Correct production behavior, but obscures the per-arg list shape several tests assert on.monkeypatch.setattr(..., "_resolve_executable", lambda name: [name])TestCmdReviewPrepare::test_do_run_batches_*tests: class-level autouse fixture doing the same**kwargsadded to all 7fake_subprocess_runsignatures (andfake_runintest_run_codex_batch_writes_live_status_before_completion). The runner addsinput=stdin_texttosubprocess.runkwargs whenever_prompt_via_stdin(prompt)is true — and_prompt_via_stdinreturns True on Windows unconditionally (return sys.platform == "win32" or len(prompt) > _PROMPT_ARG_MAX_CHARS). Without**kwargs, the fakes raised TypeError, hit_run_via_subprocess's(RuntimeError, ValueError, TypeError)except branch, and returned exit_code=1 — which is why the batches appeared to "exit 1" while the mock was set up correctlyVerification
(Skipped count differs because
test_timeout_kills_jscpd_process_groupnewly skips on Windows.)Test plan
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests