Improve test coverage for dockerfile_fragments.py with synthetic Dockerfile assertions

[coderabbitai]

Context

This is a follow-up from PR #2682 to improve test coverage in scripts/dockerfile_fragments.py.

Suggestion

The current test_dry_run test validates that main() completes against real Dockerfile trees, but doesn't assert on actual replacements. For stronger regression protection, consider:

• Extending test_dry_run to assert that at least one known marker in a small synthetic Dockerfile gets rewritten as expected
• Adding a focused test that validates marker replacement behavior decoupled from the full repository layout

This would provide better regression protection beyond just verifying the script doesn't crash.

References

• PR: RHAIENG-1965: Notebooks CLI: implement the AIPCC replace-markers.sh functionality, as a start  #2682
• Review comment: RHAIENG-1965: Notebooks CLI: implement the AIPCC replace-markers.sh functionality, as a start  #2682 (comment)
• Requested by: @jiridanek

[jiridanek]

@coderabbitai suggest an implementation

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll examine the current implementation and tests to suggest a concrete implementation.

---

🏁 Script executed:

#!/bin/bash
# Find and read the dockerfile_fragments.py file
fd -t f "dockerfile_fragments.py" --exec cat {}

Length of output: 5675

---

🏁 Script executed:

#!/bin/bash
# Find and read the test file for dockerfile_fragments
fd -t f -e py --full-path "test.*dockerfile_fragments" --exec cat {}

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check if there are any example Dockerfiles in the repo to understand the marker format
rg -n -C 3 "###" --type dockerfile | head -100

Length of output: 34

---

🏁 Script executed:

#!/bin/bash
# Look for the test file more broadly
fd -t f "test_dockerfile_fragments.py" --exec cat {}

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check if there's a tests directory
fd -t d "^tests$" --exec ls -la {}

Length of output: 1038

---

🏁 Script executed:

#!/bin/bash
# Look for actual Dockerfiles to understand the marker format
fd -t f "^Dockerfile" | head -5 | xargs -I {} sh -c 'echo "=== File: {} ===" && head -50 {}'

Length of output: 9892

---

🏁 Script executed:

#!/bin/bash
# Check if there are any existing tests that exercise main()
rg -n "test_dry_run" -A 20

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Look for existing tests of dockerfile_fragments in test_main.py
rg -n "dockerfile_fragments" tests/test_main.py -C 10

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check for any Dockerfiles that might already have the markers from the script
fd -t f "^Dockerfile" --full-path "jupyter" | head -2 | xargs -I {} sh -c 'echo "=== File: {} ===" && cat {}'

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check the structure of tests directory
ls -la tests/

Length of output: 677

---

🏁 Script executed:

#!/bin/bash
# Find Dockerfiles in jupyter or rstudio directories (non-excluded paths)
fd -t f "Dockerfile" | grep -E "(jupyter|rstudio|codeserver)" | head -3 | xargs -I {} sh -c 'echo "=== {} ===" && head -80 {}'

Length of output: 8902

---

🏁 Script executed:

#!/bin/bash
# Check if there's a scripts directory and its structure
ls -la scripts/

Length of output: 1207

---

🏁 Script executed:

#!/bin/bash
# Look for how tests are typically structured - check conftest.py
cat tests/conftest.py

Length of output: 92

---

@jiridanek, I'll suggest a comprehensive implementation for improving test coverage. Based on the code analysis, I'll create a new test file that validates marker replacements with synthetic Dockerfiles.

Suggested Implementation

Create a new file tests/test_dockerfile_fragments.py with the following content:

from __future__ import annotations

import pathlib
import textwrap
from typing import TYPE_CHECKING

import pytest

if TYPE_CHECKING:
    from pyfakefs.fake_filesystem import FakeFilesystem

# Import the functions we want to test
from scripts.dockerfile_fragments import blockinfile, main, is_jupyter, is_rstudio


