From a0aa142abf0669faab33d586b5ec621ec9b72150 Mon Sep 17 00:00:00 2001
From: Tomas Kopecek <tkopecek@redhat.com>
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 <mikem@redhat.com>
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 <mikem@redhat.com>
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 <mikem@redhat.com>
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 <mikem@redhat.com>
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 <tkopecek@redhat.com>
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):

