Project_Scanner: PythonScanner fixes and testing

src/rejuvenation/cli.py

@@ -9,6 +9,9 @@
 from renaissance.recipes.python_refactoring import PythonRefactoring
 from renaissance.syntax_tree import ASTShower
 
+# TODO: is this file working as intended?
+# See https://github.com/TNO/Renaissance.Py/issues/196
+
 
 def refactor():
     """AI: Run the `refactor`/`extract`/`inspect` CLI subcommands based on `sys.argv`."""

src/renaissance/project/project_scanner.py

@@ -1,87 +1,104 @@
-"""AI: Scanners that discover project source files from a compilation database or file system."""
+"""Language-specific source file scanners used to feed recipes with the files to process."""
 
 import json
-from os import system
+import subprocess
 from pathlib import Path
 
 
 class ProjectScanner:
-    """AI: Base scanner interface for discovering a project's source files."""
+    """Base class for language-specific source file scanners."""
 
     def find_sources(self) -> list[str]:
-        """AI: Discover and return this project's source file paths."""
+        """Return the paths of source files found by this scanner. Implemented by subclasses."""
         raise NotImplementedError
 
 
 class CppScanner(ProjectScanner):
-    """AI: Scanner that discovers C/C++ sources from a compilation database."""
+    """Scan a C/C++ project's compile_commands.json for source files."""
 
-    def __init__(self, compile_commands_path: str = "compile_commands.json"):
-        """AI: Configure a scanner that discovers C/C++ sources from a compilation database."""
+    def __init__(self, compile_commands_path: str = "compile_commands.json") -> None:
+        """Store the path to the compile_commands.json to read sources from."""
         self.compile_commands_path = compile_commands_path
 
     def find_sources(self) -> list[str]:
-        """AI: Discover C/C++ source files listed in the compilation database."""
+        """Return every file entry listed in compile_commands.json, sorted and deduplicated."""
         if not Path(self.compile_commands_path).exists():
-            raise FileNotFoundError("compile_commands.json not found")
+            message = "compile_commands.json not found"
+            raise FileNotFoundError(message)
         with Path(self.compile_commands_path).open() as f:
             commands = json.load(f)
-        return sorted(set(entry["file"] for entry in commands if "file" in entry))
+        return sorted({entry["file"] for entry in commands if "file" in entry})
 
 
 class JavaScanner(ProjectScanner):
-    """AI: Scanner that discovers Java sources under a root directory."""
+    """Scan a directory tree for Java source files."""
 
-    def __init__(self, root_dir: str = "."):
-        """AI: Configure a scanner that discovers Java sources under a root directory."""
+    def __init__(self, root_dir: str = ".") -> None:
+        """Store the root directory to scan for .java files."""
         self.root_dir = root_dir
 
     def find_sources(self) -> list[str]:
-        """AI: Discover Java source files recursively under the root directory."""
+        """Return every .java file under root_dir, sorted."""
+        # TODO: is this correct? does this filter out files correctly?
+        # See https://github.com/TNO/Renaissance.Py/issues/200
+
         java_files = Path(self.root_dir).rglob("*.java")
         return sorted(str(f) for f in java_files)
 
 
 class PythonScanner(ProjectScanner):
-    """AI: Scanner that discovers Python sources under known package directories."""
+    """Scan a directory tree for Python source files."""
+
+    EXCLUDED_DIRS = frozenset({".git", "__pycache__", ".venv", "venv"})
+    # TODO: incomplete list, extend this list with more files/directories that should always be ignored
 
-    def __init__(self, root_dir: str = ".", package_dirs: list[str] | None = None):
-        """AI: Configure a scanner that discovers Python sources under known package directories."""
-        # return (file_path for file_path in current_dir.iterdir() if is_python_file)
+    def __init__(self, root_dir: str = ".", package_dirs: list[str] | None = None) -> None:
+        """Store the scan root and optional package_dirs narrowing.
 
+        package_dirs, when given, narrows the scan to those subdirectories of root_dir.
+        Left as None (the default), the whole of root_dir is scanned instead of assuming a
+        src/lib/test layout, since that assumption silently skipped real third-party layouts.
+        """
         self.root_dir = root_dir
