fix(tests): isolate git operations in test fixtures from parent repository (#1205)
* fix(tests): isolate git operations in test fixtures from parent repository When tests run inside a git worktree (e.g., during pre-commit validation), git environment variables like GIT_DIR and GIT_WORK_TREE may be set by the pre-commit hook. These variables cause git operations in test fixtures to affect the parent repository instead of the temporary test directories. This fix adds proper git environment isolation to three test fixtures: - temp_git_repo in conftest.py - temp_project in test_merge_fixtures.py - setup_test_environment in test_recovery.py The fix clears GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, and GIT_ALTERNATE_OBJECT_DIRECTORIES before git operations, and sets GIT_CEILING_DIRECTORIES to prevent git from discovering parent .git directories. All environment variables are restored after the fixture completes. This pattern was already correctly implemented in test_pr_worktree_manager.py and has been applied consistently to all other fixtures that create temporary git repositories. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix(tests): correct git isolation fixtures to use yield and proper cleanup - test_merge_fixtures.py: Add missing 'import os', change 'return' to 'yield' in temp_project fixture so environment is restored AFTER tests complete, update return type to Generator[Path, None, None] - test_recovery.py: Add check=True to git init, add 'git branch -M main' for consistent branch naming, fix environment restoration timing by returning saved_env from setup and restoring in cleanup instead of immediately after git init Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Test User <test@example.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Andy
+64
-22
@@ -170,31 +170,73 @@ def temp_dir() -> Generator[Path, None, None]:
|
||||
|
||||
@pytest.fixture
|
||||
def temp_git_repo(temp_dir: Path) -> Generator[Path, None, None]:
|
||||
"""Create a temporary git repository with initial commit."""
|
||||
# Initialize git repo
|
||||
subprocess.run(["git", "init"], cwd=temp_dir, capture_output=True, check=True)
|
||||
subprocess.run(
|
||||
["git", "config", "user.email", "test@example.com"],
|
||||
cwd=temp_dir, capture_output=True
|
||||
)
|
||||
subprocess.run(
|
||||
["git", "config", "user.name", "Test User"],
|
||||
cwd=temp_dir, capture_output=True
|
||||
)
|
||||
"""Create a temporary git repository with initial commit.
|
||||
|
||||
# Create initial commit
|
||||
test_file = temp_dir / "README.md"
|
||||
test_file.write_text("# Test Project\n")
|
||||
subprocess.run(["git", "add", "."], cwd=temp_dir, capture_output=True)
|
||||
subprocess.run(
|
||||
["git", "commit", "-m", "Initial commit"],
|
||||
cwd=temp_dir, capture_output=True
|
||||
)
|
||||
IMPORTANT: This fixture properly isolates git operations by clearing
|
||||
git environment variables that may be set by pre-commit hooks. Without
|
||||
this isolation, git operations could affect the parent repository when
|
||||
tests run inside a git worktree (e.g., during pre-commit validation).
|
||||
|
||||
# Ensure branch is named 'main' (some git configs default to 'master')
|
||||
subprocess.run(["git", "branch", "-M", "main"], cwd=temp_dir, capture_output=True)
|
||||
See: https://git-scm.com/docs/git#_environment_variables
|
||||
"""
|
||||
# Save original environment values to restore later
|
||||
orig_env = {}
|
||||
|
||||
yield temp_dir
|
||||
# These git env vars may be set by pre-commit hooks and MUST be cleared
|
||||
# to avoid git operations affecting the parent repository instead of
|
||||
# our isolated test repo. This is critical when running inside worktrees.
|
||||
git_vars_to_clear = [
|
||||
"GIT_DIR",
|
||||
"GIT_WORK_TREE",
|
||||
"GIT_INDEX_FILE",
|
||||
"GIT_OBJECT_DIRECTORY",
|
||||
"GIT_ALTERNATE_OBJECT_DIRECTORIES",
|
||||
]
|
||||
|
||||
# Clear interfering git environment variables
|
||||
for key in git_vars_to_clear:
|
||||
orig_env[key] = os.environ.get(key)
|
||||
if key in os.environ:
|
||||
del os.environ[key]
|
||||
|
||||
# Set GIT_CEILING_DIRECTORIES to prevent git from discovering parent .git
|
||||
# directories. This is critical for test isolation when running inside
|
||||
# another git repo (like during pre-commit hooks in worktrees).
|
||||
orig_env["GIT_CEILING_DIRECTORIES"] = os.environ.get("GIT_CEILING_DIRECTORIES")
|
||||
os.environ["GIT_CEILING_DIRECTORIES"] = str(temp_dir.parent)
|
||||
|
||||
try:
|
||||
# Initialize git repo
|
||||
subprocess.run(["git", "init"], cwd=temp_dir, capture_output=True, check=True)
|
||||
subprocess.run(
|
||||
["git", "config", "user.email", "test@example.com"],
|
||||
cwd=temp_dir, capture_output=True
|
||||
)
|
||||
subprocess.run(
|
||||
["git", "config", "user.name", "Test User"],
|
||||
cwd=temp_dir, capture_output=True
|
||||
)
|
||||
|
||||
# Create initial commit
|
||||
test_file = temp_dir / "README.md"
|
||||
test_file.write_text("# Test Project\n")
|
||||
subprocess.run(["git", "add", "."], cwd=temp_dir, capture_output=True)
|
||||
subprocess.run(
|
||||
["git", "commit", "-m", "Initial commit"],
|
||||
cwd=temp_dir, capture_output=True
|
||||
)
|
||||
|
||||
# Ensure branch is named 'main' (some git configs default to 'master')
|
||||
subprocess.run(["git", "branch", "-M", "main"], cwd=temp_dir, capture_output=True)
|
||||
|
||||
yield temp_dir
|
||||
finally:
|
||||
# Restore original environment variables
|
||||
for key, value in orig_env.items():
|
||||
if value is None:
|
||||
os.environ.pop(key, None)
|
||||
else:
|
||||
os.environ[key] = value
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
|
||||
@@ -9,11 +9,12 @@ Contains:
|
||||
- Factory functions for creating test data
|
||||
"""
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
import sys
|
||||
from datetime import datetime
|
||||
from pathlib import Path
|
||||
from typing import Callable
|
||||
from typing import Callable, Generator
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import pytest
|
||||
@@ -164,35 +165,71 @@ class Greeter:
|
||||
# =============================================================================
|
||||
|
||||
@pytest.fixture
|
||||
def temp_project(tmp_path: Path) -> Path:
|
||||
"""Create a temporary project directory with git repo."""
|
||||
# Initialize git repo
|
||||
subprocess.run(["git", "init"], cwd=tmp_path, capture_output=True, check=True)
|
||||
subprocess.run(
|
||||
["git", "config", "user.email", "test@example.com"],
|
||||
cwd=tmp_path, capture_output=True
|
||||
)
|
||||
subprocess.run(
|
||||
["git", "config", "user.name", "Test User"],
|
||||
cwd=tmp_path, capture_output=True
|
||||
)
|
||||
def temp_project(tmp_path: Path) -> Generator[Path, None, None]:
|
||||
"""Create a temporary project directory with git repo.
|
||||
|
||||
# Create initial files
|
||||
(tmp_path / "src").mkdir()
|
||||
(tmp_path / "src" / "App.tsx").write_text(SAMPLE_REACT_COMPONENT)
|
||||
(tmp_path / "src" / "utils.py").write_text(SAMPLE_PYTHON_MODULE)
|
||||
IMPORTANT: This fixture properly isolates git operations by clearing
|
||||
git environment variables that may be set by pre-commit hooks. Without
|
||||
this isolation, git operations could affect the parent repository when
|
||||
tests run inside a git worktree (e.g., during pre-commit validation).
|
||||
"""
|
||||
# Save original environment values to restore later
|
||||
orig_env = {}
|
||||
|
||||
# Initial commit
|
||||
subprocess.run(["git", "add", "."], cwd=tmp_path, capture_output=True)
|
||||
subprocess.run(
|
||||
["git", "commit", "-m", "Initial commit"],
|
||||
cwd=tmp_path, capture_output=True
|
||||
)
|
||||
# These git env vars may be set by pre-commit hooks and MUST be cleared
|
||||
git_vars_to_clear = [
|
||||
"GIT_DIR",
|
||||
"GIT_WORK_TREE",
|
||||
"GIT_INDEX_FILE",
|
||||
"GIT_OBJECT_DIRECTORY",
|
||||
"GIT_ALTERNATE_OBJECT_DIRECTORIES",
|
||||
]
|
||||
|
||||
# Ensure branch is named 'main' (some git configs default to 'master')
|
||||
subprocess.run(["git", "branch", "-M", "main"], cwd=tmp_path, capture_output=True)
|
||||
# Clear interfering git environment variables
|
||||
for key in git_vars_to_clear:
|
||||
orig_env[key] = os.environ.get(key)
|
||||
if key in os.environ:
|
||||
del os.environ[key]
|
||||
|
||||
return tmp_path
|
||||
# Set GIT_CEILING_DIRECTORIES to prevent git from discovering parent .git
|
||||
orig_env["GIT_CEILING_DIRECTORIES"] = os.environ.get("GIT_CEILING_DIRECTORIES")
|
||||
os.environ["GIT_CEILING_DIRECTORIES"] = str(tmp_path.parent)
|
||||
|
||||
try:
|
||||
# Initialize git repo
|
||||
subprocess.run(["git", "init"], cwd=tmp_path, capture_output=True, check=True)
|
||||
subprocess.run(
|
||||
["git", "config", "user.email", "test@example.com"],
|
||||
cwd=tmp_path, capture_output=True
|
||||
)
|
||||
subprocess.run(
|
||||
["git", "config", "user.name", "Test User"],
|
||||
cwd=tmp_path, capture_output=True
|
||||
)
|
||||
|
||||
# Create initial files
|
||||
(tmp_path / "src").mkdir()
|
||||
(tmp_path / "src" / "App.tsx").write_text(SAMPLE_REACT_COMPONENT)
|
||||
(tmp_path / "src" / "utils.py").write_text(SAMPLE_PYTHON_MODULE)
|
||||
|
||||
# Initial commit
|
||||
subprocess.run(["git", "add", "."], cwd=tmp_path, capture_output=True)
|
||||
subprocess.run(
|
||||
["git", "commit", "-m", "Initial commit"],
|
||||
cwd=tmp_path, capture_output=True
|
||||
)
|
||||
|
||||
# Ensure branch is named 'main' (some git configs default to 'master')
|
||||
subprocess.run(["git", "branch", "-M", "main"], cwd=tmp_path, capture_output=True)
|
||||
|
||||
yield tmp_path
|
||||
finally:
|
||||
# Restore original environment variables
|
||||
for key, value in orig_env.items():
|
||||
if value is None:
|
||||
os.environ.pop(key, None)
|
||||
else:
|
||||
os.environ[key] = value
|
||||
|
||||
|
||||
# =============================================================================
|
||||
|
||||
+59
-22
@@ -11,6 +11,7 @@ Tests the recovery system functionality including:
|
||||
"""
|
||||
|
||||
import json
|
||||
import os
|
||||
import sys
|
||||
import tempfile
|
||||
import shutil
|
||||
@@ -24,7 +25,13 @@ from recovery import RecoveryManager, FailureType, RecoveryAction
|
||||
|
||||
|
||||
def setup_test_environment():
|
||||
"""Create temporary directories for testing."""
|
||||
"""Create temporary directories for testing.
|
||||
|
||||
IMPORTANT: This function properly isolates git operations by clearing
|
||||
git environment variables that may be set by pre-commit hooks. Without
|
||||
this isolation, git operations could affect the parent repository when
|
||||
tests run inside a git worktree (e.g., during pre-commit validation).
|
||||
"""
|
||||
temp_dir = Path(tempfile.mkdtemp())
|
||||
spec_dir = temp_dir / "spec"
|
||||
project_dir = temp_dir / "project"
|
||||
@@ -32,9 +39,27 @@ def setup_test_environment():
|
||||
spec_dir.mkdir(parents=True)
|
||||
project_dir.mkdir(parents=True)
|
||||
|
||||
# Initialize git repo in project dir
|
||||
# Clear git environment variables that may be set by pre-commit hooks
|
||||
# to avoid git operations affecting the parent repository
|
||||
import subprocess
|
||||
subprocess.run(["git", "init"], cwd=project_dir, capture_output=True)
|
||||
git_vars_to_clear = [
|
||||
"GIT_DIR",
|
||||
"GIT_WORK_TREE",
|
||||
"GIT_INDEX_FILE",
|
||||
"GIT_OBJECT_DIRECTORY",
|
||||
"GIT_ALTERNATE_OBJECT_DIRECTORIES",
|
||||
]
|
||||
|
||||
saved_env = {}
|
||||
for key in git_vars_to_clear:
|
||||
saved_env[key] = os.environ.pop(key, None)
|
||||
|
||||
# Set GIT_CEILING_DIRECTORIES to prevent git from discovering parent .git
|
||||
saved_env["GIT_CEILING_DIRECTORIES"] = os.environ.get("GIT_CEILING_DIRECTORIES")
|
||||
os.environ["GIT_CEILING_DIRECTORIES"] = str(temp_dir)
|
||||
|
||||
# Initialize git repo in project dir
|
||||
subprocess.run(["git", "init"], cwd=project_dir, capture_output=True, check=True)
|
||||
subprocess.run(["git", "config", "user.email", "test@example.com"], cwd=project_dir, capture_output=True)
|
||||
subprocess.run(["git", "config", "user.name", "Test User"], cwd=project_dir, capture_output=True)
|
||||
|
||||
@@ -44,19 +69,31 @@ def setup_test_environment():
|
||||
subprocess.run(["git", "add", "."], cwd=project_dir, capture_output=True)
|
||||
subprocess.run(["git", "commit", "-m", "Initial commit"], cwd=project_dir, capture_output=True)
|
||||
|
||||
return temp_dir, spec_dir, project_dir
|
||||
# Ensure branch is named 'main' (some git configs default to 'master')
|
||||
subprocess.run(["git", "branch", "-M", "main"], cwd=project_dir, capture_output=True)
|
||||
|
||||
# Return saved_env so caller can restore it in cleanup
|
||||
return temp_dir, spec_dir, project_dir, saved_env
|
||||
|
||||
|
||||
def cleanup_test_environment(temp_dir):
|
||||
"""Remove temporary directories."""
|
||||
def cleanup_test_environment(temp_dir, saved_env=None):
|
||||
"""Remove temporary directories and restore environment variables."""
|
||||
shutil.rmtree(temp_dir, ignore_errors=True)
|
||||
|
||||
# Restore original environment variables if provided
|
||||
if saved_env is not None:
|
||||
for key, value in saved_env.items():
|
||||
if value is None:
|
||||
os.environ.pop(key, None)
|
||||
else:
|
||||
os.environ[key] = value
|
||||
|
||||
|
||||
def test_initialization():
|
||||
"""Test RecoveryManager initialization."""
|
||||
print("TEST: Initialization")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -81,14 +118,14 @@ def test_initialization():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def test_record_attempt():
|
||||
"""Test recording chunk attempts."""
|
||||
print("TEST: Recording Attempts")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -130,14 +167,14 @@ def test_record_attempt():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def test_circular_fix_detection():
|
||||
"""Test circular fix detection."""
|
||||
print("TEST: Circular Fix Detection")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -162,14 +199,14 @@ def test_circular_fix_detection():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def test_failure_classification():
|
||||
"""Test failure type classification."""
|
||||
print("TEST: Failure Classification")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -192,14 +229,14 @@ def test_failure_classification():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def test_recovery_action_determination():
|
||||
"""Test recovery action determination."""
|
||||
print("TEST: Recovery Action Determination")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -232,14 +269,14 @@ def test_recovery_action_determination():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def test_good_commit_tracking():
|
||||
"""Test tracking of good commits."""
|
||||
print("TEST: Good Commit Tracking")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -285,14 +322,14 @@ def test_good_commit_tracking():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def test_mark_subtask_stuck():
|
||||
"""Test marking chunks as stuck."""
|
||||
print("TEST: Mark Chunk Stuck")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -319,14 +356,14 @@ def test_mark_subtask_stuck():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def test_recovery_hints():
|
||||
"""Test recovery hints generation."""
|
||||
print("TEST: Recovery Hints")
|
||||
|
||||
temp_dir, spec_dir, project_dir = setup_test_environment()
|
||||
temp_dir, spec_dir, project_dir, saved_env = setup_test_environment()
|
||||
|
||||
try:
|
||||
manager = RecoveryManager(spec_dir, project_dir)
|
||||
@@ -351,7 +388,7 @@ def test_recovery_hints():
|
||||
print()
|
||||
|
||||
finally:
|
||||
cleanup_test_environment(temp_dir)
|
||||
cleanup_test_environment(temp_dir, saved_env)
|
||||
|
||||
|
||||
def run_all_tests():
|
||||
|
||||
Reference in New Issue
Block a user