From 3f3ed927deb693472a038cc8ea997b07193cf615 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Wed, 22 May 2019 09:51:28 -0500 Subject: [PATCH 1/3] Test the command parsing logic Start adding some tests to Homu by testing the issue comment command-parsing logic. Take `parse_commands` and break it apart into two phases * Parsing phase * Execution phase The parsing phase returns a list of commands with action names (ideally, this would be a Rust enum, but to simulate that, we use action names on a single class) that are returned regardless of the current state. So for example, `@bors retry` will return a `retry` command regardless of the current state of `realtime`. The execution phase then inspects the list of commands and decides what to do with them. So for example, the `retry` command will be skipped if `realtime == False`. This has the positive result of having a parsing phase that has no side-effects, which makes it much easier to test. This can lead to higher confidence that the code works as expected without the high cost of testing in production and possibly disrupting the build flow. This has the negative result of adding a lot of lines of code to achieve command parsing, which we already do successfully without the tests. --- .gitignore | 1 + .travis.yml | 4 +- homu/main.py | 125 ++++---- homu/parse_issue_comment.py | 246 ++++++++++++++++ homu/test_parse_issue_comment.py | 481 +++++++++++++++++++++++++++++++ 5 files changed, 787 insertions(+), 70 deletions(-) create mode 100644 homu/parse_issue_comment.py create mode 100644 homu/test_parse_issue_comment.py diff --git a/.gitignore b/.gitignore index 258e93f..8be4882 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ /homu.egg-info/ /main.db /cache +*.pyc diff --git a/.travis.yml b/.travis.yml index 5f35324..177555f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,4 +6,6 @@ python: install: - pip install flake8 script: - - flake8 homu \ No newline at end of file + - flake8 homu + - cd homu + - python -m unittest diff --git a/homu/main.py b/homu/main.py index 7b4f98a..db14be9 100644 --- a/homu/main.py +++ b/homu/main.py @@ -6,6 +6,7 @@ import functools from . import comments from . import utils +from .parse_issue_comment import parse_issue_comment from .auth import verify as verify_auth from .utils import lazy_debug import logging @@ -15,7 +16,6 @@ import sqlite3 import requests from contextlib import contextmanager -from itertools import chain from queue import Queue import os import sys @@ -469,28 +469,20 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, my_username, ) - words = list(chain.from_iterable(re.findall(r'\S+', x) for x in body.splitlines() if '@' + my_username in x)) # noqa - if words[1:] == ["are", "you", "still", "there?"] and realtime: - state.add_comment( - ":cake: {}\n\n![]({})".format( - random.choice(PORTAL_TURRET_DIALOG), PORTAL_TURRET_IMAGE) - ) - for i, word in reversed(list(enumerate(words))): + hooks = [] + if 'hooks' in global_cfg: + hooks = list(global_cfg['hooks'].keys()) + + commands = parse_issue_comment(username, body, sha, my_username, hooks) + + for command in commands: found = True - if word == 'r+' or word.startswith('r='): + if command.action == 'approve': if not _reviewer_auth_verified(): continue - if not sha and i + 1 < len(words): - cur_sha = sha_or_blank(words[i + 1]) - else: - cur_sha = sha - - approver = word[len('r='):] if word.startswith('r=') else username - - # Ignore "r=me" - if approver == 'me': - continue + approver = command.actor + cur_sha = command.commit # Ignore WIP PRs is_wip = False @@ -582,7 +574,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, ) state.change_labels(LabelEvent.APPROVED) - elif word == 'r-': + elif command.action == 'unapprove': if not verify_auth(username, repo_label, repo_cfg, state, AuthState.REVIEWER, realtime, my_username): continue @@ -592,14 +584,12 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, if realtime: state.change_labels(LabelEvent.REJECTED) - elif word.startswith('p='): + elif command.action == 'prioritize': if not verify_auth(username, repo_label, repo_cfg, state, AuthState.TRY, realtime, my_username): continue - try: - pvalue = int(word[len('p='):]) - except ValueError: - continue + + pvalue = command.priority if pvalue > global_cfg['max_priority']: if realtime: @@ -611,12 +601,12 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, state.priority = pvalue state.save() - elif word.startswith('delegate='): + elif command.action == 'delegate': if not verify_auth(username, repo_label, repo_cfg, state, AuthState.REVIEWER, realtime, my_username): continue - state.delegate = word[len('delegate='):] + state.delegate = command.delegate_to state.save() if realtime: @@ -625,14 +615,14 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, .format(state.delegate) ) - elif word == 'delegate-': + elif command.action == 'undelegate': # TODO: why is this a TRY? if not _try_auth_verified(): continue state.delegate = '' state.save() - elif word == 'delegate+': + elif command.action == 'delegate-author': if not _reviewer_auth_verified(): continue @@ -645,7 +635,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, .format(state.delegate) ) - elif word == 'retry' and realtime: + elif command.action == 'retry' and realtime: if not _try_auth_verified(): continue state.set_status('') @@ -654,7 +644,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, state.record_retry_log(command_src, body) state.change_labels(event) - elif word in ['try', 'try-'] and realtime: + elif command.action in ['try', 'untry'] and realtime: if not _try_auth_verified(): continue if state.status == '' and state.approved_by: @@ -665,7 +655,7 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, ) continue - state.try_ = word == 'try' + state.try_ = command.action == 'try' state.merge_sha = '' state.init_build_res([]) @@ -676,14 +666,14 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, # any meaningful labeling events. state.change_labels(LabelEvent.TRY) - elif word in WORDS_TO_ROLLUP: + elif command.action == 'rollup': if not _try_auth_verified(): continue - state.rollup = WORDS_TO_ROLLUP[word] + state.rollup = command.rollup_value state.save() - elif word == 'force' and realtime: + elif command.action == 'force' and realtime: if not _try_auth_verified(): continue if 'buildbot' in repo_cfg: @@ -712,52 +702,51 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, ':bomb: Buildbot returned an error: `{}`'.format(err) ) - elif word == 'clean' and realtime: + elif command.action == 'clean' and realtime: if not _try_auth_verified(): continue state.merge_sha = '' state.init_build_res([]) state.save() - elif (word == 'hello?' or word == 'ping') and realtime: - state.add_comment(":sleepy: I'm awake I'm awake") - elif word.startswith('treeclosed='): + + elif command.action == 'ping' and realtime: + if command.ping_type == 'portal': + state.add_comment( + ":cake: {}\n\n![]({})".format( + random.choice(PORTAL_TURRET_DIALOG), + PORTAL_TURRET_IMAGE) + ) + else: + state.add_comment(":sleepy: I'm awake I'm awake") + + elif command.action == 'treeclosed': if not _reviewer_auth_verified(): continue - try: - treeclosed = int(word[len('treeclosed='):]) - state.change_treeclosed(treeclosed, command_src) - except ValueError: - pass + state.change_treeclosed(command.treeclosed_value, command_src) state.save() - elif word == 'treeclosed-': + + elif command.action == 'untreeclosed': if not _reviewer_auth_verified(): continue state.change_treeclosed(-1, None) state.save() - elif 'hooks' in global_cfg: - hook_found = False - for hook in global_cfg['hooks']: - hook_cfg = global_cfg['hooks'][hook] - if hook_cfg['realtime'] and not realtime: + + elif command.action == 'hook': + hook = command.hook_name + hook_cfg = global_cfg['hooks'][hook] + if hook_cfg['realtime'] and not realtime: + continue + if hook_cfg['access'] == "reviewer": + if not _reviewer_auth_verified(): continue - if word == hook or word.startswith('%s=' % hook): - if hook_cfg['access'] == "reviewer": - if not _reviewer_auth_verified(): - continue - else: - if not _try_auth_verified(): - continue - hook_found = True - extra_data = "" - if word.startswith('%s=' % hook): - extra_data = word.split("=")[1] - Thread( - target=handle_hook_response, - args=[state, hook_cfg, body, extra_data] - ).start() - if not hook_found: - found = False + else: + if not _try_auth_verified(): + continue + Thread( + target=handle_hook_response, + args=[state, hook_cfg, body, command.hook_extra] + ).start() else: found = False @@ -765,8 +754,6 @@ def parse_commands(body, username, repo_label, repo_cfg, state, my_username, if found: state_changed = True - words[i] = '' - return state_changed diff --git a/homu/parse_issue_comment.py b/homu/parse_issue_comment.py new file mode 100644 index 0000000..7fdd3c5 --- /dev/null +++ b/homu/parse_issue_comment.py @@ -0,0 +1,246 @@ +from itertools import chain +import re + + +class IssueCommentCommand: + """ + A command that has been parsed out of a GitHub issue comment. + + E.g., `@bors r+` => an issue command with action == 'approve' + """ + + def __init__(self, action): + self.action = action + + @classmethod + def approve(cls, approver, commit): + command = cls('approve') + command.commit = commit + command.actor = approver + return command + + @classmethod + def unapprove(cls): + return cls('unapprove') + + @classmethod + def prioritize(cls, priority): + command = cls('prioritize') + command.priority = priority + return command + + @classmethod + def delegate_author(cls): + return cls('delegate-author') + + @classmethod + def delegate(cls, delegate_to): + command = cls('delegate') + command.delegate_to = delegate_to + return command + + @classmethod + def undelegate(cls): + return cls('undelegate') + + @classmethod + def retry(cls): + return cls('retry') + + @classmethod + def try_(cls): + return cls('try') + + @classmethod + def untry(cls): + return cls('untry') + + @classmethod + def rollup(cls, rollup_value): + command = cls('rollup') + command.rollup_value = rollup_value + return command + + @classmethod + def force(cls): + return cls('force') + + @classmethod + def clean(cls): + return cls('clean') + + @classmethod + def ping(cls, ping_type='standard'): + command = cls('ping') + command.ping_type = ping_type + return command + + @classmethod + def treeclosed(cls, treeclosed_value): + command = cls('treeclosed') + command.treeclosed_value = treeclosed_value + return command + + @classmethod + def untreeclosed(cls): + return cls('untreeclosed') + + @classmethod + def hook(cls, hook_name, hook_extra=None): + command = cls('hook') + command.hook_name = hook_name + command.hook_extra = hook_extra + return command + + +WORDS_TO_ROLLUP = { + 'rollup-': 0, + 'rollup': 1, + 'rollup=maybe': 0, + 'rollup=never': -1, + 'rollup=always': 1, +} + + +def is_sha(sha): + """ + Try to determine if the input is a git sha + """ + return re.match(r'^[0-9a-f]{4,}$', sha) + + +def hook_with_extra_is_in_hooks(word, hooks): + """ + Determine if the word given is the name of a valid hook, with extra data + hanging off of it (e.g., `validhookname=extradata`). + + hook_with_extra_is_in_hooks( + 'validhookname=stuff', + ['validhookname', 'other']) + #=> True + + hook_with_extra_is_in_hooks( + 'invalidhookname=stuff', + ['validhookname', 'other']) + #=> False + + hook_with_extra_is_in_hooks( + 'validhookname', + ['validhookname', 'other']) + #=> False + """ + for hook in hooks: + if word.startswith('{}='.format(hook)): + return True + + return False + + +def parse_issue_comment(username, body, sha, botname, hooks=[]): + """ + Parse an issue comment looking for commands that Homu should handle + + Parameters: + username: the username of the user that created the issue comment. + This is without the leading @ + body: the full body of the comment (markdown) + sha: the commit that the comment applies to + botname: the name of bot. This is without the leading @. + So if we should respond to `@bors {command}`, botname will be `bors` + hooks: a list of strings that are valid hook names. + E.g. `['hook1', 'hook2', 'hook3']` + """ + + words = list(chain.from_iterable(re.findall(r'\S+', x) for x in body.splitlines() if '@' + botname in x)) # noqa + + commands = [] + + if words[1:] == ["are", "you", "still", "there?"]: + commands.append(IssueCommentCommand.ping('portal')) + + for i, word in reversed(list(enumerate(words))): + found = True + if word == 'r+' or word.startswith('r='): + approved_sha = sha + + if i + 1 < len(words) and is_sha(words[i + 1]): + approved_sha = words[i + 1] + + approver = word[len('r='):] if word.startswith('r=') else username + + # Ignore "r=me" + if approver == 'me': + continue + + commands.append( + IssueCommentCommand.approve(approver, approved_sha)) + + elif word == 'r-': + commands.append(IssueCommentCommand.unapprove()) + + elif word.startswith('p='): + try: + pvalue = int(word[len('p='):]) + except ValueError: + continue + + commands.append(IssueCommentCommand.prioritize(pvalue)) + + elif word.startswith('delegate='): + delegate = word[len('delegate='):] + commands.append(IssueCommentCommand.delegate(delegate)) + + elif word == 'delegate-': + commands.append(IssueCommentCommand.undelegate()) + + elif word == 'delegate+': + commands.append(IssueCommentCommand.delegate_author()) + + elif word == 'retry': + commands.append(IssueCommentCommand.retry()) + + elif word == 'try': + commands.append(IssueCommentCommand.try_()) + + elif word == 'try-': + commands.append(IssueCommentCommand.untry()) + + elif word in WORDS_TO_ROLLUP: + rollup_value = WORDS_TO_ROLLUP[word] + commands.append(IssueCommentCommand.rollup(rollup_value)) + + elif word == 'force': + commands.append(IssueCommentCommand.force()) + + elif word == 'clean': + commands.append(IssueCommentCommand.clean()) + + elif (word == 'hello?' or word == 'ping'): + commands.append(IssueCommentCommand.ping()) + + elif word.startswith('treeclosed='): + try: + treeclosed = int(word[len('treeclosed='):]) + commands.append(IssueCommentCommand.treeclosed(treeclosed)) + except ValueError: + pass + + elif word == 'treeclosed-': + commands.append(IssueCommentCommand.untreeclosed()) + + elif word in hooks: + commands.append(IssueCommentCommand.hook(word)) + + elif hook_with_extra_is_in_hooks(word, hooks): + # word is like `somehook=data` and `somehook` is in our list of + # potential hooks + (hook_name, hook_extra) = word.split('=', 2) + commands.append(IssueCommentCommand.hook(hook_name, hook_extra)) + + else: + found = False + + if found: + words[i] = '' + + return commands diff --git a/homu/test_parse_issue_comment.py b/homu/test_parse_issue_comment.py new file mode 100644 index 0000000..80cb0f1 --- /dev/null +++ b/homu/test_parse_issue_comment.py @@ -0,0 +1,481 @@ +import unittest +from parse_issue_comment import parse_issue_comment + +# Random commit number. Just so that we don't need to come up with a new one +# for every test. +commit = "5ffafdb1e94fa87334d4851a57564425e11a569e" +other_commit = "4e4c9ddd781729173df2720d83e0f4d1b0102a94" + + +class TestParseIssueComment(unittest.TestCase): + def test_r_plus(self): + """ + @bors r+ + """ + + author = "jack" + body = "@bors r+" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'approve') + self.assertEqual(command.actor, 'jack') + + def test_r_plus_with_sha(self): + """ + @bors r+ {sha} + """ + + author = "jack" + body = "@bors r+ {}".format(other_commit) + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'approve') + self.assertEqual(command.actor, 'jack') + self.assertEqual(command.commit, other_commit) + + def test_r_equals(self): + """ + @bors r=jill + """ + + author = "jack" + body = "@bors r=jill" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'approve') + self.assertEqual(command.actor, 'jill') + + def test_r_me(self): + """ + Ignore r=me + """ + + author = "jack" + body = "@bors r=me" + commands = parse_issue_comment(author, body, commit, "bors") + + # r=me is not a valid command, so no valid commands. + self.assertEqual(len(commands), 0) + + def test_r_minus(self): + """ + @bors r- + """ + + author = "jack" + body = "@bors r-" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'unapprove') + + def test_priority(self): + """ + @bors p=5 + """ + + author = "jack" + body = "@bors p=5" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'prioritize') + self.assertEqual(command.priority, 5) + + def test_approve_and_priority(self): + """ + @bors r+ p=5 + """ + + author = "jack" + body = "@bors r+ p=5" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 2) + approve_commands = [command for command in commands + if command.action == 'approve'] + prioritize_commands = [command for command in commands + if command.action == 'prioritize'] + self.assertEqual(len(approve_commands), 1) + self.assertEqual(len(prioritize_commands), 1) + + self.assertEqual(approve_commands[0].actor, 'jack') + self.assertEqual(prioritize_commands[0].priority, 5) + + def test_approve_specific_and_priority(self): + """ + @bors r+ {sha} p=5 + """ + + author = "jack" + body = "@bors r+ {} p=5".format(other_commit) + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 2) + approve_commands = [command for command in commands + if command.action == 'approve'] + prioritize_commands = [command for command in commands + if command.action == 'prioritize'] + self.assertEqual(len(approve_commands), 1) + self.assertEqual(len(prioritize_commands), 1) + + self.assertEqual(approve_commands[0].actor, 'jack') + self.assertEqual(approve_commands[0].commit, other_commit) + self.assertEqual(prioritize_commands[0].priority, 5) + + def test_delegate_plus(self): + """ + @bors delegate+ + """ + + author = "jack" + body = "@bors delegate+" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'delegate-author') + + def test_delegate_equals(self): + """ + @bors delegate={username} + """ + + author = "jack" + body = "@bors delegate=jill" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'delegate') + self.assertEqual(command.delegate_to, 'jill') + + def test_delegate_minus(self): + """ + @bors delegate- + """ + + author = "jack" + body = "@bors delegate-" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'undelegate') + + def test_retry(self): + """ + @bors retry + """ + + author = "jack" + body = "@bors retry" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'retry') + + def test_try(self): + """ + @bors try + """ + + author = "jack" + body = "@bors try" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'try') + + def test_try_minus(self): + """ + @bors try- + """ + + author = "jack" + body = "@bors try-" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'untry') + + def test_rollup(self): + """ + @bors rollup + """ + + author = "jack" + body = "@bors rollup" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'rollup') + self.assertEqual(command.rollup_value, 1) + + def test_rollup_minus(self): + """ + @bors rollup- + """ + + author = "jack" + body = "@bors rollup-" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'rollup') + self.assertEqual(command.rollup_value, 0) + + def test_rollup_never(self): + """ + @bors rollup=never + """ + + author = "jack" + body = "@bors rollup=never" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'rollup') + self.assertEqual(command.rollup_value, -1) + + def test_rollup_maybe(self): + """ + @bors rollup=maybe + """ + + author = "jack" + body = "@bors rollup=maybe" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'rollup') + self.assertEqual(command.rollup_value, 0) + + def test_rollup_always(self): + """ + @bors rollup=always + """ + + author = "jack" + body = "@bors rollup=always" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'rollup') + self.assertEqual(command.rollup_value, 1) + + def test_force(self): + """ + @bors force + """ + + author = "jack" + body = "@bors force" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'force') + + def test_clean(self): + """ + @bors clean + """ + + author = "jack" + body = "@bors clean" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'clean') + + def test_ping(self): + """ + @bors ping + """ + + author = "jack" + body = "@bors ping" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'ping') + self.assertEqual(command.ping_type, 'standard') + + def test_hello(self): + """ + @bors hello? + """ + + author = "jack" + body = "@bors hello?" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'ping') + self.assertEqual(command.ping_type, 'standard') + + def test_portal_ping(self): + """ + @bors are you still there? + """ + + author = "jack" + body = "@bors are you still there?" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'ping') + self.assertEqual(command.ping_type, 'portal') + + def test_treeclosed(self): + """ + @bors treeclosed=50 + """ + + author = "jack" + body = "@bors treeclosed=50" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'treeclosed') + self.assertEqual(command.treeclosed_value, 50) + + def test_treeclosed_minus(self): + """ + @bors treeclosed- + """ + + author = "jack" + body = "@bors treeclosed-" + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'untreeclosed') + + def test_hook(self): + """ + Test hooks that are defined in the configuration + + @bors secondhook + """ + + author = "jack" + body = "@bors secondhook" + commands = parse_issue_comment( + author, body, commit, "bors", + ['firsthook', 'secondhook', 'thirdhook']) + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'hook') + self.assertEqual(command.hook_name, 'secondhook') + self.assertEqual(command.hook_extra, None) + + def test_hook_equals(self): + """ + Test hooks that are defined in the configuration + + @bors secondhook=extra + """ + + author = "jack" + body = "@bors secondhook=extra" + commands = parse_issue_comment( + author, body, commit, "bors", + ['firsthook', 'secondhook', 'thirdhook']) + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'hook') + self.assertEqual(command.hook_name, 'secondhook') + self.assertEqual(command.hook_extra, 'extra') + + def test_multiple_hooks(self): + """ + Test hooks that are defined in the configuration + + @bors thirdhook secondhook=extra + """ + + author = "jack" + body = "@bors thirdhook secondhook=extra" + commands = parse_issue_comment( + author, body, commit, "bors", + ['firsthook', 'secondhook', 'thirdhook']) + + self.assertEqual(len(commands), 2) + secondhook_commands = [command for command in commands + if command.action == 'hook' + and command.hook_name == 'secondhook'] + thirdhook_commands = [command for command in commands + if command.action == 'hook' + and command.hook_name == 'thirdhook'] + self.assertEqual(len(secondhook_commands), 1) + self.assertEqual(len(thirdhook_commands), 1) + self.assertEqual(secondhook_commands[0].hook_extra, 'extra') + self.assertEqual(thirdhook_commands[0].hook_extra, None) + + def test_ignore_commands_before_bors_line(self): + """ + Test that when command-like statements appear before the @bors part, + they don't get parsed + """ + + author = "jack" + body = """ + A sentence that includes command-like statements, like r- or ping or delegate+ or the like. + + @bors r+ + """ # noqa + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'approve') + self.assertEqual(command.actor, 'jack') + + def test_ignore_commands_after_bors_line(self): + """ + Test that when command-like statements appear after the @bors part, + they don't get parsed + """ + + author = "jack" + body = """ + @bors r+ + + A sentence that includes command-like statements, like r- or ping or delegate+ or the like. + """ # noqa + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'approve') + self.assertEqual(command.actor, 'jack') + + +if __name__ == '__main__': + unittest.main() From 2f9b34c257ee5c5a482cf4651ca17a1edd2eb69a Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Fri, 31 May 2019 07:34:31 -0500 Subject: [PATCH 2/3] Use a more python-standard test setup Use `python setup.py test` to run the tests, and a few minor changes required to support that. --- .travis.yml | 4 ++-- homu/tests/__init__.py | 0 homu/{ => tests}/test_parse_issue_comment.py | 2 +- setup.py | 1 + 4 files changed, 4 insertions(+), 3 deletions(-) create mode 100644 homu/tests/__init__.py rename homu/{ => tests}/test_parse_issue_comment.py (99%) diff --git a/.travis.yml b/.travis.yml index 177555f..9a8a947 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,5 +7,5 @@ install: - pip install flake8 script: - flake8 homu - - cd homu - - python -m unittest + - pip install -e . + - python setup.py test diff --git a/homu/tests/__init__.py b/homu/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/homu/test_parse_issue_comment.py b/homu/tests/test_parse_issue_comment.py similarity index 99% rename from homu/test_parse_issue_comment.py rename to homu/tests/test_parse_issue_comment.py index 80cb0f1..e0bd862 100644 --- a/homu/test_parse_issue_comment.py +++ b/homu/tests/test_parse_issue_comment.py @@ -1,5 +1,5 @@ import unittest -from parse_issue_comment import parse_issue_comment +from homu.parse_issue_comment import parse_issue_comment # Random commit number. Just so that we don't need to come up with a new one # for every test. diff --git a/setup.py b/setup.py index 96b4561..392e0eb 100644 --- a/setup.py +++ b/setup.py @@ -5,6 +5,7 @@ version='0.3.0', author='Barosl Lee', url='https://github.com/barosl/homu', + test_suite='homu.tests', description=('A bot that integrates with GitHub ' 'and your favorite continuous integration service'), From 96be213eba7599aaee5ddd8e3c7b689339214f67 Mon Sep 17 00:00:00 2001 From: Bryan Burgers Date: Mon, 3 Jun 2019 12:55:12 -0500 Subject: [PATCH 3/3] GitHub as event stream proof-of-concept We currently have two sources-of-truth when syncing on startup: the database and issue comments. This proof of concept tries to bring us down to a single source-of-truth by effectively elimating the local database (at least for pull request state; other aspects like `treeclosed` are not addressed). In order to do this, we use GitHub's "timeline" ([v3](https://developer.github.com/v3/issues/timeline/), [v4](https://developer.github.com/v4/object/pullrequest/#timelineitems)) feature to get a fuller view of what has happened to a pull request so we can reify state in a more complete manner than using issue comments alone. For example, the timeline API interleaves issue comments with things like `AssignedEvent`, `HeadRefForcePushedEvent`, and `RenamedTitleEvent` in the order they happened so we can reliably build the full state from this historical record. The general concept is that in both `synchronizing` and `realtime` mode, we have the pull request object respond to the event to update the state. Side effects like comments and label updates are returned, and the code can use or drop those depending on which mode Homu in. PR state --------\ | +---------------+ \---> | | -----> Modified PR state | process_event | /---> | | --\ | +---------------+ +--> Realtime comments GitHub Event ----/ +--> Label updates \--> Database update The good thing is that we can model these timeline items and GitHub's event webhooks (which we use when processing in `realtime` mode) in much the same way, so that we can unify them in a way that makes it possible to effectively test the effect of events on a pull request. This particular pull request does not attempt to integrate these changes into the codebase yet, because I wanted to get feedback that this is indeed a viable path to head down. Instead, this pull request (which should not be merged) provides a new executable that demonstrates this ability on existing pull requests in a read-only mode. The output is a series of relevant timeline items, and the state of the pull request as it changes. pip install -e homu homu-sync-test --repo rust-lang/rust 61419 --- homu/PullRequest.py | 227 ++++++++++++++++++++ homu/parse_issue_comment.py | 18 +- homu/pull_request_events.py | 279 +++++++++++++++++++++++++ homu/tests/test_parse_issue_comment.py | 22 ++ setup.py | 1 + 5 files changed, 545 insertions(+), 2 deletions(-) create mode 100644 homu/PullRequest.py create mode 100644 homu/pull_request_events.py diff --git a/homu/PullRequest.py b/homu/PullRequest.py new file mode 100644 index 0000000..cf5a789 --- /dev/null +++ b/homu/PullRequest.py @@ -0,0 +1,227 @@ +from .parse_issue_comment import parse_issue_comment +import datetime + + +class PullRequest: + def __init__(self, pull): + self.owner = pull.owner + self.repo = pull.repo + self.number = pull.pull + self.title = pull.initial_title + self.author = pull.author + self.assignee = None + self.approver = None + + # Before we start going through the events, the state is 'open'. We'll + # let the history of the pull request tell us differently. + self.github_state = 'open' + + # Before we start going through the events, this is not approved We'll + # let the history of the pull request tell us differently. + self.approval_state = 'unapproved' + self.build_state = 'none' + + # The way GitHub's timeline events work, one of the first events will + # be a PullRequestCommit event that we can get the current SHA from. + # However, if this is pull request that has existed for a bit, and it + # has had a force push in it at some point, we may not get the initial + # sha. So we'll have to handle that as well. To start, we'll set this + # to None to represent that we don't know the initial sha, and if we + # get an early PullRequestCommit, we'll update it. + self.head_sha = None + + self.tries = [] + + @property + def state(self): + if self.github_state == 'open': + if self.build_state != 'none': + return self.build_state + return self.approval_state + return self.github_state + + def __str__(self): + output = """ +PullRequest: {owner}/{repo}#{number} + title: {title} + author: {author} + assignee: {assignee} + approver: {approver} + head: {head} + state: {state} + tries: {tries} +""".format( + owner=self.owner, + repo=self.repo, + number=self.number, + title=self.title, + author=self.author, + assignee=self.assignee if self.assignee is not None else 'None', + approver=self.approver if self.approver is not None else 'None', + head=self.head_sha[0:7] if self.head_sha is not None else 'None', + state=self.state, + tries=len(self.tries) + ) + + for try_ in self.tries: + output += " " + str(try_) + "\n" + + return output.strip() + + def process_event(self, event): + changed = False + if event.event_type == 'PullRequestCommit': + changed = self.head_sha != event['commit']['oid'] + self.head_sha = event['commit']['oid'] + + elif event.event_type == 'HeadRefForcePushedEvent': + changed = self.head_sha != event['afterCommit']['oid'] + self.head_sha = event['afterCommit']['oid'] + + elif event.event_type == 'IssueComment': + comments = parse_issue_comment( + username=event['author']['login'], + body=event['body'], + sha=self.head_sha, + botname='bors', + hooks=[]) + + for comment in comments: + (subchanged,) = self.process_issue_comment(event, comment) + changed = changed or subchanged + + elif event.event_type == 'RenamedTitleEvent': + changed = self.title != event['currentTitle'] + self.title = event['currentTitle'] + + elif event.event_type == 'AssignedEvent': + changed = self.assignee != event['user']['login'] + self.assignee = event['user']['login'] + + elif event.event_type == 'PullRequestReview': + # TODO: Pull commands from review comments + pass + + elif event.event_type == 'MergedEvent': + changed = self.github_state != 'merged' + self.github_state = 'merged' + + elif event.event_type == 'ClosedEvent': + if self.github_state != 'merged': + changed = self.github_state != 'closed' + self.github_state = 'closed' + + elif event.event_type == 'ReopenedEvent': + changed = self.github_state != 'open' + self.github_state = 'open' + + elif event.event_type in [ + 'SubscribedEvent', + 'MentionedEvent', + 'LabeledEvent', + 'UnlabeledEvent', + 'ReferencedEvent', + 'CrossReferencedEvent']: + # We don't care about any of these events. + pass + else: + # Ooops, did we miss this event type? Or is it new? + print("Unknown event type: {}".format(event.event_type)) + + return (changed,) + + def process_issue_comment(self, event, command): + changed = False + if command.action == 'homu-state': + return self.process_homu_state(event, command) + + if command.action == 'approve': + changed = self.approval_state != 'approved' + changed = changed or self.approver != command.actor + self.approval_state = 'approved' + self.approver = command.actor + + if command.action == 'unapprove': + changed = self.approval_state != 'unapproved' + changed = changed or self.approver is not None + self.approval_state = 'unapproved' + self.approver = None + + # if command.action == 'try': + # changed = True + # self.tries.append(PullRequestTry(1, self.head_sha, None)) + return (changed,) + + def process_homu_state(self, event, command): + changed = False + state = command.homu_state + + if state['type'] == 'Approved': + changed = self.approval_state != 'approved' + changed = changed or self.approver != state['approver'] + self.approval_state = 'approved' + self.approver = state['approver'] + + elif state['type'] == 'BuildStarted': + changed = True + self.build_state = 'pending' + + elif state['type'] == 'BuildCompleted': + changed = True + self.build_state = 'completed' + + elif state['type'] == 'BuildFailed': + changed = True + self.build_state = 'failure' + + elif state['type'] == 'TryBuildStarted': + changed = True + self.tries.append(PullRequestTry( + len(self.tries) + 1, + state['head_sha'], + state['merge_sha'], + event['publishedAt']) + ) + + elif state['type'] == 'TryBuildCompleted': + item = next((try_ + for try_ in self.tries + if try_.state == 'pending' + and try_.merge_sha == state['merge_sha']), + None) + + if item: + changed = True + item.ended_at = event['publishedAt'] + item.state = 'completed' + item.builders = state['builders'] + + return (changed,) + + +class PullRequestTry: + def __init__(self, number, head_sha, merge_sha, started_at): + self.number = number + self.head_sha = head_sha + self.merge_sha = merge_sha + self.state = 'pending' + self.started_at = started_at + + def __str__(self): + return "Try #{} for {}: {}".format( + self.number, + self.head_sha[0:7], + self.expanded_state) + + @property + def expanded_state(self): + if self.state == 'completed' and self.started_at and self.ended_at: + start = datetime.datetime.strptime( + self.started_at, + "%Y-%m-%dT%H:%M:%S%z") + end = datetime.datetime.strptime( + self.ended_at, + "%Y-%m-%dT%H:%M:%S%z") + duration = end - start + return "{} after {}s".format(self.state, duration.total_seconds()) + return self.state diff --git a/homu/parse_issue_comment.py b/homu/parse_issue_comment.py index 7fdd3c5..a1b1657 100644 --- a/homu/parse_issue_comment.py +++ b/homu/parse_issue_comment.py @@ -1,5 +1,6 @@ from itertools import chain import re +import json class IssueCommentCommand: @@ -92,6 +93,12 @@ def hook(cls, hook_name, hook_extra=None): command.hook_extra = hook_extra return command + @classmethod + def homu_state(cls, state): + command = cls('homu-state') + command.homu_state = state + return command + WORDS_TO_ROLLUP = { 'rollup-': 0, @@ -151,10 +158,17 @@ def parse_issue_comment(username, body, sha, botname, hooks=[]): E.g. `['hook1', 'hook2', 'hook3']` """ - words = list(chain.from_iterable(re.findall(r'\S+', x) for x in body.splitlines() if '@' + botname in x)) # noqa - commands = [] + states = chain.from_iterable(re.findall(r'', x) + for x + in body.splitlines()) + + for state in states: + commands.append(IssueCommentCommand.homu_state(json.loads(state))) + + words = list(chain.from_iterable(re.findall(r'\S+', x) for x in body.splitlines() if '@' + botname in x)) # noqa + if words[1:] == ["are", "you", "still", "there?"]: commands.append(IssueCommentCommand.ping('portal')) diff --git a/homu/pull_request_events.py b/homu/pull_request_events.py new file mode 100644 index 0000000..c2252fe --- /dev/null +++ b/homu/pull_request_events.py @@ -0,0 +1,279 @@ +import requests +import time + + +QUERY = """ +query ($repoName: String!, $repoOwner: String!, $pull: Int!, $after: String) { + repository(name: $repoName, owner: $repoOwner) { + pullRequest(number: $pull) { + author { + login + } + title + state + headRefOid + mergeable + timelineItems(first: 100, after: $after) { + pageInfo { + hasNextPage + endCursor + } + nodes { + eventType: __typename + ... on PullRequestCommit { + commit { + oid + } + } + ... on AssignedEvent { + actor { + login + } + user { + login + } + } + ... on UnassignedEvent { + actor { + login + } + user { + login + } + } + ... on IssueComment { + author { + login + } + body + publishedAt + } + ... on SubscribedEvent { + actor { + login + } + } + ... on LabeledEvent { + actor { + login + } + label { + name + } + } + ... on UnlabeledEvent { + actor { + login + } + label { + name + } + } + ... on HeadRefForcePushedEvent { + actor { + login + } + beforeCommit { + oid + } + afterCommit { + oid + } + } + ... on RenamedTitleEvent { + actor { + login + } + previousTitle + currentTitle + } + ... on MentionedEvent { + actor { + login + } + } + } + } + } + } +} +""" + + +class PullRequestResponse: + def __init__(self): + self.events = [] + + @property + def initial_title(self): + if not hasattr(self, '_initial_title'): + for event in self.events: + if event.event_type == 'RenamedTitleEvent': + self._initial_title = event.data['previousTitle'] + break + + # The title never changed. That means that the initial title is + # the same as the current title. + if not hasattr(self, '_initial_title'): + self._initial_title = self.title + + return self._initial_title + + +class PullRequestEvent: + def __init__(self, data): + self.data = data + + def __getitem__(self, key): + return self.data[key] + + @property + def event_type(self): + return self.data['eventType'] + + @staticmethod + def _actor(s): + return "\x1b[1m@" + s + "\x1b[0m" + + @staticmethod + def _label(s): + return "\x1b[100m" + s + "\x1b[0m" + + @staticmethod + def _commit(s): + return "\x1b[93m" + s[0:7] + "\x1b[0m" + + @staticmethod + def _comment_summary(comment): + # line_1 = comment.splitlines()[0] + # if len(line_1) > 40: + # return line_1[0:37] + '...' + # else: + # return line_1 + return '\n'.join([' \x1b[90m> \x1b[37m' + c + '\x1b[0m' + for c + in comment.splitlines()]) + + def format(self): + d = { + 'IssueComment': lambda e: + "{} left a comment:\n{}".format( + self._actor(e['author']['login']), + self._comment_summary(e['body'])), + 'SubscribedEvent': lambda e: + # "{} was subscribed".format( + # self._actor(e['actor']['login'])), + None, + 'MentionedEvent': lambda e: + # "{} was mentioned".format( + # self._actor(e['actor']['login'])), + None, + 'RenamedTitleEvent': lambda e: + "Renamed from '{}' to '{}' by {}".format( + e['previousTitle'], + e['currentTitle'], + self._actor(e['actor']['login'])), + 'LabeledEvent': lambda e: + "Label {} added by {}".format( + self._label(e['label']['name']), + self._actor(e['actor']['login'])), + 'UnlabeledEvent': lambda e: + "Label {} removed by {}".format( + self._label(e['label']['name']), + self._actor(e['actor']['login'])), + 'ReferencedEvent': lambda e: + # "Referenced", + None, + 'HeadRefForcePushedEvent': lambda e: + "{} force-pushed from {} to {}".format( + self._actor(e['actor']['login']), + self._commit(e['beforeCommit']['oid']), + self._commit(e['afterCommit']['oid'])), + 'AssignedEvent': lambda e: + "Assigned to {} by {}".format( + self._actor(e['user']['login']), + self._actor(e['actor']['login'])), + 'CrossReferencedEvent': lambda e: + # "Cross referenced", + None, + 'PullRequestReview': lambda e: + "Reviewed", + 'PullRequestCommit': lambda e: + "New commit {} pushed".format( + self._commit(self.data['commit']['oid'])), + 'MergedEvent': lambda e: + "Merged!", + 'ClosedEvent': lambda e: + "Closed.", + 'ReopenedEvent': lambda e: + "Reopened.", + } + + if self.event_type in d: + r = d[self.event_type](self) + if r: + return r + else: + return None + else: + return None + + +def all(access_token, owner, repo, pull): + after = None + result = PullRequestResponse() + result.owner = owner + result.repo = repo + result.pull = pull + + while True: + response = one(access_token=access_token, + owner=owner, + repo=repo, + pull=pull, + after=after) + if response.status_code == 502: + # 502s happen sometimes when talking to GitHub. Try again. + time.sleep(1) + continue + + r = response.json() + + pull_request = r['data']['repository']['pullRequest'] + page_info = pull_request['timelineItems']['pageInfo'] + events = pull_request['timelineItems']['nodes'] + + result.title = pull_request['title'] + result.author = pull_request['author']['login'] + result.state = pull_request['state'] + result.head_sha = pull_request['headRefOid'] + result.mergeable = pull_request['mergeable'] + + result.events.extend([PullRequestEvent(e) for e in events]) + + if not page_info['hasNextPage']: + break + after = page_info['endCursor'] + + return result + + +def one(access_token, owner, repo, pull, after): + headers = { + 'authorization': 'bearer ' + access_token, + 'accept': 'application/json', + } + json = { + 'query': QUERY, + 'variables': { + 'repoName': repo, + 'repoOwner': owner, + 'pull': int(pull), + 'after': after, + } + } + result = requests.post('https://api.github.com/graphql', + headers=headers, + json=json) + + return result diff --git a/homu/tests/test_parse_issue_comment.py b/homu/tests/test_parse_issue_comment.py index e0bd862..9c50fbf 100644 --- a/homu/tests/test_parse_issue_comment.py +++ b/homu/tests/test_parse_issue_comment.py @@ -476,6 +476,28 @@ def test_ignore_commands_after_bors_line(self): self.assertEqual(command.action, 'approve') self.assertEqual(command.actor, 'jack') + def test_homu_state(self): + """ + Test that when a comment has a Homu state in it, we return that state. + """ + + author = "bors" + body = """ + :hourglass: Trying commit 3d67c2da893aed40bc36b6ac9148c593aa0a868a with merge b7a0ff78ba2ba0b3f5e1a8e89464a84dc386aa81... + + """ # noqa + + commands = parse_issue_comment(author, body, commit, "bors") + + self.assertEqual(len(commands), 1) + command = commands[0] + self.assertEqual(command.action, 'homu-state') + self.assertEqual(command.homu_state, { + 'type': 'TryBuildStarted', + 'head_sha': '3d67c2da893aed40bc36b6ac9148c593aa0a868a', + 'merge_sha': 'b7a0ff78ba2ba0b3f5e1a8e89464a84dc386aa81', + }) + if __name__ == '__main__': unittest.main() diff --git a/setup.py b/setup.py index 392e0eb..a39a656 100644 --- a/setup.py +++ b/setup.py @@ -28,6 +28,7 @@ entry_points={ 'console_scripts': [ 'homu=homu.main:main', + 'homu-sync-test=homu.sync_test:main', ], }, zip_safe=False,