[IMP] runbot_merge: refactor some bits

* extract method to create a PR object from a github result (from the
  PR endpoint)
* move some of the remote's fixtures to a global conftest (so they can
  be reused in the forwardbot)

conftest.py

@@ -0,0 +1,106 @@
+# -*- coding: utf-8 -*-
+import configparser
+import re
+import subprocess
+import time
+
+import pytest
+import requests
+
+def pytest_addoption(parser):
+    parser.addoption(
+        '--tunnel', action="store", type="choice", choices=['ngrok', 'localtunnel'], default='ngrok',
+        help="Which tunneling method to use to expose the local Odoo server "
+             "to hook up github's webhook. ngrok is more reliable, but "
+             "creating a free account is necessary to avoid rate-limiting "
+             "issues (anonymous limiting is rate-limited at 20 incoming "
+             "queries per minute, free is 40, multi-repo batching tests will "
+             "blow through the former); localtunnel has no rate-limiting but "
+             "the servers are way less reliable")
+
+@pytest.fixture(scope="session")
+def config(pytestconfig):
+    """ Flat version of the pytest config file (pytest.ini), parses to a
+    simple dict of {section: {key: value}}
+
+    """
+    conf = configparser.ConfigParser(interpolation=None)
+    conf.read([pytestconfig.inifile])
+    return {
+        name: dict(s.items())
+        for name, s in conf.items()
+    }
+
+@pytest.fixture(scope='session')
+def rolemap(config):
+    # only fetch github logins once per session
+    rolemap = {}
+    for k, data in config.items():
+        if k.startswith('role_'):
+            role = k[5:]
+        elif k == 'github':
+            role = 'user'
+        else:
+            continue
+
+        r = requests.get('https://api.github.com/user', headers={'Authorization': 'token %s' % data['token']})
+        r.raise_for_status()
+
+        rolemap[role] = data['user'] = r.json()['login']
+    return rolemap
+
+# apparently conftests can override one another's fixtures but plugins can't
+# override conftest fixtures (?) so if this is defined as "users" it replaces
+# the one from runbot_merge/tests/local and everything breaks.
+#
+# Alternatively this could be special-cased using remote_p or something but
+# that's even more gross. It might be possible to handle that via pytest's
+# hooks as well but I didn't check
+@pytest.fixture
+def users_(env, config, rolemap):
+    for role, login in rolemap.items():
+        if role in ('user', 'other'):
+            continue
+
+        env['res.partner'].create({
+            'name': config['role_' + role].get('name', login),
+            'github_login': login,
+            'reviewer': role == 'reviewer',
+            'self_reviewer': role == 'self_reviewer',
+        })
+
+    return rolemap
+
+@pytest.fixture(scope='session')
+def tunnel(pytestconfig, port):
+    """ Creates a tunnel to localhost:<port> using ngrok or localtunnel, should yield the
+    publicly routable address & terminate the process at the end of the session
+    """
+
+    tunnel = pytestconfig.getoption('--tunnel')
+    if tunnel == 'ngrok':
+        p = subprocess.Popen(['ngrok', 'http', '--region', 'eu', str(port)], stdout=subprocess.DEVNULL)
+        time.sleep(5)
+        try:
+            r = requests.get('http://localhost:4040/api/tunnels')
+            r.raise_for_status()
+            yield next(
+                t['public_url']
+                for t in r.json()['tunnels']
+                if t['proto'] == 'https'
+            )
+        finally:
+            p.terminate()
+            p.wait(30)
+    elif tunnel == 'localtunnel':
+        p = subprocess.Popen(['lt', '-p', str(port)], stdout=subprocess.PIPE)
+        try:
+            r = p.stdout.readline()
+            m = re.match(br'your url is: (https://.*\.localtunnel\.me)', r)
+            assert m, "could not get the localtunnel URL"
+            yield m.group(1).decode('ascii')
+        finally:
+            p.terminate()
+            p.wait(30)
+    else:
+        raise ValueError("Unsupported %s tunnel method" % tunnel)

runbot_merge/controllers/__init__.py

@@ -123,21 +123,7 @@ def handle_pr(env, event):
 
     _logger.info("%s: %s:%s (%s) (%s)", event['action'], repo.name, pr['number'], pr['title'].strip(), author.github_login)
     if event['action'] == 'opened':
-        # some PRs have leading/trailing newlines in body/title (resp)
+        pr_obj = env['runbot_merge.pull_requests']._from_gh(pr)
-        message = pr['title'].strip()
-        body = pr['body'] and pr['body'].strip()
-        if body:
-            message += '\n\n' + body
-        pr_obj = env['runbot_merge.pull_requests'].create({
-            'number': pr['number'],
-            'label': pr['head']['label'],
-            'author': author.id,
-            'target': branch.id,
-            'repository': repo.id,
-            'head': pr['head']['sha'],
-            'squash': pr['commits'] == 1,
-            'message': message,
-        })
         return "Tracking PR as {}".format(pr_obj.id)
 
     pr_obj = env['runbot_merge.pull_requests']._get_or_schedule(r, pr['number'])

runbot_merge/models/pull_requests.py

@@ -523,6 +523,18 @@ class PullRequests(models.Model):
             for p in self
         }
 
+    def __str__(self):
+        if len(self) == 0:
+            separator = ''
+        elif len(self) == 1:
+            separator = ' '
+        else:
+            separator = 's '
+        return '<pull_request%s%s>' % (separator, ' '.join(
+            '%s:%s' % (p.repository.name, p.number)
+            for p in self
+        ))
+
     # missing link to other PRs
     @api.depends('priority', 'state', 'squash', 'merge_method', 'batch_id.active', 'label')
     def _compute_is_blocked(self):
@@ -842,6 +854,36 @@ class PullRequests(models.Model):
             })
         return pr
 
