Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions refactron/verification/checks/test_gate.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,61 @@
import ast
from pathlib import Path
from typing import Dict, List, Optional

# Global cache for module -> relevant tests mapping
# Structure: {project_root_str: {target_module: [test_paths]}}
_TEST_DISCOVERY_CACHE: Dict[str, Dict[str, List[Path]]] = {}

Comment on lines +1 to +8
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: New code was prepended without integrating with existing file content.

The pipeline failures (F811, E402) confirm this file now has duplicate imports and a duplicate TestSuiteGate class definition. The new caching logic in lines 1-58 must be merged into the existing class at line 73, not added as a separate block.

Additionally, the module-level _TEST_DISCOVERY_CACHE is unbounded and will grow indefinitely in long-running processes. Consider adding a max-size eviction policy or using functools.lru_cache.

🧰 Tools
🪛 GitHub Actions: Pre-commit

[error] 1-1: pre-commit hook black failed and modified files (reformatted refactron/verification/checks/test_gate.py).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@refactron/verification/checks/test_gate.py` around lines 1 - 8, The new
caching code was prepended and created duplicate imports and a second
TestSuiteGate definition; remove the duplicated top-level imports and the extra
TestSuiteGate class and integrate the caching logic into the existing
TestSuiteGate class (use the class's existing methods that discover tests to
consult/update the cache named _TEST_DISCOVERY_CACHE rather than a separate
block). Replace the unbounded module-level dict by using functools.lru_cache or
a bounded eviction structure (or wrap cache accesses with a max-size policy) and
ensure all references use the same _TEST_DISCOVERY_CACHE symbol inside
TestSuiteGate so there is a single canonical implementation.


class TestSuiteGate:
def __init__(self, search_root: Path, project_root: Optional[Path] = None):
self.search_root = search_root
self.project_root = project_root

Comment on lines +10 to +14
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Duplicate class definition missing BaseCheck inheritance.

This TestSuiteGate class (line 10) conflicts with the existing TestSuiteGate(BaseCheck) at line 73. The new class:

  1. Does not inherit from BaseCheck, violating the verification engine contract
  2. Changes the constructor signature (adds required search_root parameter), breaking API compatibility
  3. Lacks the verify() method required by BaseCheck

The caching logic should be integrated into the existing class at line 73, not defined as a separate class.

Based on learnings: "BaseCheck ABC must have 3 implementations: SyntaxVerifier, ImportIntegrityVerifier, and TestSuiteGate in verification/engine.py"

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@refactron/verification/checks/test_gate.py` around lines 10 - 14, Remove the
duplicate TestSuiteGate class and merge its caching behavior into the existing
TestSuiteGate that inherits from BaseCheck; specifically, delete the standalone
class definition that defines __init__(self, search_root: Path, project_root:
Optional[Path] = None) and instead incorporate any cache fields and logic into
the existing TestSuiteGate(BaseCheck) implementation (keep its original
constructor signature and API), update its __init__ to initialize cache using
search_root/project_root values without changing parameters expected by callers,
and ensure the existing verify(self, ...) method implements the new caching
checks so the class continues to satisfy the BaseCheck contract used by
verification/engine.py.

def _imports_module(self, test_file: Path, target_module: str) -> bool:
try:
source_code = test_file.read_text(encoding="utf-8")
tree = ast.parse(source_code)

for node in ast.walk(tree):
if isinstance(node, ast.Import):
for alias in node.names:
if target_module in alias.name:
return True
elif isinstance(node, ast.ImportFrom):
if node.module and target_module in node.module:
return True
Comment on lines +21 to +27
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Bug: Substring matching produces false positives.

Using target_module in alias.name (lines 23, 26) matches any import containing the target as a substring. For example, searching for "util" would incorrectly match "utilities", "string_util", or "my_utility_module".

The existing implementation at line 197-201 correctly uses exact matching:

# Correct approach (from existing code)
if alias.name == module_name or alias.name.startswith(module_name + "."):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@refactron/verification/checks/test_gate.py` around lines 21 - 27, The
import-check currently uses substring matching (target_module in alias.name and
target_module in node.module) which causes false positives; update the checks in
the ast.Import and ast.ImportFrom branches to use exact-module or submodule
matching instead: replace "target_module in alias.name" with "alias.name ==
target_module or alias.name.startswith(target_module + '.')", and replace
"target_module in node.module" with "node.module == target_module or
node.module.startswith(target_module + '.')", keeping the rest of the logic
around ast.Import, ast.ImportFrom, alias.name and node.module unchanged.

except Exception:
pass
return False
Comment on lines +28 to +30
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Avoid silent exception swallowing.

Catching all exceptions and silently passing hides AST parsing errors, encoding issues, and other problems that would be valuable for debugging. At minimum, log the exception.

Proposed fix
+        import logging
+        logger = logging.getLogger(__name__)
+        # ...
         except Exception:
-            pass
+            logger.debug("Failed to parse %s for import detection", test_file, exc_info=True)
         return False
🧰 Tools
🪛 Ruff (0.15.7)

[error] 28-29: try-except-pass detected, consider logging the exception

(S110)


[warning] 28-28: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@refactron/verification/checks/test_gate.py` around lines 28 - 30, The code
currently swallows all exceptions in the except Exception: pass block (returning
False), which hides parsing/encoding errors; change the handler to except
Exception as e: and log the exception (for example using logging.exception(...)
or a module-level logger.exception(...)) before returning False so errors during
AST parsing in test_gate.py are visible; ensure the logging call includes
context (e.g., "failed to evaluate test gate" and the exception).


def _find_relevant_tests(self, target_module: str) -> List[Path]:
cache_key = str(self.project_root.resolve()) if self.project_root else str(self.search_root.resolve())

# Check cache
if cache_key in _TEST_DISCOVERY_CACHE and target_module in _TEST_DISCOVERY_CACHE[cache_key]:
return _TEST_DISCOVERY_CACHE[cache_key][target_module]

relevant_tests = []
ignore_patterns = {"venv", ".venv", "site-packages", "node_modules", ".git", ".tox", ".pytest_cache", "__pycache__"}

for p in self.search_root.rglob("*.py"):
# Skip obvious non-target paths if project_root is set
if self.project_root:
# Check if any parent directory is in the ignore list
if any(part in ignore_patterns for part in p.parts):
continue

if p.is_file() and p.name.startswith("test_"):
if self._imports_module(p, target_module):
relevant_tests.append(p)
Comment on lines +42 to +51
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Logic issues: Inconsistent filtering and incomplete test file detection.

  1. Inconsistent ignore filtering (lines 44-47): The ignore_patterns filter only applies when project_root is set, but rglob always scans from search_root. This means virtual environments and cache directories are scanned when project_root is None.

  2. Missing test file pattern (line 49): Only checks for test_ prefix, but the existing implementation at line 174 also matches *_test.py suffix:

    if not (name.startswith("test_") or name.endswith("_test.py")):
Proposed fix for test pattern
-            if p.is_file() and p.name.startswith("test_"):
+            if p.is_file() and (p.name.startswith("test_") or p.name.endswith("_test.py")):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@refactron/verification/checks/test_gate.py` around lines 42 - 51, The loop
scanning files via search_root.rglob("*.py") incorrectly applies the
ignore_patterns only when project_root is set and only recognizes test files
with a "test_" prefix; update the logic so the ignore check (any(part in
ignore_patterns for part in p.parts)) runs unconditionally for every candidate
from search_root.rglob, and broaden the test-file detection to accept either
p.name.startswith("test_") or p.name.endswith("_test.py"); keep the existing
p.is_file() and _imports_module(p, target_module) checks and append matching
paths to relevant_tests as before.


# Update cache
if cache_key not in _TEST_DISCOVERY_CACHE:
_TEST_DISCOVERY_CACHE[cache_key] = {}

_TEST_DISCOVERY_CACHE[cache_key][target_module] = relevant_tests
return relevant_tests
"""TestSuiteGate — Check 3: run relevant tests against transformed code."""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Module docstring is misplaced due to code prepending.

The module docstring should be at the top of the file (before imports). Its current position confirms that lines 1-58 were incorrectly prepended to the existing file content.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@refactron/verification/checks/test_gate.py` at line 59, The module docstring
string literal """TestSuiteGate — Check 3: run relevant tests against
transformed code.""" is not at the top of the file; move that docstring to be
the very first statement in the module (before any imports) and remove the
unintended prepended lines that pushed it down (the stray content added above
the imports), ensuring the file begins with the docstring and then the imports
and definitions (e.g., classes/functions in this module such as TestSuiteGate or
any test-related functions).


import ast
Expand Down
Loading