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

Proposed by Ricardo Kirkner
Status: Merged
Approved by: Ricardo Kirkner
Approved revision: 24
Merged at revision: 21
Proposed branch: lp:~ricardokirkner/locolander/celery-tasks
Merge into: lp:locolander
Prerequisite: lp:~ricardokirkner/locolander/add-author
Diff against target: 302 lines (+215/-33)
6 files modified
locolander/locolanderweb/models.py (+0/-4)
locolander/locolanderweb/tasks.py (+52/-0)
locolander/locolanderweb/tests/__init__.py (+5/-4)
locolander/locolanderweb/tests/test_base.py (+1/-1)
locolander/locolanderweb/tests/test_models.py (+10/-24)
locolander/locolanderweb/tests/test_tasks.py (+147/-0)
To merge this branch: bzr merge lp:~ricardokirkner/locolander/celery-tasks
Reviewer Review Type Date Requested Status
Natalia Bidart Approve
Review via email: mp+174619@code.launchpad.net

Commit message

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

To post a comment you must log in.
24. By Ricardo Kirkner

merged add-author

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

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
25. By Ricardo Kirkner

fixes per review

Revision history for this message
Ricardo Kirkner (ricardokirkner) wrote :

> 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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'locolander/locolanderweb/models.py'
2--- locolander/locolanderweb/models.py 2013-07-13 17:25:25 +0000
3+++ locolander/locolanderweb/models.py 2013-07-20 21:10:30 +0000
4@@ -39,10 +39,6 @@
5
6 def approved_requests(self):
7 """Return approved merge proposals."""
8- mrs = self.service.approved_requests_for_project_url(self.url)
9- for mr in mrs:
10- MergeRequest.objects.get_or_create(project=self, **mr)
11-
12 return MergeRequest.objects.filter(project=self, status=STATUS_PENDING)
13
14
15
16=== added file 'locolander/locolanderweb/tasks.py'
17--- locolander/locolanderweb/tasks.py 1970-01-01 00:00:00 +0000
18+++ locolander/locolanderweb/tasks.py 2013-07-20 21:10:30 +0000
19@@ -0,0 +1,52 @@
20+import os
21+import subprocess
22+
23+from celery import task
24+
25+from locolanderweb.models import MergeRequest, RunLog
26+
27+
28+@task
29+def run_project(project):
30+ for request in project.approved_requests():
31+ run_request(request)
32+
33+
34+@task
35+def run_request(request):
36+ project = request.project
37+ source = request.source
38+ target = request.target
39+ author = request.author
40+ message = request.commit_message
41+
42+ cmd = [
43+ os.path.realpath(os.path.join(
44+ os.path.abspath('.'), 'docker/scripts/locolander')),
45+ project.name,
46+ source,
47+ target,
48+ message,
49+ author,
50+ ]
51+ try:
52+ output = subprocess.check_output(cmd, stderr=subprocess.STDOUT)
53+ return_code = 0
54+ error = False
55+ except subprocess.CalledProcessError as err:
56+ output = err.output
57+ return_code = err.returncode
58+ error = True
59+
60+ RunLog.objects.create(merge_request=request, raw=output,
61+ return_code=return_code)
62+
63+ # update status on merge request
64+ request.complete(error=error)
65+
66+
67+@task
68+def scan_project(project):
69+ mrs = project.service.approved_requests_for_project_url(project.url)
70+ for mr in mrs:
71+ MergeRequest.objects.get_or_create(project=project, **mr)
72
73=== modified file 'locolander/locolanderweb/tests/__init__.py'
74--- locolander/locolanderweb/tests/__init__.py 2013-06-21 15:34:22 +0000
75+++ locolander/locolanderweb/tests/__init__.py 2013-07-20 21:10:30 +0000
76@@ -1,4 +1,5 @@
77-from locolanderweb.tests.test_code_style import *
78-from locolanderweb.tests.test_forms import *
79-from locolanderweb.tests.test_models import *
80-from locolanderweb.tests.test_views import *
81+from .test_code_style import *
82+from .test_forms import *
83+from .test_models import *
84+from .test_tasks import *
85+from .test_views import *
86
87=== modified file 'locolander/locolanderweb/tests/test_base.py'
88--- locolander/locolanderweb/tests/test_base.py 2013-07-13 17:25:25 +0000
89+++ locolander/locolanderweb/tests/test_base.py 2013-07-20 21:10:30 +0000
90@@ -36,7 +36,7 @@
91
92 def make_project(self, name=None, user=None, url=None):
93 if name is None:
94- name = self.make_random_string(prefix='project-')
95+ name = self.make_random_string(prefix='project')
96 if user is None:
97 user = self.make_user()
98 if url is None:
99
100=== modified file 'locolander/locolanderweb/tests/test_models.py'
101--- locolander/locolanderweb/tests/test_models.py 2013-07-20 18:14:16 +0000
102+++ locolander/locolanderweb/tests/test_models.py 2013-07-20 21:10:30 +0000
103@@ -15,20 +15,6 @@
104 )
105 from locolanderweb.tests.test_base import BaseTestCase
106
107-MERGE_REQUESTS = [
108- dict(
109- source='source1', target='target1', reviewers='a,b,c',
110- date_created=datetime(2013, 4, 23), commit_message='something 1',
111- ),
112- dict(
113- source='source2', target='target2', reviewers='pepe',
114- date_created=datetime(2012, 2, 29), commit_message='something else',
115- ),
116- dict(
117- source='source3', target='target3', reviewers='pepe y pepa',
118- date_created=datetime.utcnow(), commit_message='',
119- ),
120-]
121
122 INVALID_URLS = (
123 'locolander',
124@@ -99,16 +85,16 @@
125 self.get_service_from_url.assert_called_once_with(self.obj.url)
126
127 def test_approved_requests(self):
128- assert MergeRequest.objects.all().count() == 0
129-
130- service = self.get_service_from_url.return_value
131- service.approved_requests_for_project_url.return_value = MERGE_REQUESTS
132-
133- result = self.obj.approved_requests()
134- self.assertEqual(MergeRequest.objects.all().count(), len(result))
135- for mr in MERGE_REQUESTS:
136- self.assertEqual(
137- MergeRequest.objects.filter(**mr).count(), 1)
138+ assert MergeRequest.objects.filter(project=self.obj).count() == 0
139+
140+ result = self.obj.approved_requests()
141+ self.assertEqual(len(result), 0)
142+
143+ # Create an approved merge request
144+ self.factory.make_merge_request(project=self.obj)
145+
146+ result = self.obj.approved_requests()
147+ self.assertEqual(len(result), 1)
148
149
150 class MergeRequestTestCase(BaseTestCase):
151
152=== added file 'locolander/locolanderweb/tests/test_tasks.py'
153--- locolander/locolanderweb/tests/test_tasks.py 1970-01-01 00:00:00 +0000
154+++ locolander/locolanderweb/tests/test_tasks.py 2013-07-20 21:10:30 +0000
155@@ -0,0 +1,147 @@
156+import subprocess
157+from datetime import datetime
158+
159+from mock import ANY, patch
160+
161+from locolanderweb.models import (
162+ STATUS_ERROR,
163+ STATUS_MERGED,
164+ MergeRequest,
165+ RunLog,
166+)
167+from locolanderweb.tasks import run_project, run_request, scan_project
168+from locolanderweb.tests.test_base import BaseTestCase
169+
170+
171+MERGE_REQUESTS = [
172+ dict(
173+ source='source1', target='target1', reviewers='a,b,c',
174+ date_created=datetime(2013, 4, 23), commit_message='something 1',
175+ ),
176+ dict(
177+ source='source2', target='target2', reviewers='pepe',
178+ date_created=datetime(2012, 2, 29), commit_message='something else',
179+ ),
180+ dict(
181+ source='source3', target='target3', reviewers='pepe y pepa',
182+ date_created=datetime.utcnow(), commit_message='',
183+ ),
184+]
185+
186+
187+class RunProjectTestCase(BaseTestCase):
188+
189+ def setUp(self):
190+ super(RunProjectTestCase, self).setUp()
191+ patcher = patch('locolanderweb.tasks.run_request')
192+ self.mock_run_request = patcher.start()
193+ self.addCleanup(patcher.stop)
194+
195+ def test_run_project(self):
196+ project = self.factory.make_project()
197+ merge_request = self.factory.make_merge_request(project=project)
198+
199+ run_project(project)
200+ self.mock_run_request.assert_called_once_with(merge_request)
201+
202+ def test_run_project_with_no_approved_requests(self):
203+ project = self.factory.make_project()
204+
205+ run_project(project)
206+ self.assertFalse(self.mock_run_request.called)
207+
208+
209+class RunRequestTestCase(BaseTestCase):
210+
211+ def setUp(self):
212+ super(RunRequestTestCase, self).setUp()
213+ patcher = patch('locolanderweb.tasks.subprocess.check_output')
214+ self.mock_check_output = patcher.start()
215+ self.addCleanup(patcher.stop)
216+
217+ def test_run_request(self):
218+ self.mock_check_output.return_value = 'SUCCESS'
219+
220+ merge_request = self.factory.make_merge_request()
221+
222+ run_request(merge_request)
223+
224+ cmd = [
225+ ANY,
226+ merge_request.project.name,
227+ merge_request.source,
228+ merge_request.target,
229+ merge_request.commit_message,
230+ merge_request.author,
231+ ]
232+ self.mock_check_output.assert_called_once_with(
233+ cmd, stderr=subprocess.STDOUT)
234+ real_cmd = self.mock_check_output.call_args[0][0][0]
235+ self.assertTrue(real_cmd.endswith('docker/scripts/locolander'))
236+
237+ self.assertEqual(RunLog.objects.all().count(), 1)
238+ run_log = RunLog.objects.get(merge_request=merge_request)
239+ self.assertEqual(run_log.raw, 'SUCCESS')
240+ self.assertEqual(run_log.return_code, 0)
241+
242+ # refresh MR object
243+ mr = MergeRequest.objects.get(pk=merge_request.pk)
244+ self.assertEqual(mr.status, STATUS_MERGED)
245+
246+ def test_run_request_with_error(self):
247+ merge_request = self.factory.make_merge_request()
248+
249+ cmd = [
250+ ANY,
251+ merge_request.project.name,
252+ merge_request.source,
253+ merge_request.target,
254+ merge_request.commit_message,
255+ merge_request.author,
256+ ]
257+ error = subprocess.CalledProcessError(1, cmd, output='FAILURE')
258+ self.mock_check_output.side_effect = error
259+
260+ run_request(merge_request)
261+
262+ self.mock_check_output.assert_called_once_with(
263+ cmd, stderr=subprocess.STDOUT)
264+ real_cmd = self.mock_check_output.call_args[0][0][0]
265+ self.assertTrue(real_cmd.endswith('docker/scripts/locolander'))
266+
267+ self.assertEqual(RunLog.objects.all().count(), 1)
268+ run_log = RunLog.objects.get(merge_request=merge_request)
269+ self.assertEqual(run_log.raw, 'FAILURE')
270+ self.assertEqual(run_log.return_code, 1)
271+
272+ # refresh MR object
273+ mr = MergeRequest.objects.get(pk=merge_request.pk)
274+ self.assertEqual(mr.status, STATUS_ERROR)
275+
276+
277+class ScanProjectTestCase(BaseTestCase):
278+
279+ def test_scan_project(self):
280+ project = self.factory.make_project()
281+
282+ service = self.get_service_from_url.return_value
283+ service.approved_requests_for_project_url.return_value = MERGE_REQUESTS
284+
285+ scan_project(project)
286+
287+ self.assertEqual(MergeRequest.objects.filter(project=project).count(),
288+ len(MERGE_REQUESTS))
289+ for mr in MERGE_REQUESTS:
290+ self.assertEqual(
291+ MergeRequest.objects.filter(**mr).count(), 1)
292+
293+ def test_scan_project_with_no_approved_request(self):
294+ project = self.factory.make_project()
295+
296+ service = self.get_service_from_url.return_value
297+ service.approved_requests_for_project_url.return_value = []
298+
299+ scan_project(project)
300+
301+ self.assertEqual(MergeRequest.objects.filter(project=project).count(),
302+ 0)

Subscribers

People subscribed via source and target branches

to all changes: