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,53 +32,7 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():
-    """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 = []
-
-def setup():
-    """Prepare the repository and virtualenvs."""
-    global venvs
-    
-    try:
-        os.mkdir(basedir)
-    except OSError as e:
-        if e.errno != errno.EEXIST:
-            raise
-    os.chdir(basedir)
-    
-    # Delete virtualenvs and recreate
-    for venv in glob('venv-*'):
-        shutil.rmtree(venv)
-    for py in available_python_versions():
-        check_call(['virtualenv', '-p', py, '--system-site-packages', 'venv-%s' % py])
-        venvs.append((py, 'venv-%s' % py))
-    
-    # Check out and update the repository
-    if not os.path.exists('ipython'):
-        try :
-            check_call(['git', 'clone', ipy_repository])
-        except CalledProcessError :
-            check_call(['git', 'clone', ipy_http_repository])
-    os.chdir(repodir)
-    check_call(['git', 'checkout', 'master'])
-    try :
-        check_call(['git', 'pull', ipy_repository, 'master'])
-    except CalledProcessError :
-        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):
@@ -85,20 +40,165 b' def get_missing_libraries(log):'
     if m:
         return m.group(1)
 
-def get_branch(repo, branch, owner, mergeable):
-    os.chdir(repodir)
-    if mergeable:
-        merged_branch = "%s-%s" % (owner, branch)
-        # Delete the branch first
-        call(['git', 'branch', '-D', merged_branch])
-        check_call(['git', 'checkout', '-b', merged_branch])
-        check_call(['git', 'pull', '--no-ff', '--no-commit', repo, branch])
-        check_call(['git', 'commit', '-m', "merge %s/%s" % (repo, branch)])
-    else:
-        # Fetch the branch without merging it.
-        check_call(['git', 'fetch', repo, branch])
-        check_call(['git', 'checkout', 'FETCH_HEAD'])
-    os.chdir(basedir)
+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.
+        """
+        for py in supported_pythons:
+            try:
+                check_call([py, '-c', 'import nose'], stdout=PIPE)
+                yield py
+            except (OSError, CalledProcessError):
+                self.unavailable_pythons.append(py)
+
+    def setup(self):
+        """Prepare the repository and virtualenvs."""        
+        try:
+            os.mkdir(basedir)
+        except OSError as e:
+            if e.errno != errno.EEXIST:
+                raise
+        os.chdir(basedir)
+        
+        # Delete virtualenvs and recreate
+        for venv in glob('venv-*'):
+            shutil.rmtree(venv)
+        for py in self.available_python_versions():
+            check_call(['virtualenv', '-p', py, '--system-site-packages', 'venv-%s' % py])
+            self.venvs.append((py, 'venv-%s' % py))
+        
+        # Check out and update the repository
+        if not os.path.exists('ipython'):
+            try :
+                check_call(['git', 'clone', ipy_repository])
+            except CalledProcessError :
+                check_call(['git', 'clone', ipy_http_repository])
+        os.chdir(repodir)
+        check_call(['git', 'checkout', 'master'])
+        try :
+            check_call(['git', 'pull', ipy_repository, 'master'])
+        except CalledProcessError :
+            check_call(['git', 'pull', ipy_http_repository, 'master'])
+        os.chdir(basedir)
+    
+    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']
+        
+        os.chdir(repodir)
+        if mergeable:
+            merged_branch = "%s-%s" % (owner, branch)
+            # Delete the branch first
+            call(['git', 'branch', '-D', merged_branch])
+            check_call(['git', 'checkout', '-b', merged_branch])
+            check_call(['git', 'pull', '--no-ff', '--no-commit', repo, branch])
+            check_call(['git', 'commit', '-m', "merge %s/%s" % (repo, branch)])
+        else:
+            # Fetch the branch without merging it.
+            check_call(['git', 'fetch', repo, branch])
+            check_call(['git', 'checkout', 'FETCH_HEAD'])
+        os.chdir(basedir)
+    
+    def markdown_format(self):
+        def format_result(result):
+            s = "* %s: " % result.py
+            if result.passed:
+                s += "OK"
+            else:
+                s += "Failed, log at %s" % result.log_url
+            if result.missing_libraries:
+                s += " (libraries not available: " + result.missing_libraries + ")"
+            return s
+        
+        if self.pr['mergeable']:
+            com = self.pr['head']['sha'][:7] + " merged into master"
+        else:
+            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 self.results] + \
+                ["",
+                 "Not available for testing: " + ", ".join(self.unavailable_pythons)]
+        return "\n".join(lines)
+    
+    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
+        
+        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 result in self.results:
+            if result.passed:
+                print(result.py, ":", "OK")
+            else:
+                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(self):
+        with open(os.path.join(basedir, 'lastresults.pkl'), 'wb') as 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(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(result.log)
+            
+                result.log_file = result_locn
+
+    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)
+    
+    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')
@@ -128,85 +228,6 b' def run_tests(venv):'
         # 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:
-            s += "OK"
-        else:
-            s += "Failed, log at %s" % gist_url
-        if missing_libraries:
-            s += " (libraries not available: " + missing_libraries + ")"
-        return s
-    
-    if pr['mergeable']:
-        com = pr['head']['sha'][:7] + " merged into master"
-    else:
-        com = 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] + \
-            ["",
-             "Not available for testing: " + ", ".join(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 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")
-        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))
-
-def dump_results(num, results, pr):
-    with open(os.path.join(basedir, 'lastresults.pkl'), 'wb') as f:
-        pickle.dump((num, results, pr, unavailable_pythons), f)
-
-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"))
-            with io.open(result_locn, 'w', encoding='utf-8') as f:
-                f.write(log)
-        
-            results_paths.append((py, False, result_locn, missing_libraries))
-    
-    return results_paths
-
-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',
-                                            filename="results.log", auth=True)
-            results_urls.append((py, False, result_locn, missing_libraries))
-    
-    return results_urls
 
 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()
     
-    dump_results(num, results, pr)
+    testrun.run()
+
+    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")