Merge lp:~ricardokirkner/locolander/celery-tasks into lp:locolander

[Ricardo Kirkner]

added celery-based tasks

- run_project: processes all outstanding merge requests for a project
- run_request: runs tests and merges a merge requests
- scan_project: finds approved requests for a project

[lp:~ricardokirkner/locolander/celery-tasks]

24. By Ricardo Kirkner on 2013-07-14

merged add-author

[Natalia Bidart (nataliabidart)]

Looks good!

One note: you should not import os.path, but import os. From the help(os.path):

"Instead of importing this module directly, import os and refer to this module as os.path."

Any reason to change the prefix on l.94-95?

The import from l.158 should have a blank line separating import from stdlib from imports from third-party.

When needing the same patch in more than one test, and specially when the same patch is needed in every test inside the test case, I advice to do the patching in the setUp method, so we can have the patch in a single place and don't change the signature of every method (but if you disagree, you can leave as is.

Approving since none of the above is a blocker.

review: Approve

[lp:~ricardokirkner/locolander/celery-tasks]

25. By Ricardo Kirkner on 2013-07-20

fixes per review

[Ricardo Kirkner (ricardokirkner)]

> Looks good!
>
> One note: you should not import os.path, but import os. From the
> help(os.path):
>
> "Instead of importing this module directly, import os and refer to this module
> as os.path."

fixed

>
> Any reason to change the prefix on l.94-95?

because the extra '-' is unnecessary; it will be added by make_random_string, so having it in the prefix will result in a double '-' in the final string

>
> The import from l.158 should have a blank line separating import from stdlib
> from imports from third-party.

fixed

>
> When needing the same patch in more than one test, and specially when the same
> patch is needed in every test inside the test case, I advice to do the
> patching in the setUp method, so we can have the patch in a single place and
> don't change the signature of every method (but if you disagree, you can leave
> as is.
>

having them in the methods is a bit nicer sometimes as it keeps context local. have changed as you suggested anyway, as it's applied to all tests.

> Approving since none of the above is a blocker.

=== modified file 'locolander/locolanderweb/models.py'
--- locolander/locolanderweb/models.py	2013-07-13 17:25:25 +0000
+++ locolander/locolanderweb/models.py	2013-07-20 21:10:30 +0000
@@ -39,10 +39,6 @@
 
     def approved_requests(self):
         """Return approved merge proposals."""
-        mrs = self.service.approved_requests_for_project_url(self.url)
-        for mr in mrs:
-            MergeRequest.objects.get_or_create(project=self, **mr)
-
         return MergeRequest.objects.filter(project=self, status=STATUS_PENDING)
 
 
 
=== added file 'locolander/locolanderweb/tasks.py'
--- locolander/locolanderweb/tasks.py	1970-01-01 00:00:00 +0000
+++ locolander/locolanderweb/tasks.py	2013-07-20 21:10:30 +0000
@@ -0,0 +1,52 @@
+import os
+import subprocess
+
+from celery import task
+
+from locolanderweb.models import MergeRequest, RunLog
+
+
+@task
+def run_project(project):
+    for request in project.approved_requests():
+        run_request(request)
+
+
+@task
+def run_request(request):
+    project = request.project
+    source = request.source
+    target = request.target
+    author = request.author
+    message = request.commit_message
+
+    cmd = [
+        os.path.realpath(os.path.join(
+            os.path.abspath('.'), 'docker/scripts/locolander')),
+        project.name,
+        source,
+        target,
+        message,
+        author,
+    ]
+    try:
+        output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+        return_code = 0
+        error = False
+    except subprocess.CalledProcessError as err:
+        output = err.output
+        return_code = err.returncode
+        error = True
+
+    RunLog.objects.create(merge_request=request, raw=output,
+                          return_code=return_code)
+
+    # update status on merge request
+    request.complete(error=error)
+
+
+@task
+def scan_project(project):
+    mrs = project.service.approved_requests_for_project_url(project.url)
+    for mr in mrs:
+        MergeRequest.objects.get_or_create(project=project, **mr)
 
