From d1d7ef9b43b3fc589f243905ee748cdee8a37206 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 21 2018 04:05:00 +0000 Subject: [PATCH 1/10] Fix tests in test_producer.py Signed-off-by: Chenxiong Qi --- diff --git a/tests/test_producer.py b/tests/test_producer.py index adeb7ef..4e2d1f7 100644 --- a/tests/test_producer.py +++ b/tests/test_producer.py @@ -99,7 +99,7 @@ class TestCheckUnfinishedKojiTasks(helpers.ModelsTestCase): hub = MagicMock() producer = FreshmakerProducer(hub) producer.check_unfinished_koji_tasks(db.session) - self.assertRaises(consumer.incoming.get, block=False) + self.assertRaises(queue.Empty, consumer.incoming.get, block=False) @patch('freshmaker.kojiservice.KojiService.get_task_info') @patch("freshmaker.consumer.get_global_consumer") @@ -113,4 +113,4 @@ class TestCheckUnfinishedKojiTasks(helpers.ModelsTestCase): hub = MagicMock() producer = FreshmakerProducer(hub) producer.check_unfinished_koji_tasks(db.session) - self.assertRaises(consumer.incoming.get, block=False) + self.assertRaises(queue.Empty, consumer.incoming.get, block=False) From 1d5518e09919f61cf8da93b59633fd11ec7afb60 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 21 2018 06:13:33 +0000 Subject: [PATCH 2/10] Fix tests in test_lightblue.py Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/lightblue.py b/freshmaker/lightblue.py index 5977596..e4000a8 100644 --- a/freshmaker/lightblue.py +++ b/freshmaker/lightblue.py @@ -1150,7 +1150,7 @@ class LightBlue(object): images, get_nvr=lambda image: image['brew']['build'], reverse=True) images = [] for k, v in groupby(sorted_images, key=lambda x: x['brew']['build']): - images.append(v.next()) + images.append(six.next(v)) # In case we query for unpublished images, we need to return just # the latest NVR for given name-version, otherwise images would diff --git a/tests/test_lightblue.py b/tests/test_lightblue.py index e5b25c1..d068fc1 100644 --- a/tests/test_lightblue.py +++ b/tests/test_lightblue.py @@ -899,17 +899,17 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): { "field": "repositories.*.tags.*.name", "op": "=", - "rvalue": "tag1" + "rvalue": "latest" }, { "field": "repositories.*.tags.*.name", "op": "=", - "rvalue": "tag2" + "rvalue": "tag1" }, { "field": "repositories.*.tags.*.name", "op": "=", - "rvalue": "latest" + "rvalue": "tag2" }, ], }, @@ -937,7 +937,16 @@ class TestQueryEntityFromLightBlue(helpers.FreshmakerTestCase): "projection": lb._get_default_projection(srpm_names=["openssl"]) } - cont_images.assert_called_with(expected_image_request) + # auto_rebuild_tags is a set in the source code. When generate + # criteria for tags, the order is not guaranteed. Following lines sort + # the tags criteria in order to assert with expected value. + args, _ = cont_images.call_args + request_arg = args[0] + tags_criteira = request_arg['query']['$and'][1]['$or'] + request_arg['query']['$and'][1]['$or'] = sorted( + tags_criteira, key=lambda item: item['rvalue']) + + self.assertEqual(expected_image_request, request_arg) # Only the second image should be returned, because the first one # is in repository "product1/repo1", but we have asked for images From 7cc344cfcda563f5fe27a2e2bed822e50adc6590 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 21 2018 06:21:28 +0000 Subject: [PATCH 3/10] Fix tests in test_odcsclient.py Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/odcsclient.py b/freshmaker/odcsclient.py index e7955c9..5ff2f36 100644 --- a/freshmaker/odcsclient.py +++ b/freshmaker/odcsclient.py @@ -363,9 +363,10 @@ class FreshmakerODCSClient(object): packages.add(parsed_nvr["name"]) # ODCS client expects list and not set for packages/builds, so convert - # them to lists. - builds = list(builds) - packages = list(packages) + # them to lists. Sorting the lists to make them easy to look up, e.g. + # in logs, and easy to test. + builds = sorted(builds) + packages = sorted(packages) if not self.handler.dry_run: with krb_context(): diff --git a/tests/test_odcsclient.py b/tests/test_odcsclient.py index b5474f3..eb89982 100644 --- a/tests/test_odcsclient.py +++ b/tests/test_odcsclient.py @@ -299,8 +299,8 @@ class TestPrepareYumRepo(helpers.ModelsTestCase): # Ensure new_compose is called to request a new compose odcs.new_compose.assert_called_once_with( - '', 'build', builds=['avalon-logkit-2.1-14.el7', 'apache-commons-lang-2.6-15.el7'], - flags=['no_deps'], packages=[u'avalon-logkit', u'apache-commons-lang'], sigkeys=[]) + '', 'build', builds=['apache-commons-lang-2.6-15.el7', 'avalon-logkit-2.1-14.el7'], + flags=['no_deps'], packages=[u'apache-commons-lang', u'avalon-logkit'], sigkeys=[]) def _create_consumer(self): hub = MagicMock() From 44ea2ab22e94e06f7105720ba684ab17fc7c655e Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 21 2018 06:43:05 +0000 Subject: [PATCH 4/10] Fix Py3 compatibility in errata.py Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/errata.py b/freshmaker/errata.py index 08ea8e3..3b30dfa 100644 --- a/freshmaker/errata.py +++ b/freshmaker/errata.py @@ -62,7 +62,7 @@ class ErrataAdvisory(object): Creates new ErrataAdvisory instance from the Erratum ID. """ data = errata._get_advisory(errata_id) - erratum_data = data["errata"].values() + erratum_data = list(data["errata"].values()) if not erratum_data: return None erratum_data = erratum_data[0] From 0c11b0ecae48d3027cfab7dc3925b58510d00b71 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 21 2018 07:42:28 +0000 Subject: [PATCH 5/10] Use werkzeug.exceptions.Unauthorized instead Without this change, test TestOpenIDCLogin.test_openidc_manual_trigger_unauthorized fails due to flask responses 500 internal server error rather than 401 unauthorized. It happens because freshmaker defines its own Unauthorized exception and register a handler for it. However, when user is not authenticated and abort is called, flask searches an error handler internally by werkzeug.exception.Unauthorized. As a result, no asscoiated handler is found and handler of internal server error is returned finally. To fix this issue, this patch uses werkzeug's Unauthorized instead and tests are updated accordingly. Signed-off-by: Chenxiong Qi --- diff --git a/freshmaker/auth.py b/freshmaker/auth.py index d3fda62..fce0206 100644 --- a/freshmaker/auth.py +++ b/freshmaker/auth.py @@ -31,9 +31,10 @@ from itertools import chain from flask import g from flask_login import login_required as _login_required +from werkzeug.exceptions import Unauthorized from freshmaker import conf, log -from freshmaker.errors import Unauthorized, Forbidden +from freshmaker.errors import Forbidden from freshmaker.models import User, commit_on_success diff --git a/freshmaker/errors.py b/freshmaker/errors.py index cbb661b..2506284 100644 --- a/freshmaker/errors.py +++ b/freshmaker/errors.py @@ -24,6 +24,7 @@ from flask import jsonify from freshmaker import app, log +from werkzeug.exceptions import Unauthorized class ValidationError(ValueError): @@ -46,10 +47,6 @@ class ProgrammingError(ValueError): pass -class Unauthorized(ValueError): - pass - - class Forbidden(ValueError): pass @@ -72,7 +69,7 @@ def notfound_error(e): @app.errorhandler(Unauthorized) def unauthorized_error(e): """Flask error handler for Unauthorized exceptions""" - return json_error(401, 'Unauthorized', e.args[0]) + return json_error(401, 'Unauthorized', e.description) @app.errorhandler(Forbidden) diff --git a/tests/test_auth.py b/tests/test_auth.py index 4fade09..30ad69d 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -25,6 +25,7 @@ import flask from mock import patch, Mock +from werkzeug.exceptions import Unauthorized import freshmaker.auth @@ -32,7 +33,6 @@ from freshmaker.auth import init_auth from freshmaker.auth import load_krb_user_from_request from freshmaker.auth import load_openidc_user from freshmaker.auth import query_ldap_groups -from freshmaker.errors import Unauthorized from freshmaker import app, db from freshmaker.models import User from tests.helpers import ModelsTestCase, FreshmakerTestCase @@ -83,8 +83,8 @@ class TestLoadKrbUserFromRequest(ModelsTestCase): with app.test_request_context(): with self.assertRaises(Unauthorized) as ctx: load_krb_user_from_request(flask.request) - self.assertTrue( - 'REMOTE_USER is not present in request.' in ctx.exception.args) + self.assertIn('REMOTE_USER is not present in request.', + ctx.exception.description) class TestLoadOpenIDCUserFromRequest(ModelsTestCase): @@ -187,7 +187,7 @@ class TestLoadOpenIDCUserFromRequest(ModelsTestCase): load_openidc_user(flask.request) self.assertTrue( 'Required OIDC scope new-compose not present.' in - ctx.exception.args) + ctx.exception.description) class TestQueryLdapGroups(FreshmakerTestCase): From c362146ffee6e689729b8754d60c655af8bc317b Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 21 2018 07:54:11 +0000 Subject: [PATCH 6/10] Ignore .env for flake8 check Signed-off-by: Chenxiong Qi --- diff --git a/tox.ini b/tox.ini index 7743809..63d24e5 100644 --- a/tox.ini +++ b/tox.ini @@ -39,4 +39,4 @@ ignore_outcome = True [flake8] ignore = E501,E731 -exclude = freshmaker/migrations/*,.tox/*,build/*,__pycache__,scripts/print_handlers_md.py,.copr/* +exclude = freshmaker/migrations/*,.tox/*,build/*,__pycache__,scripts/print_handlers_md.py,.copr/*,.env From 95c8f10ab9fe6ced54be8fc7926495260ddcc1ad Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 21 2018 07:58:35 +0000 Subject: [PATCH 7/10] Fix flake8 errors It should not be worth to fix W504 (line break after binary operator), so ignore it. Signed-off-by: Chenxiong Qi --- diff --git a/contrib/get_freshmaker_stats.py b/contrib/get_freshmaker_stats.py index f2d7ee3..7e55720 100644 --- a/contrib/get_freshmaker_stats.py +++ b/contrib/get_freshmaker_stats.py @@ -232,7 +232,7 @@ if __name__ == '__main__': freshmaker_images = [] for image in security_images: - if re.match('.*\d{10}$', image['brew']['build']): + if re.match(r'.*\d{10}$', image['brew']['build']): freshmaker_images.append(image) logging.debug("All shipped containers with security fixes: ") diff --git a/freshmaker/messaging.py b/freshmaker/messaging.py index 2e5a5a2..e4d2860 100644 --- a/freshmaker/messaging.py +++ b/freshmaker/messaging.py @@ -108,7 +108,7 @@ def _in_memory_publish(topic, msg): work_queue_put(wrapped_msg) except ValueError as e: log.warn("No FreshmakerConsumer found. Shutting down? %r" % e) - except AttributeError as e: + except AttributeError: # In the event that `moksha.hub._hub` hasn't yet been initialized, we # need to store messages on the side until it becomes available. # As a last-ditch effort, try to hang initial messages in the config. diff --git a/tests/test_handler.py b/tests/test_handler.py index 92d58ca..b195811 100644 --- a/tests/test_handler.py +++ b/tests/test_handler.py @@ -364,7 +364,7 @@ class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): @patch.object(freshmaker.conf, 'handler_build_whitelist', new={ 'MyHandler': { 'image': { - 'advisory_name': 'RHSA-\d+:\d+' + 'advisory_name': r'RHSA-\d+:\d+' } } }) @@ -381,7 +381,7 @@ class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): @patch.object(freshmaker.conf, 'handler_build_whitelist', new={ 'MyHandler': { 'image': { - 'advisory_name': 'RHSA-\d+:\d+', + 'advisory_name': r'RHSA-\d+:\d+', 'advisory_state': 'REL_PREP' } } @@ -401,7 +401,7 @@ class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): @patch.object(freshmaker.conf, 'handler_build_whitelist', new={ 'MyHandler': { 'image': any_( - {'advisory_name': 'RHSA-\d+:\d+'}, + {'advisory_name': r'RHSA-\d+:\d+'}, {'advisory_state': 'REL_PREP'}, ) } @@ -421,7 +421,7 @@ class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): @patch.object(freshmaker.conf, 'handler_build_whitelist', new={ 'MyHandler': { 'image': all_( - {'advisory_name': 'RHSA-\d+:\d+'}, + {'advisory_name': r'RHSA-\d+:\d+'}, any_( {'has_hightouch_bugs': True}, {'severity': ['critical', 'important']} @@ -457,12 +457,12 @@ class TestAllowBuildBasedOnWhitelist(helpers.FreshmakerTestCase): @patch.object(freshmaker.conf, 'handler_build_whitelist', new={ 'MyHandler': { - 'image': {'advisory_name': 'RHSA-\d+:\d+'}, + 'image': {'advisory_name': r'RHSA-\d+:\d+'}, } }) @patch.object(freshmaker.conf, 'handler_build_blacklist', new={ 'MyHandler': { - 'image': {'advisory_name': 'RHSA-2016:\d+'}, + 'image': {'advisory_name': r'RHSA-2016:\d+'}, } }) def test_blacklist(self): diff --git a/tox.ini b/tox.ini index 63d24e5..9c83f0e 100644 --- a/tox.ini +++ b/tox.ini @@ -38,5 +38,5 @@ commands = ignore_outcome = True [flake8] -ignore = E501,E731 +ignore = E501,E731,W504 exclude = freshmaker/migrations/*,.tox/*,build/*,__pycache__,scripts/print_handlers_md.py,.copr/*,.env From d24c75eb358d761826792e842e114b242b95a6dc Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 22 2018 02:31:57 +0000 Subject: [PATCH 8/10] sitepackages is not necessary to run tests As per the subject, virtualenv environment could be created without sitepackages enabled. Both py27 and py35 work now. Signed-off-by: Chenxiong Qi --- diff --git a/tests/test_monitor.py b/tests/test_monitor.py index 046c108..d919e94 100644 --- a/tests/test_monitor.py +++ b/tests/test_monitor.py @@ -22,7 +22,7 @@ import fedmsg.config import mock import freshmaker -import queue +from six.moves import queue from freshmaker import app, db, events, models, login_manager from tests import helpers diff --git a/tests/test_odcsclient.py b/tests/test_odcsclient.py index eb89982..44019d3 100644 --- a/tests/test_odcsclient.py +++ b/tests/test_odcsclient.py @@ -24,7 +24,7 @@ import fedmsg import six -import queue +from six.moves import queue from mock import patch, Mock, MagicMock from odcs.client.odcs import AuthMech diff --git a/tests/test_producer.py b/tests/test_producer.py index 4e2d1f7..cf66ede 100644 --- a/tests/test_producer.py +++ b/tests/test_producer.py @@ -22,10 +22,11 @@ # # Written by Jan Kaluza -from mock import patch, MagicMock import koji import fedmsg.config -import queue + +from mock import patch, MagicMock +from six.moves import queue import freshmaker from freshmaker import db diff --git a/tox.ini b/tox.ini index 9c83f0e..9943d8a 100644 --- a/tox.ini +++ b/tox.ini @@ -7,7 +7,6 @@ envlist = py27, py35, coverage, flake8, bandit [testenv] -sitepackages=True skip_install = True deps = -r{toxinidir}/test-requirements.txt commands = From 9d9e90fe8c0432189f3ab75ae1ab3a73a880fcef Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 22 2018 03:22:28 +0000 Subject: [PATCH 9/10] Adjust tox to run coverage Show code coverage just after each tests run. Also, for some purpose, running single testenv also shows the coverage without additional step to run coverage. Signed-off-by: Chenxiong Qi --- diff --git a/test-requirements.txt b/test-requirements.txt index d7eb98b..9a8d623 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -3,5 +3,6 @@ copr mock pytest +pytest-cov vcrpy tox diff --git a/tox.ini b/tox.ini index 9943d8a..cac5f68 100644 --- a/tox.ini +++ b/tox.ini @@ -4,7 +4,7 @@ # and then run "tox" from this directory. [tox] -envlist = py27, py35, coverage, flake8, bandit +envlist = py27, py35, flake8, bandit [testenv] skip_install = True @@ -12,16 +12,6 @@ deps = -r{toxinidir}/test-requirements.txt commands = py.test {posargs} -[testenv:coverage] -basepython = python3 -deps = - {[testenv]deps} - coverage -commands = - coverage run --parallel-mode -m pytest - coverage combine - coverage report --omit=tests/*,.tox/*,/usr/* -m --skip-covered - [testenv:flake8] basepython = python3 skip_install = true @@ -39,3 +29,15 @@ ignore_outcome = True [flake8] ignore = E501,E731,W504 exclude = freshmaker/migrations/*,.tox/*,build/*,__pycache__,scripts/print_handlers_md.py,.copr/*,.env + +[pytest] +addopts = --cov=freshmaker + +[coverage:report] +skip_covered = 1 +show_missing = 1 +omit = + .tox + .env + tests/* + /usr/* \ No newline at end of file From 085b1a077a7f5a46901bc079ef890678a3d62ca9 Mon Sep 17 00:00:00 2001 From: Chenxiong Qi Date: Nov 22 2018 03:39:42 +0000 Subject: [PATCH 10/10] Remove krbcontext from requirements.txt krbcontext is not used any more. Signed-off-by: Chenxiong Qi --- diff --git a/requirements.txt b/requirements.txt index f8ba94f..adefad0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,7 +20,6 @@ Flask-Login requests enum34 ; python_version <= '2.7' odcs[client] -krbcontext dogpile.cache pyldap koji