[Concept,07/24] pickman: Improve testing with write_history()

Message ID 20251217022823.392557-8-sjg@u-boot.org
State New
Headers
Series pickman: Refine the feature set |

Commit Message

Simon Glass Dec. 17, 2025, 2:27 a.m. UTC
  From: Simon Glass <simon.glass@canonical.com>

Extract get_history() from write_history() to separate the history
content generation and file I/O from the git operations. The function
now:
- Takes a filename parameter
- Reads existing content from the file
- Writes updated content to the file
- Returns both the content and the git commit message

This makes the history generation logic fully testable without needing
to mock git operations.

Add tests for get_history():
- test_get_history_empty: Test with no existing file
- test_get_history_with_existing: Test appending to existing content
- test_get_history_replaces_existing_branch: Test replacing entry
- test_get_history_multiple_commits: Test commit message with multiple
  commits

Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com>
Signed-off-by: Simon Glass <simon.glass@canonical.com>
---

 tools/pickman/control.py |  44 +++++++++++----
 tools/pickman/ftest.py   | 114 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+), 10 deletions(-)
  

Patch

diff --git a/tools/pickman/control.py b/tools/pickman/control.py
index 474315f7db0..190f92fc57a 100644
--- a/tools/pickman/control.py
+++ b/tools/pickman/control.py
@@ -258,14 +258,19 @@  Commits:
 {commit_list}"""
 
 
-def write_history(source, commits, branch_name, conversation_log):
-    """Write an entry to the pickman history file
+def get_history(fname, source, commits, branch_name, conversation_log):
+    """Read, update and write history file for a cherry-pick operation
 
     Args:
+        fname (str): History filename to read/write
         source (str): Source branch name
         commits (list): list of CommitInfo tuples
         branch_name (str): Name of the cherry-pick branch
         conversation_log (str): The agent's conversation output
+
+    Returns:
+        tuple: (content, commit_msg) where content is the updated history
+            and commit_msg is the git commit message
     """
     summary = format_history_summary(source, commits, branch_name)
     entry = f"""{summary}
@@ -277,24 +282,43 @@  def write_history(source, commits, branch_name, conversation_log):
 
 """
 
-    # Read existing content and remove any entry for this branch
+    # Read existing content
     existing = ''
-    if os.path.exists(HISTORY_FILE):
-        with open(HISTORY_FILE, 'r', encoding='utf-8') as fhandle:
+    if os.path.exists(fname):
+        with open(fname, 'r', encoding='utf-8') as fhandle:
             existing = fhandle.read()
         # Remove existing entry for this branch (from ## header to ---)
         pattern = rf'## [^\n]+\n\nBranch: {re.escape(branch_name)}\n.*?---\n\n'
         existing = re.sub(pattern, '', existing, flags=re.DOTALL)
 
+    content = existing + entry
+
     # Write updated history file
-    with open(HISTORY_FILE, 'w', encoding='utf-8') as fhandle:
-        fhandle.write(existing + entry)
+    with open(fname, 'w', encoding='utf-8') as fhandle:
+        fhandle.write(content)
+
+    # Generate commit message
+    commit_msg = f'pickman: Record cherry-pick of {len(commits)} commits from {source}\n\n'
+    commit_msg += '\n'.join(f'- {c.short_hash} {c.subject}' for c in commits)
+
+    return content, commit_msg
+
+
+def write_history(source, commits, branch_name, conversation_log):
+    """Write an entry to the pickman history file and commit it
+
+    Args:
+        source (str): Source branch name
+        commits (list): list of CommitInfo tuples
+        branch_name (str): Name of the cherry-pick branch
+        conversation_log (str): The agent's conversation output
+    """
+    _, commit_msg = get_history(HISTORY_FILE, source, commits, branch_name,
+                                conversation_log)
 
     # Commit the history file (use -f in case .gitignore patterns match)
     run_git(['add', '-f', HISTORY_FILE])
-    msg = f'pickman: Record cherry-pick of {len(commits)} commits from {source}\n\n'
-    msg += '\n'.join(f'- {c.short_hash} {c.subject}' for c in commits)
-    run_git(['commit', '-m', msg])
+    run_git(['commit', '-m', commit_msg])
 
     tout.info(f'Updated {HISTORY_FILE}')
 
diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py
index 8865296ff11..d50be16fea7 100644
--- a/tools/pickman/ftest.py
+++ b/tools/pickman/ftest.py
@@ -1404,6 +1404,120 @@  class TestFormatHistorySummary(unittest.TestCase):
         self.assertIn('- bbb222b Second commit', result)
 
 