=== modified file 'locolander/locolanderweb/tests/__init__.py'
--- locolander/locolanderweb/tests/__init__.py	2013-06-21 15:34:22 +0000
+++ locolander/locolanderweb/tests/__init__.py	2013-07-20 21:10:30 +0000
@@ -1,4 +1,5 @@
-from locolanderweb.tests.test_code_style import *
-from locolanderweb.tests.test_forms import *
-from locolanderweb.tests.test_models import *
-from locolanderweb.tests.test_views import *
+from .test_code_style import *
+from .test_forms import *
+from .test_models import *
+from .test_tasks import *
+from .test_views import *
 
=== modified file 'locolander/locolanderweb/tests/test_base.py'
--- locolander/locolanderweb/tests/test_base.py	2013-07-13 17:25:25 +0000
+++ locolander/locolanderweb/tests/test_base.py	2013-07-20 21:10:30 +0000
@@ -36,7 +36,7 @@
 
     def make_project(self, name=None, user=None, url=None):
         if name is None:
-            name = self.make_random_string(prefix='project-')
+            name = self.make_random_string(prefix='project')
         if user is None:
             user = self.make_user()
         if url is None:
 
=== modified file 'locolander/locolanderweb/tests/test_models.py'
--- locolander/locolanderweb/tests/test_models.py	2013-07-20 18:14:16 +0000
+++ locolander/locolanderweb/tests/test_models.py	2013-07-20 21:10:30 +0000
@@ -15,20 +15,6 @@
 )
 from locolanderweb.tests.test_base import BaseTestCase
 