-        self.package_dirs = package_dirs or ["src", "lib", "test"]
-        # TODO: Why this hardcoded default heuristic?
-        #       Why not what Python by default enforces or what is derived from the project config?
+        self.package_dirs = package_dirs
 
     def find_sources(self) -> list[str]:
-        """AI: Discover Python source files under each configured package directory."""
-        files = []
-
-        for d in self.package_dirs:
-            file_path = Path(self.root_dir) / d
-            if file_path.exists():
-                files.extend(file_path.glob("**/*.py"))
-        return sorted(files)
+        """Return every .py file under root_dir (or package_dirs, if given), sorted, excluding EXCLUDED_DIRS."""
+        path = Path(self.root_dir)
+        if not path.exists():
+            message = f"root_dir does not exist: {self.root_dir}"
+            raise FileNotFoundError(message)
+        if not path.is_dir():
+            message = f"root_dir is not a directory: {self.root_dir}"
+            raise NotADirectoryError(message)
+
+        roots = [Path(self.root_dir) / d for d in self.package_dirs] if self.package_dirs else [Path(self.root_dir)]
+        files: list[Path] = []
+        for root in roots:
+            if not root.exists():
+                continue
+            files.extend(path for path in root.rglob("*.py") if not any(part in self.EXCLUDED_DIRS for part in path.parts))
+        return sorted(str(path) for path in files)
 
 
 class BearCppScanner(CppScanner):
-    """AI: Scanner that generates a compilation database via Bear before discovering sources."""
+    """CppScanner that generates compile_commands.json via Bear when it's missing."""
 
-    def __init__(self, build_dir: str = ".", compile_commands_path: str = "compile_commands.json"):
-        """AI: Configure a scanner that generates a compilation database via Bear before discovering sources."""
+    def __init__(self, build_dir: str = ".", compile_commands_path: str = "compile_commands.json") -> None:
+        """Store the build directory to run Bear in, alongside the compile_commands.json path."""
         super().__init__(compile_commands_path)
         self.build_dir = build_dir
 
-    def run_bear(self):
-        """AI: Regenerate the compilation database by running Bear over the configured build."""
-        print("Running Bear to generate compile_commands.json...")
-        result = system(f"bear -- make -C {self.build_dir}")
-        if result != 0:
-            raise RuntimeError("Bear failed to run or make failed.")
+    def run_bear(self) -> None:
+        """Run Bear to generate compile_commands.json by wrapping the project's make invocation."""
+        # bear is resolved via PATH intentionally; build_dir isn't attacker-controlled here.
+        result = subprocess.run(["bear", "--", "make", "-C", self.build_dir], check=False)  # noqa: S603, S607
+        if result.returncode != 0:
+            message = "Bear failed to run or make failed."
+            raise RuntimeError(message)
 
     def find_sources(self) -> list[str]:
-        """AI: Generate the compilation database via Bear if missing, then discover C/C++ sources from it."""
+        """Generate compile_commands.json via Bear if missing, then return its listed sources."""
         if not Path(self.compile_commands_path).exists():
             self.run_bear()
         return super().find_sources()

test/project/test_project_scanner.py

@@ -1,6 +1,8 @@
 """Tests for the project source-file scanners."""
 
 import json
+from pathlib import Path
+from unittest.mock import Mock
 
 import pytest
 from hamcrest import assert_that, calling, contains_inanyorder, empty, equal_to, is_, raises
@@ -196,15 +198,69 @@ def test_searches_multiple_package_dirs(self, tmp_path):
         )
 
     def test_default_package_dirs(self):
-        """AI: Assert PythonScanner defaults package_dirs to ["src", "lib", "test"]."""
+        """Assert PythonScanner defaults package_dirs to None."""
         scanner = PythonScanner()
-        assert_that(scanner.package_dirs, equal_to(["src", "lib", "test"]))
+        assert_that(scanner.package_dirs, is_(None))
 
     def test_default_root_dir(self):
         """AI: Assert PythonScanner defaults root_dir to "."."""
         scanner = PythonScanner()
         assert_that(scanner.root_dir, is_("."))
 
