Skip to content

test(windows): fix CLAUDE_CONFIG_DIR scope tests on Windows#1086

Merged
danielmeppiel merged 2 commits intomainfrom
fix/windows-claude-config-dir-tests
Apr 30, 2026
Merged

test(windows): fix CLAUDE_CONFIG_DIR scope tests on Windows#1086
danielmeppiel merged 2 commits intomainfrom
fix/windows-claude-config-dir-tests

Conversation

@danielmeppiel
Copy link
Copy Markdown
Collaborator

Problem

Three Windows unit tests added in #1055 fail on windows-latest (CI run 25190857376):

  • test_user_scope_with_claude_config_dir
  • test_user_scope_outside_home_keeps_absolute
  • test_user_scope_collapses_dotdot_segments

All three patch HOME via monkeypatch and then exercise code that calls Path.home(). On Windows, Path.home() ignores HOME and reads USERPROFILE (with HOMEDRIVE+HOMEPATH as fallback). So Path.home() returned the real runner profile (C:\Users\runneradmin), relative_to(home) succeeded against tmp_path (which lives under AppData\Local\Temp\...), and the assertions on root_dir failed:

AssertionError: assert 'AppData/Loca...g/test-claude' == '.config/test-claude'
AssertionError: assert 'AppData/Loca..._k0/elsewhere' == 'C:\\Users\\r...k0\\elsewhere'
AssertionError: assert 'AppData/Loca...dotd0/outside' == 'C:\\Users\\r...otd0\\elsewhere'

Fix

Add a small _set_home() helper in both scope test modules that, on Windows, also patches USERPROFILE / HOMEDRIVE / HOMEPATH so Path.home() returns the fake home. Use it in the three failing tests plus test_user_scope_expands_tilde for consistency.

Also call .resolve() on the expected outside path in test_user_scope_outside_home_keeps_absolute to match the source's Path(env).expanduser().resolve(strict=False) output (covers macOS /var -> /private/var and Windows 8.3 short-name expansion).

Test-only change. No production code touched.

Validation

uv run --extra dev ruff check tests/unit/integration/test_scope_integration.py tests/unit/integration/test_scope_install_uninstall.py
uv run --extra dev ruff format --check tests/unit/integration/test_scope_integration.py tests/unit/integration/test_scope_install_uninstall.py
uv run --extra dev pytest tests/unit/integration/test_scope_integration.py tests/unit/integration/test_scope_install_uninstall.py
# 38 passed

The CLAUDE_CONFIG_DIR scope tests added in #1055 patched only HOME, but
Path.home() on Windows uses USERPROFILE (with HOMEDRIVE+HOMEPATH as
fallback) and ignores HOME. As a result, Path.home() returned the real
runner profile, relative_to(home) succeeded against the AppData/Local
tmp_path, and the assertions on root_dir failed on windows-latest.

Add a small _set_home() helper in both scope test modules that also sets
USERPROFILE / HOMEDRIVE / HOMEPATH on Windows, and use it in the three
failing tests:
  - test_user_scope_with_claude_config_dir
  - test_user_scope_outside_home_keeps_absolute
  - test_user_scope_collapses_dotdot_segments
plus test_user_scope_expands_tilde for consistency.

Also resolve() the expected outside path in
test_user_scope_outside_home_keeps_absolute to match the source's
abs_path.expanduser().resolve() output (e.g. /var -> /private/var on
macOS, 8.3 short-name expansion on Windows).

Fixes failures from CI run 25190857376.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 30, 2026 21:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes failing Windows CI unit tests by ensuring Path.home() honors the monkeypatched home directory on Windows, and aligns expected path normalization with the implementation.

Changes:

  • Added a _set_home() helper to patch HOME plus Windows-specific home variables (USERPROFILE, HOMEDRIVE, HOMEPATH).
  • Updated affected scope tests to use _set_home() for consistent behavior across OSes.
  • Updated one assertion to compare against a resolved absolute path.
Show a summary per file
File Description
tests/unit/integration/test_scope_integration.py Adds portable home env patch helper and updates user-scope path tests (including resolved-path expectation).
tests/unit/integration/test_scope_install_uninstall.py Adds the same helper and updates the Windows-failing CLAUDE_CONFIG_DIR scope test to use it.

Copilot's findings

  • Files reviewed: 2/2 changed files
  • Comments generated: 3

Comment on lines +25 to +40
def _set_home(monkeypatch, home: Path) -> None:
"""Set the user's home directory portably across POSIX and Windows.

``Path.home()`` consults ``HOME`` on POSIX but ``USERPROFILE`` (with
``HOMEDRIVE`` + ``HOMEPATH`` fallback) on Windows. Setting only ``HOME``
is a no-op on Windows and causes ``relative_to(Path.home())`` checks in
code under test to compare against the real user's profile.
"""
home_str = str(home)
monkeypatch.setenv("HOME", home_str)
if os.name == "nt":
monkeypatch.setenv("USERPROFILE", home_str)
drive, _, tail = home_str.partition(":")
if tail:
monkeypatch.setenv("HOMEDRIVE", f"{drive}:")
monkeypatch.setenv("HOMEPATH", tail)
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

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

The _set_home() helper is duplicated across two test modules with the same logic. To avoid drift and keep behavior consistent, consider moving this into a shared test utility (e.g., tests/unit/conftest.py fixture or a tests/unit/utils.py helper) and importing/using it from both files.

Copilot uses AI. Check for mistakes.
Comment thread tests/unit/integration/test_scope_integration.py Outdated
Comment thread tests/unit/integration/test_scope_integration.py Outdated
- Update comment to reflect that paths outside $HOME are resolved
  (normalized), not preserved verbatim.
- Make the .resolve() call explicit with strict=False to mirror the
  implementation in for_scope() and document the expected behavior
  for non-existent paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@danielmeppiel
Copy link
Copy Markdown
Collaborator Author

Thanks for the review. Addressed in 82cebf3:

  • Integrate copilot runtime #2 (comment wording): Updated to Paths outside $HOME remain absolute and are resolved/normalized.
  • Will there be MCP coverage? #3 (explicit strict=False): Adopted; mirrors for_scope() and documents non-existent-path behavior.
  • Why do we need a GitHub token? #1 (dedup _set_home into a shared fixture): Intentionally not adopted. Three call sites total (two added here + two preexisting integration tests in test_deps_update_e2e.py / test_marketplace_e2e.py that already inline the same pattern). Extracting a fixture saves ~10 lines and adds an import + indirection without removing the existing duplication. Happy to revisit if a 4th call site appears.

@danielmeppiel danielmeppiel merged commit 064bd3e into main Apr 30, 2026
9 checks passed
@danielmeppiel danielmeppiel deleted the fix/windows-claude-config-dir-tests branch April 30, 2026 22:33
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