+    def _from_gh(self, description, author=None, branch=None, repo=None):
+        if repo is None:
+            repo = self.env['runbot_merge.repository'].search([
+                ('name', '=', description['base']['repo']['full_name']),
+            ])
+        if branch is None:
+            branch = self.env['runbot_merge.branch'].search([
+                ('name', '=', description['base']['ref']),
+                ('project_id', '=', repo.project_id.id),
+            ])
+        if author is None:
+            author = self.env['res.partner'].search([
+                ('github_login', '=', description['user']['login']),
+            ], limit=1)
+
+        message = description['title'].strip()
+        body = description['body'] and description['body'].strip()
+        if body:
+            message += '\n\n' + body
+        return self.env['runbot_merge.pull_requests'].create({
+            'number': description['number'],
+            'label': description['head']['label'],
+            'author': author.id,
+            'target': branch.id,
+            'repository': repo.id,
+            'head': description['head']['sha'],
+            'squash': description['commits'] == 1,
+            'message': message,
+        })
+
     @api.multi
     def write(self, vals):
         oldstate = { pr: pr._tagstate for pr in self }

runbot_merge/tests/remote.py

@@ -26,7 +26,6 @@ Configuration:
 
   ``role_reviewer``, ``role_self_reviewer`` and ``role_other``
     - name (optional)
-    - user, the login of the user for that role
     - token, a personal access token with the ``public_repo`` scope (otherwise
       the API can't leave comments)
 
@@ -45,12 +44,10 @@ logic errors.
 """
 import base64
 import collections
-import configparser
 import itertools
 import re
 import socket
 import subprocess
-import sys
 import time
 import xmlrpc.client
 
@@ -68,27 +65,14 @@ def pytest_addhooks(pluginmanager):
 
 def pytest_addoption(parser):
     parser.addoption("--no-delete", action="store_true", help="Don't delete repo after a failed run")
-    parser.addoption(
-        '--tunnel', action="store", type="choice", choices=['ngrok', 'localtunnel'],
-        help="Which tunneling method to use to expose the local Odoo server "
-             "to hook up github's webhook. ngrok is more reliable, but "
-             "creating a free account is necessary to avoid rate-limiting "
-             "issues (anonymous limiting is rate-limited at 20 incoming "
-             "queries per minute, free is 40, multi-repo batching tests will "
-             "blow through the former); localtunnel has no rate-limiting but "
-             "the servers are way less reliable")
 
-@pytest.fixture(scope="session")
-def config(pytestconfig):
-    conf = configparser.ConfigParser(interpolation=None)
-    conf.read([pytestconfig.inifile])
-    return {
-        name: dict(s.items())
-        for name, s in conf.items()
-    }
 
 PORT=8069
 
+@pytest.fixture(scope='session')
+def port():
+    return PORT
+
 def wait_for_hook(n=1):
     # TODO: find better way to wait for roundtrip of actions which can trigger webhooks
     time.sleep(10 * n)
@@ -151,63 +135,9 @@ def env(request):
         p.terminate()
         p.wait(timeout=30)
 
-@pytest.fixture(scope='session')
-def tunnel(request):
-    """ Creates a tunnel to localhost:8069 using ~~ngrok~~ localtunnel, should yield the
-    publicly routable address & terminate the process at the end of the session
-    """
-
-    tunnel = request.config.getoption('--tunnel')
-    if tunnel == 'ngrok':
-        p = subprocess.Popen(['ngrok', 'http', '--region', 'eu', str(PORT)])
-        time.sleep(5)
-        try:
-            r = requests.get('http://localhost:4040/api/tunnels')
-            r.raise_for_status()
-            yield next(
-                t['public_url']
-                for t in r.json()['tunnels']
-                if t['proto'] == 'https'
-            )
-        finally:
-            p.terminate()
-            p.wait(30)
-    elif tunnel == 'localtunnel':
-        p = subprocess.Popen(['lt', '-p', str(PORT)], stdout=subprocess.PIPE)
-        try:
-            r = p.stdout.readline()
-            m = re.match(br'your url is: (https://.*\.localtunnel\.me)', r)
-            assert m, "could not get the localtunnel URL"
-            yield m.group(1).decode('ascii')
-        finally:
-            p.terminate()
-            p.wait(30)
-    else:
-        raise ValueError("Unsupported %s tunnel method" % tunnel)
-
-ROLES = ['reviewer', 'self_reviewer', 'other']
 @pytest.fixture(autouse=True)
-def users(env, github, config):
+def users(users_):
-    # get github login of "current user"
+    return users_
-    r = github.get('https://api.github.com/user')
-    r.raise_for_status()
-    rolemap = {
-        'user': r.json()['login']
-    }
-    for role in ROLES:
-        data = config['role_' + role]
-        username = data['user']
-        rolemap[role] = username
-        if role == 'other':
-            continue
-        env['res.partner'].create({
-            'name': data.get('name', username),
-            'github_login': username,
-            'reviewer': role == 'reviewer',
-            'self_reviewer': role == 'self_reviewer',
-        })
-
-    return rolemap
 
 @pytest.fixture
 def project(env, config):
@@ -287,13 +217,14 @@ def make_repo(request, config, project, github, tunnel, users, owner):
         })
         project.write({'repo_ids': [(0, 0, {'name': fullname})]})
 
-        tokens = {
+        role_tokens = {
-            r: config['role_' + r]['token']
+            n[5:]: vals['token']
-            for r in ROLES
+            for n, vals in config.items()
+            if n.startswith('role_')
         }
-        tokens['user'] = config['github']['token']
+        role_tokens['user'] = config['github']['token']
 
-        return Repo(github, fullname, tokens)
+        return Repo(github, fullname, role_tokens)
 
     yield repomaker