Refactor test_pr.py to a more coherent design.

tools/gh_api.py

@@ -16,6 +16,17 b' import json'
 # password
 fake_username = 'ipython_tools'
 
+class Obj(dict):
+    """Dictionary with attribute access to names."""
+    def __getattr__(self, name):
+        try:
+            return self[name]
+        except KeyError:
+            raise AttributeError(name)
+    
+    def __setattr__(self, name, val):
+        self[name] = val
+
 token = None
 def get_auth_token():
     global token
@@ -88,7 +99,7 b' def get_pull_request(project, num, github_api=3):'
     response.raise_for_status()
     if github_api == 2 :
         return json.loads(response.text)['pull']
-    return json.loads(response.text)
+    return json.loads(response.text, object_hook=Obj)
 
 def get_pulls_list(project, github_api=3):
     """get pull request list

tools/post_pr_test.py

@@ -1,13 +1,13 b''
 #!/usr/bin/env python
 """Post the results of a pull request test to Github.
 """
-from test_pr import load_results, post_logs, post_results_comment, print_results
+from test_pr import TestRun
 
-num, results, pr, unavailable_pythons = load_results()
-results_urls = post_logs(results)
-print_results(pr, results_urls, unavailable_pythons)
-post_results_comment(pr, results_urls, num, unavailable_pythons)
+testrun = TestRun.load_results()
+testrun.post_logs()
+testrun.print_results()
+testrun.post_results_comment()
 
 print()
 print("Posted test results to pull request")
-print("  " + pr['html_url'])
+print("  " + testrun.pr['html_url'])

tools/test_pr.py

@@ -23,6 +23,7 b' from subprocess import call, check_call, check_output, PIPE, STDOUT, CalledProce'
 import sys
 
 import gh_api
+from gh_api import Obj
 
 basedir = os.path.join(os.path.expanduser("~"), ".ipy_pr_tests")
 repodir = os.path.join(basedir, "ipython")
@@ -31,25 +32,38 b" ipy_http_repository = 'http://github.com/ipython/ipython.git'"
 gh_project="ipython/ipython"
 
 supported_pythons = ['python2.6', 'python2.7', 'python3.1', 'python3.2']
-unavailable_pythons = []
 
-def available_python_versions():
+missing_libs_re = re.compile(r"Tools and libraries NOT available at test time:\n"
+                             r"\s*(.*?)\n")
+def get_missing_libraries(log):
+    m = missing_libs_re.search(log)
+    if m:
+        return m.group(1)
+
+class TestRun(object):
+    def __init__(self, pr_num):
+        self.unavailable_pythons = []
+        self.venvs = []
+        self.pr_num = pr_num
+        
+        self.pr = gh_api.get_pull_request(gh_project, pr_num)
+        
+        self.setup()
+        
+        self.results = []
+    
+    def available_python_versions(self):
     """Get the executable names of available versions of Python on the system.
     """
-    del unavailable_pythons[:]
     for py in supported_pythons:
         try:
             check_call([py, '-c', 'import nose'], stdout=PIPE)
             yield py
         except (OSError, CalledProcessError):
-            unavailable_pythons.append(py)
-
-venvs = []
+                self.unavailable_pythons.append(py)
 
-def setup():
+    def setup(self):
     """Prepare the repository and virtualenvs."""
-    global venvs
-    
     try:
         os.mkdir(basedir)
     except OSError as e:
@@ -60,9 +74,9 b' def setup():'
     # Delete virtualenvs and recreate
     for venv in glob('venv-*'):
         shutil.rmtree(venv)
-    for py in available_python_versions():
+        for py in self.available_python_versions():
         check_call(['virtualenv', '-p', py, '--system-site-packages', 'venv-%s' % py])
-        venvs.append((py, 'venv-%s' % py))
+            self.venvs.append((py, 'venv-%s' % py))
     
     # Check out and update the repository
     if not os.path.exists('ipython'):
@@ -78,14 +92,12 b' def setup():'
         check_call(['git', 'pull', ipy_http_repository, 'master'])
     os.chdir(basedir)
 
-missing_libs_re = re.compile(r"Tools and libraries NOT available at test time:\n"
-                             r"\s*(.*?)\n")
-def get_missing_libraries(log):
-    m = missing_libs_re.search(log)
-    if m:
-        return m.group(1)
+    def get_branch(self):
+        repo = self.pr['head']['repo']['clone_url']
+        branch = self.pr['head']['ref']
+        owner = self.pr['head']['repo']['owner']['login']
+        mergeable = self.pr['mergeable']
 
-def get_branch(repo, branch, owner, mergeable):
     os.chdir(repodir)
     if mergeable:
         merged_branch = "%s-%s" % (owner, branch)
@@ -100,113 +112,122 b' def get_branch(repo, branch, owner, mergeable):'
         check_call(['git', 'checkout', 'FETCH_HEAD'])
     os.chdir(basedir)
 
-def run_tests(venv):
-    py = os.path.join(basedir, venv, 'bin', 'python')
-    print(py)
-    os.chdir(repodir)
-    # cleanup build-dir
-    if os.path.exists('build'):
-        shutil.rmtree('build')
-    check_call([py, 'setup.py', 'install'])
-    os.chdir(basedir)
-    
-    # Environment variables:
-    orig_path = os.environ["PATH"]
-    os.environ["PATH"] = os.path.join(basedir, venv, 'bin') + ':' + os.environ["PATH"]
-    os.environ.pop("PYTHONPATH", None)
-    
-    iptest = os.path.join(basedir, venv, 'bin', 'iptest')
-    if not os.path.exists(iptest):
-        iptest = os.path.join(basedir, venv, 'bin', 'iptest3')
-        
-    print("\nRunning tests, this typically takes a few minutes...")
-    try:
-        return True, check_output([iptest], stderr=STDOUT).decode('utf-8')
-    except CalledProcessError as e:
-        return False, e.output.decode('utf-8')
-    finally:
-        # Restore $PATH
-        os.environ["PATH"] = orig_path
-
-def markdown_format(pr, results_urls, unavailable_pythons):
-    def format_result(py, passed, gist_url, missing_libraries):
-        s = "* %s: " % py
-        if passed:
+    def markdown_format(self):
+        def format_result(result):
+            s = "* %s: " % result.py
+            if result.passed:
             s += "OK"
         else:
-            s += "Failed, log at %s" % gist_url
-        if missing_libraries:
-            s += " (libraries not available: " + missing_libraries + ")"
+                s += "Failed, log at %s" % result.log_url
+            if result.missing_libraries:
+                s += " (libraries not available: " + result.missing_libraries + ")"
         return s
     
-    if pr['mergeable']:
-        com = pr['head']['sha'][:7] + " merged into master"
+        if self.pr['mergeable']:
+            com = self.pr['head']['sha'][:7] + " merged into master"
     else:
-        com = pr['head']['sha'][:7] + " (can't merge cleanly)"
+            com = self.pr['head']['sha'][:7] + " (can't merge cleanly)"
     lines = ["**Test results for commit %s**" % com,
              "Platform: " + sys.platform,
              ""] + \
-            [format_result(*r) for r in results_urls] + \
+                [format_result(r) for r in self.results] + \
             ["",
-             "Not available for testing: " + ", ".join(unavailable_pythons)] 
+                 "Not available for testing: " + ", ".join(self.unavailable_pythons)]
     return "\n".join(lines)
 
-def post_results_comment(pr, results, num, unavailable_pythons=unavailable_pythons):
-    body = markdown_format(pr, results, unavailable_pythons)
-    gh_api.post_issue_comment(gh_project, num, body)
+    def post_results_comment(self):
+        body = self.markdown_format()
+        gh_api.post_issue_comment(gh_project, self.pr_num, body)
+    
+    def print_results(self):
+        pr = self.pr
 
-def print_results(pr, results_urls, unavailable_pythons=unavailable_pythons):
     print("\n")
     if pr['mergeable']:
         print("**Test results for commit %s merged into master**" % pr['head']['sha'][:7])
     else:
         print("**Test results for commit %s (can't merge cleanly)**" % pr['head']['sha'][:7])
     print("Platform:", sys.platform)
-    for py, passed, gist_url, missing_libraries in results_urls:
-        if passed:
-            print(py, ":", "OK")
+        for result in self.results:
+            if result.passed:
+                print(result.py, ":", "OK")
         else:
-            print(py, ":", "Failed")
-            print("    Test log:", gist_url)
-        if missing_libraries:
-            print("    Libraries not available:", missing_libraries)
-    print("Not available for testing:", ", ".join(unavailable_pythons))
+                print(result.py, ":", "Failed")
+                print("    Test log:", result.get('log_url') or result.log_file)
+            if result.missing_libraries:
+                print("    Libraries not available:", result.missing_libraries)
+        print("Not available for testing:", ", ".join(self.unavailable_pythons))
 
-def dump_results(num, results, pr):
+    def dump_results(self):
     with open(os.path.join(basedir, 'lastresults.pkl'), 'wb') as f:
-        pickle.dump((num, results, pr, unavailable_pythons), f)
+            pickle.dump(self, f)
 
+    @staticmethod
 def load_results():
     with open(os.path.join(basedir, 'lastresults.pkl'), 'rb') as f:
         return pickle.load(f)
 
-def save_logs(results, pr):
-    results_paths = []
-    for py, passed, log, missing_libraries in results:
-        if passed:
-            results_paths.append((py, passed, None, missing_libraries))
-        else:
-            
-            result_locn = os.path.abspath(os.path.join('venv-%s' % py,
-                                        pr['head']['sha'][:7]+".log"))
+    def save_logs(self):
+        for result in self.results:
+            if not result.passed:
+                result_locn = os.path.abspath(os.path.join('venv-%s' % result.py,
+                                            self.pr['head']['sha'][:7]+".log"))
             with io.open(result_locn, 'w', encoding='utf-8') as f:
-                f.write(log)
-        
-            results_paths.append((py, False, result_locn, missing_libraries))
+                    f.write(result.log)
     
-    return results_paths
+                result.log_file = result_locn
 
-def post_logs(results):
-    results_urls = []
-    for py, passed, log, missing_libraries in results:
-        if passed:
-            results_urls.append((py, passed, None, missing_libraries))
-        else:
-            result_locn = gh_api.post_gist(log, description='IPython test log',
+    def post_logs(self):
+        for result in self.results:
+            if not result.passed:
+                result.log_url = gh_api.post_gist(result.log,
+                                                description='IPython test log',
                                             filename="results.log", auth=True)
-            results_urls.append((py, False, result_locn, missing_libraries))
     
-    return results_urls
+    def run(self):
+        for py, venv in self.venvs:
+            tic = time.time()
+            passed, log = run_tests(venv)
+            elapsed = int(time.time() - tic)
+            print("Ran tests with %s in %is" % (py, elapsed))
+            missing_libraries = get_missing_libraries(log)
+            
+            self.results.append(Obj(py=py,
+                                   passed=passed,
+                                   log=log,
+                                   missing_libraries=missing_libraries
+                                  )
+                               )
+
+
+def run_tests(venv):
+    py = os.path.join(basedir, venv, 'bin', 'python')
+    print(py)
+    os.chdir(repodir)
+    # cleanup build-dir
+    if os.path.exists('build'):
+        shutil.rmtree('build')
+    check_call([py, 'setup.py', 'install'])
+    os.chdir(basedir)
+    
+    # Environment variables:
+    orig_path = os.environ["PATH"]
+    os.environ["PATH"] = os.path.join(basedir, venv, 'bin') + ':' + os.environ["PATH"]
+    os.environ.pop("PYTHONPATH", None)
+    
+    iptest = os.path.join(basedir, venv, 'bin', 'iptest')
+    if not os.path.exists(iptest):
+        iptest = os.path.join(basedir, venv, 'bin', 'iptest3')
+        
+    print("\nRunning tests, this typically takes a few minutes...")
+    try:
+        return True, check_output([iptest], stderr=STDOUT).decode('utf-8')
+    except CalledProcessError as e:
+        return False, e.output.decode('utf-8')
+    finally:
+        # Restore $PATH
+        os.environ["PATH"] = orig_path
+
 
 def test_pr(num, post_results=True):
     # Get Github authorisation first, so that the user is prompted straight away
@@ -214,34 +235,20 b' def test_pr(num, post_results=True):'
     if post_results:
         gh_api.get_auth_token()
     
-    setup()
-    pr = gh_api.get_pull_request(gh_project, num)
-    get_branch(repo=pr['head']['repo']['clone_url'], 
-                 branch=pr['head']['ref'],
-                 owner=pr['head']['repo']['owner']['login'],
-                 mergeable=pr['mergeable'],
-              )
+    testrun = TestRun(num)
     
-    results = []
-    for py, venv in venvs:
-        tic = time.time()
-        passed, log = run_tests(venv)
-        elapsed = int(time.time() - tic)
-        print("Ran tests with %s in %is" % (py, elapsed))
-        missing_libraries = get_missing_libraries(log)
-        if passed:
-            results.append((py, True, None, missing_libraries))
-        else:
-            results.append((py, False, log, missing_libraries))
+    testrun.get_branch()
+    
+    testrun.run()
     
-    dump_results(num, results, pr)
+    testrun.dump_results()
     
-    results_paths = save_logs(results, pr)
-    print_results(pr, results_paths)
+    testrun.save_logs()
+    testrun.print_results()
     
     if post_results:
-        results_urls = post_logs(results)
-        post_results_comment(pr, results_urls, num)
+        results_urls = testrun.post_logs
+        testrun.post_results_comment()
         print("(Posted to Github)")
     else:
         post_script = os.path.join(os.path.dirname(sys.argv[0]), "post_pr_test.py")