[Concept] [PATCH 3/9] pickman: Improve function names and line-length compliance

From: Simon Glass <simon.glass@canonical.com> Make some simple tweaks to reduces the size of lines: - Rename format_history_summary() -> format_history() - Rename update_history_with_review() -> update_history() - Rename update_mr_description() -> update_mr_desc() - Rename SIGNAL_ALREADY_APPLIED -> SIGNAL_APPLIED - Import gitlab_api as 'gitlab' in ftest.py - Shorten test hash strings by 1 character - Remove unused _cmd variable assignment - Shorten exception message 'branch not found' -> 'not found' Co-developed-by: Claude <noreply@anthropic.com> Signed-off-by: Simon Glass <simon.glass@canonical.com> --- tools/pickman/agent.py | 2 +- tools/pickman/control.py | 16 +-- tools/pickman/ftest.py | 278 ++++++++++++++++++------------------ tools/pickman/gitlab_api.py | 2 +- 4 files changed, 150 insertions(+), 148 deletions(-) diff --git a/tools/pickman/agent.py b/tools/pickman/agent.py index 9b37428af3e..d2a9ba562f8 100644 --- a/tools/pickman/agent.py +++ b/tools/pickman/agent.py @@ -21,7 +21,7 @@ SIGNAL_FILE = '.pickman-signal' # Signal status codes SIGNAL_SUCCESS = 'success' -SIGNAL_ALREADY_APPLIED = 'already_applied' +SIGNAL_APPLIED = 'already_applied' SIGNAL_CONFLICT = 'conflict' # Check if claude_agent_sdk is available diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 7a52b99a9ed..a6789b930ba 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -537,7 +537,7 @@ def handle_skip_comments(remote, mr_iid, title, unresolved, dbs): return True -def format_history_summary(source, commits, branch_name): +def format_history(source, commits, branch_name): """Format a summary of the cherry-pick operation Args: @@ -575,7 +575,7 @@ def get_history(fname, source, commits, branch_name, conv_log): 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) + summary = format_history(source, commits, branch_name) entry = f"""{summary} ### Conversation log @@ -735,7 +735,7 @@ def handle_already_applied(dbs, source, commits, branch_name, conv_log, args, # Use merge commit subject as title with [skip] prefix title = f'{SKIPPED_TAG} [pickman] {commits[-1].subject}' - summary = format_history_summary(source, commits, branch_name) + summary = format_history(source, commits, branch_name) description = (f'{summary}\n\n' f'**Status:** Commits already applied to {target} ' f'with different hashes.\n\n' @@ -778,7 +778,7 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t # Check for signal file from agent signal_status, signal_commit = agent.read_signal_file() - if signal_status == agent.SIGNAL_ALREADY_APPLIED: + if signal_status == agent.SIGNAL_APPLIED: ret = handle_already_applied(dbs, source, commits, branch_name, conv_log, args, signal_commit) return ret, False, conv_log @@ -809,7 +809,7 @@ def execute_apply(dbs, source, commits, branch_name, args): # pylint: disable=t title = f'[pickman] {commits[-1].subject}' # Description matches .pickman-history entry # (summary + conversation) - summary = format_history_summary(source, commits, branch_name) + summary = format_history(source, commits, branch_name) description = f'{summary}\n\n### Conversation log\n{conv_log}' mr_url = gitlab_api.push_and_create_mr( @@ -1000,10 +1000,10 @@ def process_single_mr(remote, merge_req, dbs, target): new_desc = (f"{old_desc}\n\n### Review response\n\n" f"**Comments addressed:**\n{comment_summary}\n\n" f"**Response:**\n{conversation_log}") - gitlab_api.update_mr_description(remote, mr_iid, new_desc) + gitlab_api.update_mr_desc(remote, mr_iid, new_desc) # Update .pickman-history - update_history_with_review(merge_req.source_branch, + update_history(merge_req.source_branch, unresolved, conversation_log) tout.info(f'Updated MR !{mr_iid} description and history') @@ -1047,7 +1047,7 @@ def process_mr_reviews(remote, mrs, dbs, target='master'): return processed -def update_history_with_review(branch_name, comments, conversation_log): +def update_history(branch_name, comments, conversation_log): """Append review handling to .pickman-history Args: diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 5f126984c9e..664825ef105 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -28,7 +28,7 @@ from pickman import __main__ as pickman from pickman import agent from pickman import control from pickman import database -from pickman import gitlab_api +from pickman import gitlab_api as gitlab # Test URL constants TEST_OAUTH_URL = 'https://oauth2:test-token@gitlab.com/group/project.git' @@ -1370,55 +1370,55 @@ class TestParseUrl(unittest.TestCase): def test_parse_ssh_url(self): """Test parsing SSH URL.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'git@gitlab.com:group/project.git') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_ssh_url_no_git_suffix(self): """Test parsing SSH URL without .git suffix.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'git@gitlab.com:group/project') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_ssh_url_nested_group(self): """Test parsing SSH URL with nested group.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'git@gitlab.denx.de:u-boot/custodians/u-boot-dm.git') self.assertEqual(host, 'gitlab.denx.de') self.assertEqual(path, 'u-boot/custodians/u-boot-dm') def test_parse_https_url(self): """Test parsing HTTPS URL.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'https://gitlab.com/group/project.git') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_https_url_no_git_suffix(self): """Test parsing HTTPS URL without .git suffix.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'https://gitlab.com/group/project') self.assertEqual(host, 'gitlab.com') self.assertEqual(path, 'group/project') def test_parse_http_url(self): """Test parsing HTTP URL.""" - host, path = gitlab_api.parse_url( + host, path = gitlab.parse_url( 'http://gitlab.example.com/group/project.git') self.assertEqual(host, 'gitlab.example.com') self.assertEqual(path, 'group/project') def test_parse_invalid_url(self): """Test parsing invalid URL.""" - host, path = gitlab_api.parse_url('not-a-valid-url') + host, path = gitlab.parse_url('not-a-valid-url') self.assertIsNone(host) self.assertIsNone(path) def test_parse_empty_url(self): """Test parsing empty URL.""" - host, path = gitlab_api.parse_url('') + host, path = gitlab.parse_url('') self.assertIsNone(host) self.assertIsNone(path) @@ -1428,16 +1428,16 @@ class TestCheckAvailable(unittest.TestCase): def test_check_available_false(self): """Test check_available returns False when gitlab not installed.""" - with mock.patch.object(gitlab_api, 'AVAILABLE', False): + with mock.patch.object(gitlab, 'AVAILABLE', False): with terminal.capture(): - result = gitlab_api.check_available() + result = gitlab.check_available() self.assertFalse(result) def test_check_available_true(self): """Test check_available returns True when gitlab is installed.""" - with mock.patch.object(gitlab_api, 'AVAILABLE', True): + with mock.patch.object(gitlab, 'AVAILABLE', True): with terminal.capture(): - result = gitlab_api.check_available() + result = gitlab.check_available() self.assertTrue(result) @@ -1446,36 +1446,36 @@ class TestGetPushUrl(unittest.TestCase): def test_get_push_url_success(self): """Test successful push URL generation.""" - with mock.patch.object(gitlab_api, 'get_token', + with mock.patch.object(gitlab, 'get_token', return_value='test-token'): with mock.patch.object( - gitlab_api, 'get_remote_url', + gitlab, 'get_remote_url', return_value=TEST_SSH_URL): - url = gitlab_api.get_push_url('origin') + url = gitlab.get_push_url('origin') self.assertEqual(url, TEST_OAUTH_URL) def test_get_push_url_no_token(self): """Test returns None when no token available.""" - with mock.patch.object(gitlab_api, 'get_token', return_value=None): - url = gitlab_api.get_push_url('origin') + with mock.patch.object(gitlab, 'get_token', return_value=None): + url = gitlab.get_push_url('origin') self.assertIsNone(url) def test_get_push_url_invalid_remote(self): """Test returns None for invalid remote URL.""" - with mock.patch.object(gitlab_api, 'get_token', + with mock.patch.object(gitlab, 'get_token', return_value='test-token'): - with mock.patch.object(gitlab_api, 'get_remote_url', + with mock.patch.object(gitlab, 'get_remote_url', return_value='not-a-valid-url'): - url = gitlab_api.get_push_url('origin') + url = gitlab.get_push_url('origin') self.assertIsNone(url) def test_get_push_url_https_remote(self): """Test with HTTPS remote URL.""" - with mock.patch.object(gitlab_api, 'get_token', + with mock.patch.object(gitlab, 'get_token', return_value='test-token'): - with mock.patch.object(gitlab_api, 'get_remote_url', + with mock.patch.object(gitlab, 'get_remote_url', return_value=TEST_HTTPS_URL): - url = gitlab_api.get_push_url('origin') + url = gitlab.get_push_url('origin') self.assertEqual(url, TEST_OAUTH_URL) @@ -1484,10 +1484,10 @@ class TestPushBranch(unittest.TestCase): def test_push_branch_force_with_remote_ref(self): """Test force push when remote branch exists uses --force-with-lease.""" - with mock.patch.object(gitlab_api, 'get_push_url', + with mock.patch.object(gitlab, 'get_push_url', return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: - result = gitlab_api.push_branch('ci', 'test-branch', force=True) + result = gitlab.push_branch('ci', 'test-branch', force=True) self.assertTrue(result) # Should fetch first, then push with --force-with-lease @@ -1496,11 +1496,12 @@ class TestPushBranch(unittest.TestCase): self.assertEqual(calls[0], mock.call('git', 'fetch', 'ci', 'test-branch')) push_args = calls[1][0] - self.assertIn('--force-with-lease=refs/remotes/ci/test-branch', push_args) + self.assertIn('--force-with-lease=refs/remotes/ci/test-branch', + push_args) def test_push_branch_force_no_remote_ref(self): """Test force push when remote branch doesn't exist uses --force.""" - with mock.patch.object(gitlab_api, 'get_push_url', + with mock.patch.object(gitlab, 'get_push_url', return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: # Fetch fails (branch doesn't exist on remote) @@ -1509,10 +1510,11 @@ class TestPushBranch(unittest.TestCase): command.CommandResult()), # fetch fails None, # push succeeds ] - result = gitlab_api.push_branch('ci', 'new-branch', force=True) + result = gitlab.push_branch('ci', 'new-branch', force=True) self.assertTrue(result) - # Should try fetch, fail, then push with --force (not --force-with-lease) + # Should try fetch, fail, then push with --force + # (not --force-with-lease) calls = mock_output.call_args_list self.assertEqual(len(calls), 2) push_args = calls[1][0] @@ -1521,10 +1523,10 @@ class TestPushBranch(unittest.TestCase): def test_push_branch_no_force(self): """Test regular push without force doesn't fetch or use force flags.""" - with mock.patch.object(gitlab_api, 'get_push_url', + with mock.patch.object(gitlab, 'get_push_url', return_value=TEST_SHORT_OAUTH_URL): with mock.patch.object(command, 'output') as mock_output: - result = gitlab_api.push_branch('ci', 'test-branch', force=False) + result = gitlab.push_branch('ci', 'test-branch', force=False) self.assertTrue(result) # Should only push, no fetch, no force flags @@ -1552,16 +1554,16 @@ class TestConfigFile(unittest.TestCase): with open(self.config_file, 'w', encoding='utf-8') as fhandle: fhandle.write('[gitlab]\ntoken = test-config-token\n') - with mock.patch.object(gitlab_api, 'CONFIG_FILE', self.config_file): - token = gitlab_api.get_token() + with mock.patch.object(gitlab, 'CONFIG_FILE', self.config_file): + token = gitlab.get_token() self.assertEqual(token, 'test-config-token') def test_get_token_fallback_to_env(self): """Test falling back to environment variable.""" # Config file doesn't exist - with mock.patch.object(gitlab_api, 'CONFIG_FILE', '/nonexistent/path'): + with mock.patch.object(gitlab, 'CONFIG_FILE', '/nonexistent/path'): with mock.patch.dict(os.environ, {'GITLAB_TOKEN': 'env-token'}): - token = gitlab_api.get_token() + token = gitlab.get_token() self.assertEqual(token, 'env-token') def test_get_token_config_missing_section(self): @@ -1569,9 +1571,9 @@ class TestConfigFile(unittest.TestCase): with open(self.config_file, 'w', encoding='utf-8') as fhandle: fhandle.write('[other]\nkey = value\n') - with mock.patch.object(gitlab_api, 'CONFIG_FILE', self.config_file): + with mock.patch.object(gitlab, 'CONFIG_FILE', self.config_file): with mock.patch.dict(os.environ, {'GITLAB_TOKEN': 'env-token'}): - token = gitlab_api.get_token() + token = gitlab.get_token() self.assertEqual(token, 'env-token') def test_get_config_value(self): @@ -1579,17 +1581,17 @@ class TestConfigFile(unittest.TestCase): with open(self.config_file, 'w', encoding='utf-8') as fhandle: fhandle.write('[section1]\nkey1 = value1\n') - with mock.patch.object(gitlab_api, 'CONFIG_FILE', self.config_file): - value = gitlab_api.get_config_value('section1', 'key1') + with mock.patch.object(gitlab, 'CONFIG_FILE', self.config_file): + value = gitlab.get_config_value('section1', 'key1') self.assertEqual(value, 'value1') class TestCheckPermissions(unittest.TestCase): """Tests for check_permissions function.""" - @mock.patch.object(gitlab_api, 'get_remote_url') - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_remote_url') + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_check_permissions_developer(self, mock_token, mock_url): """Test checking permissions for a developer.""" mock_token.return_value = 'test-token' @@ -1610,7 +1612,7 @@ class TestCheckPermissions(unittest.TestCase): mock_glab.projects.get.return_value = mock_project with mock.patch('gitlab.Gitlab', return_value=mock_glab): - perms = gitlab_api.check_permissions('origin') + perms = gitlab.check_permissions('origin') self.assertIsNotNone(perms) self.assertEqual(perms.user, 'testuser') @@ -1620,30 +1622,30 @@ class TestCheckPermissions(unittest.TestCase): self.assertTrue(perms.can_create_mr) self.assertFalse(perms.can_merge) - @mock.patch.object(gitlab_api, 'AVAILABLE', False) + @mock.patch.object(gitlab, 'AVAILABLE', False) def test_check_permissions_not_available(self): """Test check_permissions when gitlab not available.""" with terminal.capture(): - perms = gitlab_api.check_permissions('origin') + perms = gitlab.check_permissions('origin') self.assertIsNone(perms) - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_check_permissions_no_token(self, mock_token): """Test check_permissions when no token set.""" mock_token.return_value = None with terminal.capture(): - perms = gitlab_api.check_permissions('origin') + perms = gitlab.check_permissions('origin') self.assertIsNone(perms) class TestUpdateMrDescription(unittest.TestCase): - """Tests for update_mr_description function.""" + """Tests for update_mr_desc function.""" - @mock.patch.object(gitlab_api, 'get_remote_url') - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) - def test_update_mr_description_success(self, mock_token, mock_url): + @mock.patch.object(gitlab, 'get_remote_url') + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) + def test_update_mr_desc_success(self, mock_token, mock_url): """Test successful MR description update.""" mock_token.return_value = 'test-token' mock_url.return_value = 'git@gitlab.com:group/project.git' @@ -1655,36 +1657,36 @@ class TestUpdateMrDescription(unittest.TestCase): with mock.patch('gitlab.Gitlab') as mock_gitlab: mock_gitlab.return_value.projects.get.return_value = mock_project - result = gitlab_api.update_mr_description('origin', 123, + result = gitlab.update_mr_desc('origin', 123, 'New description') self.assertTrue(result) self.assertEqual(mock_mr.description, 'New description') mock_mr.save.assert_called_once() - @mock.patch.object(gitlab_api, 'AVAILABLE', False) - def test_update_mr_description_not_available(self): - """Test update_mr_description when gitlab not available.""" + @mock.patch.object(gitlab, 'AVAILABLE', False) + def test_update_mr_desc_not_available(self): + """Test update_mr_desc when gitlab not available.""" with terminal.capture(): - result = gitlab_api.update_mr_description('origin', 123, 'desc') + result = gitlab.update_mr_desc('origin', 123, 'desc') self.assertFalse(result) - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) - def test_update_mr_description_no_token(self, mock_token): - """Test update_mr_description when no token set.""" + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) + def test_update_mr_desc_no_token(self, mock_token): + """Test update_mr_desc when no token set.""" mock_token.return_value = None with terminal.capture(): - result = gitlab_api.update_mr_description('origin', 123, 'desc') + result = gitlab.update_mr_desc('origin', 123, 'desc') self.assertFalse(result) class TestGetPickmanMrs(unittest.TestCase): """Tests for get_pickman_mrs function.""" - @mock.patch.object(gitlab_api, 'get_remote_url') - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_remote_url') + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_get_pickman_mrs_sorted_oldest_first(self, mock_token, mock_url): """Test that MRs are requested sorted by created_at ascending.""" mock_token.return_value = 'test-token' @@ -1719,7 +1721,7 @@ class TestGetPickmanMrs(unittest.TestCase): with mock.patch('gitlab.Gitlab') as mock_gitlab: mock_gitlab.return_value.projects.get.return_value = mock_project - result = gitlab_api.get_pickman_mrs('origin', state='opened') + result = gitlab.get_pickman_mrs('origin', state='opened') # Verify the list call includes sorting parameters mock_project.mergerequests.list.assert_called_once_with( @@ -1734,8 +1736,8 @@ class TestGetPickmanMrs(unittest.TestCase): class TestCreateMr(unittest.TestCase): """Tests for create_mr function.""" - @mock.patch.object(gitlab_api, 'get_token') - @mock.patch.object(gitlab_api, 'AVAILABLE', True) + @mock.patch.object(gitlab, 'get_token') + @mock.patch.object(gitlab, 'AVAILABLE', True) def test_create_mr_409_returns_existing(self, mock_token): """Test that 409 error returns existing MR URL.""" tout.init(tout.INFO) @@ -1750,13 +1752,13 @@ class TestCreateMr(unittest.TestCase): # Simulate 409 Conflict error mock_project.mergerequests.create.side_effect = \ - gitlab_api.MrCreateError(response_code=409) + gitlab.MrCreateError(response_code=409) with mock.patch('gitlab.Gitlab') as mock_gitlab: mock_gitlab.return_value.projects.get.return_value = mock_project with terminal.capture(): - result = gitlab_api.create_mr( + result = gitlab.create_mr( 'gitlab.com', 'group/project', 'cherry-abc', 'master', 'Test MR') @@ -1883,7 +1885,7 @@ class TestStep(unittest.TestCase): def test_step_with_pending_mr(self): """Test step does nothing when MR is pending.""" - mock_mr = gitlab_api.PickmanMr( + mock_mr = gitlab.PickmanMr( iid=123, title='[pickman] Test MR', web_url='https://gitlab.com/mr/123', @@ -1891,9 +1893,9 @@ class TestStep(unittest.TestCase): description='Test', ) with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): args = argparse.Namespace(cmd='step', source='us/next', remote='ci', target='master', @@ -1905,7 +1907,7 @@ class TestStep(unittest.TestCase): def test_step_gitlab_error(self): """Test step when GitLab API returns error.""" - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=None): args = argparse.Namespace(cmd='step', source='us/next', remote='ci', target='master', @@ -1917,9 +1919,9 @@ class TestStep(unittest.TestCase): def test_step_open_mrs_error(self): """Test step when get_open_pickman_mrs returns error.""" - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=None): args = argparse.Namespace(cmd='step', source='us/next', remote='ci', target='master', @@ -1931,7 +1933,7 @@ class TestStep(unittest.TestCase): def test_step_allows_below_max(self): """Test step allows new MR when count is below max_mrs.""" - mock_mr = gitlab_api.PickmanMr( + mock_mr = gitlab.PickmanMr( iid=123, title='[pickman] Test MR', web_url='https://gitlab.com/mr/123', @@ -1939,9 +1941,9 @@ class TestStep(unittest.TestCase): description='Test', ) with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): with mock.patch.object(control, 'do_apply', return_value=0) as mock_apply: @@ -1958,7 +1960,7 @@ class TestStep(unittest.TestCase): def test_step_blocks_at_max(self): """Test step blocks new MR when at max_mrs limit.""" mock_mrs = [ - gitlab_api.PickmanMr( + gitlab.PickmanMr( iid=i, title=f'[pickman] Test MR {i}', web_url=f'https://gitlab.com/mr/{i}', @@ -1968,9 +1970,9 @@ class TestStep(unittest.TestCase): for i in range(3) ] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=[]): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=mock_mrs): with mock.patch.object(control, 'do_apply') as mock_apply: args = argparse.Namespace(cmd='step', source='us/next', @@ -2005,7 +2007,7 @@ class TestReview(unittest.TestCase): def test_review_no_mrs(self): """Test review when no open MRs found.""" - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[]): args = argparse.Namespace(cmd='review', remote='ci') with terminal.capture(): @@ -2015,7 +2017,7 @@ class TestReview(unittest.TestCase): def test_review_gitlab_error(self): """Test review when GitLab API returns error.""" - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=None): args = argparse.Namespace(cmd='review', remote='ci') with terminal.capture(): @@ -2025,7 +2027,7 @@ class TestReview(unittest.TestCase): class TestUpdateHistoryWithReview(unittest.TestCase): - """Tests for update_history_with_review function.""" + """Tests for update_history function.""" def setUp(self): """Set up test fixtures.""" @@ -2045,20 +2047,20 @@ class TestUpdateHistoryWithReview(unittest.TestCase): os.chdir(self.orig_dir) shutil.rmtree(self.test_dir) - def test_update_history_with_review(self): + def test_update_history(self): """Test that review handling is appended to history.""" comments = [ - gitlab_api.MrComment(id=1, author='reviewer1', + gitlab.MrComment(id=1, author='reviewer1', body='Please fix the indentation here', created_at='2025-01-01', resolvable=True, resolved=False), - gitlab_api.MrComment(id=2, author='reviewer2', body='Add a docstring', + gitlab.MrComment(id=2, author='reviewer2', body='Add a docstring', created_at='2025-01-01', resolvable=True, resolved=False), ] conversation_log = 'Fixed indentation and added docstring.' - control.update_history_with_review('cherry-abc123', comments, + control.update_history('cherry-abc123', comments, conversation_log) # Check history file was created @@ -2083,10 +2085,10 @@ class TestUpdateHistoryWithReview(unittest.TestCase): subprocess.run(['git', 'commit', '-m', 'Initial'], check=True, capture_output=True) - comments = [gitlab_api.MrComment(id=1, author='reviewer', body='Fix this', - created_at='2025-01-01', resolvable=True, - resolved=False)] - control.update_history_with_review('cherry-xyz', comments, 'Fixed it') + comms = [gitlab.MrComment(id=1, author='reviewer', body='Fix this', + created_at='2025-01-01', resolvable=True, + resolved=False)] + control.update_history('cherry-xyz', comms, 'Fixed it') with open(control.HISTORY_FILE, 'r', encoding='utf-8') as fhandle: content = fhandle.read() @@ -2132,7 +2134,7 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): dbs.comment_mark_processed(100, 1) dbs.commit() - mrs = [gitlab_api.PickmanMr( + mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', source_branch='cherry-test', @@ -2142,25 +2144,25 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): # Comment 1 is processed, comment 2 is new comments = [ - gitlab_api.MrComment(id=1, author='reviewer', body='Old comment', + gitlab.MrComment(id=1, author='reviewer', body='Old comment', created_at='2025-01-01', resolvable=True, resolved=False), - gitlab_api.MrComment(id=2, author='reviewer', body='New comment', + gitlab.MrComment(id=2, author='reviewer', body='New comment', created_at='2025-01-01', resolvable=True, resolved=False), ] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=comments): with mock.patch.object(agent, 'handle_mr_comments', - return_value=(True, 'Done')) as mock_agent: - with mock.patch.object(gitlab_api, 'update_mr_description'): - with mock.patch.object(control, 'update_history_with_review'): + return_value=(True, 'Done')) as moc: + with mock.patch.object(gitlab, 'update_mr_desc'): + with mock.patch.object(control, 'update_history'): control.process_mr_reviews('ci', mrs, dbs) # Agent should only receive the new comment - call_args = mock_agent.call_args + call_args = moc.call_args passed_comments = call_args[0][2] self.assertEqual(len(passed_comments), 1) self.assertEqual(passed_comments[0].id, 2) @@ -2174,7 +2176,7 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): dbs.start() # MR needs rebase but has no comments - mrs = [gitlab_api.PickmanMr( + mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', source_branch='cherry-test', @@ -2185,17 +2187,17 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): )] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=[]): with mock.patch.object(agent, 'handle_mr_comments', - return_value=(True, 'Rebased')) as mock_agent: - with mock.patch.object(gitlab_api, 'update_mr_description'): - with mock.patch.object(control, 'update_history_with_review'): + return_value=(True, 'Rebased')) as m: + with mock.patch.object(gitlab, 'update_mr_desc'): + with mock.patch.object(control, 'update_history'): control.process_mr_reviews('ci', mrs, dbs) # Agent should be called with needs_rebase=True - mock_agent.assert_called_once() - call_kwargs = mock_agent.call_args[1] + m.assert_called_once() + call_kwargs = m.call_args[1] self.assertTrue(call_kwargs.get('needs_rebase')) self.assertFalse(call_kwargs.get('has_conflicts')) @@ -2208,7 +2210,7 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): dbs.start() # MR has no comments and doesn't need rebase - mrs = [gitlab_api.PickmanMr( + mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', source_branch='cherry-test', @@ -2219,14 +2221,14 @@ class TestProcessMrReviewsCommentTracking(unittest.TestCase): )] with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=[]): with mock.patch.object(agent, 'handle_mr_comments', - return_value=(True, 'Done')) as mock_agent: + return_value=(True, 'Done')) as moc: control.process_mr_reviews('ci', mrs, dbs) # Agent should NOT be called - mock_agent.assert_not_called() + moc.assert_not_called() dbs.close() @@ -2360,20 +2362,20 @@ class TestParseInstruction(unittest.TestCase): class TestFormatHistorySummary(unittest.TestCase): - """Tests for format_history_summary function.""" + """Tests for format_history function.""" - def test_format_history_summary(self): + def test_format_history(self): """Test formatting history summary.""" commits = [ control.CommitInfo('aaa111', 'aaa111a', 'First commit', 'Author 1'), - control.CommitInfo('bbb222', 'bbb222b', 'Second commit', 'Author 2'), + control.CommitInfo('bbb222', 'bbb222b', 'Second one', 'Author 2'), ] - result = control.format_history_summary('us/next', commits, 'cherry-abc') + result = control.format_history('us/next', commits, 'cherry-abc') self.assertIn('us/next', result) self.assertIn('Branch: cherry-abc', result) self.assertIn('- aaa111a First commit', result) - self.assertIn('- bbb222b Second commit', result) + self.assertIn('- bbb222b Second one', result) class TestGetHistory(unittest.TestCase): @@ -2472,7 +2474,7 @@ Other content """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('bbb222', 'bbb222b', 'Second one', 'Author 2'), control.CommitInfo('ccc333', 'ccc333c', 'Third commit', 'Author 3'), ] content, commit_msg = control.get_history( @@ -2480,13 +2482,13 @@ Other content # Verify all commits in content self.assertIn('- aaa111a First commit', content) - self.assertIn('- bbb222b Second commit', content) + self.assertIn('- bbb222b Second one', 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('- bbb222b Second one', commit_msg) self.assertIn('- ccc333c Third commit', commit_msg) @@ -2684,7 +2686,7 @@ class TestExecuteApply(unittest.TestCase): return_value=(True, 'log')): with mock.patch.object(control, 'run_git', return_value='abc123'): - with mock.patch.object(gitlab_api, 'push_and_create_mr', + with mock.patch.object(gitlab, 'push_and_create_mr', return_value='https://mr/url'): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2710,7 +2712,7 @@ class TestExecuteApply(unittest.TestCase): return_value=(True, 'log')): with mock.patch.object(control, 'run_git', return_value='abc123'): - with mock.patch.object(gitlab_api, 'push_and_create_mr', + with mock.patch.object(gitlab, 'push_and_create_mr', return_value=None): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2735,7 +2737,7 @@ class TestExecuteApply(unittest.TestCase): with mock.patch.object(control.agent, 'cherry_pick_commits', return_value=(True, 'aborted log')): with mock.patch.object(control, 'run_git', - side_effect=Exception('branch not found')): + side_effect=Exception('not found')): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2766,7 +2768,7 @@ class TestExecuteApply(unittest.TestCase): with mock.patch.object(control.agent, 'cherry_pick_commits', return_value=(True, 'already applied log')): with mock.patch.object(control.agent, 'read_signal_file', - return_value=(agent.SIGNAL_ALREADY_APPLIED, + return_value=(agent.SIGNAL_APPLIED, 'hhh888')): ret, success, _ = control.execute_apply( dbs, 'us/next', commits, 'cherry-branch', args) @@ -2971,9 +2973,9 @@ class TestGetNextCommitsEmptyLine(unittest.TestCase): call_count = [0] + # pylint: disable=unused-argument def mock_git(pipe_list): call_count[0] += 1 - _cmd = pipe_list[0] if pipe_list else [] # pylint: disable=unused-variable # First call: get first-parent log if call_count[0] == 1: return command.CommandResult(stdout=fp_log) @@ -3047,7 +3049,7 @@ class TestDoPushBranch(unittest.TestCase): tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', remote='ci', force=False, run_ci=False) - with mock.patch.object(gitlab_api, 'push_branch', + with mock.patch.object(gitlab, 'push_branch', return_value=True) as mock_push: with terminal.capture(): ret = control.do_push_branch(args, None) @@ -3060,7 +3062,7 @@ class TestDoPushBranch(unittest.TestCase): tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', remote='origin', force=True, run_ci=False) - with mock.patch.object(gitlab_api, 'push_branch', + with mock.patch.object(gitlab, 'push_branch', return_value=True) as mock_push: with terminal.capture(): ret = control.do_push_branch(args, None) @@ -3073,7 +3075,7 @@ class TestDoPushBranch(unittest.TestCase): tout.init(tout.INFO) args = argparse.Namespace(cmd='push-branch', branch='test-branch', remote='ci', force=False) - with mock.patch.object(gitlab_api, 'push_branch', + with mock.patch.object(gitlab, 'push_branch', return_value=False): with terminal.capture(): ret = control.do_push_branch(args, None) @@ -3114,7 +3116,7 @@ class TestDoReviewWithMrs(unittest.TestCase): """Test review with open MRs but no comments.""" tout.init(tout.INFO) - mock_mr = gitlab_api.PickmanMr( + mock_mr = gitlab.PickmanMr( iid=123, title='[pickman] Test MR', web_url='https://gitlab.com/mr/123', @@ -3122,9 +3124,9 @@ class TestDoReviewWithMrs(unittest.TestCase): description='Test', ) with mock.patch.object(control, 'run_git'): - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab, 'get_open_pickman_mrs', return_value=[mock_mr]): - with mock.patch.object(gitlab_api, 'get_mr_comments', + with mock.patch.object(gitlab, 'get_mr_comments', return_value=[]): args = argparse.Namespace(cmd='review', remote='ci', target='master') @@ -3157,10 +3159,10 @@ class TestProcessMergedMrs(unittest.TestCase): with terminal.capture(): dbs = database.Database(self.db_path) dbs.start() - dbs.source_set('us/next', 'aaa111aaa111aaa111aaa111aaa111aaa111aaa1') + dbs.source_set('us/next', 'aaa111aaa111aaa111aaa111aaa111aaa111aaa') dbs.commit() - merged_mrs = [gitlab_api.PickmanMr( + merged_mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', description='## 2025-01-01: us/next\n\n- bbb222b Subject', @@ -3176,7 +3178,7 @@ class TestProcessMergedMrs(unittest.TestCase): return '' return '' - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=merged_mrs): with mock.patch.object(control, 'run_git', mock_git): processed = control.process_merged_mrs('ci', 'us/next', dbs) @@ -3196,7 +3198,7 @@ class TestProcessMergedMrs(unittest.TestCase): dbs.source_set('us/next', 'bbb222bbb222bbb222bbb222bbb222bbb222bbb2') dbs.commit() - merged_mrs = [gitlab_api.PickmanMr( + merged_mrs = [gitlab.PickmanMr( iid=100, title='[pickman] Test MR', description='## 2025-01-01: us/next\n\n- aaa111a Subject', @@ -3212,7 +3214,7 @@ class TestProcessMergedMrs(unittest.TestCase): raise RuntimeError('Not an ancestor') return '' - with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + with mock.patch.object(gitlab, 'get_merged_pickman_mrs', return_value=merged_mrs): with mock.patch.object(control, 'run_git', mock_git): processed = control.process_merged_mrs('ci', 'us/next', dbs) diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index 94af6880b7c..bfaf828edae 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -455,7 +455,7 @@ def reply_to_mr(remote, mr_iid, message): return False -def update_mr_description(remote, mr_iid, desc): +def update_mr_desc(remote, mr_iid, desc): """Update a merge request's description Args: -- 2.43.0