From 2f8560193d4917ba4957ff1280afb97eb897eba6 Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: Sep 10 2021 12:39:15 +0000 Subject: pagure_bot: generate correct message when updating a ticket without votes This fixes a small glitch that if we updated a discussion ticket which haven't had any votes yet, we'd have the vote summary completely empty instead of saying 'Nobody voted yet'. This fixes the problem and covers the function with unit tests as a bonus (it was completely untested before). Merges: https://pagure.io/fedora-qa/blockerbugs/pull-request/213 --- diff --git a/blockerbugs/util/pagure_bot.py b/blockerbugs/util/pagure_bot.py index a9c9e7e..03c67ac 100644 --- a/blockerbugs/util/pagure_bot.py +++ b/blockerbugs/util/pagure_bot.py @@ -36,7 +36,6 @@ TRACKER_MATCHER = re.compile(TRACKER_RE) OUTCOME_RE = r'(accepted|rejected)' + TRACKER_RE OUTCOME_MATCHER = re.compile(OUTCOME_RE) -SUMMARY_HEADER = "" CLOSED_VOTES_HEADER = "The following votes have been closed:" # Containers for parsed commands @@ -342,14 +341,15 @@ def tracker_summary(tracker_name, tracker): return out -def summary(trackers, non_voting_users=None, last_comment_id=None, header=CLOSED_VOTES_HEADER): - '''Create a summary text in a Markdown format which can be used either in +def summary(trackers: dict[str, BugVoteTracker], non_voting_users: dict[str, int] = None, + last_comment_id: int = None, header: str = "") -> str: + '''Create an overall vote summary in the Markdown format which can be used either in a ticket description, or as a comment when a vote is closed. :param trackers: a dict as returned from :func:`parse_comments_to_trackers` - :param non_voting_users: a list of users who haven't voted yet + :param non_voting_users: users who haven't voted yet as returned by :func:`voting_info` :param int last_comment_id: the last comment id that was counted - :param str head: an initial message text + :param str header: an initial message text ''' md_text = "%s\n\n" % header @@ -382,6 +382,9 @@ def summary(trackers, non_voting_users=None, last_comment_id=None, header=CLOSED "processed comment was [#comment-%s](#comment-%s)*" % ( time, last_comment_id, last_comment_id)) + if not md_text or md_text.isspace(): + md_text = 'Nobody voted yet.' + return md_text @@ -449,11 +452,11 @@ def webhook_handler(issue_id): if bot_loop_detected(comments): app.logger.error('Bot is probably stuck in loop, not posting summary comment!') else: - comment = summary(trackers_without_summary) + comment = summary(trackers_without_summary, header=CLOSED_VOTES_HEADER) pagure_interface.post_comment(issue_id, comment) app.logger.debug('Updating issue summary') - vote_summary = summary(trackers, non_voting_users, last_comment_id, SUMMARY_HEADER) + vote_summary = summary(trackers, non_voting_users, last_comment_id) pagure_interface.update_issue(issue_id, vote_summary) app.logger.debug('Saving voting info to database') diff --git a/testing/test_pagure_bot.py b/testing/test_pagure_bot.py index 4cb01da..bd3fcef 100644 --- a/testing/test_pagure_bot.py +++ b/testing/test_pagure_bot.py @@ -8,11 +8,7 @@ from blockerbugs.util import pagure_bot EXTENDED_TRACKERS = pagure_bot.TRACKER_KEYWORDS + ["betafe", "finalfe"] -def powerset(iterable): - s = list(iterable) - return itertools.chain.from_iterable(itertools.combinations(s, r) for r in range(len(s) + 1)) - -stub_admin_user1 = "fakey" +stub_admin_user1 = "fakey1" stub_admin_user2 = "fakey2" stub_group_data = { "creator": { @@ -28,10 +24,17 @@ stub_group_data = { stub_admin_user1, stub_admin_user2, "fakey3", + "lbrabec", ], "name": "fake-group" } + +def powerset(iterable): + s = list(iterable) + return itertools.chain.from_iterable(itertools.combinations(s, r) for r in range(len(s) + 1)) + + def expand_fe(tracker): if tracker == 'betafe': return 'betafreezeexception' @@ -40,7 +43,22 @@ def expand_fe(tracker): else: return tracker -@pytest.fixture + +def line_contains(lines: str, words: list[str]) -> bool: + '''Find out if there is a line in ``lines`` which contains all the ``words`` + ''' + for line in lines.splitlines(): + for word in words: + if word not in line: + break + else: + # we didn't break, i.e. we found all words + return True + + return False + + +@pytest.fixture(autouse=True) def setup_stubs(monkeypatch): stub_pagure_group_call = mock.MagicMock(return_value=stub_group_data) monkeypatch.setattr(pagure_interface, 'get_group', stub_pagure_group_call) @@ -97,6 +115,138 @@ class TestPagureBotBasics(object): } +class TestSummary: + def test_summary_empty(self): + summary = pagure_bot.summary(trackers={}) + assert summary.strip() == 'Nobody voted yet.' + + @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) + def test_summary_proposed(self, keyword): + nicer_keyword = pagure_bot.NICER[expand_fe(keyword)] + comments = [pagure_bot.Comment(c) for c in [ + { + 'id': 1, + 'comment': "%s +1" % keyword, + 'user': { + 'name': stub_admin_user1 + } + }, + ]] + trackers = pagure_bot.parse_comments_to_trackers(comments) + non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers) + summary = pagure_bot.summary(trackers, non_voting_users, last_comment_id) + + assert line_contains(summary, [nicer_keyword, '+1', ' 0,', '-0']) + assert line_contains(summary, ['+1 by', stub_admin_user1]) + assert "Commented but haven't voted yet" not in summary + assert line_contains(summary, ['the last processed comment was', '#comment-1']) + + @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) + def test_summary_accepted(self, keyword): + nicer_keyword = pagure_bot.NICER[expand_fe(keyword)] + comments = [pagure_bot.Comment(c) for c in [ + { + 'id': 1, + 'comment': "%s +1" % keyword, + 'user': { + 'name': stub_admin_user1 + } + }, + { + 'id': 2, + 'comment': "AGREED Accepted%s" % keyword, + 'user': { + 'name': stub_admin_user1 + } + }, + ]] + trackers = pagure_bot.parse_comments_to_trackers(comments) + non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers) + summary = pagure_bot.summary(trackers, non_voting_users, last_comment_id) + + assert line_contains(summary, ['Accepted', nicer_keyword, '+1', ' 0,', '-0']) + assert line_contains(summary, ['+1 by', stub_admin_user1]) + assert "Commented but haven't voted yet" not in summary + assert line_contains(summary, ['the last processed comment was', '#comment-2']) + + @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) + def test_summary_rejected(self, keyword): + nicer_keyword = pagure_bot.NICER[expand_fe(keyword)] + comments = [pagure_bot.Comment(c) for c in [ + { + 'id': 1, + 'comment': "%s +1" % keyword, + 'user': { + 'name': stub_admin_user1 + } + }, + { + 'id': 2, + 'comment': "AGREED Rejected%s" % keyword, + 'user': { + 'name': stub_admin_user1 + } + }, + ]] + trackers = pagure_bot.parse_comments_to_trackers(comments) + non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers) + summary = pagure_bot.summary(trackers, non_voting_users, last_comment_id) + + assert line_contains(summary, ['Rejected', nicer_keyword, '+1', ' 0,', '-0']) + assert line_contains(summary, ['+1 by', stub_admin_user1]) + assert "Commented but haven't voted yet" not in summary + assert line_contains(summary, ['the last processed comment was', '#comment-2']) + + def test_summary_non_voting(self): + comments = [pagure_bot.Comment(c) for c in [ + { + 'id': 1, + 'comment': "BetaBlocker +1", + 'user': { + 'name': stub_admin_user1 + } + }, + { + 'id': 2, + 'comment': "Oooh shiny!", + 'user': { + 'name': stub_admin_user2 + } + }, + ]] + trackers = pagure_bot.parse_comments_to_trackers(comments) + non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers) + summary = pagure_bot.summary(trackers, non_voting_users, last_comment_id) + + assert line_contains(summary, ['BetaBlocker', '+1', ' 0,', '-0']) + assert line_contains(summary, ['+1 by', stub_admin_user1]) + assert line_contains(summary, ["Commented but haven't voted yet", stub_admin_user2]) + assert not line_contains(summary, ["Commented but haven't voted yet", stub_admin_user1]) + assert line_contains(summary, ['the last processed comment was', '#comment-2']) + + @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) + @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) + def test_summary_close_without_votes(self, keyword, outcome): + nicer_keyword = pagure_bot.NICER[expand_fe(keyword)] + comments = [pagure_bot.Comment(c) for c in [ + { + 'id': 1, + 'comment': "AGREED %s%s" % (outcome, keyword), + 'user': { + 'name': stub_admin_user1 + } + }, + ]] + trackers = pagure_bot.parse_comments_to_trackers(comments) + non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers) + summary = pagure_bot.summary(trackers, non_voting_users, last_comment_id) + + assert line_contains(summary, [outcome, nicer_keyword, '+0', ' 0,', '-0']) + assert not line_contains(summary, ['+1 by', stub_admin_user1]) + assert line_contains(summary, ["Commented but haven't voted yet", stub_admin_user1]) + assert line_contains(summary, ['the last processed comment was', '#comment-1']) + + class TestPagureBotTypos(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("sign", ['+', '-']) @@ -490,7 +640,7 @@ class TestPagureBotSinglelineMultivote(object): class TestPagureBotAgreedRevote(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) - def test_agreed_single_acceptedrejected_needs_summary(self, outcome, keyword, setup_stubs): + def test_agreed_single_acceptedrejected_needs_summary(self, outcome, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -519,7 +669,7 @@ class TestPagureBotAgreedRevote(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) - def test_agreed_single_acceptedrejected_no_justification_needs_summary(self, outcome, keyword, setup_stubs): + def test_agreed_single_acceptedrejected_no_justification_needs_summary(self, outcome, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -548,7 +698,7 @@ class TestPagureBotAgreedRevote(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) - def test_agreed_single_acceptedrejected_by_impostor_need_summary(self, outcome, keyword, setup_stubs): + def test_agreed_single_acceptedrejected_by_impostor_need_summary(self, outcome, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -583,7 +733,7 @@ class TestPagureBotAgreedRevote(object): assert trackers[expand_fe(keyword)].need_summary_post @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS)) - def test_agreed_multiple_accepts_need_summary(self, keywords, setup_stubs): + def test_agreed_multiple_accepts_need_summary(self, keywords): outcomes = " ".join(["accepted%s" % keyword for keyword in keywords]) comments = [pagure_bot.Comment(c) for c in [ { @@ -600,7 +750,7 @@ class TestPagureBotAgreedRevote(object): assert trackers[expand_fe(keyword)].need_summary_post @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS)) - def test_agreed_multiple_accepts_no_justification_need_summary(self, keywords, setup_stubs): + def test_agreed_multiple_accepts_no_justification_need_summary(self, keywords): outcomes = " ".join(["accepted%s" % keyword for keyword in keywords]) comments = [pagure_bot.Comment(c) for c in [ { @@ -617,7 +767,7 @@ class TestPagureBotAgreedRevote(object): assert trackers[expand_fe(keyword)].need_summary_post @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS)) - def test_agreed_multiple_accepts_has_summary(self, keywords, setup_stubs): + def test_agreed_multiple_accepts_has_summary(self, keywords): outcomes = " ".join(["accepted%s" % keyword for keyword in keywords]) comments = [pagure_bot.Comment(c) for c in [ { @@ -642,7 +792,7 @@ class TestPagureBotAgreedRevote(object): assert not trackers[expand_fe(keyword)].need_summary_post @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) - def test_revote_single(self, keyword, setup_stubs): + def test_revote_single(self, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -679,7 +829,7 @@ class TestPagureBotAgreedRevote(object): assert not trackers[expand_fe(keyword)].need_summary_post @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) - def test_revote_without_previous_summary(self, keyword, setup_stubs): + def test_revote_without_previous_summary(self, keyword): '''If the summary didn't arrive in time and the tracker got revoted in the meantime, no need to supply summary anymore. ''' @@ -713,7 +863,7 @@ class TestPagureBotAgreedRevote(object): @pytest.mark.parametrize("keyword_a, keyword_b", itertools.permutations(EXTENDED_TRACKERS, 2)) - def test_agreed_stops_counting_counts_other(self, keyword_a, keyword_b, setup_stubs): + def test_agreed_stops_counting_counts_other(self, keyword_a, keyword_b): # if we hit two keywords which are synonyms for each other, # move on if keyword_a.replace("fe", "freezeexception") == keyword_b.replace("fe", "freezeexception"): @@ -869,7 +1019,7 @@ class TestPagureBotWhitespaces(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) - def test_agreed_single_acceptedrejected_leading_wspace_needs_summary(self, outcome, keyword, setup_stubs): + def test_agreed_single_acceptedrejected_leading_wspace_needs_summary(self, outcome, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -892,7 +1042,7 @@ class TestPagureBotWhitespaces(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) - def test_agreed_single_acceptedrejected_trailing_wspace_needs_summary(self, outcome, keyword, setup_stubs): + def test_agreed_single_acceptedrejected_trailing_wspace_needs_summary(self, outcome, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -915,7 +1065,7 @@ class TestPagureBotWhitespaces(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) - def test_agreed_single_acceptedrejected_multi_wspace_inside_needs_summary(self, outcome, keyword, setup_stubs): + def test_agreed_single_acceptedrejected_multi_wspace_inside_needs_summary(self, outcome, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -938,7 +1088,7 @@ class TestPagureBotWhitespaces(object): @pytest.mark.parametrize("keyword", EXTENDED_TRACKERS) @pytest.mark.parametrize("outcome", ['Accepted', 'Rejected']) - def test_agreed_single_acceptedrejected_newline_inside(self, outcome, keyword, setup_stubs): + def test_agreed_single_acceptedrejected_newline_inside(self, outcome, keyword): comments = [pagure_bot.Comment(c) for c in [ { 'id': 1, @@ -960,7 +1110,7 @@ class TestPagureBotWhitespaces(object): assert not trackers[expand_fe(keyword)].need_summary_post @pytest.mark.parametrize("keywords", powerset(EXTENDED_TRACKERS)) - def test_agreed_multiple_accepts_multi_wspace_inside_need_summary(self, keywords, setup_stubs): + def test_agreed_multiple_accepts_multi_wspace_inside_need_summary(self, keywords): outcomes = " ".join(["accepted%s" % keyword for keyword in keywords]) comments = [pagure_bot.Comment(c) for c in [ { @@ -1140,7 +1290,7 @@ class TestPagureBotRealWorld(object): } ] - def test_realworld(self, setup_stubs): + def test_realworld(self): comments = [pagure_bot.Comment(c) for c in self.realworld_comments] trackers = pagure_bot.parse_comments_to_trackers(comments) non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers) @@ -1215,3 +1365,18 @@ class TestPagureBotRealWorld(object): } assert non_voting_users == {'jskladan': 9} assert last_comment_id == 14 + + def test_summary(self): + comments = [pagure_bot.Comment(c) for c in self.realworld_comments] + trackers = pagure_bot.parse_comments_to_trackers(comments) + non_voting_users, last_comment_id = pagure_bot.voting_info(comments, trackers) + summary = pagure_bot.summary(trackers, non_voting_users, last_comment_id) + + print(summary) + assert line_contains(summary, ['Accepted', 'FinalBlocker', '+3', ' 0,', '-1']) + assert line_contains(summary, ['Rejected', 'BetaBlocker', '+1', ' 0,', '-3']) + assert line_contains(summary, ['BetaFreezeException', '+3', ' 0,', '-0']) + assert line_contains(summary, ['FinalFreezeException', '+1', ' 1,', '-0']) + assert line_contains(summary, ['0Day', '+0', ' 1,', '-0']) + assert line_contains(summary, ["Commented but haven't voted yet", 'jskladan']) + assert line_contains(summary, ['the last processed comment was', '#comment-14'])