-MERGE_REQUESTS = [
-    dict(
-        source='source1', target='target1', reviewers='a,b,c',
-        date_created=datetime(2013, 4, 23), commit_message='something 1',
-    ),
-    dict(
-        source='source2', target='target2', reviewers='pepe',
-        date_created=datetime(2012, 2, 29), commit_message='something else',
-    ),
-    dict(
-        source='source3', target='target3', reviewers='pepe y pepa',
-        date_created=datetime.utcnow(), commit_message='',
-    ),
-]
 
 INVALID_URLS = (
     'locolander',
@@ -99,16 +85,16 @@
         self.get_service_from_url.assert_called_once_with(self.obj.url)
 
     def test_approved_requests(self):
-        assert MergeRequest.objects.all().count() == 0
-
-        service = self.get_service_from_url.return_value
-        service.approved_requests_for_project_url.return_value = MERGE_REQUESTS
-
-        result = self.obj.approved_requests()
-        self.assertEqual(MergeRequest.objects.all().count(), len(result))
-        for mr in MERGE_REQUESTS:
-            self.assertEqual(
-                MergeRequest.objects.filter(**mr).count(), 1)
+        assert MergeRequest.objects.filter(project=self.obj).count() == 0
+
+        result = self.obj.approved_requests()
+        self.assertEqual(len(result), 0)
+
+        # Create an approved merge request
+        self.factory.make_merge_request(project=self.obj)
+
+        result = self.obj.approved_requests()
+        self.assertEqual(len(result), 1)
 
 
 class MergeRequestTestCase(BaseTestCase):
 
=== added file 'locolander/locolanderweb/tests/test_tasks.py'
--- locolander/locolanderweb/tests/test_tasks.py	1970-01-01 00:00:00 +0000
+++ locolander/locolanderweb/tests/test_tasks.py	2013-07-20 21:10:30 +0000
@@ -0,0 +1,147 @@
+import subprocess
+from datetime import datetime
+
+from mock import ANY, patch
+
+from locolanderweb.models import (
+    STATUS_ERROR,
+    STATUS_MERGED,
+    MergeRequest,
+    RunLog,
+)
+from locolanderweb.tasks import run_project, run_request, scan_project
+from locolanderweb.tests.test_base import BaseTestCase
+
+
+MERGE_REQUESTS = [
+    dict(
+        source='source1', target='target1', reviewers='a,b,c',
+        date_created=datetime(2013, 4, 23), commit_message='something 1',
+    ),
+    dict(
+        source='source2', target='target2', reviewers='pepe',
+        date_created=datetime(2012, 2, 29), commit_message='something else',
+    ),
+    dict(
+        source='source3', target='target3', reviewers='pepe y pepa',
+        date_created=datetime.utcnow(), commit_message='',
+    ),
+]
+
+
+class RunProjectTestCase(BaseTestCase):
+
+    def setUp(self):
+        super(RunProjectTestCase, self).setUp()
+        patcher = patch('locolanderweb.tasks.run_request')
+        self.mock_run_request = patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def test_run_project(self):
+        project = self.factory.make_project()
+        merge_request = self.factory.make_merge_request(project=project)
+
+        run_project(project)
+        self.mock_run_request.assert_called_once_with(merge_request)
+
+    def test_run_project_with_no_approved_requests(self):
+        project = self.factory.make_project()
+
+        run_project(project)
+        self.assertFalse(self.mock_run_request.called)
+
+
+class RunRequestTestCase(BaseTestCase):
+
+    def setUp(self):
+        super(RunRequestTestCase, self).setUp()
+        patcher = patch('locolanderweb.tasks.subprocess.check_output')
+        self.mock_check_output = patcher.start()
+        self.addCleanup(patcher.stop)
+
+    def test_run_request(self):
+        self.mock_check_output.return_value = 'SUCCESS'
+
+        merge_request = self.factory.make_merge_request()
+
+        run_request(merge_request)
+
+        cmd = [
+            ANY,
+            merge_request.project.name,
+            merge_request.source,
+            merge_request.target,
+            merge_request.commit_message,
+            merge_request.author,
+            ]
+        self.mock_check_output.assert_called_once_with(
+            cmd, stderr=subprocess.STDOUT)
+        real_cmd = self.mock_check_output.call_args[0][0][0]
+        self.assertTrue(real_cmd.endswith('docker/scripts/locolander'))
+
+        self.assertEqual(RunLog.objects.all().count(), 1)
+        run_log = RunLog.objects.get(merge_request=merge_request)
+        self.assertEqual(run_log.raw, 'SUCCESS')
+        self.assertEqual(run_log.return_code, 0)
+
+        # refresh MR object
+        mr = MergeRequest.objects.get(pk=merge_request.pk)
+        self.assertEqual(mr.status, STATUS_MERGED)
+
+    def test_run_request_with_error(self):
+        merge_request = self.factory.make_merge_request()
+
+        cmd = [
+            ANY,
+            merge_request.project.name,
+            merge_request.source,
+            merge_request.target,
+            merge_request.commit_message,
+            merge_request.author,
+            ]
+        error = subprocess.CalledProcessError(1, cmd, output='FAILURE')
+        self.mock_check_output.side_effect = error
+
+        run_request(merge_request)
+
+        self.mock_check_output.assert_called_once_with(
+            cmd, stderr=subprocess.STDOUT)
+        real_cmd = self.mock_check_output.call_args[0][0][0]
+        self.assertTrue(real_cmd.endswith('docker/scripts/locolander'))
+
+        self.assertEqual(RunLog.objects.all().count(), 1)
+        run_log = RunLog.objects.get(merge_request=merge_request)
+        self.assertEqual(run_log.raw, 'FAILURE')
+        self.assertEqual(run_log.return_code, 1)
+
+        # refresh MR object
+        mr = MergeRequest.objects.get(pk=merge_request.pk)
+        self.assertEqual(mr.status, STATUS_ERROR)
+
+
+class ScanProjectTestCase(BaseTestCase):
+
+    def test_scan_project(self):
+        project = self.factory.make_project()
+
+        service = self.get_service_from_url.return_value
+        service.approved_requests_for_project_url.return_value = MERGE_REQUESTS
+
+        scan_project(project)
+
+        self.assertEqual(MergeRequest.objects.filter(project=project).count(),
+                         len(MERGE_REQUESTS))
+        for mr in MERGE_REQUESTS:
+            self.assertEqual(
+                MergeRequest.objects.filter(**mr).count(), 1)
+
+    def test_scan_project_with_no_approved_request(self):
+        project = self.factory.make_project()
+
+        service = self.get_service_from_url.return_value
+        service.approved_requests_for_project_url.return_value = []
+
+        scan_project(project)
+
+        self.assertEqual(MergeRequest.objects.filter(project=project).count(),
+                         0)