class TestBlockinfile:
    """Tests for the blockinfile function (moved from inline tests)"""

    def test_adding_new_block(self, fs: FakeFilesystem):
        fs.create_file("/config.txt", contents="hello\nworld")

        blockinfile("/config.txt", "key=value")

        assert fs.get_object("/config.txt").contents == "hello\nworld\n#  begin\nkey=value\n#  end\n"

    def test_lastnewline_removal(self, fs: FakeFilesystem):
        fs.create_file("/config.txt", contents="hello\nworld")

        blockinfile("/config.txt", "key=value\n\n")

        assert fs.get_object("/config.txt").contents == "hello\nworld\n#  begin\nkey=value\n\n#  end\n"

    def test_updating_value_in_block(self, fs: FakeFilesystem):
        fs.create_file("/config.txt", contents="hello\nworld\n#  begin\nkey=value1\n#  end\n")

        blockinfile("/config.txt", "key=value2")

        assert fs.get_object("/config.txt").contents == "hello\nworld\n#  begin\nkey=value2\n#  end\n"

    def test_block_with_custom_prefix(self, fs: FakeFilesystem):
        fs.create_file("/config.txt", contents="initial content\n")

        blockinfile("/config.txt", "some content", prefix="my custom prefix")

        contents = fs.get_object("/config.txt").contents
        assert "# my custom prefix begin" in contents
        assert "# my custom prefix end" in contents
        assert "some content" in contents

    def test_block_replacement_preserves_surrounding_content(self, fs: FakeFilesystem):
        fs.create_file(
            "/config.txt",
            contents="before\n# marker begin\nold content\n# marker end\nafter\n"
        )

        blockinfile("/config.txt", "new content", prefix="marker")

        contents = fs.get_object("/config.txt").contents
        assert contents == "before\n# marker begin\nnew content\n# marker end\nafter\n"

    def test_multiple_blocks_in_same_file(self, fs: FakeFilesystem):
        fs.create_file("/config.txt", contents="start\n")

        blockinfile("/config.txt", "block1 content", prefix="section1")
        blockinfile("/config.txt", "block2 content", prefix="section2")

        contents = fs.get_object("/config.txt").contents
        assert "# section1 begin" in contents
        assert "block1 content" in contents
        assert "# section2 begin" in contents
        assert "block2 content" in contents

    def test_error_on_mismatched_markers(self, fs: FakeFilesystem):
        # Only begin marker
        fs.create_file("/bad1.txt", contents="# marker begin\n")
        with pytest.raises(ValueError, match="Found begin marker but no matching end marker"):
            blockinfile("/bad1.txt", "content", prefix="marker")

        # Only end marker
        fs.create_file("/bad2.txt", contents="# marker end\n")
        with pytest.raises(ValueError, match="Found end marker but no matching begin marker"):
            blockinfile("/bad2.txt", "content", prefix="marker")

        # Begin after end
        fs.create_file("/bad3.txt", contents="# marker end\nstuff\n# marker begin\n")
        with pytest.raises(ValueError, match="Begin marker appears after end marker"):
            blockinfile("/bad3.txt", "content", prefix="marker")


class TestMain:
    """Tests for the main() function with synthetic Dockerfiles"""

    def test_main_processes_jupyter_dockerfile(self, fs: FakeFilesystem, monkeypatch):
        """Test that main() correctly processes a Jupyter Dockerfile with all expected markers"""
        # Set up fake filesystem with repository structure
        repo_root = pathlib.Path("/fake/repo")
        monkeypatch.setattr("scripts.dockerfile_fragments.ROOT_DIR", repo_root)
        
        # Create a minimal Jupyter Dockerfile
        dockerfile_path = repo_root / "jupyter" / "minimal" / "ubi9-python-3.11" / "Dockerfile.cpu"
        fs.create_file(
            dockerfile_path,
            contents=textwrap.dedent("""
                FROM registry.access.redhat.com/ubi9/python-311:latest
                USER 0
                RUN dnf install -y some-package
                USER 1001
            """)
        )

        # Run main()
        main()

        # Read the modified Dockerfile
        contents = dockerfile_path.read_text()

        # Assert that all expected markers were inserted
        assert "# upgrade first to avoid fixable vulnerabilities begin" in contents
        assert "# upgrade first to avoid fixable vulnerabilities end" in contents
        assert "dnf -y upgrade --refresh --nobest" in contents

        assert "# Install micropipenv and uv to deploy packages from requirements.txt begin" in contents
        assert "# Install micropipenv and uv to deploy packages from requirements.txt end" in contents
        assert 'pip install --no-cache-dir --extra-index-url https://pypi.org/simple -U "micropipenv[toml]' in contents

        assert "# Install the oc client begin" in contents
        assert "# Install the oc client end" in contents
        assert "openshift-client-linux.tar.gz" in contents

        # Jupyter-specific: PDF dependencies
        assert "# Dependencies for PDF export begin" in contents
        assert "# Dependencies for PDF export end" in contents
        assert "./utils/install_pdf_deps.sh" in contents

    def test_main_processes_rstudio_dockerfile(self, fs: FakeFilesystem, monkeypatch):
        """Test that main() correctly processes an RStudio Dockerfile (no oc client, no PDF deps)"""
        repo_root = pathlib.Path("/fake/repo")
        monkeypatch.setattr("scripts.dockerfile_fragments.ROOT_DIR", repo_root)
        
        dockerfile_path = repo_root / "rstudio" / "c9s-python-3.11" / "Dockerfile"
        fs.create_file(
            dockerfile_path,
            contents=textwrap.dedent("""
                FROM some/rstudio-base:latest
                USER 0
                RUN echo "rstudio setup"
                USER 1001
            """)
        )

        main()
        contents = dockerfile_path.read_text()

        # Common markers should be present
        assert "# upgrade first to avoid fixable vulnerabilities begin" in contents
        assert "# Install micropipenv and uv to deploy packages from requirements.txt begin" in contents

        # RStudio-specific: should NOT have oc client or PDF dependencies
        assert "# Install the oc client begin" not in contents
        assert "# Dependencies for PDF export begin" not in contents

    def test_main_skips_excluded_directories(self, fs: FakeFilesystem, monkeypatch):
        """Test that main() skips Dockerfiles in base-images/ and examples/"""
        repo_root = pathlib.Path("/fake/repo")
        monkeypatch.setattr("scripts.dockerfile_fragments.ROOT_DIR", repo_root)
        
        # Create Dockerfiles in excluded directories
        base_dockerfile = repo_root / "base-images" / "cpu" / "Dockerfile"
        example_dockerfile = repo_root / "examples" / "test" / "Dockerfile"
        
        original_content = "FROM scratch\nRUN echo 'original'\n"
        fs.create_file(base_dockerfile, contents=original_content)
        fs.create_file(example_dockerfile, contents=original_content)

        main()

        # These files should remain unchanged
        assert base_dockerfile.read_text() == original_content
        assert example_dockerfile.read_text() == original_content

    def test_main_idempotent_on_second_run(self, fs: FakeFilesystem, monkeypatch):
        """Test that running main() twice produces the same result"""
        repo_root = pathlib.Path("/fake/repo")
        monkeypatch.setattr("scripts.dockerfile_fragments.ROOT_DIR", repo_root)
        
        dockerfile_path = repo_root / "jupyter" / "Dockerfile.cpu"
        fs.create_file(dockerfile_path, contents="FROM base\nRUN echo 'test'\n")

        # First run
        main()
        first_result = dockerfile_path.read_text()

        # Second run
        main()
        second_result = dockerfile_path.read_text()

        # Should be identical
        assert first_result == second_result

    def test_main_updates_existing_markers(self, fs: FakeFilesystem, monkeypatch):
        """Test that main() updates content within existing markers"""
        repo_root = pathlib.Path("/fake/repo")
        monkeypatch.setattr("scripts.dockerfile_fragments.ROOT_DIR", repo_root)
        
        dockerfile_path = repo_root / "jupyter" / "Dockerfile"
        fs.create_file(
            dockerfile_path,
            contents=textwrap.dedent("""
                FROM base
                # upgrade first to avoid fixable vulnerabilities begin
                RUN echo 'old upgrade command'
                # upgrade first to avoid fixable vulnerabilities end
                RUN echo 'other stuff'
            """)
        )

        main()
        contents = dockerfile_path.read_text()

        # Should replace old content with current script's content
        assert "old upgrade command" not in contents
        assert "dnf -y upgrade --refresh --nobest" in contents
        assert "other stuff" in contents  # Surrounding content preserved

    def test_main_handles_dockerfile_glob_patterns(self, fs: FakeFilesystem, monkeypatch):
        """Test that main() processes Dockerfile.cpu, Dockerfile.cuda, etc."""
        repo_root = pathlib.Path("/fake/repo")
        monkeypatch.setattr("scripts.dockerfile_fragments.ROOT_DIR", repo_root)
        
        # Create various Dockerfile variants
        for suffix in ["cpu", "cuda", ""]:
            name = f"Dockerfile.{suffix}" if suffix else "Dockerfile"
            path = repo_root / "jupyter" / "test" / name
            fs.create_file(path, contents="FROM base\n")

        main()

        # All should be processed
        for suffix in ["cpu", "cuda", ""]:
            name = f"Dockerfile.{suffix}" if suffix else "Dockerfile"
            path = repo_root / "jupyter" / "test" / name
            contents = path.read_text()
            assert "# upgrade first to avoid fixable vulnerabilities begin" in contents


