Skip to content

test(git): close test repositories instead of rmtree so teardown works on Windows - #4880

Open
tsurutanmen wants to merge 1 commit into
modelcontextprotocol:mainfrom
tsurutanmen:fix/git-tests-windows-teardown
Open

tsurutanmen wants to merge 1 commit into
modelcontextprotocol:mainfrom
tsurutanmen:fix/git-tests-windows-teardown

Conversation

@tsurutanmen

Copy link
Copy Markdown

Description

On Windows, 40 of the 87 tests in src/git error during teardown of the test_repository fixture. The assertions pass, but pytest reports PermissionError from shutil.rmtree(repo_path):

  • [WinError 32] (file in use): GitPython still holds handles, such as its persistent git cat-file processes, when the fixture removes the directory.
  • [WinError 5] (access denied): git writes loose object files as read-only, and shutil.rmtree cannot delete read-only files on Windows. This shows up in the two git_add tests, where the git CLI writes the objects.

This PR calls test_repo.close() to release the handles, and drops the explicit rmtree. The repository lives under pytest's tmp_path, and pytest removes that directory itself, including read-only files.

Server Details

  • Server: git
  • Changes to: tests only

Motivation and Context

Contributors on Windows currently see 40 errors from a clean checkout. That makes it hard to tell whether a change broke anything.

How Has This Been Tested?

On Windows 11, Python 3.11, git for Windows:

  • before: 47 passed, 40 errors
  • after: 47 passed

ruff check and pyright are clean. There are no behavior changes on Linux or macOS: close() is a no-op there apart from releasing resources, and tmp_path cleanup already applies.

Breaking Changes

None. Only tests change.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Protocol Documentation
  • My changes follows MCP security best practices
  • I have updated the server's README accordingly
  • I have tested this with an LLM client
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have documented all environment variables and configuration options

Additional context

I found this while working on #4879, which also touches this server.

🤖 Generated with Claude Code

…s on Windows

On Windows the test_repository fixture teardown raised PermissionError
for 40 of 87 tests: shutil.rmtree ran while GitPython still held
handles (WinError 32), and git's loose object files are read-only,
which rmtree cannot delete (WinError 5). Close the repo to release its
handles and leave tmp_path removal to pytest, which handles read-only
files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

1 participant