[TECH DEBT] Document and test Folder and FolderLoader (#3651)

## Changes

Document and test `Folder` and `FolderLoader`

### Linked issues

Progresses #3645

### Functionality

- [x] modified `Folder` and `FolderLoader`

### Tests

- [x] added unit tests

Author: JCZuurmond

src/databricks/labs/ucx/source_code/folders.py

@@ -17,6 +17,18 @@
 
 
 class Folder(SourceContainer):
+    """A source container that represents a folder."""
+
+    # The following paths names are ignore as they do not contain source code
+    _IGNORE_PATH_NAMES = {
+        "__pycache__",
+        ".mypy_cache",
+        ".git",
+        ".github",
+        # Code from libraries are accessed through `imports`, not directly via the folder
+        ".venv",
+        "site-packages",
+    }
 
     def __init__(
         self,
@@ -30,17 +42,18 @@ def __init__(
         self._file_loader = file_loader
         self._folder_loader = folder_loader
 
-    @property
-    def path(self) -> Path:
-        return self._path
-
     def build_dependency_graph(self, parent: DependencyGraph) -> list[DependencyProblem]:
-        # don't directly scan non-source directories, let it be done for relevant imports only
-        if self._path.name in {"__pycache__", ".git", ".github", ".venv", ".mypy_cache", "site-packages"}:
+        """Build the dependency graph for the folder.
+
+        Here we skip certain directories, like:
+        - the ones that are not source code.
+        """
+        if self._path.name in self._IGNORE_PATH_NAMES:
             return []
         return list(self._build_dependency_graph(parent))
 
     def _build_dependency_graph(self, parent: DependencyGraph) -> Iterable[DependencyProblem]:
+        """Build the dependency graph for the contents of the folder."""
         for child_path in self._path.iterdir():
             is_file = child_path.is_file()
             is_notebook = is_a_notebook(child_path)
@@ -60,6 +73,7 @@ def __init__(self, notebook_loader: NotebookLoader, file_loader: FileLoader):
         self._file_loader = file_loader
 
     def load_dependency(self, path_lookup: PathLookup, dependency: Dependency) -> Folder | None:
+        """Load the folder as a dependency."""
         absolute_path = path_lookup.resolve(dependency.path)
         if not absolute_path:
             return None

tests/unit/source_code/linters/test_files.py

@@ -13,7 +13,7 @@
 from databricks.sdk.service.workspace import Language
 
 from databricks.labs.ucx.hive_metastore.table_migration_status import TableMigrationIndex
-from databricks.labs.ucx.source_code.folders import Folder, FolderLoader
+from databricks.labs.ucx.source_code.folders import FolderLoader
 from databricks.labs.ucx.source_code.files import FileLoader, ImportFileResolver
 from databricks.labs.ucx.source_code.linters.folders import LocalCodeLinter, LocalFileMigrator
 from databricks.labs.ucx.source_code.linters.context import LinterContext
@@ -167,13 +167,6 @@ def test_single_dot_import() -> None:
     path_lookup.resolve.assert_called_once_with(Path('/some/path/to/folder/foo.py'))
 
 
-def test_folder_has_repr() -> None:
-    notebook_loader = NotebookLoader()
-    file_loader = FileLoader()
-    folder = Folder(Path("test"), notebook_loader, file_loader, FolderLoader(notebook_loader, file_loader))
-    assert len(repr(folder)) > 0
-
-
 site_packages = locate_site_packages()
 
 

tests/unit/source_code/test_folders.py

@@ -0,0 +1,80 @@
+from pathlib import Path
+from unittest.mock import create_autospec
+
+import pytest
+
+from databricks.labs.ucx.source_code.base import CurrentSessionState
+from databricks.labs.ucx.source_code.files import FileLoader
+from databricks.labs.ucx.source_code.folders import FolderLoader, Folder
+from databricks.labs.ucx.source_code.graph import Dependency, DependencyGraph
+from databricks.labs.ucx.source_code.notebooks.loaders import NotebookLoader
+from databricks.labs.ucx.source_code.path_lookup import PathLookup
+
+
+def test_folder_has_repr() -> None:
+    notebook_loader = NotebookLoader()
+    file_loader = FileLoader()
+    folder = Folder(Path("test"), notebook_loader, file_loader, FolderLoader(notebook_loader, file_loader))
+    assert len(repr(folder)) > 0
+
+
+@pytest.fixture
+def graph_parent_child_context(mock_path_lookup, simple_dependency_resolver) -> DependencyGraph:
+    """Dependency graph for the parent-child-context sample directory.
+
+    The directory contains three files `grand_parent.py`, `parent.py` and `child.py`. That import each other as such:
+    grand parent imports parent import child.
+    """
+    path = mock_path_lookup.resolve(Path("parent-child-context/"))
+    dependency = Dependency(FolderLoader(NotebookLoader(), FileLoader()), path, False)
+    graph = DependencyGraph(dependency, None, simple_dependency_resolver, mock_path_lookup, CurrentSessionState())
+    return graph
+
+
+def test_folder_build_dependency_graph_without_problems(mock_path_lookup, graph_parent_child_context) -> None:
+    """No problems should arise form building the dependency graph for the sample folder"""
+    folder = graph_parent_child_context.dependency.load(mock_path_lookup)
+    assert folder is not None
+    problems = folder.build_dependency_graph(graph_parent_child_context)
+    assert not problems
+
+
+def test_folder_loads_content(mock_path_lookup, graph_parent_child_context) -> None:
+    """The files in the folder should be added to the dependency graph after building."""
+    expected_dependencies = {graph_parent_child_context.dependency}
+    for relative_path in "grand_parent.py", "parent.py", "child.py":
+        path = mock_path_lookup.resolve(Path("parent-child-context") / relative_path)
+        dependency = Dependency(FileLoader(), path)
+        expected_dependencies.add(dependency)
+
+    folder = graph_parent_child_context.dependency.load(mock_path_lookup)
+    assert folder is not None
+    folder.build_dependency_graph(graph_parent_child_context)
+
+    assert graph_parent_child_context.all_dependencies == expected_dependencies
+
+
+def test_folder_cannot_load_unresolved_path(graph_parent_child_context) -> None:
+    """An unresolved path cannot be loaded."""
+    path_lookup = create_autospec(PathLookup)
+    path_lookup.resolve.return_value = None
+    folder = graph_parent_child_context.dependency.load(path_lookup)
+    assert folder is None
+    path_lookup.resolve.assert_called_once_with(graph_parent_child_context.dependency.path)
+
+
+@pytest.mark.parametrize("subdirectory", [".venv"])
+def test_folder_build_dependency_graph_ignore_subdirectories(
+    tmp_path, mock_path_lookup, simple_dependency_resolver, subdirectory: str
+) -> None:
+    """The folder loader should only include directories with source code"""
+    path = tmp_path / subdirectory / "file.py"
+    path.parent.mkdir(parents=True, exist_ok=True)
+    path.touch()
+    dependency = Dependency(FolderLoader(NotebookLoader(), FileLoader()), tmp_path, inherits_context=False)
+    graph = DependencyGraph(dependency, None, simple_dependency_resolver, mock_path_lookup, CurrentSessionState())
+    folder = graph.dependency.load(mock_path_lookup)
+    assert folder is not None
+    problems = folder.build_dependency_graph(graph)
+    assert not problems
+    assert path not in mock_path_lookup.successfully_resolved_paths, "Subdirectory should be ignored"

tests/unit/source_code/test_graph.py

@@ -28,17 +28,6 @@ def test_dependency_graph_registers_library_from_egg(mock_path_lookup, simple_de
     assert lookup_resolve.exists()
 
 
-def test_folder_loads_content(mock_path_lookup, simple_dependency_resolver) -> None:
-    path = Path(__file__).parent / "samples" / "parent-child-context"
-    dependency = Dependency(FolderLoader(NotebookLoader(), FileLoader()), path, False)
-    graph = DependencyGraph(dependency, None, simple_dependency_resolver, mock_path_lookup, CurrentSessionState())
-    container = dependency.load(mock_path_lookup)
-    assert container is not None
-    container.build_dependency_graph(graph)
-    all_paths = [d.path for d in graph.all_dependencies]
-    assert len(all_paths) == 4
-
-
 def test_root_dependencies_returns_only_files(mock_path_lookup, simple_dependency_resolver) -> None:
     path = Path(__file__).parent / "samples" / "parent-child-context"
     dependency = Dependency(FolderLoader(NotebookLoader(), FileLoader()), path, False)