+    def test_raises_when_root_dir_does_not_exist(self, tmp_path):
+        """Assert PythonScanner.find_sources raises FileNotFoundError when root_dir doesn't exist."""
+        scanner = PythonScanner(str(tmp_path / "does-not-exist"))
+
+        assert_that(calling(scanner.find_sources), raises(FileNotFoundError))
+
+    def test_raises_when_root_dir_is_not_a_directory(self, tmp_path):
+        """Assert PythonScanner.find_sources raises NotADirectoryError when root_dir is a file."""
+        root_file = tmp_path / "not_a_dir.py"
+        root_file.write_text("")
+        scanner = PythonScanner(str(root_file))
+
+        assert_that(calling(scanner.find_sources), raises(NotADirectoryError))
+
+    @pytest.mark.parametrize("excluded_dir", sorted(PythonScanner.EXCLUDED_DIRS))
+    def test_excludes_known_noise_dirs_in_default_whole_tree_scan(self, tmp_path, excluded_dir):
+        """Assert PythonScanner.find_sources excludes known noise directories during a default whole-tree scan."""
+        noise_dir = tmp_path / excluded_dir
+        noise_dir.mkdir()
+        (noise_dir / "ignored.py").write_text("")
+        (tmp_path / "kept.py").write_text("")
+
+        scanner = PythonScanner(str(tmp_path))
+        result = [Path(p).name for p in scanner.find_sources()]
+
+        assert_that(result, equal_to(["kept.py"]))
+
+    @pytest.mark.parametrize("excluded_dir", sorted(PythonScanner.EXCLUDED_DIRS))
+    def test_excludes_known_noise_dirs_within_explicit_package_dirs(self, tmp_path, excluded_dir):
+        """Assert PythonScanner.find_sources excludes known noise directories within an explicit package_dirs."""
+        src = tmp_path / "src"
+        src.mkdir()
+        noise_dir = src / excluded_dir
+        noise_dir.mkdir()
+        (noise_dir / "ignored.py").write_text("")
+        (src / "kept.py").write_text("")
+
+        scanner = PythonScanner(str(tmp_path), package_dirs=["src"])
+        result = [Path(p).name for p in scanner.find_sources()]
+
+        assert_that(result, equal_to(["kept.py"]))
+
+    def test_default_package_dirs_scans_whole_root_dir(self, tmp_path):
+        """Assert PythonScanner scans the whole root_dir by default when package_dirs is not given."""
+        # Motivating case: source living outside src/lib/test (e.g. redis-py's redis/ layout).
+        redis_like = tmp_path / "redis"
+        redis_like.mkdir()
+        (redis_like / "client.py").write_text("")
+
+        scanner = PythonScanner(str(tmp_path))
+        result = [str(p) for p in scanner.find_sources()]
+
+        assert_that(result, equal_to([str(redis_like / "client.py")]))
+
 
 # ---------------------------------------------------------------------------
 # BearCppScanner
@@ -249,14 +305,14 @@ def test_find_sources_does_not_call_run_bear_when_compile_commands_exists(self,
     def test_run_bear_raises_on_nonzero_exit(self, mocker):
         """AI: Assert BearCppScanner.run_bear raises RuntimeError when the Bear subprocess exits non-zero."""
         scanner = BearCppScanner()
-        mocker.patch("renaissance.project.project_scanner.system", return_value=1)
+        mocker.patch("renaissance.project.project_scanner.subprocess.run", return_value=Mock(returncode=1))
 
         assert_that(calling(scanner.run_bear), raises(RuntimeError))
 
     def test_run_bear_succeeds_on_zero_exit(self, mocker):
         """AI: Assert BearCppScanner.run_bear does not raise when the Bear subprocess exits zero."""
         scanner = BearCppScanner()
-        mocker.patch("renaissance.project.project_scanner.system", return_value=0)
+        mocker.patch("renaissance.project.project_scanner.subprocess.run", return_value=Mock(returncode=0))
 
         # Should not raise
         scanner.run_bear()