class TestPathHelpers:
    """Tests for path-based helper functions"""

    def test_is_jupyter(self):
        assert is_jupyter(pathlib.Path("/repo/jupyter/minimal/Dockerfile"))
        assert is_jupyter(pathlib.Path("/repo/jupyter/datascience/ubi9-python-3.11/Dockerfile.cpu"))
        assert not is_jupyter(pathlib.Path("/repo/rstudio/Dockerfile"))
        assert not is_jupyter(pathlib.Path("/repo/codeserver/Dockerfile"))

    def test_is_rstudio(self):
        assert is_rstudio(pathlib.Path("/repo/rstudio/c9s-python-3.11/Dockerfile"))
        assert not is_rstudio(pathlib.Path("/repo/jupyter/Dockerfile"))
        assert not is_rstudio(pathlib.Path("/repo/codeserver/Dockerfile"))

Key Features of This Implementation

1. Moved inline tests: The existing TestBlockinfile tests are moved from the script to a proper test file

2. Comprehensive main() tests:

   • Tests marker insertion in Jupyter and RStudio Dockerfiles
   • Validates that correct content is inserted for each marker type
   • Tests path-based conditional logic (PDF deps only for Jupyter, no oc client for RStudio)

3. Edge case coverage:

   • Excluded directories (base-images/, examples/)
   • Idempotency (running twice produces same result)
   • Updating existing markers
   • Various Dockerfile naming patterns (Dockerfile, Dockerfile.cpu, Dockerfile.cuda)

4. Path helper tests: Validates is_jupyter() and is_rstudio() functions

5. Uses pyfakefs: All tests use the fake filesystem fixture for isolation

To run these tests

# Install pytest and pyfakefs if needed
pip install pytest pyfakefs

# Run the new tests
pytest tests/test_dockerfile_fragments.py -v

This implementation provides strong regression protection by asserting on actual marker content and replacement behavior, going well beyond just checking that the script doesn't crash.