From cede6e9ab6a0b5956ae7d01b53df638e2b3c66f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Kone=C4=8Dn=C3=BD?= Date: Tue, 15 Mar 2022 15:52:27 +0100 Subject: [PATCH] Add first unit tests for scm_request_processor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently process, process_ticket and accepts_topic are covered. Signed-off-by: Michal Konečný --- tests/plugins/test_scm_request_processor.py | 350 ++++++++++++++++++++ toddlers/plugins/scm_request_processor.py | 176 +++------- 2 files changed, 391 insertions(+), 135 deletions(-) create mode 100644 tests/plugins/test_scm_request_processor.py diff --git a/tests/plugins/test_scm_request_processor.py b/tests/plugins/test_scm_request_processor.py new file mode 100644 index 0000000..1fc40e8 --- /dev/null +++ b/tests/plugins/test_scm_request_processor.py @@ -0,0 +1,350 @@ +""" +Unit tests for `toddlers.plugins.scm_request_processor` +""" +import json +from unittest.mock import call, patch, Mock +import logging + +from pagure_messages.issue_schema import IssueNewV1 +import pytest + +import toddlers.plugins.scm_request_processor as scm_request_processor +from toddlers.exceptions import ValidationError + +class TestAcceptsTopic: + """ + Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.accepts_topic` method. + """ + toddler_cls = scm_request_processor.SCMRequestProcessor + + def test_accetps_topic_invalid(self, toddler): + """ + Assert that invalid topic is not accepted. + """ + assert toddler.accepts_topic("foo.bar") is False + + @pytest.mark.parametrize( + "topic", + [ + "org.fedoraproject.*.pagure.issue.new", + "org.fedoraproject.*.pagure.issue.edit", + "org.fedoraproject.stg.pagure.issue.new", + "org.fedoraproject.stg.pagure.issue.edit", + "org.fedoraproject.prod.pagure.issue.new", + "org.fedoraproject.prod.pagure.issue.edit", + ] + ) + def test_accetps_topic_valid(self, topic, toddler): + """ + Assert that valid topics are accepted. + """ + assert toddler.accepts_topic(topic) + + +class TestProcess: + """ + Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.process` method. + """ + toddler_cls = scm_request_processor.SCMRequestProcessor + + def test_process_invalid_project(self, caplog, toddler): + """ + Assert that messages from other projects than fedora_scm_requests will be skipped. + """ + caplog.set_level(logging.INFO) + + msg = IssueNewV1() + msg.body = { + "project": { + "fullname": "foo/bar" + } + } + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket" + ) as mock_process_ticket: + toddler.process({}, msg) + + mock_process_ticket.assert_not_called() + + assert ( + caplog.records[-1].message == + "The message doesn't belong to project releng/fedora-scm-requests. Skipping message." + ) + + def test_process_issue_not_open(self, caplog, toddler): + """ + Assert that messages with closed issues will be skipped. + """ + caplog.set_level(logging.INFO) + + msg = IssueNewV1() + msg.body = { + "project": { + "fullname": scm_request_processor.PROJECT_NAMESPACE + }, + "issue": { + "id": 100, + "close_status": "Closed" + } + } + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket" + ) as mock_process_ticket: + toddler.process({}, msg) + + mock_process_ticket.assert_not_called() + + assert ( + caplog.records[-1].message == + "The issue 100 is not open. Skipping message." + ) + + @patch("toddlers.utils.pdc.set_pdc") + @patch("toddlers.utils.pagure.set_pagure") + @patch("toddlers.utils.fedora_account.set_fasjson") + @patch("toddlers.utils.bugzilla_system.set_bz") + def test_process(self, mock_bugzilla, mock_fasjson, mock_pagure, mock_pdc, toddler): + """ + Assert that message toddler will be initialized correctly, if message passes + initial processing. + """ + msg = IssueNewV1() + issue = { + "id": 100, + "close_status": "Open" + } + msg.body = { + "project": { + "fullname": scm_request_processor.PROJECT_NAMESPACE + }, + "issue": issue + } + config = { + "branch_slas": {}, + "monitoring_choices": [], + "pagure_namespace_to_component": {}, + "pagure_namespace_to_product": {}, + "temp_dir": "", + "dist_git_url": "https://src.fedoraproject.org", + "dist_git_token": "Private API Key" + } + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket" + ) as mock_process_ticket: + toddler.process(config, msg) + + mock_process_ticket.assert_called_with(issue) + + mock_pdc.assert_called_with(config) + mock_pagure.assert_has_calls([ + call(config), + call({ + "pagure_url": "https://src.fedoraproject.org", + "pagure_api_key": "Private API Key" + }) + ]) + mock_fasjson.assert_called_with(config) + mock_bugzilla.assert_called_with(config) + + +class TestProcessTicket: + """ + Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket` method. + """ + + def setup(self): + """ + Initialize toddler. + """ + self.toddler = scm_request_processor.SCMRequestProcessor() + self.toddler.pagure_io = Mock() + self.toddler.dist_git = Mock() + + def test_process_ticket_invalid_json(self): + """ + Assert that invalid json in issue will end the processing. + """ + issue = { + "id": 100, + "content": "invalid JSON", + "full_url": "https://blacklibrary.wh40k" + } + self.toddler.process_ticket(issue) + + self.toddler.pagure_io.close_issue.assert_called_with( + 100, namespace=scm_request_processor.PROJECT_NAMESPACE, + message="Invalid JSON provided", + reason="Invalid" + ) + + def test_process_ticket_invalid_slas(self): + """ + Assert that invalid SLAs in issue will end the processing. + """ + content = { + "sls": {}, + "branch": "branch", + } + issue = { + "id": 100, + "content": json.dumps(content), + "full_url": "https://blacklibrary.wh40k" + } + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.verify_slas" + ) as mock_verify_slas: + mock_verify_slas.side_effect = ValidationError("error") + self.toddler.process_ticket(issue) + + mock_verify_slas.assert_called_with("branch", {}) + + self.toddler.pagure_io.close_issue.assert_called_with( + 100, namespace=scm_request_processor.PROJECT_NAMESPACE, + message="error", + reason="Invalid" + ) + + def test_process_ticket_missing_action(self): + """ + Assert that missing action in issue will end the processing. + """ + content = { + "sls": {}, + "branch": "branch", + } + issue = { + "id": 100, + "content": json.dumps(content), + "full_url": "https://blacklibrary.wh40k" + } + self.toddler.process_ticket(issue) + + self.toddler.pagure_io.close_issue.assert_called_with( + 100, namespace=scm_request_processor.PROJECT_NAMESPACE, + message="Invalid or missing action field", + reason="Invalid" + ) + + def test_process_ticket_missing_sla(self): + """ + Assert that missing SLA for branch will end the processing. + """ + content = { + "branch": "branch", + "action": "new_repo" + } + issue = { + "id": 100, + "content": json.dumps(content), + "full_url": "https://blacklibrary.wh40k" + } + self.toddler.process_ticket(issue) + + self.toddler.pagure_io.close_issue.assert_called_with( + 100, namespace=scm_request_processor.PROJECT_NAMESPACE, + message="Couldn't find standard SLA for branch 'branch'", + reason="Invalid" + ) + + def test_process_ticket_invalid_branch_name(self): + """ + Assert that invalid name for branch in specific namespace will end the processing. + """ + content = { + "branch": "branch/", + "action": "new_repo", + "namespace": "flatpaks" + } + issue = { + "id": 100, + "content": json.dumps(content), + "full_url": "https://blacklibrary.wh40k" + } + self.toddler.branch_slas = {"branch/": "SLA"} + + self.toddler.process_ticket(issue) + + self.toddler.pagure_io.close_issue.assert_called_with( + 100, namespace=scm_request_processor.PROJECT_NAMESPACE, + message=("Only characters, numbers, periods, dashes, underscores, " + "and pluses are allowed in flatpak branch names"), + reason="Invalid" + ) + + def test_process_ticket_invalid_monitoring_setting(self): + """ + Assert that invalid monitoring setting for repo will end the processing. + """ + content = { + "branch": "branch", + "action": "new_repo", + "namespace": "flatpaks", + "monitor": "monitor" + } + issue = { + "id": 100, + "content": json.dumps(content), + "full_url": "https://blacklibrary.wh40k" + } + self.toddler.branch_slas = {"branch": "SLA"} + + self.toddler.process_ticket(issue) + + self.toddler.pagure_io.close_issue.assert_called_with( + 100, namespace=scm_request_processor.PROJECT_NAMESPACE, + message='The monitor choice of "monitor" is invalid', + reason="Invalid" + ) + + def test_process_ticket_action_new_repo(self): + """ + Assert that action new_repo is correctly processed. + """ + content = { + "branch": "branch", + "action": "new_repo", + } + issue = { + "id": 100, + "content": json.dumps(content), + "full_url": "https://blacklibrary.wh40k" + } + self.toddler.branch_slas = {"branch": "SLA"} + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.create_new_repo" + ) as mock_new_repo: + self.toddler.process_ticket(issue) + + content["sls"] = "SLA" + mock_new_repo.assert_called_with( + issue, content, initial_commit=True, + ) + + def test_process_ticket_action_new_branch(self): + """ + Assert that action new_branch is correctly processed. + """ + content = { + "branch": "branch", + "action": "new_branch", + } + issue = { + "id": 100, + "content": json.dumps(content), + "full_url": "https://blacklibrary.wh40k" + } + self.toddler.branch_slas = {"branch": "SLA"} + + with patch( + "toddlers.plugins.scm_request_processor.SCMRequestProcessor.create_new_branch" + ) as mock_new_branch: + self.toddler.process_ticket(issue) + + content["sls"] = "SLA" + mock_new_branch.assert_called_with( + issue, content, + ) diff --git a/toddlers/plugins/scm_request_processor.py b/toddlers/plugins/scm_request_processor.py index 6adf623..fdc16dc 100644 --- a/toddlers/plugins/scm_request_processor.py +++ b/toddlers/plugins/scm_request_processor.py @@ -7,15 +7,17 @@ Authors: Michal Konecny """ import fnmatch +import json import logging import re from tempfile import TemporaryDirectory +from typing import Optional import arrow from pagure_messages.issue_schema import IssueNewV1 from toddlers.base import ToddlerBase -from toddlers.utils import bugzilla_system, fedora_account, pagure, pdc, git +from toddlers.utils import bugzilla_system, fedora_account, git, pagure, pdc, requests from toddlers.exceptions import ValidationError # Regex for branch name validation @@ -72,6 +74,9 @@ class SCMRequestProcessor(ToddlerBase): # Path to temporary dir temp_dir = None + # Requests session + requests_session = None + def accepts_topic(self, topic: str) -> bool: """Returns a boolean whether this toddler is interested in messages from this specific topic. @@ -98,11 +103,11 @@ class SCMRequestProcessor(ToddlerBase): :arg config: Toddlers configuration :arg message: Message to process """ - project_name = message.body["project"]["name"] + project_name = message.body["project"]["fullname"] - if project_name is not PROJECT_NAME: + if project_name is not PROJECT_NAMESPACE: _log.info( - "The message doesn't belong to project {0}. Skipping message.".format(PROJECT_NAME) + "The message doesn't belong to project {0}. Skipping message.".format(PROJECT_NAMESPACE) ) return @@ -110,7 +115,7 @@ class SCMRequestProcessor(ToddlerBase): if issue["close_status"] != "Open": _log.info( - "The issue {0} is closed. Skipping message.".format(issue["id"]) + "The issue {0} is not open. Skipping message.".format(issue["id"]) ) return @@ -119,6 +124,7 @@ class SCMRequestProcessor(ToddlerBase): self.pagure_namespace_to_component = config.get("pagure_namespace_to_component") self.pagure_namespace_to_product = config.get("pagure_namespace_to_product") self.temp_dir = config.get("temp_dir") + self.requests_session = requests.make_session() _log.info("Setting up PDC client") pdc.set_pdc(config) @@ -160,11 +166,12 @@ class SCMRequestProcessor(ToddlerBase): message="Invalid JSON provided", reason="Invalid" ) + return if 'sls' in issue_body: try: # If a ValueError is raised, that means they aren't valid SLAs - log.info('- Verifying service-levels from the ticket.') + _log.info('- Verifying service-levels from the ticket.') self.verify_slas(issue_body.get('branch', None), issue_body['sls']) except ValidationError as error: _log.info("Couldn't verify SLAs. Closing '{0}'".format(issue["full_url"])) @@ -174,6 +181,7 @@ class SCMRequestProcessor(ToddlerBase): message=str(error), reason="Invalid" ) + return else: branch = issue_body.get('branch', '').strip() if issue_body.get('action') in ['new_repo', 'new_branch'] and branch: @@ -191,6 +199,7 @@ class SCMRequestProcessor(ToddlerBase): message=error, reason="Invalid" ) + return if 'branch' in issue_body and 'namespace' in issue_body: branch = issue_body['branch'] @@ -219,6 +228,7 @@ class SCMRequestProcessor(ToddlerBase): message=error_msg, reason="Invalid" ) + return if issue_body.get('action') == 'new_repo': self.create_new_repo(issue, issue_body, @@ -314,12 +324,12 @@ class SCMRequestProcessor(ToddlerBase): 'characters long with only letters, numbers, hyphens, ' 'underscores, plus signs, and/or periods. Please note that ' 'the project cannot start with a period or a plus sign.') - self.pagure_io.close_issue( - issue["id"], - namespace=PROJECT_NAMESPACE, - message=error, - reason="Invalid" - ) + self.pagure_io.close_issue( + issue["id"], + namespace=PROJECT_NAMESPACE, + message=error, + reason="Invalid" + ) return if exception is True or namespace in ('modules', 'flatpaks', 'tests', 'container'): @@ -632,6 +642,8 @@ class SCMRequestProcessor(ToddlerBase): message=new_branch_comment, reason="Processed" ) + if bug_id: + bugzilla_system.comment_on_bug(bug_id, new_repo_comment) def validate_review_bug(self, bug_id: str, pkg: str, branch: str, require_auth: bool = False, check_fas: bool = False, @@ -768,69 +780,29 @@ class SCMRequestProcessor(ToddlerBase): 'the one provided "{1}"'.format(pkg_in_bug, pkg)) raise ValidationError(error) - def comment_and_close_ticket(issue_id, bug_id, comment=None, - close_status='Processed', - prompt_for_comment=True): - """ - A helper function that adds a comment, and then prompts the user if they'd - like to add one - :param issue_id: a string or int of the id of the issue to comment and - close - :param bug_id: a string or int of the id of the Bugzilla review bug. - None is an acceptable value if there isn't a bug. - :param comment: a string of the comment to add to the issue - :param close_status: a string of the status of the ticket when closed - :param prompt_for_comment: a boolean that determines the admin should be - prompted for an extra comment. This defaults to True. - :return: None - """ - if comment is not None: - pagure.add_comment_to_issue(issue_id, comment) - if bug_id: - BUGZILLA_CLIENT.comment(bug_id, comment) - click.echo('The following comment was added to the issue "{0}"' - .format(comment)) - custom_comment = None - if prompt_for_comment: - if click.confirm('Would you like to add another comment?'): - custom_comment = click.prompt('Please enter a comment') - - pagure.close_issue(issue_id, custom_comment, close_status) - if custom_comment and bug_id: - BUGZILLA_CLIENT.comment(bug_id, custom_comment) - - def valid_project_name(project): - """ - A helper function to determine if a project name meets naming standards - :param project: a string of the project name - :return: a boolean detremining if the project name is valid - """ - return bool(re.match(r'^[a-zA-Z0-9_][a-zA-Z0-9-_.+]*$', project)) - - - def valid_module_stream_name(stream): - """ - A helper function to determine if a module's stream name meets naming - standards - :param stream: a string of the module stream name - :return: a boolean detremining if the module stream name is valid - """ - return bool(re.match(r'^[a-zA-Z0-9.\-_+]+$', stream)) - - - def valid_epel_package(self, name, branch): + def valid_epel_package(self, name: str, branch: str) -> bool: """ Determines if the package is allowed to have an EPEL branch. - :param name: a string of the package name - :param branch: a string of the EPEL branch name (e.g. epel7) - :return: a boolean + + Params: + name: a string of the package name + branch: a string of the EPEL branch name (e.g. epel7) + + Returns: + If package is valid EPEL package. + + Raises: + `ValidationError`: When we can't retrieve list of official EL packages. """ # Extract any digits in the branch name to determine the EL version version = ''.join([i for i in branch if re.match(r'\d', i)]) url = f'https://infrastructure.fedoraproject.org/repo/json/pkg_el{version}.json' - rv = requests_wrapper( - url, timeout=60, service_name='infrastructure.fedoraproject.org') - rv_json = get_request_json(rv, 'getting the list of official EL packages') + response = self.requests_session.get(url) + + if response.status_code != 200: + raise ValidationError("Couldn't retrieve the list of official EL packages") + rv_json = response.json() + # Remove noarch from this because noarch is treated specially all_arches = set(rv_json['arches']) - set(['noarch']) # On EL6, also remove ppc and i386 as many packages will @@ -849,69 +821,3 @@ class SCMRequestProcessor(ToddlerBase): if pkg_arches == set(['noarch']) or not (all_arches - pkg_arches): return False return True - - - def bool_to_word(bool_value): - """ - Converts a boolean to a "Yes" or "No" - :param bool_value: a boolean - :return: a string of the word representing the boolean - """ - if bool_value is True: - return 'Yes' - else: - return 'No' - - - def assert_git_repo_initialized_remotely(namespace, repo): - """ - Determines if the git repo is initialized remotely or not. If it isn't, - a ValidationError is raised. - :param namespace: a string of the namespace of the project - :param project: a string of the project name - :return: None or ValidationError - """ - click.echo('- Verifying that the git repo is initialized') - git_url = pagure.get_project_git_url( - namespace, repo, url_type='git', - username=FAS_CLIENT.client.username) - git_obj = git.GitRepo(git_url) - if not git_obj.initialized_remotely(namespace, repo): - raise ValidationError('The git repository is not initialized. The git ' - 'branch can\'t be created.') - - - def new_git_branch(namespace, repo, branch, use_default_branch=False): - """ - Create a new branch in git using Pagure. This does some sanity checking - before sending off the API request. - :param namespace: a string of the namespace of the project - :param project: a string of the project name - :param branch: a string of the branch to create - :param use_default_branch: a boolean that determines whether to use the default - branch or the first commit of the default branch as the starting point for - the new branch - :return: None or ValidationError - """ - if use_default_branch is True: - default_branch = get_project_default_branch(namespace, repo) - if default_branch: - pagure.new_branch( - namespace, repo, branch, from_branch=default_branch) - else: - raise ValidationError('There is no default branch for {0}/{1}. A ' - 'git branch can\'t be created.'.format(namespace, repo)) - else: - # Even though the branches are created using pagure api which dont - # require ssh, but the code supports adding package.cfg file. - # This should be pushed using ssh. - git_url = pagure.get_project_git_url( - namespace, repo, url_type='ssh', - username=FAS_CLIENT.client.username) - git_obj = git.GitRepo(git_url) - git_obj.clone_repo() - if not git_obj.initialized: - raise ValidationError('The git repository is not initialized. A ' - 'git branch can\'t be created.') - pagure.new_branch( - namespace, repo, branch, from_commit=git_obj.first_commit(namespace, repo))