From aa7f7729f0d482721ed4f9556c94a60812969fa3 Mon Sep 17 00:00:00 2001 From: LeSeulArtichaut Date: Thu, 27 Feb 2020 14:21:15 +0100 Subject: [PATCH 1/2] Add functionality to assign a random team member with r? @ --- highfive/newpr.py | 48 ++++++++++++++++++++++++++---------- highfive/tests/test_newpr.py | 10 ++++---- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/highfive/newpr.py b/highfive/newpr.py index 60c56cf4..8b00b723 100644 --- a/highfive/newpr.py +++ b/highfive/newpr.py @@ -33,7 +33,7 @@ review_with_reviewer = 'r? @%s\n\n(rust-highfive has picked a reviewer for you, use r? to override)' review_without_reviewer = '@%s: no appropriate reviewer found, use r? to override' -reviewer_re = re.compile("\\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)") +reviewer_re = re.compile("\\b[rR]\?[:\- ]*@(?:([a-zA-Z0-9\-]+)/)?([a-zA-Z0-9\-]+)") submodule_re = re.compile(".*\+Subproject\scommit\s.*", re.DOTALL | re.MULTILINE) target_re = re.compile("^[+-]{3} [ab]/compiler/rustc_target/src/spec/", re.MULTILINE) @@ -230,27 +230,46 @@ def is_new_contributor(self, username, owner, repo): else: raise e - def find_reviewer(self, msg): + def get_groups(self): + groups = deepcopy(self.repo_config['groups']) + + # fill in the default groups, ensuring that overwriting is an + # error. + global_ = self._load_json_file('_global.json') + for name, people in global_['groups'].items(): + assert name not in groups, "group %s overlaps with _global.json" % name + groups[name] = people + + return groups + + def find_reviewer(self, msg, exclude): """ If the user specified a reviewer, return the username, otherwise returns None. """ if msg is not None: match = reviewer_re.search(msg) - return match.group(1) if match else None + if match: + if match.group(1): + # assign someone from the specified team + groups = self.get_groups() + potential = groups.get(match.group(2)) + if potential is None: + potential = groups.get("%s/%s" % (match.group(1), match.group(2))) + if potential is None: + potential = groups["all"] + else: + potential.extend(groups["all"]) + + return self.pick_reviewer(groups, potential, exclude) + else: + return match.group(2) def choose_reviewer(self, repo, owner, diff, exclude): """Choose a reviewer for the PR.""" # Get JSON data on reviewers. dirs = self.repo_config.get('dirs', {}) - groups = deepcopy(self.repo_config['groups']) - - # fill in the default groups, ensuring that overwriting is an - # error. - global_ = self._load_json_file('_global.json') - for name, people in global_['groups'].items(): - assert name not in groups, "group %s overlaps with _global.json" % name - groups[name] = people + groups = self.get_groups() most_changed = None # If there's directories with specially assigned groups/users @@ -293,6 +312,9 @@ def choose_reviewer(self, repo, owner, diff, exclude): if not potential: potential = groups['core'] + return self.pick_reviewer(groups, potential, exclude) + + def pick_reviewer(self, groups, potential, exclude): # expand the reviewers list by group reviewers = [] seen = {"all"} @@ -382,7 +404,7 @@ def new_pr(self): if not self.payload['pull_request', 'assignees']: # Only try to set an assignee if one isn't already set. msg = self.payload['pull_request', 'body'] - reviewer = self.find_reviewer(msg) + reviewer = self.find_reviewer(msg, author) post_msg = False if not reviewer: @@ -437,7 +459,7 @@ def new_comment(self): # Check for r? and set the assignee. msg = self.payload['comment', 'body'] - reviewer = self.find_reviewer(msg) + reviewer = self.find_reviewer(msg, author) if reviewer: issue = str(self.payload['issue', 'number']) self.set_assignee( diff --git a/highfive/tests/test_newpr.py b/highfive/tests/test_newpr.py index 3634ede3..eb4352f8 100644 --- a/highfive/tests/test_newpr.py +++ b/highfive/tests/test_newpr.py @@ -351,11 +351,11 @@ def test_find_reviewer(self): handler = HighfiveHandlerMock(Payload({})).handler for (msg, reviewer) in found_cases: - assert handler.find_reviewer(msg) == reviewer, \ + assert handler.find_reviewer(msg, None) == reviewer, \ "expected '%s' from '%s'" % (reviewer, msg) for msg in not_found_cases: - assert handler.find_reviewer(msg) is None, \ + assert handler.find_reviewer(msg, None) is None, \ "expected '%s' to have no reviewer extracted" % msg class TestApiReq(TestNewPR): @@ -792,7 +792,7 @@ def assert_set_assignee_branch_calls(self, reviewer, to_mention): self.mocks['api_req'].assert_called_once_with( 'GET', 'https://the.url/', None, 'application/vnd.github.v3.diff' ) - self.mocks['find_reviewer'].assert_called_once_with('The PR comment.') + self.mocks['find_reviewer'].assert_called_once_with('The PR comment.', 'prAuthor') self.mocks['set_assignee'].assert_called_once_with( reviewer, 'repo-owner', 'repo-name', '7', self.user, 'prAuthor', to_mention @@ -1052,7 +1052,7 @@ def test_no_reviewer(self): self.mocks['find_reviewer'].return_value = None handler.new_comment() self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_called_with('comment!') + self.mocks['find_reviewer'].assert_called_with('comment!', 'userA') self.mocks['set_assignee'].assert_not_called() def test_has_reviewer(self): @@ -1061,7 +1061,7 @@ def test_has_reviewer(self): self.mocks['find_reviewer'].return_value = 'userD' handler.new_comment() self.mocks['is_collaborator'].assert_not_called() - self.mocks['find_reviewer'].assert_called_with('comment!') + self.mocks['find_reviewer'].assert_called_with('comment!', 'userA') self.mocks['set_assignee'].assert_called_with( 'userD', 'repo-owner', 'repo-name', '7', 'integrationUser', 'userA', None From 699c894e9658ac7ff29a652f3e9959751b533ed0 Mon Sep 17 00:00:00 2001 From: LeSeulArtichaut Date: Thu, 27 Feb 2020 14:22:35 +0100 Subject: [PATCH 2/2] Add unit test --- highfive/tests/fakes.py | 3 +++ highfive/tests/test_newpr.py | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/highfive/tests/fakes.py b/highfive/tests/fakes.py index ec7cf8fa..7fb56045 100644 --- a/highfive/tests/fakes.py +++ b/highfive/tests/fakes.py @@ -75,6 +75,9 @@ def get_repo_configs(): "reviewers": ["@JohnTitor"], } } + }, + 'teams': { + "groups": {"all": [], "a": ["@pnkfelix"], "b/c": ["@nrc"]} } } diff --git a/highfive/tests/test_newpr.py b/highfive/tests/test_newpr.py index eb4352f8..c72ca007 100644 --- a/highfive/tests/test_newpr.py +++ b/highfive/tests/test_newpr.py @@ -1260,6 +1260,31 @@ def test_mentions_without_dirs(self): ) assert set(["@JohnTitor"]) == mentions + def test_with_team_ping(self): + """Test choosing a reviewer when passed a team ping""" + handler = HighfiveHandlerMock( + Payload({}), repo_config=self.fakes['config']['teams'] + ).handler + + found_cases = ( + ("r? @foo/a", "pnkfelix"), + ("r? @b/c", "nrc"), + ) + + not_found_cases = ( + "r? @/a", + "r? @a/b", + ) + + for (msg, reviewer) in found_cases: + assert handler.find_reviewer(msg, None) == reviewer, \ + "expected '%s' from '%s'" % (reviewer, msg) + + for msg in not_found_cases: + assert handler.find_reviewer(msg, None) is None, \ + "expected '%s' to have no reviewer extracted" % msg + + class TestRun(TestNewPR): @pytest.fixture(autouse=True) def make_mocks(cls, patcherize):