Improve coverage (#168)

---------

Co-authored-by: Sushant Gaurav <[email protected]>

Author: tomschr

changelog.d/168.infra.rst

@@ -0,0 +1,2 @@
+Improve test coverage for :file:`git.py`, :file:`env.py`, and
+:file:`lifecycle.py`. Raises coverage report by 1%.

src/docbuild/models/config/env.py

@@ -156,7 +156,7 @@ class EnvGeneralConfig(BaseModel):
     )
     "The canonical domain for URLs."
 
-    # --- NEW: Custom Serialization for LanguageCode Models ---
+    # --- Custom Serialization for LanguageCode Models ---
     @field_serializer("default_lang")
     def serialize_default_lang(self, lang_obj: LanguageCode) -> str:
         """Serialize the LanguageCode model back to a simple string (e.g., 'en-us')."""

src/docbuild/models/doctype.py

@@ -268,7 +268,7 @@ def product_xpath_segment(self: Self) -> str:
 
         Example: "product[@productid='sles']" or "product"
         """
-        if self.product != Product.ALL:
+        if self.product != Product["ALL"]:
             return f"product[@productid={self.product.value!r}]"
         return "product"
 

tests/models/config/test_env.py

@@ -174,6 +174,14 @@ def test_envconfig_full_success(mock_valid_raw_env_data: dict[str, Any]):
     assert isinstance(config.xslt_params["external"], dict)
     assert config.xslt_params["show"]["edit"]["link"] == 1
 
+    # Test serialization of LanguageCode fields back to strings
+    dumped_config = config.model_dump()
+    assert isinstance(dumped_config["config"]["default_lang"], str)
+    assert dumped_config["config"]["default_lang"] == "en-us"
+    assert isinstance(dumped_config["config"]["languages"], list)
+    assert all(isinstance(lang, str) for lang in dumped_config["config"]["languages"])
+    assert set(dumped_config["config"]["languages"]) == {"en-us", "de-de"}
+
 
 def test_envconfig_type_coercion_ip_host(mock_valid_raw_env_data: dict[str, Any]):
     """Test that the host field handles IPvAnyAddress correctly."""
@@ -267,3 +275,23 @@ def test_envconfig_invalid_role_fails(mock_valid_raw_env_data: dict[str, Any]):
 
     locs = excinfo.value.errors()[0]["loc"]
     assert ("server", "role") == tuple(locs)
+
+
+def test_envconfig_unresolved_placeholder_fails(
+    mock_valid_raw_env_data: dict[str, Any]
+):
+    """Test that an unresolved placeholder raises a ValueError."""
+    data = mock_valid_raw_env_data.copy()
+    # Introduce an unresolvable placeholder
+    data["paths"]["jinja_dir"] = "/etc/docbuild/{UNRESOLVED_PLACEHOLDER}"
+
+    with pytest.raises(ValueError, match="Configuration placeholder error"):
+        EnvConfig.from_dict(data)
+
+
+def test_envconfig_with_non_dict_input():
+    """Test that passing non-dict input to the validator is handled."""
+    # This covers the `else: return data` path in `_resolve_placeholders`.
+    # Pydantic will then raise a ValidationError because the input is not a dict.
+    with pytest.raises(ValidationError, match="Input should be a valid dictionary"):
+        EnvConfig.model_validate("not a dict")

tests/models/test_doctype.py

@@ -309,3 +309,17 @@ def test_xpath_in_doctype(string, xpath):
     """Test the XPath extraction from a Doctype."""
     doctype = Doctype.from_str(string)
     assert xpath == doctype.xpath()
+
+
+def test_product_xpath_segment():
+    """Test the product_xpath_segment method."""
+    # Test with all products (*)
+    dt_all = Doctype.from_str("*/15-SP6/en-us")
+    assert dt_all.product_xpath_segment() == "product"
+
+
+def test_docset_xpath_segment():
+    """Test the docset_xpath_segment method."""
+    # Test with all docsets (*)
+    dt_all = Doctype.from_str("sles/*/en-us")
+    assert dt_all.docset_xpath_segment("*") == "docset"

tests/models/test_lifecycle.py

@@ -20,6 +20,12 @@ def test_unknown_lifecycle():
     assert instance.name == "unknown"
 
 
+def test_invalid_lifecycle_from_constructor():
+    """Creating a lifecycle from an invalid string via constructor raises ValueError."""
+    with pytest.raises(ValueError, match="'invalid' is not a valid LifecycleFlag"):
+        LifecycleFlag("invalid")
+
+
 def test_lifecycle_flag_from_str_with_empty_string():
     instance = LifecycleFlag.from_str("")
     assert instance == LifecycleFlag.unknown

tests/utils/test_git.py

@@ -3,7 +3,7 @@
 import asyncio
 from pathlib import Path
 from subprocess import CompletedProcess
-from unittest.mock import AsyncMock
+from unittest.mock import AsyncMock, patch
 
 import pytest
 
@@ -26,13 +26,8 @@ def mock_subprocess_exec(monkeypatch) -> AsyncMock:
 @pytest.fixture
 def mock_execute_git(monkeypatch) -> AsyncMock:
     """Fixture to mock execute_git_command."""
-
-    async def side_effect(*args, **kwargs):
-        if args[0] == "clone":
-            return CompletedProcess(args, 0, "Cloning...", "")
-        return CompletedProcess(args, 0, "stdout success", "")
-
-    mock = AsyncMock(side_effect=side_effect)
+    # Remove the side_effect to allow tests to set their own return_value
+    mock = AsyncMock()
     monkeypatch.setattr(git_module, "execute_git_command", mock)
     return mock
 
@@ -48,6 +43,9 @@ async def test_managed_repo_clone_bare_new(
 ):
     """Test clone_bare when the repository does not exist yet."""
     repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
     # Simulate repo does not exist
     monkeypatch.setattr(Path, "exists", lambda self: False)
 
@@ -70,6 +68,9 @@ async def test_managed_repo_clone_bare_exists(
 ):
     """Test clone_bare when the repository already exists."""
     repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
     # Simulate repo exists
     monkeypatch.setattr(Path, "exists", lambda self: True)
 
@@ -94,6 +95,9 @@ async def test_managed_repo_create_worktree_success(
     tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
 ):
     """Test create_worktree successfully creates a worktree."""
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
     repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
     # Simulate bare repo exists
     monkeypatch.setattr(Path, "exists", lambda self: True)
@@ -117,6 +121,9 @@ async def test_managed_repo_create_worktree_with_options(
     tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
 ):
     """Test create_worktree with additional clone options."""
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
     repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
     # Simulate bare repo exists
     monkeypatch.setattr(Path, "exists", lambda self: True)
@@ -157,6 +164,9 @@ async def test_managed_repo_create_worktree_not_local(
     tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
 ):
     """Test create_worktree without the --local flag."""
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
     repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
     # Simulate bare repo exists
     monkeypatch.setattr(Path, "exists", lambda self: True)
@@ -218,6 +228,9 @@ async def test_fetch_updates_success(
     tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
 ):
     """Test fetch_updates successfully fetches updates."""
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
     repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
     # Simulate bare repo exists
     monkeypatch.setattr(Path, "exists", lambda self: True)
@@ -260,6 +273,9 @@ async def test_managed_repo_clone_bare_already_processed(
     tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
 ):
     """Test clone_bare when the repository has been processed in this run."""
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
     repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
     # Simulate repo does not exist for the first call
     monkeypatch.setattr(Path, "exists", lambda self: False)
@@ -284,3 +300,103 @@ async def test_managed_repo_clone_bare_already_processed(
     assert result2 is True
     # The mock should still have only been called once
     mock_execute_git.assert_awaited_once()
+
+
+async def test_ls_tree_no_repo(
+    tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
+):
+    """Test ls_tree when the bare repository does not exist."""
+    repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
+    # Simulate bare repo does not exist
+    monkeypatch.setattr(Path, "exists", lambda self: False)
+
+    with patch.object(git_module.log, "warning") as mock_log_warning:
+        result = await repo.ls_tree("main")
+
+        assert result == []
+        mock_log_warning.assert_called_once_with(
+            "Cannot run ls-tree: Bare repository does not exist at %s",
+            repo.bare_repo_path,
+        )
+        mock_execute_git.assert_not_awaited()
+
+
+async def test_ls_tree_success_recursive(
+    tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
+):
+    """Test ls_tree successfully lists files recursively."""
+    repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
+    monkeypatch.setattr(Path, "exists", lambda self: True)
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="file1.txt\ndir/file2.txt", stderr=""
+    )
+
+    result = await repo.ls_tree("main")
+
+    assert result == ["file1.txt", "dir/file2.txt"]
+    mock_execute_git.assert_awaited_once_with(
+        "ls-tree",
+        "--name-only",
+        "-r",
+        "main",
+        cwd=repo.bare_repo_path,
+        gitconfig=None,
+    )
+
+
+async def test_ls_tree_success_non_recursive(
+    tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
+):
+    """Test ls_tree successfully lists files non-recursively."""
+    repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
+    monkeypatch.setattr(Path, "exists", lambda self: True)
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="file1.txt", stderr=""
+    )
+
+    result = await repo.ls_tree("main", recursive=False)
+
+    assert result == ["file1.txt"]
+    mock_execute_git.assert_awaited_once_with(
+        "ls-tree",
+        "--name-only",
+        "main",
+        cwd=repo.bare_repo_path,
+        gitconfig=None,
+    )
+
+
+async def test_ls_tree_empty_output(
+    tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
+):
+    """Test ls_tree handles empty stdout correctly."""
+    repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
+    monkeypatch.setattr(Path, "exists", lambda self: True)
+    mock_execute_git.return_value = CompletedProcess(
+        args=[], returncode=0, stdout="", stderr=""
+    )
+
+    result = await repo.ls_tree("main")
+
+    assert result == []
+
+
+async def test_ls_tree_command_failure(
+    tmp_path: Path, mock_execute_git: AsyncMock, monkeypatch
+):
+    """Test ls_tree handles a RuntimeError from execute_git_command."""
+    repo = ManagedGitRepo("http://a.b/org/c.git", tmp_path)
+    monkeypatch.setattr(Path, "exists", lambda self: True)
+    error = RuntimeError("Git command failed")
+    mock_execute_git.side_effect = error
+
+    with patch.object(git_module.log, "error") as mock_log_error:
+        result = await repo.ls_tree("develop")
+
+        assert result == []
+        mock_log_error.assert_called_once_with(
+            "Failed to run ls-tree on branch '%s' in '%s': %s",
+            "develop",
+            repo.slug,
+            error,
+        )