Skip to content

perf: Cache or narrow TestSuiteGate test discovery (avoid full rglob … #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shrutu0929 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

perf: Cache or narrow TestSuiteGate test discovery (avoid full rglob … #159

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 24 additions & 4 deletions refactron/verification/checks/test_gate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class TestSuiteGate(BaseCheck):

def __init__(self, project_root: Optional[Path] = None):
self.project_root = project_root
self._test_file_cache: Optional[Dict[str, List[Path]]] = None
self._all_test_files: Optional[List[Path]] = None
Comment on lines 21 to +24
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Add explicit -> None on __init__ to satisfy typed-def policy.

At Line 21, __init__ is missing an explicit return type, which violates the typed-def rule under refactron/.

Proposed fix 
-    def __init__(self, project_root: Optional[Path] = None):
+    def __init__(self, project_root: Optional[Path] = None) -> None:
         self.project_root = project_root
         self._test_file_cache: Optional[Dict[str, List[Path]]] = None
         self._all_test_files: Optional[List[Path]] = None

As per coding guidelines, "Type annotations are required in refactron/ with mypy disallow_untyped_defs = true enabled."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def __init__(self, project_root: Optional[Path] = None):
self.project_root = project_root
self._test_file_cache: Optional[Dict[str, List[Path]]] = None
self._all_test_files: Optional[List[Path]] = None
def __init__(self, project_root: Optional[Path] = None) -> None:
self.project_root = project_root
self._test_file_cache: Optional[Dict[str, List[Path]]] = None
self._all_test_files: Optional[List[Path]] = None
🤖 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 - 24, The
__init__ method in the TestGate class is missing an explicit return type; add an
explicit "-> None" annotation to the __init__ signature (the constructor defined
as def __init__(...)) to comply with the typed-def policy and mypy
disallow_untyped_defs; update the signature only (no body changes) so the method
becomes explicitly typed.


def verify(self, original: str, transformed: str, file_path: Path) -> CheckResult:
start = time.monotonic()
Expand Down Expand Up @@ -110,11 +112,27 @@ def _find_relevant_tests(self, file_path: Path) -> List[Path]:
module_name = file_path.stem
search_root = self.project_root or file_path.parent
Comment on lines 112 to 113
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Cache key should include module path, not only file stem.

At Line 112, using file_path.stem as the cache key can collide across packages (a/utils.py vs b/utils.py),
causing incorrect reuse from Line 131 and stale writes at Line 145.

Proposed fix 
         module_name = file_path.stem
+        module_cache_key = str(file_path.resolve())
         search_root = self.project_root or file_path.parent
@@
-        if module_name in self._test_file_cache:
-            return self._test_file_cache[module_name]
+        if module_cache_key in self._test_file_cache:
+            return self._test_file_cache[module_cache_key]
@@
-        self._test_file_cache[module_name] = test_files
+        self._test_file_cache[module_cache_key] = test_files
         return test_files

Also applies to: 131-132, 145-146

🤖 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 112 - 113, The cache
key currently uses module_name = file_path.stem which can collide across
packages; instead compute a module-scoped identifier based on the file path
relative to the search_root/project_root (use search_root or self.project_root
to derive file_path.relative_to(search_root) and strip the suffix, converting
separators to a dotted module path or a normalized path string) and replace all
uses of module_name in the cache lookup/write sites (the cache read around where
reuse occurs and the cache write where stale writes happen) to use this new
module_path; also defend against files not under the search_root by falling back
to file_path.as_posix() to avoid exceptions.


if self._test_file_cache is None:
self._test_file_cache = {}
self._all_test_files = []

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Remove trailing whitespace to unblock pre-commit.

Lines around Line 118/122/130/144 include trailing spaces, and pre-commit already failed on this hook.

Also applies to: 122-122, 130-130, 144-144

🤖 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 118, Remove the trailing
whitespace in the test file (test_gate.py) that is causing the pre-commit hook
to fail: open the file and delete the trailing spaces at the ends of the
affected lines, ensure your editor is set to trim trailing whitespace on save
(or run a trim command), then re-run the pre-commit hooks (or git commit) to
verify the hook passes before pushing.

test_dirs = [d for d in [search_root / "tests", search_root / "test"] if d.is_dir()]
search_dirs = test_dirs if test_dirs else [search_root]
Comment on lines +119 to +120
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Discovery narrowing can miss valid tests in mixed layouts.

At Line 120, when tests/ or test/ exists, discovery skips all other directories. Repos with colocated tests
(e.g., pkg/foo_test.py) will be silently excluded, which can let regressions pass verification.

Safer fallback pattern (keep fast path, avoid false negatives) 
             test_dirs = [d for d in [search_root / "tests", search_root / "test"] if d.is_dir()]
             search_dirs = test_dirs if test_dirs else [search_root]
@@
         test_files: List[Path] = []
         for py_file in self._all_test_files:  # type: ignore
@@
                 if self._imports_module(source, module_name):
                     test_files.append(py_file)
             except Exception:
                 continue
+
+        # Mixed-layout fallback: if canonical test dirs exist but yielded nothing relevant,
+        # do one broader pass to catch colocated tests.
+        if not test_files and test_dirs:
+            for py_file in search_root.rglob("*.py"):
+                if any(excluded in py_file.parts for excluded in excluded_dirs):
+                    continue
+                name = py_file.name
+                if not (name.startswith("test_") or name.endswith("_test.py")):
+                    continue
+                try:
+                    source = py_file.read_text(encoding="utf-8")
+                    if self._imports_module(source, module_name):
+                        test_files.append(py_file)
+                except Exception:
+                    continue
🤖 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 119 - 120, The
current discovery narrows search_dirs to only test_dirs when a tests/ or test/
exists, which misses colocated tests; modify the logic in test_gate.py so that
search_dirs includes both the found test directories and the repository root for
fallback (e.g., make search_dirs = test_dirs + [search_root] with
deduplication/ordering) instead of replacing the root entirely, ensuring
colocated tests like pkg/foo_test.py are still discovered while retaining the
fast-path of scanning tests/ or test/.

excluded_dirs = {".git", ".rag", "__pycache__", "venv", ".venv", "env", "node_modules"}

for root_dir in search_dirs:
for py_file in root_dir.rglob("*.py"):
if any(excluded in py_file.parts for excluded in excluded_dirs):
continue
name = py_file.name
if name.startswith("test_") or name.endswith("_test.py"):
self._all_test_files.append(py_file)

if module_name in self._test_file_cache:
return self._test_file_cache[module_name]

test_files: List[Path] = []
for py_file in search_root.rglob("*.py"):
name = py_file.name
if not (name.startswith("test_") or name.endswith("_test.py")):
continue
for py_file in self._all_test_files: # type: ignore
if py_file == file_path:
continue
try:
Expand All @@ -123,6 +141,8 @@ def _find_relevant_tests(self, file_path: Path) -> List[Path]:
test_files.append(py_file)
except Exception:
continue

self._test_file_cache[module_name] = test_files
return test_files

@staticmethod
Expand Down
Loading