From 31dbf679602de14ba303f298d38618b364f5974d Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Thu, 29 Sep 2022 15:35:43 +0200 Subject: [PATCH] readability fixes --- hub/kojihub.py | 20 ++++----- tests/test_hub/test_add_host.py | 9 ++-- tests/test_hub/test_add_rpm_sig.py | 17 ++++++-- tests/test_hub/test_edit_build_target.py | 18 ++++++-- tests/test_hub/test_edit_tag.py | 20 ++++++--- tests/test_hub/test_edit_user.py | 21 +++++++--- tests/test_hub/test_find_build_id.py | 23 ++++++---- tests/test_hub/test_models/test_host.py | 53 +++++++++++------------- 8 files changed, 114 insertions(+), 67 deletions(-) diff --git a/hub/kojihub.py b/hub/kojihub.py index 06982580..0899e66d 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -83,7 +83,6 @@ from koji.db import ( UpdateProcessor, _applyQueryOpts, _dml, - _fetchMulti, _fetchSingle, _multiRow, _singleRow, @@ -456,7 +455,7 @@ class Task(object): """Cancel child tasks""" query = QueryProcessor(tables=['task'], columns=['id'], clauses=['parent = %(task_id)i'], values={'task_id': self.id}, opts={'asList': True}) - for (id, ) in query.execute(): + for (id,) in query.execute(): Task(id).cancel(recurse=True) def cancelFull(self, strict=True): @@ -497,8 +496,7 @@ class Task(object): query = QueryProcessor(tables=['task'], columns=['id'], clauses=['parent = %(task_id)i'], values={'task_id': task_id}, opts={'asList': True}) - result = query.execute() - for (child_id,) in result: + for (child_id,) in query.execute(): tasklist.append(child_id) def getRequest(self): @@ -2549,9 +2547,9 @@ def get_ready_hosts(): columns=['host.id', 'name', 'arches', 'task_load', 'capacity'], aliases=['id', 'name', 'arches', 'task_load', 'capacity'], clauses=[ - 'enabled = TRUE', - 'ready = TRUE', - 'expired = FALSE', + 'enabled IS TRUE', + 'ready IS TRUE', + 'expired IS FALSE', 'master IS NULL', 'active IS TRUE', "update_time > NOW() - '5 minutes'::interval" @@ -2749,7 +2747,7 @@ def repo_init(tag, task_id=None, with_src=False, with_debuginfo=False, event=Non # make sure event is valid query = QueryProcessor(tables=['events'], columns=['time'], clauses=['id=%(event)s'], values={'event': event}) - query.singleValue() + query.singleValue(strict=True) event_id = event insert = InsertProcessor('repo') insert.set(id=repo_id, create_event=event_id, tag_id=tag_id, state=state, task_id=task_id) @@ -8373,7 +8371,7 @@ def build_references(build_id, limit=None, lazy=False): AND build.state = %(st_complete)i""" if limit is not None: q += "\nLIMIT %(limit)i" - for (rpm_id, ) in build_rpm_ids: + for (rpm_id,) in build_rpm_ids: for row in _multiRow(q, locals(), fields): idx.setdefault(row['id'], row) if limit is not None and len(idx) > limit: @@ -13905,7 +13903,7 @@ class Host(object): is a list of task ids.""" # check to see if any of the tasks have finished query = QueryProcessor(tables=['task'], columns=['id', 'state'], - clauses=['parent=%(parent)s', 'awaited = TRUE'], + clauses=['parent=%(parent)s', 'awaited IS TRUE'], values={'parent': parent}, opts={'rowlock': True}) result = query.execute() @@ -13929,7 +13927,7 @@ class Host(object): context.commit_pending = True for id in finished: update = UpdateProcessor('task', clauses=['id=%(id)s'], - values={'id': id}, rawdata={'awaited': 'false'}) + values={'id': id}, data={'awaited': False}) update.execute() return [finished, unfinished] diff --git a/tests/test_hub/test_add_host.py b/tests/test_hub/test_add_host.py index 36a57821..4b851922 100644 --- a/tests/test_hub/test_add_host.py +++ b/tests/test_hub/test_add_host.py @@ -27,6 +27,7 @@ class TestAddHost(unittest.TestCase): query = QP(*args, **kwargs) query.execute = mock.MagicMock() query.executeOne = mock.MagicMock() + query.singleValue = self.query_singleValue self.queries.append(query) return query @@ -51,9 +52,9 @@ class TestAddHost(unittest.TestCase): self.verify_host_name = mock.patch('kojihub.verify_host_name').start() self.verify_name_user = mock.patch('kojihub.verify_name_user').start() self.get_host = mock.patch('kojihub.get_host').start() - self._singleValue = mock.patch('kojihub._singleValue').start() self.nextval = mock.patch('kojihub.nextval').start() self.get_user = mock.patch('kojihub.get_user').start() + self.query_singleValue = mock.MagicMock() def tearDown(self): mock.patch.stopall() @@ -70,10 +71,10 @@ class TestAddHost(unittest.TestCase): def test_add_host_valid(self): self.verify_host_name.return_value = None self.get_host.return_value = {} - self._singleValue.return_value = 333 self.nextval.return_value = 12 self.context.session.createUser.return_value = 456 self.get_user.return_value = None + self.query_singleValue.return_value = 333 r = self.exports.addHost('hostname', ['i386', 'x86_64']) self.assertEqual(r, 12) @@ -150,9 +151,11 @@ class TestAddHost(unittest.TestCase): 'usertype': koji.USERTYPES['GROUP'] } self.get_host.return_value = {} + self.query_singleValue.return_value = 333 - with self.assertRaises(koji.GenericError): + with self.assertRaises(koji.GenericError) as ex: self.exports.addHost('hostname', ['i386', 'x86_64'], force=True) + self.assertEqual("user hostname already exists and it is not a host", str(ex.exception)) self.get_user.assert_called_once_with(userInfo={'name': 'hostname'}) self.get_host.assert_called_once_with('hostname') diff --git a/tests/test_hub/test_add_rpm_sig.py b/tests/test_hub/test_add_rpm_sig.py index 40d1325f..7d0e7ea3 100644 --- a/tests/test_hub/test_add_rpm_sig.py +++ b/tests/test_hub/test_add_rpm_sig.py @@ -7,6 +7,7 @@ import koji import kojihub IP = kojihub.InsertProcessor +QP = kojihub.QueryProcessor class TestAddRPMSig(unittest.TestCase): @@ -16,10 +17,20 @@ class TestAddRPMSig(unittest.TestCase): self.inserts.append(insert) return insert + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = self.query_execute + self.queries.append(query) + return query + def setUp(self): self.InsertProcessor = mock.patch('kojihub.InsertProcessor', side_effect=self.getInsert).start() self.inserts = [] + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_execute = mock.MagicMock() self.context = mock.patch('kojihub.context').start() # It seems MagicMock will not automatically handle attributes that # start with "assert" @@ -32,7 +43,6 @@ class TestAddRPMSig(unittest.TestCase): def tearDown(self): mock.patch.stopall() - @mock.patch('kojihub._fetchMulti') @mock.patch('koji.plugin.run_callbacks') @mock.patch('kojihub.get_rpm') @mock.patch('kojihub.get_build') @@ -46,10 +56,9 @@ class TestAddRPMSig(unittest.TestCase): isdir, get_build, get_rpm, - run_callbacks, - _fetchMulti): + run_callbacks): """Test addRPMSig with header-only signed RPM""" - _fetchMulti.side_effect = [[]] + self.query_execute.side_effect = [[]] isdir.side_effect = [True] get_rpm.side_effect = [{ 'id': 1, diff --git a/tests/test_hub/test_edit_build_target.py b/tests/test_hub/test_edit_build_target.py index 68f9e9cf..6d351d44 100644 --- a/tests/test_hub/test_edit_build_target.py +++ b/tests/test_hub/test_edit_build_target.py @@ -5,14 +5,23 @@ import mock import koji import kojihub +QP = kojihub.QueryProcessor + class TestEditBuildTarget(unittest.TestCase): + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = mock.MagicMock() + query.executeOne = mock.MagicMock() + query.singleValue = self.query_singleValue + self.queries.append(query) + return query + def setUp(self): self.lookup_build_target = mock.patch('kojihub.lookup_build_target').start() self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start() self.get_tag = mock.patch('kojihub.get_tag').start() - self._singleValue = mock.patch('kojihub._singleValue').start() self.exports = kojihub.RootExports() self.target_name = 'build-target' self.name = 'build-target-rename' @@ -23,6 +32,10 @@ class TestEditBuildTarget(unittest.TestCase): self.dest_tag_info = {'id': 112, 'name': self.dest_tag} self.session = kojihub.context.session = mock.MagicMock() self.session.assertPerm = mock.MagicMock() + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_singleValue = mock.MagicMock() def tearDown(self): mock.patch.stopall() @@ -81,7 +94,7 @@ class TestEditBuildTarget(unittest.TestCase): self.verify_name_internal.return_value = None self.lookup_build_target.return_value = self.target_info self.get_tag.side_effect = [self.build_tag_info, self.dest_tag_info] - self._singleValue.return_value = 2 + self.query_singleValue.return_value = 2 with self.assertRaises(koji.GenericError) as cm: self.exports.editBuildTarget(self.target_name, self.name, self.build_tag, self.dest_tag) @@ -91,4 +104,3 @@ class TestEditBuildTarget(unittest.TestCase): self.verify_name_internal.called_once_with(name=self.name) self.lookup_build_target.called_once_with(self.target_name) self.get_tag.has_calls([mock.call(self.build_tag), mock.call(self.dest_tag)]) - self._singleValue.called_once_with(self.name) diff --git a/tests/test_hub/test_edit_tag.py b/tests/test_hub/test_edit_tag.py index 192b8b7e..4295ca4d 100644 --- a/tests/test_hub/test_edit_tag.py +++ b/tests/test_hub/test_edit_tag.py @@ -8,6 +8,7 @@ import kojihub UP = kojihub.UpdateProcessor IP = kojihub.InsertProcessor +QP = kojihub.QueryProcessor class TestEditTag(unittest.TestCase): @@ -23,6 +24,14 @@ class TestEditTag(unittest.TestCase): self.updates.append(update) return update + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = mock.MagicMock() + query.executeOne = mock.MagicMock() + query.singleValue = self.query_singleValue + self.queries.append(query) + return query + def setUp(self): self.InsertProcessor = mock.patch('kojihub.InsertProcessor', side_effect=self.getInsert).start() @@ -30,7 +39,6 @@ class TestEditTag(unittest.TestCase): self.UpdateProcessor = mock.patch('kojihub.UpdateProcessor', side_effect=self.getUpdate).start() self.updates = [] - self._singleValue = mock.patch('kojihub._singleValue').start() self.get_tag = mock.patch('kojihub.get_tag').start() self.get_perm_id = mock.patch('kojihub.get_perm_id').start() self.verify_name_internal = mock.patch('kojihub.verify_name_internal').start() @@ -39,6 +47,10 @@ class TestEditTag(unittest.TestCase): # It seems MagicMock will not automatically handle attributes that # start with "assert" self.context_db.session.assertLogin = mock.MagicMock() + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_singleValue = mock.MagicMock() def tearDown(self): mock.patch.stopall() @@ -64,7 +76,7 @@ class TestEditTag(unittest.TestCase): 'extra': {'exA': 1, 'exC': 3, 'exD': 4}} - self._singleValue.return_value = None + self.query_singleValue.return_value = None self.verify_name_internal.return_value = None self.context_db.event_id = 42 self.context_db.session.user_id = 23 @@ -198,8 +210,7 @@ class TestEditTag(unittest.TestCase): # no4 invoke self.get_perm_id.reset_mock() self.get_perm_id.return_value = 99 - self._singleValue.reset_mock() - self._singleValue.return_value = 2 + self.query_singleValue.return_value = 2 kwargs = { 'perm': 'admin', @@ -208,7 +219,6 @@ class TestEditTag(unittest.TestCase): with self.assertRaises(koji.GenericError) as cm: kojihub._edit_tag('tag', **kwargs) self.get_perm_id.assert_called_once() - self._singleValue.assert_called_once() self.assertEqual(cm.exception.args[0], 'Name newtag already taken by tag 2') def test_invalid_archs(self): diff --git a/tests/test_hub/test_edit_user.py b/tests/test_hub/test_edit_user.py index 5fc34ae0..b7b74867 100644 --- a/tests/test_hub/test_edit_user.py +++ b/tests/test_hub/test_edit_user.py @@ -6,6 +6,7 @@ import koji import kojihub UP = kojihub.UpdateProcessor +QP = kojihub.QueryProcessor class TestEditUser(unittest.TestCase): @@ -16,9 +17,16 @@ class TestEditUser(unittest.TestCase): self.updates.append(update) return update + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = mock.MagicMock() + query.executeOne = mock.MagicMock() + query.singleValue = self.query_singleValue + self.queries.append(query) + return query + def setUp(self): self.updates = [] - self._singleValue = mock.patch('kojihub._singleValue').start() self.get_user = mock.patch('kojihub.get_user').start() self.verify_name_user = mock.patch('kojihub.verify_name_user').start() self.context = mock.patch('kojihub.context').start() @@ -27,6 +35,10 @@ class TestEditUser(unittest.TestCase): # It seems MagicMock will not automatically handle attributes that # start with "assert" self.context.session.assertLogin = mock.MagicMock() + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_singleValue = mock.MagicMock() def tearDown(self): mock.patch.stopall() @@ -35,7 +47,7 @@ class TestEditUser(unittest.TestCase): self.get_user.return_value = {'id': 333, 'name': 'user', 'krb_principals': ['krb']} - self._singleValue.return_value = None + self.query_singleValue.return_value = None self.verify_name_user.return_value = None kojihub._edit_user('user', name='newuser') @@ -88,11 +100,10 @@ class TestEditUser(unittest.TestCase): self.context.session.removeKrbPrincipal.assert_not_called() self.context.session.setKrbPrincipal.assert_not_called() - self._singleValue.reset_mock() - self._singleValue.return_value = 2 + self.query_singleValue.reset_mock() + self.query_singleValue.return_value = 2 with self.assertRaises(koji.GenericError) as cm: kojihub._edit_user('user', name='newuser') - self._singleValue.assert_called_once() self.assertEqual(cm.exception.args[0], 'Name newuser already taken by user 2') diff --git a/tests/test_hub/test_find_build_id.py b/tests/test_hub/test_find_build_id.py index dd29172b..4f947977 100644 --- a/tests/test_hub/test_find_build_id.py +++ b/tests/test_hub/test_find_build_id.py @@ -5,12 +5,24 @@ import mock import koji import kojihub +QP = kojihub.QueryProcessor + class TestFindBuildId(unittest.TestCase): + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = mock.MagicMock() + query.executeOne = mock.MagicMock() + query.singleValue = self.query_singleValue + self.queries.append(query) + return query + def setUp(self): - self.context = mock.patch('kojihub.context').start() - self.cursor = mock.MagicMock() + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_singleValue = mock.MagicMock() def test_non_exist_build_dict(self): build = { @@ -18,16 +30,13 @@ class TestFindBuildId(unittest.TestCase): 'version': 'test_version', 'release': 'test_release', } - self.cursor.fetchone.return_value = None - self.context.cnx.cursor.return_value = self.cursor + self.query_singleValue.return_value = None with self.assertRaises(koji.GenericError) as cm: kojihub.find_build_id(build, strict=True) self.assertEqual("No such build: %s" % build, str(cm.exception)) def test_invalid_argument(self): build = ['test-build'] - self.cursor.fetchone.return_value = None - self.context.cnx.cursor.return_value = self.cursor with self.assertRaises(koji.GenericError) as cm: kojihub.find_build_id(build) self.assertEqual("Invalid type for argument: %s" % type(build), str(cm.exception)) @@ -40,8 +49,6 @@ class TestFindBuildId(unittest.TestCase): 'owner': 'test_owner', 'extra': {'extra_key': 'extra_value'}, } - self.cursor.fetchone.return_value = None - self.context.cnx.cursor.return_value = self.cursor with self.assertRaises(koji.GenericError) as cm: kojihub.find_build_id(build, strict=True) self.assertEqual("did not provide name, version, and release", str(cm.exception)) diff --git a/tests/test_hub/test_models/test_host.py b/tests/test_hub/test_models/test_host.py index 9df50cd9..ef010526 100644 --- a/tests/test_hub/test_models/test_host.py +++ b/tests/test_hub/test_models/test_host.py @@ -6,6 +6,7 @@ import koji import kojihub UP = kojihub.UpdateProcessor +QP = kojihub.QueryProcessor class TestHost(unittest.TestCase): @@ -16,10 +17,20 @@ class TestHost(unittest.TestCase): self.updates.append(update) return update + def getQuery(self, *args, **kwargs): + query = QP(*args, **kwargs) + query.execute = self.query_execute + self.queries.append(query) + return query + def setUp(self): self.UpdateProcessor = mock.patch('kojihub.UpdateProcessor', side_effect=self.getUpdate).start() self.updates = [] + self.QueryProcessor = mock.patch('kojihub.QueryProcessor', + side_effect=self.getQuery).start() + self.queries = [] + self.query_execute = mock.MagicMock() @mock.patch('kojihub.context') def test_instantiation_not_a_host(self, context): @@ -118,55 +129,41 @@ class TestHost(unittest.TestCase): ) self.assertEqual(processor.call_args_list[2], update3) - @mock.patch('kojihub.context') - def test_task_wait_check(self, context): - cursor = mock.MagicMock() - context.cnx.cursor.return_value = cursor - cursor.fetchall.return_value = [ - (1, 1), - (2, 2), - (3, 3), - (4, 4), - ] + def test_task_wait_check(self): + self.query_execute.return_value = [{'id': 1, 'state': 1}, + {'id': 2, 'state': 2}, + {'id': 3, 'state': 3}, + {'id': 4, 'state': 4}, ] host = kojihub.Host(id=1234) finished, unfinished = host.taskWaitCheck(parent=123) - cursor.execute.assert_called_once() self.assertEqual(finished, [2, 3]) self.assertEqual(unfinished, [1, 4]) @mock.patch('kojihub.context') def test_task_wait(self, context): - cursor = mock.MagicMock() - context.cnx.cursor.return_value = cursor - context.session.assertLogin = mock.MagicMock() - cursor.fetchall.return_value = [ - (1, 1), - (2, 2), - (3, 3), - (4, 4), - ] - context.event_id = 42 - context.session.user_id = 23 + self.query_execute.return_value = [{'id': 1, 'state': 1}, + {'id': 2, 'state': 2}, + {'id': 3, 'state': 3}, + {'id': 4, 'state': 4}, ] kojihub.Host.return_value = 1234 host = kojihub.Host(id=1234) host.taskWait(parent=123) self.assertEqual(len(self.updates), 2) - self.assertEqual(len(cursor.execute.mock_calls), 1) - rawdata = {'awaited': 'false'} + data = {'awaited': False} update = self.updates[0] values = {'id': 2} self.assertEqual(update.table, 'task') self.assertEqual(update.values, values) - self.assertEqual(update.data, {}) - self.assertEqual(update.rawdata, rawdata) + self.assertEqual(update.data, data) + self.assertEqual(update.rawdata, {}) self.assertEqual(update.clauses, ['id=%(id)s']) update = self.updates[1] values = {'id': 3} self.assertEqual(update.table, 'task') self.assertEqual(update.values, values) - self.assertEqual(update.data, {}) - self.assertEqual(update.rawdata, rawdata) + self.assertEqual(update.data, data) + self.assertEqual(update.rawdata, {}) self.assertEqual(update.clauses, ['id=%(id)s'])