From a0aa142abf0669faab33d586b5ec621ec9b72150 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 31 2017 12:30:29 +0000 Subject: [PATCH 1/6] Treat canceled tasks as failed for optional_archs Related: https://pagure.io/koji/issue/529 --- diff --git a/hub/kojihub.py b/hub/kojihub.py index cbad388..d78ad62 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -11415,7 +11415,18 @@ class Host(object): for task_id in tasks: task = Task(task_id) raise_fault = (task_id not in canfail) - results.append([task_id, task.getResult(raise_fault=raise_fault)]) + try: + results.append([task_id, task.getResult(raise_fault=raise_fault)]) + except koji.GenericError: + # Canceled tasks raises error even for tasks in can_fail as + # it signals higher level problem. So, ignore it here. + if not raise_fault and task.isCanceled(): + results.append([task_id, { + 'faultCode': koji.GenericError.fault_code, + 'faultString': 'Task canceled' + }]) + continue + raise return results def getHostTasks(self): From d36ad80fc33d8f1320eff5c9f3797f2809ed93a6 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Oct 31 2017 12:30:29 +0000 Subject: [PATCH 2/6] include faultCode in return for canceled, can-fail tasks --- diff --git a/hub/kojihub.py b/hub/kojihub.py index d78ad62..76d107f 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -11417,14 +11417,13 @@ class Host(object): raise_fault = (task_id not in canfail) try: results.append([task_id, task.getResult(raise_fault=raise_fault)]) - except koji.GenericError: - # Canceled tasks raises error even for tasks in can_fail as - # it signals higher level problem. So, ignore it here. + except koji.GenericError, e: + # Asking for result of canceled task raises an error + # For canfail tasks, return error in neutral form if not raise_fault and task.isCanceled(): - results.append([task_id, { - 'faultCode': koji.GenericError.fault_code, - 'faultString': 'Task canceled' - }]) + f_info = {'faultCode': e.faultCode, + 'faultString': str(e)} + results.append([task_id, f_info]) continue raise return results From 8eedd33b20b25a730a5bd105888307cd8ab0bbd3 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Oct 31 2017 12:30:29 +0000 Subject: [PATCH 3/6] add unit test for host.taskWaitResults --- diff --git a/tests/test_hub/test_task_wait_results.py b/tests/test_hub/test_task_wait_results.py new file mode 100644 index 0000000..dde749b --- /dev/null +++ b/tests/test_hub/test_task_wait_results.py @@ -0,0 +1,40 @@ +import mock +import unittest + +import kojihub + + +class TestTaskWaitResults(unittest.TestCase): + + def setUp(self): + self.context = mock.patch('kojihub.context').start() + self.host_id = 99 + self.context.session.getHostId.return_value = self.host_id + self.host_exports = kojihub.Host(self.host_id) + self.host_exports.taskUnwait = mock.MagicMock() + self.Task = mock.patch('kojihub.Task', side_effect=self.getTask).start() + self.cnx = mock.patch('kojihub.context.cnx').start() + self.tasks = {} + + def tearDown(self): + mock.patch.stopall() + + def getTask(self, task_id): + if task_id in self.tasks: + return self.tasks[task_id] + task = mock.MagicMock() + task.id = task_id + self.tasks[task_id] = task + return task + + def test_basic(self): + parent = 1 + task_ids = [5,6,7] + for t in task_ids: + task = self.getTask(t) + task.getResult.return_value = "OK" + task.isCanceled.return_value = False + results = self.host_exports.taskWaitResults(parent, task_ids) + expect = [[t, "OK"] for t in task_ids] + self.assertEqual(results, expect) + self.host_exports.taskUnwait.assert_called_with(parent) From 62a1cd24c9e6a9c05aa4746e964bdcb800ff71e1 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Oct 31 2017 12:30:29 +0000 Subject: [PATCH 4/6] taskWaitResults: fix canfail/canceled case when tasks=None --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 76d107f..1e7dd25 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -11391,25 +11391,16 @@ class Host(object): def taskWaitResults(self, parent, tasks, canfail=None): if canfail is None: canfail = [] - results = {} # If we're getting results, we're done waiting self.taskUnwait(parent) - c = context.cnx.cursor() - canceled = koji.TASK_STATES['CANCELED'] - closed = koji.TASK_STATES['CLOSED'] - failed = koji.TASK_STATES['FAILED'] - q = """ - SELECT id,state FROM task - WHERE parent=%(parent)s""" if tasks is None: - # Query all subtasks - tasks = [] - c.execute(q, locals()) - for task_id, state in c.fetchall(): - if state == canceled: - raise koji.GenericError("Subtask canceled") - elif state in (closed, failed): - tasks.append(task_id) + # Query all finished subtasks + states = tuple([koji.TASK_STATES[s] + for s in ['CLOSED', 'FAILED','CANCELED']]) + query = QueryProcessor(tables=['task'], columns=['id'], + clauses=['parent=%(parent)s', 'state in %(states)s'], + values=locals(), opts={'asList': True}) + tasks = [r[0] for r in query.execute()] # Would use a dict, but xmlrpc requires the keys to be strings results = [] for task_id in tasks: From 27ac1e10d85dbee0eae93ca030caf1f832450df8 Mon Sep 17 00:00:00 2001 From: Mike McLean Date: Oct 31 2017 12:30:29 +0000 Subject: [PATCH 5/6] taskWaitResults: more unit tests --- diff --git a/tests/test_hub/test_task_wait_results.py b/tests/test_hub/test_task_wait_results.py index dde749b..951c54a 100644 --- a/tests/test_hub/test_task_wait_results.py +++ b/tests/test_hub/test_task_wait_results.py @@ -1,9 +1,14 @@ import mock import unittest +import xmlrpclib +import koji import kojihub +QP = kojihub.QueryProcessor + + class TestTaskWaitResults(unittest.TestCase): def setUp(self): @@ -13,8 +18,11 @@ class TestTaskWaitResults(unittest.TestCase): self.host_exports = kojihub.Host(self.host_id) self.host_exports.taskUnwait = mock.MagicMock() self.Task = mock.patch('kojihub.Task', side_effect=self.getTask).start() - self.cnx = mock.patch('kojihub.context.cnx').start() self.tasks = {} + self.queries = [] + self.execute = mock.MagicMock() + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.get_query).start() def tearDown(self): mock.patch.stopall() @@ -27,6 +35,12 @@ class TestTaskWaitResults(unittest.TestCase): self.tasks[task_id] = task return task + def get_query(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = self.execute + self.queries.append(query) + return query + def test_basic(self): parent = 1 task_ids = [5,6,7] @@ -37,4 +51,59 @@ class TestTaskWaitResults(unittest.TestCase): results = self.host_exports.taskWaitResults(parent, task_ids) expect = [[t, "OK"] for t in task_ids] self.assertEqual(results, expect) + self.assertEqual(self.queries, []) + self.host_exports.taskUnwait.assert_called_with(parent) + + def test_error(self): + """Ensure that errors is propagated when they should be""" + parent = 1 + task_ids = [5,6,7] + for t in task_ids: + task = self.getTask(t) + task.getResult.return_value = "OK" + task.isCanceled.return_value = False + self.tasks[6].getResult.side_effect = xmlrpclib.Fault(1, "error") + with self.assertRaises(xmlrpclib.Fault): + results = self.host_exports.taskWaitResults(parent, task_ids) + self.tasks[6].getResult.side_effect = koji.GenericError('problem') + with self.assertRaises(koji.GenericError): + results = self.host_exports.taskWaitResults(parent, task_ids) + self.assertEqual(self.queries, []) + + def test_canfail_canceled(self): + """Canceled canfail tasks should not raise exceptions""" + parent = 1 + task_ids = [5,6,7] + canfail = [7] + for t in task_ids: + task = self.getTask(t) + task.getResult.return_value = "OK" + task.isCanceled.return_value = False + self.tasks[7].getResult.side_effect = koji.GenericError('canceled') + self.tasks[7].isCanceled.return_value = True + results = self.host_exports.taskWaitResults(parent, task_ids, + canfail=canfail) + expect_f = {'faultCode': koji.GenericError.faultCode, + 'faultString': 'canceled'} + expect = [[5, "OK"], [6, "OK"], [7, expect_f]] + self.assertEqual(results, expect) + self.host_exports.taskUnwait.assert_called_with(parent) + self.assertEqual(self.queries, []) + + def test_all_tasks(self): + """Canceled canfail tasks should not raise exceptions""" + parent = 1 + task_ids = [5,6,7] + self.execute.return_value = [[t] for t in task_ids] + for t in task_ids: + task = self.getTask(t) + task.getResult.return_value = "OK" + task.isCanceled.return_value = False + results = self.host_exports.taskWaitResults(parent, None) + expect = [[t, "OK"] for t in task_ids] + self.assertEqual(results, expect) self.host_exports.taskUnwait.assert_called_with(parent) + self.assertEqual(len(self.queries), 1) + query = self.queries[0] + self.assertEqual(query.tables, ['task']) + self.assertEqual(query.columns, ['id']) From 05202724d4dc3374a46df7f06c7854726280820a Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Oct 31 2017 12:30:29 +0000 Subject: [PATCH 6/6] test intermediate calls --- diff --git a/tests/test_hub/test_task_wait_results.py b/tests/test_hub/test_task_wait_results.py index 951c54a..332293e 100644 --- a/tests/test_hub/test_task_wait_results.py +++ b/tests/test_hub/test_task_wait_results.py @@ -65,9 +65,11 @@ class TestTaskWaitResults(unittest.TestCase): self.tasks[6].getResult.side_effect = xmlrpclib.Fault(1, "error") with self.assertRaises(xmlrpclib.Fault): results = self.host_exports.taskWaitResults(parent, task_ids) + self.assertEqual(results, []) self.tasks[6].getResult.side_effect = koji.GenericError('problem') with self.assertRaises(koji.GenericError): results = self.host_exports.taskWaitResults(parent, task_ids) + self.assertEqual(results, []) self.assertEqual(self.queries, []) def test_canfail_canceled(self):