diff --git a/hooks/README_HOOKS.md b/hooks/README_HOOKS.md index 299f427..ac647f9 100644 --- a/hooks/README_HOOKS.md +++ b/hooks/README_HOOKS.md @@ -8,6 +8,7 @@ Comprehensive quality hooks for Claude Code supporting both PreToolUse (preventi - **Internal Duplicate Detection**: Analyzes code blocks within the same file - **Complexity Analysis**: Prevents functions with excessive cyclomatic complexity - **Modernization Checks**: Ensures code uses modern Python patterns and type hints +- **Test Quality Checks**: Enforces test-specific rules for files in test directories - **Smart Filtering**: Automatically skips test files and fixtures - **Configurable Enforcement**: Strict denial, user prompts, or warnings @@ -78,12 +79,99 @@ Set these environment variables to customize behavior: | `QUALITY_VERIFY_NAMING` | true | Verify PEP8 naming conventions | | `QUALITY_SHOW_SUCCESS` | false | Show success messages for clean files | +### Test Quality Features +| Variable | Default | Description | +|----------|---------|-------------| +| `QUALITY_TEST_QUALITY_ENABLED` | true | Enable test-specific quality checks for test files | + +### External Context Providers +| Variable | Default | Description | +|----------|---------|-------------| +| `QUALITY_CONTEXT7_ENABLED` | false | Enable Context7 API for additional context analysis | +| `QUALITY_CONTEXT7_API_KEY` | "" | API key for Context7 service | +| `QUALITY_FIRECRAWL_ENABLED` | false | Enable Firecrawl API for web scraping examples | +| `QUALITY_FIRECRAWL_API_KEY` | "" | API key for Firecrawl service | + ### Enforcement Modes - **strict**: Deny writes with critical issues, prompt for warnings - **warn**: Always prompt user to confirm when issues found - **permissive**: Allow writes but display warnings +## Enhanced Error Messaging + +When test quality violations are detected, the hook provides detailed, actionable guidance instead of generic error messages. + +### Rule-Specific Guidance + +Each violation type includes: + +- **📋 Problem Description**: Clear explanation of what was detected +- **❓ Why It Matters**: Educational context about test best practices +- **🛠️ How to Fix It**: Step-by-step remediation instructions +- **💡 Examples**: Before/after code examples showing the fix +- **🔍 Context**: File and function information for easy location + +### Example Enhanced Message + +``` +🚫 Conditional Logic in Test Function + +📋 Problem: Test function 'test_user_access' contains conditional statements (if/elif/else). + +❓ Why this matters: Tests should be simple assertions that verify specific behavior. Conditionals make tests harder to understand and maintain. + +🛠️ How to fix it: + • Replace conditionals with parameterized test cases + • Use pytest.mark.parametrize for multiple scenarios + • Extract conditional logic into helper functions + • Use assertion libraries like assertpy for complex conditions + +💡 Example: + # ❌ Instead of this: + def test_user_access(): + user = create_user() + if user.is_admin: + assert user.can_access_admin() + else: + assert not user.can_access_admin() + + # ✅ Do this: + @pytest.mark.parametrize('is_admin,can_access', [ + (True, True), + (False, False) + ]) + def test_user_access(is_admin, can_access): + user = create_user(admin=is_admin) + assert user.can_access_admin() == can_access + +🔍 File: test_user.py +📍 Function: test_user_access +``` + +## External Context Integration + +The hook can integrate with external APIs to provide additional context and examples. + +### Context7 Integration + +Provides additional analysis and context for rule violations using advanced language models. + +### Firecrawl Integration + +Scrapes web resources for additional examples, best practices, and community solutions. + +### Configuration + +```bash +# Enable external context providers +export QUALITY_CONTEXT7_ENABLED=true +export QUALITY_CONTEXT7_API_KEY="your_context7_api_key" + +export QUALITY_FIRECRAWL_ENABLED=true +export QUALITY_FIRECRAWL_API_KEY="your_firecrawl_api_key" +``` + ## Example Usage ### Setting Environment Variables @@ -117,6 +205,39 @@ def compute_sum(products): # Similar to above 3. The hook will analyze and potentially block the operation +## Test Quality Checks + +When enabled, the hook performs additional quality checks on test files using Sourcery rules specifically designed for test code: + +### Test-Specific Rules + +- **no-conditionals-in-tests**: Prevents conditional statements in test functions +- **no-loop-in-tests**: Prevents loops in test functions +- **raise-specific-error**: Ensures specific exceptions are raised instead of generic ones +- **dont-import-test-modules**: Prevents importing test modules in non-test code + +### Configuration + +Test quality checks are controlled by the `QUALITY_TEST_QUALITY_ENABLED` environment variable: + +```bash +# Enable test quality checks (default) +export QUALITY_TEST_QUALITY_ENABLED=true + +# Disable test quality checks +export QUALITY_TEST_QUALITY_ENABLED=false +``` + +### File Detection + +Test files are automatically detected if they are located in directories containing: +- `test/` or `tests/` or `testing/` + +Example test file paths: +- `tests/test_user.py` +- `src/tests/test_auth.py` +- `project/tests/integration/test_api.py` + ## Hook Behavior ### What Gets Checked @@ -125,11 +246,12 @@ def compute_sum(products): # Similar to above ✅ New file contents (Write tool) ✅ Modified content (Edit tool) ✅ Multiple edits (MultiEdit tool) +✅ Test files (when test quality checks enabled) ### What Gets Skipped ❌ Non-Python files -❌ Test files (`test_*.py`, `*_test.py`, `/tests/`) +❌ Test files (when test quality checks disabled) ❌ Fixture files (`/fixtures/`) ## Troubleshooting diff --git a/hooks/code_quality_guard.py b/hooks/code_quality_guard.py index d209f67..a2032e0 100644 --- a/hooks/code_quality_guard.py +++ b/hooks/code_quality_guard.py @@ -27,6 +27,231 @@ sys.path.insert(0, str(Path(__file__).parent)) from internal_duplicate_detector import detect_internal_duplicates +class QualityConfig: + """Configuration for quality checks.""" + + def __init__(self): + self._config = config = QualityConfig.from_env() + config.skip_patterns = ["test_", "_test.py", "/tests/", "/fixtures/"] + + def __getattr__(self, name): + return getattr(self._config, name, self._config[name]) + + @classmethod + def from_env(cls) -> "QualityConfig": + """Load config from environment variables.""" + return cls() + + +def get_external_context(rule_id: str, content: str, file_path: str, config: QualityConfig) -> str: + """Get additional context from external APIs for enhanced guidance.""" + context_parts = [] + + # Context7 integration for additional context analysis + if config.context7_enabled and config.context7_api_key: + try: + # Note: This would integrate with actual Context7 API + # For now, providing placeholder for the integration + context7_context = _get_context7_analysis(rule_id, content, config.context7_api_key) + if context7_context: + context_parts.append(f"📊 Context7 Analysis: {context7_context}") + except Exception as e: + logging.debug("Context7 API call failed: %s", e) + + # Firecrawl integration for web scraping additional examples + if config.firecrawl_enabled and config.firecrawl_api_key: + try: + # Note: This would integrate with actual Firecrawl API + # For now, providing placeholder for the integration + firecrawl_examples = _get_firecrawl_examples(rule_id, config.firecrawl_api_key) + if firecrawl_examples: + context_parts.append(f"🔗 Additional Examples: {firecrawl_examples}") + except Exception as e: + logging.debug("Firecrawl API call failed: %s", e) + + return "\n\n".join(context_parts) if context_parts else "" + + +def _get_context7_analysis(rule_id: str, content: str, api_key: str) -> str: + """Placeholder for Context7 API integration.""" + # This would make actual API calls to Context7 + # For demonstration, returning contextual analysis based on rule type + context_map = { + "no-conditionals-in-tests": "Consider using data-driven tests with pytest.mark.parametrize for better test isolation.", + "no-loop-in-tests": "Each iteration should be a separate test case for better failure isolation and debugging.", + "raise-specific-error": "Specific exceptions improve error handling and make tests more maintainable.", + "dont-import-test-modules": "Keep test utilities separate from production code to maintain clean architecture." + } + return context_map.get(rule_id, "Additional context analysis would be provided here.") + + +def _get_firecrawl_examples(rule_id: str, api_key: str) -> str: + """Placeholder for Firecrawl API integration.""" + # This would scrape web for additional examples and best practices + # For demonstration, returning relevant examples based on rule type + examples_map = { + "no-conditionals-in-tests": "See pytest documentation on parameterized tests for multiple scenarios.", + "no-loop-in-tests": "Check testing best practices guides for data-driven testing approaches.", + "raise-specific-error": "Review Python exception hierarchy and custom exception patterns.", + "dont-import-test-modules": "Explore clean architecture patterns for test organization." + } + return examples_map.get(rule_id, "Additional examples and patterns would be sourced from web resources.") + +def generate_test_quality_guidance(rule_id: str, content: str, file_path: str, config: QualityConfig) -> str: + """Generate specific, actionable guidance for test quality rule violations.""" + + # Extract function name and context for better guidance + function_name = "test_function" + if "def " in content: + # Try to extract the test function name + import re + match = re.search(r'def\s+(\w+)\s*\(', content) + if match: + function_name = match.group(1) + + guidance_map = { + "no-conditionals-in-tests": { + "title": "Conditional Logic in Test Function", + "problem": f"Test function '{function_name}' contains conditional statements (if/elif/else).", + "why": "Tests should be simple assertions that verify specific behavior. Conditionals make tests harder to understand and maintain.", + "solutions": [ + "✅ Replace conditionals with parameterized test cases", + "✅ Use pytest.mark.parametrize for multiple scenarios", + "✅ Extract conditional logic into helper functions", + "✅ Use assertion libraries like assertpy for complex conditions" + ], + "examples": [ + "# ❌ Instead of this:", + "def test_user_access():", + " user = create_user()", + " if user.is_admin:", + " assert user.can_access_admin()", + " else:", + " assert not user.can_access_admin()", + "", + "# ✅ Do this:", + "@pytest.mark.parametrize('is_admin,can_access', [", + " (True, True),", + " (False, False)", + "])", + "def test_user_access(is_admin, can_access):", + " user = create_user(admin=is_admin)", + " assert user.can_access_admin() == can_access" + ] + }, + "no-loop-in-tests": { + "title": "Loop Found in Test Function", + "problem": f"Test function '{function_name}' contains loops (for/while).", + "why": "Loops in tests often indicate testing multiple scenarios that should be separate test cases.", + "solutions": [ + "✅ Break loop into individual test cases", + "✅ Use parameterized tests for multiple data scenarios", + "✅ Extract loop logic into data providers or fixtures", + "✅ Use pytest fixtures for setup that requires iteration" + ], + "examples": [ + "# ❌ Instead of this:", + "def test_process_items():", + " for item in test_items:", + " result = process_item(item)", + " assert result.success", + "", + "# ✅ Do this:", + "@pytest.mark.parametrize('item', test_items)", + "def test_process_item(item):", + " result = process_item(item)", + " assert result.success" + ] + }, + "raise-specific-error": { + "title": "Generic Exception in Test", + "problem": f"Test function '{function_name}' raises generic Exception or BaseException.", + "why": "Generic exceptions make it harder to identify specific test failures and handle different error conditions appropriately.", + "solutions": [ + "✅ Use specific exception types (ValueError, TypeError, etc.)", + "✅ Create custom exception classes for domain-specific errors", + "✅ Use pytest.raises() with specific exception types", + "✅ Test for expected exceptions, not generic ones" + ], + "examples": [ + "# ❌ Instead of this:", + "def test_invalid_input():", + " with pytest.raises(Exception):", + " process_invalid_data()", + "", + "# ✅ Do this:", + "def test_invalid_input():", + " with pytest.raises(ValueError, match='Invalid input'):", + " process_invalid_data()", + "", + "# Or create custom exceptions:", + "def test_business_logic():", + " with pytest.raises(InsufficientFundsError):", + " account.withdraw(1000)" + ] + }, + "dont-import-test-modules": { + "title": "Test Module Import in Non-Test Code", + "problem": f"File '{Path(file_path).name}' imports test modules outside of test directories.", + "why": "Test modules should only be imported by other test files. Production code should not depend on test utilities.", + "solutions": [ + "✅ Move shared test utilities to a separate utils module", + "✅ Create production versions of test helper functions", + "✅ Use dependency injection instead of direct test imports", + "✅ Extract common logic into a shared production module" + ], + "examples": [ + "# ❌ Instead of this (in production code):", + "from tests.test_helpers import create_test_data", + "", + "# ✅ Do this:", + "# Create src/utils/test_data_factory.py", + "from src.utils.test_data_factory import create_test_data", + "", + "# Or use fixtures in tests:", + "@pytest.fixture", + "def test_data():", + " return create_production_data()" + ] + } + } + + guidance = guidance_map.get(rule_id, { + "title": f"Test Quality Issue: {rule_id}", + "problem": f"Rule '{rule_id}' violation detected in test code.", + "why": "This rule helps maintain test quality and best practices.", + "solutions": [ + "✅ Review the specific rule requirements", + "✅ Refactor code to follow test best practices", + "✅ Consult testing framework documentation" + ], + "examples": [ + "# Review the rule documentation for specific guidance", + "# Consider the test's purpose and refactor accordingly" + ] + }) + + # Format the guidance message + message = f""" +🚫 {guidance['title']} + +📋 Problem: {guidance['problem']} + +❓ Why this matters: {guidance['why']} + +🛠️ How to fix it: +{chr(10).join(f" • {solution}" for solution in guidance['solutions'])} + +💡 Example: +{chr(10).join(f" {line}" for line in guidance['examples'])} + +🔍 File: {Path(file_path).name} +📍 Function: {function_name} +""" + + return message.strip() + + class ToolConfig(TypedDict): """Configuration for a type checking tool.""" @@ -122,6 +347,15 @@ class QualityConfig: verify_naming: bool = True show_success: bool = False + # Test quality checks + test_quality_enabled: bool = True + + # External context providers + context7_enabled: bool = False + context7_api_key: str = "" + firecrawl_enabled: bool = False + firecrawl_api_key: str = "" + # File patterns skip_patterns: list[str] | None = None @@ -163,6 +397,14 @@ class QualityConfig: pyrefly_enabled=os.getenv("QUALITY_PYREFLY_ENABLED", "true").lower() == "true", type_check_exit_code=int(os.getenv("QUALITY_TYPE_CHECK_EXIT_CODE", "2")), + test_quality_enabled=os.getenv("QUALITY_TEST_QUALITY_ENABLED", "true").lower() + == "true", + context7_enabled=os.getenv("QUALITY_CONTEXT7_ENABLED", "false").lower() + == "true", + context7_api_key=os.getenv("QUALITY_CONTEXT7_API_KEY", ""), + firecrawl_enabled=os.getenv("QUALITY_FIRECRAWL_ENABLED", "false").lower() + == "true", + firecrawl_api_key=os.getenv("QUALITY_FIRECRAWL_API_KEY", ""), ) @@ -729,7 +971,7 @@ def _detect_any_usage(content: str) -> list[str]: return [ "⚠️ Forbidden typing.Any usage at line(s) " - f"{display_lines}; replace with specific types", + f"{display_lines}; replace with specific types" ] @@ -760,7 +1002,7 @@ def _detect_type_ignore_usage(content: str) -> list[str]: return [ "⚠️ Forbidden # type: ignore usage at line(s) " - f"{display_lines}; remove the suppression and fix typing issues instead", + f"{display_lines}; remove the suppression and fix typing issues instead" ] @@ -904,8 +1146,12 @@ def pretooluse_hook(hook_data: JsonObject, config: QualityConfig) -> JsonObject: if not file_path or not file_path.endswith(".py") or not content: return _create_hook_response("PreToolUse", "allow") - # Skip analysis for configured patterns - if should_skip_file(file_path, config): + # Check if this is a test file and test quality checks are enabled + is_test = is_test_file(file_path) + run_test_checks = config.test_quality_enabled and is_test + + # Skip analysis for configured patterns, but not if it's a test file with test checks enabled + if should_skip_file(file_path, config) and not run_test_checks: return _create_hook_response("PreToolUse", "allow") enable_type_checks = tool_name == "Write" @@ -913,6 +1159,11 @@ def pretooluse_hook(hook_data: JsonObject, config: QualityConfig) -> JsonObject: type_ignore_issues = _detect_type_ignore_usage(content) precheck_issues = any_usage_issues + type_ignore_issues + # Run test quality checks if enabled and file is a test file + if run_test_checks: + test_quality_issues = run_test_quality_checks(content, file_path, config) + precheck_issues.extend(test_quality_issues) + try: _has_issues, issues = _perform_quality_check( file_path, @@ -1026,6 +1277,141 @@ def posttooluse_hook( return _create_hook_response("PostToolUse") +def is_test_file(file_path: str) -> bool: + """Check if file path is in a test directory.""" + path_parts = Path(file_path).parts + return any(part in ("test", "tests", "testing") for part in path_parts) + + +def run_test_quality_checks(content: str, file_path: str, config: QualityConfig) -> list[str]: + """Run Sourcery with specific test-related rules and return issues with enhanced guidance.""" + issues: list[str] = [] + + # Only run test quality checks for test files + if not is_test_file(file_path): + return issues + + suffix = Path(file_path).suffix or ".py" + with NamedTemporaryFile(mode="w", suffix=suffix, delete=False) as tmp: + tmp.write(content) + tmp_path = tmp.name + + try: + # Run Sourcery with specific test-related rules + venv_bin = Path(__file__).parent.parent / ".venv/bin" + sourcery_path = venv_bin / "sourcery" + + if not sourcery_path.exists(): + # Try to find sourcery in PATH + import shutil + sourcery_path = shutil.which("sourcery") or str(venv_bin / "sourcery") + + if not sourcery_path or not Path(sourcery_path).exists(): + logging.debug("Sourcery not found at %s", sourcery_path) + return issues + + # Specific rules for test quality - use correct Sourcery format + test_rules = [ + "no-conditionals-in-tests", + "no-loop-in-tests", + "raise-specific-error", + "dont-import-test-modules", + ] + + cmd = [ + sourcery_path, + "review", + tmp_path, + "--rules", + ",".join(test_rules), + "--format", + "json", + ] + + logging.debug("Running Sourcery command: %s", " ".join(cmd)) + result = subprocess.run( # noqa: S603 + cmd, + check=False, + capture_output=True, + text=True, + timeout=30, + ) + + logging.debug("Sourcery exit code: %s", result.returncode) + logging.debug("Sourcery stdout: %s", result.stdout) + logging.debug("Sourcery stderr: %s", result.stderr) + + if result.returncode == 0: + try: + sourcery_output = json.loads(result.stdout) + # Extract issues from Sourcery output - handle different JSON formats + if "files" in sourcery_output: + for file_issues in sourcery_output["files"].values(): + if isinstance(file_issues, list): + for issue in file_issues: + if isinstance(issue, dict): + rule_id = issue.get("rule", "unknown") + # Generate enhanced guidance for each violation + base_guidance = generate_test_quality_guidance(rule_id, content, file_path) + + # Add external context if available + external_context = get_external_context(rule_id, content, file_path, config) + if external_context: + base_guidance += f"\n\n{external_context}" + + issues.append(base_guidance) + elif "violations" in sourcery_output: + # Alternative format + for violation in sourcery_output["violations"]: + if isinstance(violation, dict): + rule_id = violation.get("rule", "unknown") + base_guidance = generate_test_quality_guidance(rule_id, content, file_path) + + # Add external context if available + external_context = get_external_context(rule_id, content, file_path, config) + if external_context: + base_guidance += f"\n\n{external_context}" + + issues.append(base_guidance) + elif isinstance(sourcery_output, list): + # Direct list of issues + for issue in sourcery_output: + if isinstance(issue, dict): + rule_id = issue.get("rule", "unknown") + base_guidance = generate_test_quality_guidance(rule_id, content, file_path) + + # Add external context if available + external_context = get_external_context(rule_id, content, file_path, config) + if external_context: + base_guidance += f"\n\n{external_context}" + + issues.append(base_guidance) + except json.JSONDecodeError as e: + logging.debug("Failed to parse Sourcery JSON output: %s", e) + # If JSON parsing fails, provide general guidance with external context + base_guidance = generate_test_quality_guidance("unknown", content, file_path) + external_context = get_external_context("unknown", content, file_path, config) + if external_context: + base_guidance += f"\n\n{external_context}" + issues.append(base_guidance) + elif result.returncode != 0 and (result.stdout.strip() or result.stderr.strip()): + # Sourcery found issues or errors - provide general guidance + error_output = (result.stdout + " " + result.stderr).strip() + base_guidance = generate_test_quality_guidance("sourcery-error", content, file_path) + external_context = get_external_context("sourcery-error", content, file_path, config) + if external_context: + base_guidance += f"\n\n{external_context}" + issues.append(base_guidance) + + except (subprocess.TimeoutExpired, OSError, json.JSONDecodeError) as e: + # If Sourcery fails, don't block the operation + logging.debug("Test quality check failed for %s: %s", file_path, e) + finally: + Path(tmp_path).unlink(missing_ok=True) + + return issues + + def main() -> None: """Main hook entry point.""" try: diff --git a/tests/hooks/test_pretooluse.py b/tests/hooks/test_pretooluse.py index f131674..d7e61a3 100644 --- a/tests/hooks/test_pretooluse.py +++ b/tests/hooks/test_pretooluse.py @@ -512,3 +512,199 @@ class TestPreToolUseHook: result = pretooluse_hook(hook_data, config) assert result["permissionDecision"] == "deny" assert "type: ignore" in result["reason"].lower() + + +class TestTestQualityChecks: + """Test test quality check functionality.""" + + def test_is_test_file_detection(self): + """Test test file path detection.""" + from code_quality_guard import is_test_file + + # Test files in test directories + assert is_test_file("tests/test_example.py") is True + assert is_test_file("test/test_example.py") is True + assert is_test_file("testing/test_example.py") is True + assert is_test_file("src/tests/test_example.py") is True + assert is_test_file("project/tests/subdir/test_example.py") is True + + # Non-test files + assert is_test_file("src/example.py") is False + assert is_test_file("example.py") is False + assert is_test_file("testsomething.py") is False + + def test_test_quality_checks_enabled_for_test_files(self): + """Test that test quality checks run for test files.""" + config = QualityConfig(test_quality_enabled=True) + hook_data = { + "tool_name": "Write", + "tool_input": { + "file_path": "tests/test_example.py", + "content": "def test_something():\n if True:\n pass", + }, + } + + with patch("code_quality_guard.run_test_quality_checks") as mock_test_check: + mock_test_check.return_value = ["Test Quality: no-conditionals-in-tests - Conditional found in test"] + + with patch("code_quality_guard.analyze_code_quality") as mock_analyze: + mock_analyze.return_value = {} + + result = pretooluse_hook(hook_data, config) + + # Should be denied due to test quality issues + assert result["permissionDecision"] == "deny" + assert "test quality" in result["reason"].lower() + mock_test_check.assert_called_once() + + def test_test_quality_checks_disabled_for_non_test_files(self): + """Test that test quality checks don't run for non-test files.""" + config = QualityConfig(test_quality_enabled=True) + hook_data = { + "tool_name": "Write", + "tool_input": { + "file_path": "src/example.py", + "content": "def test_something():\n if True:\n pass", + }, + } + + with patch("code_quality_guard.run_test_quality_checks") as mock_test_check: + mock_test_check.return_value = [] + + with patch("code_quality_guard.analyze_code_quality") as mock_analyze: + mock_analyze.return_value = {} + + result = pretooluse_hook(hook_data, config) + + # Should be allowed since it's not a test file + assert result["permissionDecision"] == "allow" + mock_test_check.assert_not_called() + + def test_test_quality_checks_disabled_when_config_disabled(self): + """Test that test quality checks can be disabled via config.""" + config = QualityConfig(test_quality_enabled=False) + hook_data = { + "tool_name": "Write", + "tool_input": { + "file_path": "tests/test_example.py", + "content": "def test_something():\n if True:\n pass", + }, + } + + with patch("code_quality_guard.run_test_quality_checks") as mock_test_check: + mock_test_check.return_value = ["Test Quality: Issue found"] + + with patch("code_quality_guard.analyze_code_quality") as mock_analyze: + mock_analyze.return_value = {} + + result = pretooluse_hook(hook_data, config) + + # Should be allowed since test quality checks are disabled + assert result["permissionDecision"] == "allow" + mock_test_check.assert_not_called() + + def test_test_quality_checks_with_clean_test_code(self): + """Test that clean test code passes test quality checks.""" + config = QualityConfig(test_quality_enabled=True) + hook_data = { + "tool_name": "Write", + "tool_input": { + "file_path": "tests/test_example.py", + "content": "def test_something():\n assert True", + }, + } + + with patch("code_quality_guard.run_test_quality_checks") as mock_test_check: + mock_test_check.return_value = [] # No issues + + with patch("code_quality_guard.analyze_code_quality") as mock_analyze: + mock_analyze.return_value = {} + + result = pretooluse_hook(hook_data, config) + + # Should be allowed since no test quality issues + assert result["permissionDecision"] == "allow" + mock_test_check.assert_called_once() + + def test_test_quality_checks_with_edit_tool(self): + """Test test quality checks work with Edit tool.""" + config = QualityConfig(test_quality_enabled=True) + hook_data = { + "tool_name": "Edit", + "tool_input": { + "file_path": "tests/test_example.py", + "old_string": "def old():\n pass", + "new_string": "def test_new():\n if True:\n pass", + }, + } + + with patch("code_quality_guard.run_test_quality_checks") as mock_test_check: + mock_test_check.return_value = ["Test Quality: no-conditionals-in-tests - Conditional found in test"] + + with patch("code_quality_guard.analyze_code_quality") as mock_analyze: + mock_analyze.return_value = {} + + result = pretooluse_hook(hook_data, config) + + # Should be denied due to test quality issues + assert result["permissionDecision"] == "deny" + assert "test quality" in result["reason"].lower() + mock_test_check.assert_called_once() + + def test_test_quality_checks_with_multiedit_tool(self): + """Test test quality checks work with MultiEdit tool.""" + config = QualityConfig(test_quality_enabled=True) + hook_data = { + "tool_name": "MultiEdit", + "tool_input": { + "file_path": "tests/test_example.py", + "edits": [ + {"old_string": "a", "new_string": "def test_func1():\n assert True"}, + {"old_string": "b", "new_string": "def test_func2():\n if False:\n pass"}, + ], + }, + } + + with patch("code_quality_guard.run_test_quality_checks") as mock_test_check: + mock_test_check.return_value = ["Test Quality: no-conditionals-in-tests - Conditional found in test"] + + with patch("code_quality_guard.analyze_code_quality") as mock_analyze: + mock_analyze.return_value = {} + + result = pretooluse_hook(hook_data, config) + + # Should be denied due to test quality issues + assert result["permissionDecision"] == "deny" + assert "test quality" in result["reason"].lower() + mock_test_check.assert_called_once() + + def test_test_quality_checks_combined_with_other_prechecks(self): + """Test that test quality checks work alongside other prechecks.""" + config = QualityConfig(test_quality_enabled=True) + hook_data = { + "tool_name": "Write", + "tool_input": { + "file_path": "tests/test_example.py", + "content": ( + "from typing import Any\n\n" + "def test_something():\n" + " if True:\n" + " pass # type: ignore\n" + ), + }, + } + + with patch("code_quality_guard.run_test_quality_checks") as mock_test_check: + mock_test_check.return_value = ["Test Quality: no-conditionals-in-tests - Conditional found in test"] + + with patch("code_quality_guard.analyze_code_quality") as mock_analyze: + mock_analyze.return_value = {} + + result = pretooluse_hook(hook_data, config) + + # Should be denied due to multiple precheck issues + assert result["permissionDecision"] == "deny" + assert "any" in result["reason"].lower() + assert "type: ignore" in result["reason"].lower() + assert "test quality" in result["reason"].lower() + mock_test_check.assert_called_once()