Split newpr.py into a set of dynamically-loaded event handlers

.gitignore

@@ -0,0 +1 @@
+*.pyc

README.md

@@ -1,7 +1,46 @@
 Highfive
 ========
 
-GitHub hooks to provide an encouraging atmosphere for new contributors
+GitHub hooks to provide an encouraging atmosphere for new contributors.
 
-Docs for this highfive live [on the Servo
-wiki](https://github.com/servo/servo/wiki/Highfive)
+Docs for the highfive instance for servo/servo repository live [on the Servo
+wiki](https://github.com/servo/servo/wiki/Highfive).
+
+## Design
+
+Highfive is built as a modular, loosely-coupled set of handlers for Github
+API events. Each time an API event is processed, each handler is given the
+opportunity to respond to it, either by making direct API calls (such as
+manipulating PR labels) or using cross-handler features such as logging a
+warning (which are aggregated at the end and posted as a single comment).
+
+## Testing
+
+Per-handler tests can be run using `python test.py`. These consist of
+a set of JSON documents collected from the `tests/` subdirectory of
+each handler, using the following format:
+```json
+{
+  "initial": {
+    // Initial state of the PR before any handlers process the payload.
+    // Defaults:
+    "labels": [],
+    "diff": "",
+    "new_contributor": false,
+    "assignee": null,
+  },
+  "expected": {
+    // Expected state of the PR after all the handlers process
+    // the following payload.
+    // Only fields present in this object will be checked.
+    // comments: 5,
+    // labels: ["S-awaiting-review"],
+    // assignee: "jdm"
+  },
+  "payload": {
+    // Github API event payload in JSON format
+  }
+}
+```
+Each test runs with a mock Github API provider, so no account information
+or network connection is required to run the test suite.

eventhandler.py

@@ -0,0 +1,64 @@
+import imp
+import json
+import os
+
+_warnings = []
+
+class EventHandler:
+    def on_pr_opened(self, api, payload):
+        pass
+
+    def on_pr_updated(self, api, payload):
+        pass
+
+    def on_new_comment(self, api, payload):
+        pass
+
+    def warn(self, msg):
+        global _warnings
+        _warnings += [msg]
+
+    def is_open_pr(self, payload):
+        return payload['issue']['state'] == 'open' and 'pull_request' in payload['issue']
+
+    def register_tests(self, path):
+        from test import create_test
+        tests_location = os.path.join(path, 'tests')
+        if not os.path.isdir(tests_location):
+            return
+        tests = [os.path.join(tests_location, f) for f in os.listdir(tests_location) if f.endswith('.json')]
+        for testfile in tests:
+            with open(testfile) as f:
+                contents = json.load(f)
+                if not isinstance(contents['initial'], list):
+                    assert not isinstance(contents['expected'], list)
+                    contents['initial'] = [contents['initial']]
+                    contents['expected'] = [contents['expected']]
+                for initial, expected in zip(contents['initial'], contents['expected']):
+                    yield create_test(testfile, initial, expected)
+
+def reset_test_state():
+    global _warnings
+    _warnings = []
+
+def get_warnings():
+    global _warnings
+    return _warnings
+
+def get_handlers():
+    modules = []
+    handlers = []
+    possiblehandlers = os.listdir('handlers')
+    for i in possiblehandlers:
+        location = os.path.join('handlers', i)
+        module = '__init__'
+        if not os.path.isdir(location) or not module + ".py" in os.listdir(location):
+            continue
+        try:
+            (file, pathname, description) = imp.find_module(module, [location])
+            module = imp.load_module(module, file, pathname, description)
+            handlers.append(module.handler_interface())
+            modules.append((module, location))
+        finally:
+            file.close()
+    return (modules, handlers)

handlers/assign_reviewer/__init__.py

@@ -0,0 +1,22 @@
+from eventhandler import EventHandler
+import re
+
+# If the user specified a reviewer, return the username, otherwise returns None.
+def find_reviewer(commit_msg):
+    reviewer_re = re.compile("\\b[rR]\?[:\- ]*@([a-zA-Z0-9\-]+)")
+    match = reviewer_re.search(commit_msg)
+    if not match:
+        return None
+    return match.group(1)
+
+class AssignReviewerHandler(EventHandler):
+    def on_new_comment(self, api, payload):
+        if not self.is_open_pr(payload):
+            return
+
+        reviewer = find_reviewer(payload["comment"]["body"])
+        if reviewer:
+            api.set_assignee(reviewer)
+
+
+handler_interface = AssignReviewerHandler

test_comment.json → handlers/assign_reviewer/tests/comment.json

@@ -1,3 +1,10 @@
+{
+  "initial": {
+  },
+  "expected": {
+      "assignee": "jdm"
+  },
+  "payload":
 {
   "action": "created",
   "issue": {
@@ -197,3 +204,4 @@
     "site_admin": false
   }
 }
+}

handlers/homu_status/__init__.py

@@ -0,0 +1,34 @@
+from eventhandler import EventHandler
+
+class HomuStatusHandler(EventHandler):
+    def on_new_comment(self, api, payload):
+        if not self.is_open_pr(payload):
+            return
+
+        if payload['comment']['user']['login'] != 'bors-servo':
+            return
+
+        labels = api.get_labels();
+        msg = payload["comment"]["body"]
+
+        def remove_if_exists(label):
+            if label in labels:
+                api.remove_label(label)
+
+        if 'has been approved by' in msg or 'Testing commit' in msg:
+            for label in ["S-awaiting-review", "S-needs-rebase", "S-tests-failed",
+                          "S-needs-code-changes", "S-needs-squash", "S-awaiting-answer"]:
+                remove_if_exists(label)
+            if not "S-awaiting-merge" in labels:
+                api.add_label("S-awaiting-merge")
+
+        elif 'Test failed' in msg:
+            remove_if_exists("S-awaiting-merge")
+            api.add_label("S-tests-failed")
+
+        elif 'Please resolve the merge conflicts' in msg:
+            remove_if_exists("S-awaiting-merge")
+            api.add_label("S-needs-rebase")
+
+
+handler_interface = HomuStatusHandler

test_merge_approved.json → handlers/homu_status/tests/merge_approved.json

@@ -1,3 +1,12 @@
+{
+  "initial": {
+      "labels": ["S-needs-code-changes", "S-needs-rebase", "S-tests-failed",
+                 "S-needs-squash", "S-awaiting-review"]
+  },
+  "expected": {
+      "labels": ["S-awaiting-merge"]
+  },
+  "payload":
 {
   "action": "created",
   "issue": {
@@ -197,3 +206,4 @@
     "site_admin": false
   }
 }
+}

test_merge_conflict.json → handlers/homu_status/tests/merge_conflict.json

@@ -1,3 +1,11 @@
+{
+  "initial": {
+      "labels": ["S-awaiting-merge"]
+  },
+  "expected": {
+      "labels": ["S-needs-rebase"]
+  },
+  "payload":
 {
   "action": "created",
   "issue": {
@@ -197,3 +205,4 @@
     "site_admin": false
   }
 }
+}

test_post_retry.json → handlers/homu_status/tests/post_retry.json

@@ -1,3 +1,17 @@
+{
+  "initial": [{
+      "labels": ["S-tests-failed"]
+  },
+  {
+      "labels": ["S-awaiting-merge"]
+  }],
+  "expected": [{
+      "labels": ["S-awaiting-merge"]
+  },
+  {
+      "labels": ["S-awaiting-merge"]
+  }],
+  "payload":
 {
   "action": "created",
   "issue": {
@@ -197,3 +211,4 @@
     "site_admin": false
   }
 }
+}

test_tests_failed.json → handlers/homu_status/tests/tests_failed.json

@@ -1,3 +1,11 @@
+{
+  "initial": {
+      "labels": ["S-awaiting-merge"]
+  },
+  "expected": {
+      "labels": ["S-tests-failed"]
+  },
+  "payload":
 {
   "action": "created",
   "issue": {
@@ -197,3 +205,4 @@
     "site_admin": false
   }
 }
+}

handlers/missing_test/__init__.py

@@ -0,0 +1,20 @@
+from eventhandler import EventHandler
+
+reftest_required_msg = 'These commits modify layout code, but no reftests are modified. Please consider adding a reftest!'
+
+class MissingTestHandler(EventHandler):
+    def on_pr_opened(self, api, payload):
+        diff = api.get_diff()
+        layout_changed = False
+        for line in diff.split('\n'):
+            if line.startswith('diff --git') and line.find('components/layout/') > -1:
+                layout_changed = True
+            if line.startswith('diff --git') and \
+               (line.find('tests/ref') > -1 or line.find('tests/wpt') > -1):
+                return
+
+        if layout_changed:
+            self.warn(reftest_required_msg)
+
+
+handler_interface = MissingTestHandler