feat: implement test quality checks with enhanced guidance and external context integration
- Added a new QualityConfig class to manage test quality check configurations. - Implemented test quality checks for specific rules in test files, including prevention of conditionals, loops, and generic exceptions. - Integrated external context providers (Context7 and Firecrawl) for additional guidance on test quality violations. - Enhanced error messaging to provide detailed, actionable guidance for detected issues. - Updated README_HOOKS.md to document new test quality features and configuration options. - Added unit tests to verify the functionality of test quality checks and their integration with the pretooluse_hook.
This commit is contained in:
@@ -8,6 +8,7 @@ Comprehensive quality hooks for Claude Code supporting both PreToolUse (preventi
|
|||||||
- **Internal Duplicate Detection**: Analyzes code blocks within the same file
|
- **Internal Duplicate Detection**: Analyzes code blocks within the same file
|
||||||
- **Complexity Analysis**: Prevents functions with excessive cyclomatic complexity
|
- **Complexity Analysis**: Prevents functions with excessive cyclomatic complexity
|
||||||
- **Modernization Checks**: Ensures code uses modern Python patterns and type hints
|
- **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
|
- **Smart Filtering**: Automatically skips test files and fixtures
|
||||||
- **Configurable Enforcement**: Strict denial, user prompts, or warnings
|
- **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_VERIFY_NAMING` | true | Verify PEP8 naming conventions |
|
||||||
| `QUALITY_SHOW_SUCCESS` | false | Show success messages for clean files |
|
| `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
|
### Enforcement Modes
|
||||||
|
|
||||||
- **strict**: Deny writes with critical issues, prompt for warnings
|
- **strict**: Deny writes with critical issues, prompt for warnings
|
||||||
- **warn**: Always prompt user to confirm when issues found
|
- **warn**: Always prompt user to confirm when issues found
|
||||||
- **permissive**: Allow writes but display warnings
|
- **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
|
## Example Usage
|
||||||
|
|
||||||
### Setting Environment Variables
|
### Setting Environment Variables
|
||||||
@@ -117,6 +205,39 @@ def compute_sum(products): # Similar to above
|
|||||||
|
|
||||||
3. The hook will analyze and potentially block the operation
|
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
|
## Hook Behavior
|
||||||
|
|
||||||
### What Gets Checked
|
### What Gets Checked
|
||||||
@@ -125,11 +246,12 @@ def compute_sum(products): # Similar to above
|
|||||||
✅ New file contents (Write tool)
|
✅ New file contents (Write tool)
|
||||||
✅ Modified content (Edit tool)
|
✅ Modified content (Edit tool)
|
||||||
✅ Multiple edits (MultiEdit tool)
|
✅ Multiple edits (MultiEdit tool)
|
||||||
|
✅ Test files (when test quality checks enabled)
|
||||||
|
|
||||||
### What Gets Skipped
|
### What Gets Skipped
|
||||||
|
|
||||||
❌ Non-Python files
|
❌ Non-Python files
|
||||||
❌ Test files (`test_*.py`, `*_test.py`, `/tests/`)
|
❌ Test files (when test quality checks disabled)
|
||||||
❌ Fixture files (`/fixtures/`)
|
❌ Fixture files (`/fixtures/`)
|
||||||
|
|
||||||
## Troubleshooting
|
## Troubleshooting
|
||||||
|
|||||||
@@ -27,6 +27,231 @@ sys.path.insert(0, str(Path(__file__).parent))
|
|||||||
from internal_duplicate_detector import detect_internal_duplicates
|
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):
|
class ToolConfig(TypedDict):
|
||||||
"""Configuration for a type checking tool."""
|
"""Configuration for a type checking tool."""
|
||||||
|
|
||||||
@@ -122,6 +347,15 @@ class QualityConfig:
|
|||||||
verify_naming: bool = True
|
verify_naming: bool = True
|
||||||
show_success: bool = False
|
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
|
# File patterns
|
||||||
skip_patterns: list[str] | None = None
|
skip_patterns: list[str] | None = None
|
||||||
|
|
||||||
@@ -163,6 +397,14 @@ class QualityConfig:
|
|||||||
pyrefly_enabled=os.getenv("QUALITY_PYREFLY_ENABLED", "true").lower()
|
pyrefly_enabled=os.getenv("QUALITY_PYREFLY_ENABLED", "true").lower()
|
||||||
== "true",
|
== "true",
|
||||||
type_check_exit_code=int(os.getenv("QUALITY_TYPE_CHECK_EXIT_CODE", "2")),
|
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 [
|
return [
|
||||||
"⚠️ Forbidden typing.Any usage at line(s) "
|
"⚠️ 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 [
|
return [
|
||||||
"⚠️ Forbidden # type: ignore usage at line(s) "
|
"⚠️ 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:
|
if not file_path or not file_path.endswith(".py") or not content:
|
||||||
return _create_hook_response("PreToolUse", "allow")
|
return _create_hook_response("PreToolUse", "allow")
|
||||||
|
|
||||||
# Skip analysis for configured patterns
|
# Check if this is a test file and test quality checks are enabled
|
||||||
if should_skip_file(file_path, config):
|
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")
|
return _create_hook_response("PreToolUse", "allow")
|
||||||
|
|
||||||
enable_type_checks = tool_name == "Write"
|
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)
|
type_ignore_issues = _detect_type_ignore_usage(content)
|
||||||
precheck_issues = any_usage_issues + type_ignore_issues
|
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:
|
try:
|
||||||
_has_issues, issues = _perform_quality_check(
|
_has_issues, issues = _perform_quality_check(
|
||||||
file_path,
|
file_path,
|
||||||
@@ -1026,6 +1277,141 @@ def posttooluse_hook(
|
|||||||
return _create_hook_response("PostToolUse")
|
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:
|
def main() -> None:
|
||||||
"""Main hook entry point."""
|
"""Main hook entry point."""
|
||||||
try:
|
try:
|
||||||
|
|||||||
@@ -512,3 +512,199 @@ class TestPreToolUseHook:
|
|||||||
result = pretooluse_hook(hook_data, config)
|
result = pretooluse_hook(hook_data, config)
|
||||||
assert result["permissionDecision"] == "deny"
|
assert result["permissionDecision"] == "deny"
|
||||||
assert "type: ignore" in result["reason"].lower()
|
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()
|
||||||
|
|||||||
Reference in New Issue
Block a user