From 3b741ee4417b50ec8b172cb4337814402ef821e2 Mon Sep 17 00:00:00 2001 From: Jakub Stejskal Date: Wed, 5 Nov 2025 18:35:10 +0100 Subject: [PATCH] Notify users on GitHub when comment contains id/label missmatch Signed-off-by: Jakub Stejskal --- .gitignore | 1 + packit_service/worker/checker/abstract.py | 18 + packit_service/worker/checker/testing_farm.py | 56 +++- packit_service/worker/handlers/abstract.py | 21 +- packit_service/worker/jobs.py | 187 ++++++++++- tests/integration/test_dg_commit.py | 16 +- tests/integration/test_handler.py | 12 +- tests/integration/test_koji_build.py | 2 +- tests/integration/test_listen_to_fedmsg.py | 4 +- tests/integration/test_pr_comment.py | 314 +++++++++++++++++- tests/unit/test_checkers.py | 26 +- tests/unit/test_copr_build.py | 22 +- tests/unit/test_distgit.py | 4 +- tests/unit/test_jobs.py | 5 +- tests/unit/test_testing_farm.py | 3 +- 15 files changed, 625 insertions(+), 66 deletions(-) diff --git a/.gitignore b/.gitignore index a4bbe6f6e..cff4d5bed 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ secrets *.egg-info .mypy_cache/ /build/ +node_modules diff --git a/packit_service/worker/checker/abstract.py b/packit_service/worker/checker/abstract.py index ac7b180a1..61d26412e 100644 --- a/packit_service/worker/checker/abstract.py +++ b/packit_service/worker/checker/abstract.py @@ -29,10 +29,28 @@ def __init__( self.job_config = job_config self.data = EventData.from_event_dict(event) self.task_name = task_name + self._mismatch_data: Optional[dict] = None @abstractmethod def pre_check(self) -> bool: ... + def get_failure_message(self) -> Optional[dict]: + """ + Get the failure message/mismatch data if the check failed. + This is used to aggregate failure messages into a single comment. + + Returns: + Failure message/mismatch data if check failed, None otherwise. + Dict with structured data: + { + "type": str, # type of the matcher failure + "job_value": str | list[str], # job's identifier or labels + "comment_value": str | list[str], # what was specified in command + "targets": list[str], # all targets for this job + } + """ + return self._mismatch_data + class ActorChecker(Checker): @property diff --git a/packit_service/worker/checker/testing_farm.py b/packit_service/worker/checker/testing_farm.py index a49809a20..831925cee 100644 --- a/packit_service/worker/checker/testing_farm.py +++ b/packit_service/worker/checker/testing_farm.py @@ -154,6 +154,8 @@ class IsIdentifierFromCommentMatching(Checker, GetTestingFarmJobHelperMixin): otherwise only jobs with the same identifier. """ + DESCRIPTION: str = "**Identifiers don't match** - test run will be skipped" + def pre_check(self) -> bool: if ( not self.testing_farm_job_helper.comment_arguments.labels @@ -163,7 +165,17 @@ def pre_check(self) -> bool: logger.info( f"Using the default identifier for test command: {default_identifier}", ) - return self.job_config.identifier == default_identifier + if self.job_config.identifier != default_identifier: + # Create structured mismatch data for aggregation + self._mismatch_data = { + "type": "identifier_default", + "job_value": self.job_config.identifier, + "comment_value": default_identifier, + "targets": list(self.job_config.targets), + } + + return False + return True if ( not self.testing_farm_job_helper.comment_arguments.identifier @@ -177,6 +189,15 @@ def pre_check(self) -> bool: f"(job:{self.job_config.identifier} " f"!= comment:${self.testing_farm_job_helper.comment_arguments.identifier})", ) + + # Create structured mismatch data for aggregation + self._mismatch_data = { + "type": "identifier_explicit", + "job_value": self.job_config.identifier, + "comment_value": self.testing_farm_job_helper.comment_arguments.identifier, + "targets": list(self.job_config.targets), + } + return False @@ -187,6 +208,8 @@ class IsLabelFromCommentMatching(Checker, GetTestingFarmJobHelperMixin): otherwise only jobs with the same label. """ + DESCRIPTION: str = "**Labels don't match** - test run will be skipped" + def pre_check(self) -> bool: if ( not self.testing_farm_job_helper.comment_arguments.labels @@ -194,10 +217,30 @@ def pre_check(self) -> bool: and (default_labels := self.job_config.test_command.default_labels) ): logger.info(f"Using the default labels for test command: {default_labels}") + if not self.job_config.labels: + # Create structured mismatch data for aggregation + self._mismatch_data = { + "type": "labels_default", + "job_value": self.job_config.labels or [], + "comment_value": list(default_labels), + "targets": list(self.job_config.targets), + } + return False - return any(x in default_labels for x in self.job_config.labels) + if not any(x in default_labels for x in self.job_config.labels): + # Create structured mismatch data for aggregation + self._mismatch_data = { + "type": "labels_default", + "job_value": list(self.job_config.labels), + "comment_value": list(default_labels), + "targets": list(self.job_config.targets), + } + + return False + + return True if not self.testing_farm_job_helper.comment_arguments.labels or ( self.job_config.labels @@ -213,4 +256,13 @@ def pre_check(self) -> bool: f"(job:{self.job_config.labels} " f"!= comment:${self.testing_farm_job_helper.comment_arguments.labels})", ) + + # Create structured mismatch data for aggregation + self._mismatch_data = { + "type": "labels_explicit", + "job_value": list(self.job_config.labels) if self.job_config.labels else [], + "comment_value": list(self.testing_farm_job_helper.comment_arguments.labels), + "targets": list(self.job_config.targets), + } + return False diff --git a/packit_service/worker/handlers/abstract.py b/packit_service/worker/handlers/abstract.py index 3fbba6abb..c4988c998 100644 --- a/packit_service/worker/handlers/abstract.py +++ b/packit_service/worker/handlers/abstract.py @@ -336,12 +336,18 @@ def pre_check( package_config: PackageConfig, job_config: JobConfig, event: dict, - ) -> bool: + ) -> tuple[bool, list[dict]]: """ - Returns - bool: False if we have to skip the job execution. + Run pre-checks for the handler. + + Returns: + tuple[bool, list[dict]]: (checks_pass, failure_messages) + - checks_pass: False if we have to skip the job execution + - failure_messages: List of failure messages from failed checkers """ checks_pass = True + failure_messages = [] + for checker_cls in cls.get_checkers(): task_name = getattr(cls, "task_name", None) checker = checker_cls( @@ -350,9 +356,14 @@ def pre_check( event=event, task_name=task_name.value if task_name else None, ) - checks_pass = checks_pass and checker.pre_check() + passed = checker.pre_check() + checks_pass = checks_pass and passed + + # Collect failure messages to be aggregated at event level + if not passed and (failure_msg := checker.get_failure_message()): + failure_messages.append(failure_msg) - return checks_pass + return checks_pass, failure_messages @staticmethod def get_handler_specific_task_accepted_message( diff --git a/packit_service/worker/jobs.py b/packit_service/worker/jobs.py index 4c5db9430..d735a9f15 100644 --- a/packit_service/worker/jobs.py +++ b/packit_service/worker/jobs.py @@ -38,6 +38,10 @@ pr_labels_match_configuration, ) from packit_service.worker.allowlist import Allowlist +from packit_service.worker.checker.testing_farm import ( + IsIdentifierFromCommentMatching, + IsLabelFromCommentMatching, +) from packit_service.worker.handlers import ( CoprBuildHandler, GithubAppInstallationHandler, @@ -79,7 +83,7 @@ from packit_service.worker.helpers.testing_farm import TestingFarmJobHelper from packit_service.worker.monitoring import Pushgateway from packit_service.worker.parser import Parser -from packit_service.worker.reporting import BaseCommitStatus +from packit_service.worker.reporting import BaseCommitStatus, StatusReporter from packit_service.worker.result import TaskResults logger = logging.getLogger(__name__) @@ -169,6 +173,7 @@ class SteveJobs: def __init__(self, event: Optional[Event] = None) -> None: self.event = event self.pushgateway = Pushgateway() + self.mismatch_data: list[dict] = [] @cached_property def service_config(self) -> ServiceConfig: @@ -210,6 +215,10 @@ def process_message( result = [] if (event_not_handled or pre_check_failed) else steve.process() + # Post aggregated failure comment if there are any failure messages + if steve.mismatch_data: + steve._post_aggregated_mismatch_comment() + steve.pushgateway.push() return result @@ -252,11 +261,12 @@ def process(self) -> list[TaskResults]: self.event, github.issue.Comment, ) and self.is_fas_verification_comment(self.event.comment): - if GithubFasVerificationHandler.pre_check( + checks_pass, _ = GithubFasVerificationHandler.pre_check( package_config=None, job_config=None, event=self.event.get_dict(), - ): + ) + if checks_pass: self.event.comment_object.add_reaction(COMMENT_REACTION) GithubFasVerificationHandler.get_signature( event=self.event, @@ -530,11 +540,12 @@ def process_fedora_ci_jobs(self) -> list[TaskResults]: processing_results: list[TaskResults] = [] for handler_kls in matching_handlers: - if not handler_kls.pre_check( + checks_pass, _ = handler_kls.pre_check( package_config=None, job_config=None, event=self.event.get_dict(), - ): + ) + if not checks_pass: continue self.report_task_accepted_for_fedora_ci(handler_kls) @@ -631,9 +642,17 @@ def process_jobs(self) -> list[TaskResults]: for job_config in job_configs ] - processing_results.extend( - self.create_tasks(job_configs, handler_kls, statuses_check_feedback), + handler_results, failure_messages = self.create_tasks( + job_configs, handler_kls, statuses_check_feedback ) + processing_results.extend(handler_results) + + # Only add failure messages if NO tasks were created for this handler + # If at least one job matched and created a task, we don't want to post + # failure messages about the jobs that didn't match + if not handler_results and failure_messages: + self.mismatch_data.extend(failure_messages) + self.push_statuses_metrics(statuses_check_feedback) return processing_results @@ -643,22 +662,37 @@ def create_tasks( job_configs: list[JobConfig], handler_kls: type[JobHandler], statuses_check_feedback: list[datetime], - ) -> list[TaskResults]: + ) -> tuple[list[TaskResults], list[dict]]: """ Create handler tasks for handler and job configs. Args: job_configs: Matching job configs. handler_kls: Handler class that will be used. + statuses_check_feedback: List to collect status check feedback times. + + Returns: + tuple[list[TaskResults], list[dict]]: (processing_results, failure_messages) + - processing_results: List of task results + - failure_messages: Aggregated mismatch data dicts from failed checkers """ processing_results: list[TaskResults] = [] signatures = [] + all_failure_messages: list[dict] = [] + # we want to run handlers for all possible jobs, not just the first one for job_config in job_configs: - if self.should_task_be_created_for_job_config_and_handler( - job_config, - handler_kls, - ): + should_create, failure_messages = ( + self.should_task_be_created_for_job_config_and_handler( + job_config, + handler_kls, + ) + ) + + # Collect failure messages from this job + all_failure_messages.extend(failure_messages) + + if should_create: self.report_task_accepted( handler_kls=handler_kls, job_config=job_config, @@ -689,13 +723,13 @@ def create_tasks( # https://docs.celeryq.dev/en/stable/userguide/canvas.html#groups celery.group(signatures).apply_async() logger.debug("Signatures were sent to Celery.") - return processing_results + return processing_results, all_failure_messages def should_task_be_created_for_job_config_and_handler( self, job_config: JobConfig, handler_kls: type[JobHandler], - ) -> bool: + ) -> tuple[bool, list[dict]]: """ Check whether a new task should be created for job config and handler. @@ -704,7 +738,9 @@ def should_task_be_created_for_job_config_and_handler( handler_kls: Type of handler class to check. Returns: - Whether the task should be created. + tuple[bool, list[dict]]: (should_create, failure_messages) + - should_create: Whether the task should be created + - failure_messages: List of failure messages from failed checkers """ if self.service_config.deployment not in job_config.packit_instances: logger.debug( @@ -712,7 +748,7 @@ def should_task_be_created_for_job_config_and_handler( f"does not match the job configuration ({job_config.packit_instances}). " "The job will not be run.", ) - return False + return False, [] return handler_kls.pre_check( package_config=( @@ -1142,3 +1178,122 @@ def report_task_accepted_for_downstream_retrigger_comments( self.event.pull_request_object.comment(message) if isinstance(self.event, abstract.comment.Issue): self.event.issue_object.comment(message) + + def _post_aggregated_mismatch_comment(self) -> None: + """ + Post an aggregated comment with all mismatch messages from failed checkers. + Groups structured mismatch data into tables for better readability. + """ + if not self.mismatch_data: + return + + # Group dict messages by (type, comment_value) + grouped_dict_messages: dict[tuple[str, str], list[dict]] = {} + + for mismatch in self.mismatch_data: + key = (mismatch["type"], str(mismatch["comment_value"])) + if key not in grouped_dict_messages: + grouped_dict_messages[key] = [] + grouped_dict_messages[key].append(mismatch) + + # Generate formatted messages for grouped dicts + formatted_messages = [] + for (msg_type, _), messages in grouped_dict_messages.items(): + formatted = self._format_mismatch_table(msg_type, messages) + formatted_messages.append(formatted) + + if not formatted_messages: + return + + # Create aggregated message + aggregated_body = "\n\n---\n\n".join(formatted_messages) + + # Post comment using StatusReporter + try: + metadata = EventData.from_event_dict(self.event.get_dict()) + + reporter = StatusReporter.get_instance( + project=self.event.project, + commit_sha=metadata.commit_sha, + packit_user=self.service_config.get_github_account_name(), + project_event_id=self.event.db_project_event.id + if self.event.db_project_event + else None, + pr_id=metadata.pr_id, + ) + reporter.comment(body=aggregated_body) + logger.info( + f"Posted aggregated failure comment with {len(formatted_messages)} table(s)." + ) + except Exception as e: + logger.error(f"Failed to post aggregated failure comment: {e}") + + def _format_mismatch_table(self, msg_type: str, messages: list[dict]) -> str: + """ + Format mismatch data into a markdown table. + + Args: + msg_type: Type of mismatch (identifier_explicit, identifier_default, etc.) + messages: List of mismatch data dicts + + Returns: + Formatted markdown string with header, table, and footer + """ + # Determine header and footer based on type + if "identifier" in msg_type: + header = IsIdentifierFromCommentMatching.DESCRIPTION + if "default" in msg_type: + comment_label = "Default identifier" + footer = ( + "The default identifier is configured but does not match these jobs' " + "identifiers." + ) + else: + comment_label = "Comment identifier" + footer = "Please check for typos in your `/packit test --identifier` command." + job_column_header = "Job Identifier" + else: + header = IsLabelFromCommentMatching.DESCRIPTION + if "default" in msg_type: + comment_label = "Default labels" + footer = "The default labels are configured but do not match these jobs' labels." + else: + comment_label = "Comment labels" + footer = "Please check for typos in your `/packit test --labels` command." + job_column_header = "Job Labels" + + # Get comment value (same for all messages in group) + comment_value = messages[0]["comment_value"] + if isinstance(comment_value, list): + comment_value_str = ", ".join(f"`{v}`" for v in comment_value) + else: + comment_value_str = f"`{comment_value}`" + + # Build table rows + rows: list[str] = [] + for msg in messages: + job_value = msg["job_value"] + if isinstance(job_value, list): + if not job_value: + job_value_str = "(none)" + else: + job_value_str = ", ".join(f"`{v}`" for v in job_value) + else: + job_value_str = f"`{job_value}`" + + # Add row for each target + rows.extend( + f"| {job_value_str} | {target} |" for target in msg.get("targets", ["unknown"]) + ) + + # Assemble the message + return "\n".join( + [ + f"{header}\n", + f"{comment_label}: {comment_value_str}\n", + f"| {job_column_header} | Target |", + "|" + "-" * (len(job_column_header) + 2) + "|" + "-" * 8 + "|", + *rows, + f"\n{footer}", + ] + ) diff --git a/tests/integration/test_dg_commit.py b/tests/integration/test_dg_commit.py index 19f7746ba..306852b01 100644 --- a/tests/integration/test_dg_commit.py +++ b/tests/integration/test_dg_commit.py @@ -332,7 +332,7 @@ def test_downstream_koji_build(sidetag_group): flexmock(grouped_targets=[koji_build]), ) - flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) + flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True, []) flexmock(LocalProjectBuilder, _refresh_the_state=lambda *args: None) flexmock(group).should_receive("apply_async").once() @@ -433,7 +433,7 @@ def test_downstream_koji_build_failure_no_issue(): flexmock(id=1, grouped_targets=[koji_build]), ) - flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) + flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True, []) flexmock(Pushgateway).should_receive("push").times(1).and_return() flexmock(LocalProjectBuilder, _refresh_the_state=lambda *args: None) @@ -531,7 +531,7 @@ def test_downstream_koji_build_failure_issue_created(): flexmock(DistGit).should_receive("get_nvr").and_return(nvr) - flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) + flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True, []) flexmock(KojiBuildTargetModel).should_receive("create").and_return(koji_build) flexmock(KojiBuildTargetModel).should_receive( @@ -651,7 +651,7 @@ def test_downstream_koji_build_failure_issue_comment(): flexmock(grouped_targets=[koji_build]), ) - flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) + flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True, []) flexmock(Pushgateway).should_receive("push").times(2).and_return() flexmock(LocalProjectBuilder, _refresh_the_state=lambda *args: None) @@ -836,7 +836,7 @@ def test_downstream_koji_build_where_multiple_branches_defined(jobs_config): flexmock(DistGit).should_receive("get_nvr").and_return(nvr) - flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) + flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True, []) flexmock(KojiBuildTargetModel).should_receive("create").and_return(koji_build) flexmock(KojiBuildTargetModel).should_receive( @@ -1025,7 +1025,8 @@ def test_precheck_koji_build_push( ) job_config = jobs[0] event = distgit_push_event.get_dict() - assert DownstreamKojiBuildHandler.pre_check(package_config, job_config, event) == should_pass + checks_pass, _ = DownstreamKojiBuildHandler.pre_check(package_config, job_config, event) + assert checks_pass == should_pass @pytest.mark.parametrize( @@ -1102,4 +1103,5 @@ def test_precheck_koji_build_push_pr( ) job_config = jobs[0] event = distgit_push_event.get_dict() - assert DownstreamKojiBuildHandler.pre_check(package_config, job_config, event) == should_pass + checks_pass, _ = DownstreamKojiBuildHandler.pre_check(package_config, job_config, event) + assert checks_pass == should_pass diff --git a/tests/integration/test_handler.py b/tests/integration/test_handler.py index 7e21321c8..388dc1383 100644 --- a/tests/integration/test_handler.py +++ b/tests/integration/test_handler.py @@ -126,7 +126,8 @@ def test_precheck(github_pr_event): flexmock(CoprBuildJobHelper).should_receive( "is_custom_copr_project_defined", ).and_return(False).once() - assert CoprBuildHandler.pre_check(package_config, job_config, event) + checks_pass, _ = CoprBuildHandler.pre_check(package_config, job_config, event) + assert checks_pass def test_precheck_gitlab(gitlab_mr_event): @@ -180,7 +181,8 @@ def test_precheck_gitlab(gitlab_mr_event): flexmock(CoprBuildJobHelper).should_receive( "is_custom_copr_project_defined", ).and_return(False).once() - assert CoprBuildHandler.pre_check(package_config, job_config, event) + checks_pass, _ = CoprBuildHandler.pre_check(package_config, job_config, event) + assert checks_pass def test_precheck_push(github_push_event): @@ -279,7 +281,8 @@ def test_precheck_push_to_a_different_branch(github_push_event): }, ) event = github_push_event.get_dict() - assert not CoprBuildHandler.pre_check(package_config, job_config, event) + checks_pass, _ = CoprBuildHandler.pre_check(package_config, job_config, event) + assert not checks_pass def test_precheck_push_actor_check(github_push_event): @@ -370,4 +373,5 @@ def test_precheck_koji_build_non_scratch(github_pr_event): }, ) event = github_pr_event.get_dict() - assert not KojiBuildHandler.pre_check(package_config, job_config, event) + checks_pass, _ = KojiBuildHandler.pre_check(package_config, job_config, event) + assert not checks_pass diff --git a/tests/integration/test_koji_build.py b/tests/integration/test_koji_build.py index eac68abad..4dfceb229 100644 --- a/tests/integration/test_koji_build.py +++ b/tests/integration/test_koji_build.py @@ -183,7 +183,7 @@ def test_koji_build_error_msg(distgit_push_packit): flexmock(grouped_targets=[koji_build]), ) - flexmock(DownstreamKojiBuildHandler).should_receive("pre_check").and_return(True) + flexmock(DownstreamKojiBuildHandler).should_receive("pre_check").and_return((True, [])) flexmock(Pushgateway).should_receive("push").times(2).and_return() flexmock(group).should_receive("apply_async").once() diff --git a/tests/integration/test_listen_to_fedmsg.py b/tests/integration/test_listen_to_fedmsg.py index 706c0c0ec..c9de41106 100644 --- a/tests/integration/test_listen_to_fedmsg.py +++ b/tests/integration/test_listen_to_fedmsg.py @@ -2996,8 +2996,8 @@ def test_koji_build_tag( "get_package_config_from_repo", ).and_return(pc_koji_build_tag_specfile).and_return(pc_koji_build_tag_packit) - flexmock(DownstreamKojiBuildHandler).should_receive("pre_check").and_return(True) - flexmock(BodhiUpdateFromSidetagHandler).should_receive("pre_check").and_return(True) + flexmock(DownstreamKojiBuildHandler).should_receive("pre_check").and_return((True, [])) + flexmock(BodhiUpdateFromSidetagHandler).should_receive("pre_check").and_return((True, [])) flexmock(Signature).should_receive("apply_async").twice() flexmock(celery_group).should_receive("apply_async").once() diff --git a/tests/integration/test_pr_comment.py b/tests/integration/test_pr_comment.py index 75743ad29..8def033e9 100644 --- a/tests/integration/test_pr_comment.py +++ b/tests/integration/test_pr_comment.py @@ -73,6 +73,10 @@ from packit_service.worker.allowlist import Allowlist from packit_service.worker.celery_task import CeleryTask from packit_service.worker.checker.run_condition import IsRunConditionSatisfied +from packit_service.worker.checker.testing_farm import ( + IsIdentifierFromCommentMatching, + IsLabelFromCommentMatching, +) from packit_service.worker.handlers import distgit from packit_service.worker.handlers.bodhi import ( RetriggerBodhiUpdateHandler, @@ -823,6 +827,302 @@ def test_pr_test_command_handler_identifiers( ) +@pytest.mark.parametrize( + "test_type,job_1_config_extra,job_2_config_extra,comment_body,should_post_comment,expected_tasks,expected_comment_body", + [ + pytest.param( + "default_labels", + { + "labels": [], + "test_command": {"default_labels": ["fedora"]}, + }, + { + "labels": [], + "test_command": {"default_labels": ["fedora"]}, + }, + "/packit test", + True, + 0, + f"""{IsLabelFromCommentMatching.DESCRIPTION} + +Default labels: `fedora` + +| Job Labels | Target | +|------------|--------| +| (none) | regression-1-x86_64 | +| (none) | regression-2-x86_64 | + +The default labels are configured but do not match these jobs' labels.""", + id="default_labels_mismatch_no_job_labels", + ), + pytest.param( + "default_labels", + { + "labels": ["centos"], + "test_command": {"default_labels": ["fedora"]}, + }, + { + "labels": ["centos"], + "test_command": {"default_labels": ["fedora"]}, + }, + "/packit test", + True, + 0, + f"""{IsLabelFromCommentMatching.DESCRIPTION} + +Default labels: `fedora` + +| Job Labels | Target | +|------------|--------| +| `centos` | regression-1-x86_64 | +| `centos` | regression-2-x86_64 | + +The default labels are configured but do not match these jobs' labels.""", + id="default_labels_mismatch_different_labels", + ), + pytest.param( + "default_identifier", + { + "identifier": "wrong-id-1", + "test_command": {"default_identifier": "test-job"}, + }, + { + "identifier": "wrong-id-2", + "test_command": {"default_identifier": "test-job"}, + }, + "/packit test", + True, + 0, + f"""{IsIdentifierFromCommentMatching.DESCRIPTION} + +Default identifier: `test-job` + +| Job Identifier | Target | +|----------------|--------| +| `wrong-id-1` | regression-1-x86_64 | +| `wrong-id-2` | regression-2-x86_64 | + +The default identifier is configured but does not match these jobs' identifiers.""", + id="default_identifier_mismatch", + ), + pytest.param( + "explicit_labels", + { + "labels": [], + }, + { + "labels": [], + }, + "/packit test --labels test", + True, + 0, + f"""{IsLabelFromCommentMatching.DESCRIPTION} + +Comment labels: `test` + +| Job Labels | Target | +|------------|--------| +| (none) | regression-1-x86_64 | +| (none) | regression-2-x86_64 | + +Please check for typos in your `/packit test --labels` command.""", + id="explicit_labels_mismatch_no_job_labels", + ), + pytest.param( + "explicit_labels", + { + "labels": ["centos"], + }, + { + "labels": ["centos"], + }, + "/packit test --labels test", + True, + 0, + f"""{IsLabelFromCommentMatching.DESCRIPTION} + +Comment labels: `test` + +| Job Labels | Target | +|------------|--------| +| `centos` | regression-1-x86_64 | +| `centos` | regression-2-x86_64 | + +Please check for typos in your `/packit test --labels` command.""", + id="explicit_labels_mismatch_different_labels", + ), + pytest.param( + "explicit_labels_single_match", + { + "labels": ["centos"], + }, + { + "labels": ["magic-label"], + }, + "/packit test --labels magic-label", + False, + 1, + None, + id="explicit_labels_single_match", + ), + pytest.param( + "explicit_identifier", + { + "identifier": "wrong-id-1", + }, + { + "identifier": "wrong-id-2", + }, + "/packit test --identifier test", + True, + 0, + f"""{IsIdentifierFromCommentMatching.DESCRIPTION} + +Comment identifier: `test` + +| Job Identifier | Target | +|----------------|--------| +| `wrong-id-1` | regression-1-x86_64 | +| `wrong-id-2` | regression-2-x86_64 | + +Please check for typos in your `/packit test --identifier` command.""", + id="explicit_identifier_mismatch", + ), + pytest.param( + "explicit_identifier_label", + { + "identifier": "wrong-id-1", + "labels": ["centos"], + }, + { + "identifier": "wrong-id-2", + "labels": ["centos"], + }, + "/packit test --identifier test --label test", + True, + 0, + f"""{IsIdentifierFromCommentMatching.DESCRIPTION} + +Comment identifier: `test` + +| Job Identifier | Target | +|----------------|--------| +| `wrong-id-1` | regression-1-x86_64 | +| `wrong-id-2` | regression-2-x86_64 | + +Please check for typos in your `/packit test --identifier` command. + +--- + +{IsLabelFromCommentMatching.DESCRIPTION} + +Comment labels: `test` + +| Job Labels | Target | +|------------|--------| +| `centos` | regression-1-x86_64 | +| `centos` | regression-2-x86_64 | + +Please check for typos in your `/packit test --labels` command.""", + id="explicit_identifier_label_mismatch", + ), + ], +) +def test_pr_test_command_handler_mismatch( + add_pull_request_event_with_pr_id_9, + pr_embedded_command_comment_event, + test_type, + job_1_config_extra, + job_2_config_extra, + comment_body, + should_post_comment, + expected_tasks, + expected_comment_body, +): + """ + Test that when default labels/identifiers don't match, + a comment and neutral status are posted. + """ + jobs = [ + { + "trigger": "pull_request", + "job": "tests", + "metadata": {"targets": "regression-1-x86_64", "skip_build": True}, + **job_1_config_extra, + }, + { + "trigger": "pull_request", + "job": "tests", + "metadata": {"targets": "regression-2-x86_64", "skip_build": True}, + **job_2_config_extra, + }, + ] + packit_yaml = "{'specfile_path': 'the-specfile.spec', 'jobs': " + str(jobs) + "}" + db_project_object, db_project_event = add_pull_request_event_with_pr_id_9 + # Add id attribute to db_project_event for StatusReporter + flexmock(db_project_event, id=1) + pr = flexmock(head_commit="12345") + flexmock(GithubProject).should_receive("get_pr").and_return(pr) + comment = flexmock() + flexmock(pr).should_receive("get_comment").and_return(comment) + flexmock(comment).should_receive("add_reaction").with_args(COMMENT_REACTION).once() + flexmock( + GithubProject, + full_repo_name="packit-service/hello-world", + get_file_content=lambda path, ref: packit_yaml, + get_files=lambda ref, filter_regex: ["the-specfile.spec"], + get_web_url=lambda: "https://github.com/the-namespace/the-repo", + ) + flexmock(Github, get_repo=lambda full_name_or_id: None) + flexmock(LocalProject, refresh_the_arguments=lambda: None) + flexmock(Allowlist, check_and_report=True) + pr_embedded_command_comment_event["comment"]["body"] = comment_body + flexmock( + GithubProject, + get_files=lambda ref, recursive: ["the-specfile.spec", "packit.yaml"], + ) + flexmock(GithubProject).should_receive("is_private").and_return(False) + + comment_body_capture = [] + + if should_post_comment: + flexmock(StatusReporter).should_receive("comment").replace_with( + lambda body: ( + print(f"\n{'=' * 80}\nAGGREGATED COMMENT BODY:\n{'=' * 80}\n{body}\n{'=' * 80}\n"), + comment_body_capture.append(body), + )[-1] + ).once() + else: + flexmock(TestingFarmJobHelper).should_receive("report_status_to_tests").with_args( + description=TASK_ACCEPTED, + state=BaseCommitStatus.pending, + url="", + markdown_content=None, + links_to_external_services=None, + update_feedback_time=object, + ).once() + # Mock Celery since a task will be created + flexmock(celery_group).should_receive("apply_async").once() + flexmock(Pushgateway).should_receive("push").once().and_return() + flexmock(StatusReporter).should_receive("comment").never() + + # Should NOT run testing farm for jobs that don't match + flexmock(TestingFarmJobHelper).should_receive("run_testing_farm").never() + + processing_results = SteveJobs().process_message(pr_embedded_command_comment_event) + + # Verify expected number of tasks created + assert len(processing_results) == expected_tasks + + if should_post_comment: + # Verify that a comment was posted with expected content + assert len(comment_body_capture) == 1 + assert comment_body_capture[0] == expected_comment_body + else: + # Verify NO comment was posted when at least one job matched + assert len(comment_body_capture) == 0 + + @pytest.mark.parametrize( "retry_number,description,markdown_content,status,response,delay", ( @@ -1557,7 +1857,7 @@ def test_pr_test_command_handler_not_allowed_external_contributor_on_internal_TF namespace="packit-service", repo_name="hello-world", project_url="https://github.com/packit-service/hello-world", - ).and_return(db_project_object).times(5) + ).and_return(db_project_object).times(7) pr_embedded_command_comment_event["comment"]["body"] = "/packit test" flexmock( GithubProject, @@ -1619,7 +1919,7 @@ def test_pr_build_command_handler_not_allowed_external_contributor_on_internal_T namespace="packit-service", repo_name="hello-world", project_url="https://github.com/packit-service/hello-world", - ).and_return(db_project_object).times(8) + ).and_return(db_project_object).times(10) pr_embedded_command_comment_event["comment"]["body"] = "/packit build" flexmock( GithubProject, @@ -2424,7 +2724,7 @@ def test_koji_build_retrigger_via_dist_git_pr_comment(pagure_pr_comment_added): get_pr=lambda id: pr_mock, ) - flexmock(DownstreamKojiBuildHandler).should_receive("pre_check").and_return(True) + flexmock(DownstreamKojiBuildHandler).should_receive("pre_check").and_return((True, [])) flexmock(LocalProjectBuilder, _refresh_the_state=lambda *args: flexmock()) flexmock(LocalProject, refresh_the_arguments=lambda: None) flexmock(celery_group).should_receive("apply_async").once() @@ -2721,7 +3021,7 @@ def test_bodhi_update_retrigger_via_dist_git_pr_comment(pagure_pr_comment_added) recursive=False, ).and_return(["jouduv-dort.spec", ".packit.yaml"]) - flexmock(RetriggerBodhiUpdateHandler).should_receive("pre_check").and_return(True) + flexmock(RetriggerBodhiUpdateHandler).should_receive("pre_check").and_return((True, [])) flexmock(LocalProject, refresh_the_arguments=lambda: None) flexmock(celery_group).should_receive("apply_async").once() @@ -2922,7 +3222,7 @@ def _get_project(url, *_, **__): status=SyncReleaseStatus.finished, ).once() - flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) + flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return((True, [])) flexmock(AddPullRequestEventToDb).should_receive("db_project_object").and_return( flexmock( @@ -3093,7 +3393,7 @@ def _get_project(url, *_, **__): status=SyncReleaseStatus.finished, ).once() - flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) + flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return((True, [])) flexmock(AddPullRequestEventToDb).should_receive("db_project_object").and_return( flexmock( @@ -3211,7 +3511,7 @@ def test_koji_build_tag_via_dist_git_pr_comment(pagure_pr_comment_added, all_bra get_pr=lambda id: pr_mock, ) - flexmock(TagIntoSidetagHandler).should_receive("pre_check").and_return(True) + flexmock(TagIntoSidetagHandler).should_receive("pre_check").and_return((True, [])) flexmock(celery_group).should_receive("apply_async").once() flexmock(PackitAPI).should_receive("init_kerberos_ticket").and_return() diff --git a/tests/unit/test_checkers.py b/tests/unit/test_checkers.py index e310b8454..386a7b444 100644 --- a/tests/unit/test_checkers.py +++ b/tests/unit/test_checkers.py @@ -561,7 +561,10 @@ def test_tf_comment_identifier(comment, result): pr_id=1, ) flexmock(EventData).should_receive("db_project_event").and_return( - flexmock().should_receive("get_project_event_object").and_return(db_project_object).mock(), + flexmock(id=1) + .should_receive("get_project_event_object") + .and_return(db_project_object) + .mock(), ) checker = IsIdentifierFromCommentMatching( @@ -654,7 +657,10 @@ def test_tf_comment_default_identifier( pr_id=1, ) flexmock(EventData).should_receive("db_project_event").and_return( - flexmock().should_receive("get_project_event_object").and_return(db_project_object).mock(), + flexmock(id=1) + .should_receive("get_project_event_object") + .and_return(db_project_object) + .mock(), ) checker = IsIdentifierFromCommentMatching( @@ -662,6 +668,7 @@ def test_tf_comment_default_identifier( job_config=job_config, event=event, ) + assert checker.pre_check() == result @@ -719,7 +726,10 @@ def test_tf_comment_labels(comment, result): pr_id=1, ) flexmock(EventData).should_receive("db_project_event").and_return( - flexmock().should_receive("get_project_event_object").and_return(db_project_object).mock(), + flexmock(id=1) + .should_receive("get_project_event_object") + .and_return(db_project_object) + .mock(), ) checker = IsLabelFromCommentMatching( @@ -808,7 +818,10 @@ def test_tf_comment_default_labels(comment, default_labels, job_labels, result): pr_id=1, ) flexmock(EventData).should_receive("db_project_event").and_return( - flexmock().should_receive("get_project_event_object").and_return(db_project_object).mock(), + flexmock(id=1) + .should_receive("get_project_event_object") + .and_return(db_project_object) + .mock(), ) checker = IsLabelFromCommentMatching( @@ -871,7 +884,10 @@ def test_tf_comment_labels_none_in_config(comment, result): pr_id=1, ) flexmock(EventData).should_receive("db_project_event").and_return( - flexmock().should_receive("get_project_event_object").and_return(db_project_object).mock(), + flexmock(id=1) + .should_receive("get_project_event_object") + .and_return(db_project_object) + .mock(), ) checker = IsLabelFromCommentMatching( diff --git a/tests/unit/test_copr_build.py b/tests/unit/test_copr_build.py index defa842c8..6049777a0 100644 --- a/tests/unit/test_copr_build.py +++ b/tests/unit/test_copr_build.py @@ -1021,16 +1021,14 @@ def test_check_if_actor_can_run_job_and_report(jobs, should_pass): if not should_pass: flexmock(CoprBuildJobHelper).should_receive("report_status_to_build").once() - assert ( - CoprBuildHandler.pre_check( - package_config, - jobs[0], - { - "event_type": "github.pr.Action", - "actor": "actor", - "project_url": "url", - "commit_sha": "abcdef", - }, - ) - == should_pass + checks_pass, _ = CoprBuildHandler.pre_check( + package_config, + jobs[0], + { + "event_type": "github.pr.Action", + "actor": "actor", + "project_url": "url", + "commit_sha": "abcdef", + }, ) + assert checks_pass == should_pass diff --git a/tests/unit/test_distgit.py b/tests/unit/test_distgit.py index 549073055..0639f9735 100644 --- a/tests/unit/test_distgit.py +++ b/tests/unit/test_distgit.py @@ -106,12 +106,12 @@ def test_retrigger_downstream_koji_build_pre_check(user_groups, data, check_pass flexmock(IsRunConditionSatisfied).should_receive("pre_check").and_return(True) - result = DownstreamKojiBuildHandler.pre_check( + checks_pass, _ = DownstreamKojiBuildHandler.pre_check( None, flexmock(issue_repository=flexmock()), data_dict, ) - assert result == check_passed + assert checks_pass == check_passed def test_downstream_handler_init_order(): diff --git a/tests/unit/test_jobs.py b/tests/unit/test_jobs.py index d37cc9181..06e0472d7 100644 --- a/tests/unit/test_jobs.py +++ b/tests/unit/test_jobs.py @@ -3451,7 +3451,7 @@ def actor(self): ).and_return(flexmock().should_receive("apply_async").mock()) statuses_check_feedback = flexmock() assert tasks_created == len( - SteveJobs(event).create_tasks(jobs, handler_kls, statuses_check_feedback), + SteveJobs(event).create_tasks(jobs, handler_kls, statuses_check_feedback)[0], ) @@ -3607,7 +3607,8 @@ def test_invalid_packit_deployment(): job_config = flexmock(packit_instances=["dev", "stg"]) # handler class doesn't matter in this case - assert not jobs.should_task_be_created_for_job_config_and_handler(job_config, None) + checks_pass, _ = jobs.should_task_be_created_for_job_config_and_handler(job_config, None) + assert not checks_pass def test_unapproved_jobs(): diff --git a/tests/unit/test_testing_farm.py b/tests/unit/test_testing_farm.py index 7202d21e4..e63966267 100644 --- a/tests/unit/test_testing_farm.py +++ b/tests/unit/test_testing_farm.py @@ -2197,7 +2197,8 @@ def test_check_if_actor_can_run_job_and_report(jobs, event, should_pass): event.update({"actor": "actor", "project_url": "url"}) - assert TestingFarmHandler.pre_check(package_config, jobs[0], event) == should_pass + checks_pass, _ = TestingFarmHandler.pre_check(package_config, jobs[0], event) + assert checks_pass == should_pass @pytest.mark.parametrize(