+class TestGetHistory(unittest.TestCase):
+    """Tests for get_history function."""
+
+    def setUp(self):
+        """Set up test fixtures."""
+        fd, self.history_file = tempfile.mkstemp(suffix='.history')
+        os.close(fd)
+        os.unlink(self.history_file)  # Remove so we start fresh
+
+    def tearDown(self):
+        """Clean up test fixtures."""
+        if os.path.exists(self.history_file):
+            os.unlink(self.history_file)
+
+    def test_get_history_empty(self):
+        """Test get_history with no existing file."""
+        commits = [
+            control.CommitInfo('aaa111', 'aaa111a', 'First commit', 'Author 1'),
+        ]
+        content, commit_msg = control.get_history(
+            self.history_file, 'us/next', commits, 'cherry-abc',
+            'Conversation output')
+
+        self.assertIn('us/next', content)
+        self.assertIn('Branch: cherry-abc', content)
+        self.assertIn('- aaa111a First commit', content)
+        self.assertIn('### Conversation log', content)
+        self.assertIn('Conversation output', content)
+        self.assertIn('---', content)
+
+        # Verify commit message
+        self.assertIn('pickman: Record cherry-pick of 1 commits', commit_msg)
+        self.assertIn('- aaa111a First commit', commit_msg)
+
+        # Verify file was written
+        with open(self.history_file, 'r', encoding='utf-8') as fhandle:
+            file_content = fhandle.read()
+        self.assertEqual(file_content, content)
+
+    def test_get_history_with_existing(self):
+        """Test get_history appends to existing content."""
+        # Create existing file
+        with open(self.history_file, 'w', encoding='utf-8') as fhandle:
+            fhandle.write('Previous history content\n')
+
+        commits = [
+            control.CommitInfo('bbb222', 'bbb222b', 'New commit', 'Author 2'),
+        ]
+        content, commit_msg = control.get_history(
+            self.history_file, 'us/next', commits, 'cherry-new',
+            'New conversation')
+
+        self.assertIn('Previous history content', content)
+        self.assertIn('cherry-new', content)
+        self.assertIn('New conversation', content)
+        self.assertIn('- bbb222b New commit', commit_msg)
+
+    def test_get_history_replaces_existing_branch(self):
+        """Test get_history removes existing entry for same branch."""
+        # Create existing file with an entry for cherry-abc
+        existing = """## 2025-01-01: us/next
+
+Branch: cherry-abc
+
+Commits:
+- aaa111a Old commit
+
+### Conversation log
+Old conversation
+
+---
+
+Other content
+"""
+        with open(self.history_file, 'w', encoding='utf-8') as fhandle:
+            fhandle.write(existing)
+
+        commits = [
+            control.CommitInfo('ccc333', 'ccc333c', 'Updated commit', 'Author'),
+        ]
+        content, _ = control.get_history(self.history_file, 'us/next', commits,
+                                         'cherry-abc', 'New conversation')
+
+        # Old entry should be removed
+        self.assertNotIn('Old commit', content)
+        self.assertNotIn('Old conversation', content)
+        # New entry should be present
+        self.assertIn('Updated commit', content)
+        self.assertIn('New conversation', content)
+        # Other content should be preserved
+        self.assertIn('Other content', content)
+
+    def test_get_history_multiple_commits(self):
+        """Test get_history with multiple commits."""
+        commits = [
+            control.CommitInfo('aaa111', 'aaa111a', 'First commit', 'Author 1'),
+            control.CommitInfo('bbb222', 'bbb222b', 'Second commit', 'Author 2'),
+            control.CommitInfo('ccc333', 'ccc333c', 'Third commit', 'Author 3'),
+        ]
+        content, commit_msg = control.get_history(
+            self.history_file, 'us/next', commits, 'cherry-abc', 'Log')
+
+        # Verify all commits in content
+        self.assertIn('- aaa111a First commit', content)
+        self.assertIn('- bbb222b Second commit', content)
+        self.assertIn('- ccc333c Third commit', content)
+
+        # Verify commit message
+        self.assertIn('pickman: Record cherry-pick of 3 commits', commit_msg)
+        self.assertIn('- aaa111a First commit', commit_msg)
+        self.assertIn('- bbb222b Second commit', commit_msg)
+        self.assertIn('- ccc333c Third commit', commit_msg)
+
+
 class TestGetNextCommitsEmptyLine(unittest.TestCase):
     """Tests for get_next_commits with empty lines."""