From a37db924e0ba5b12a861edac7ac03378f74676d0 Mon Sep 17 00:00:00 2001 From: Laura Barcziova Date: Thu, 4 Apr 2024 11:35:04 +0200 Subject: [PATCH] Hide manual test statuses only when build handler is required We should not check the labels and identifiers, this should be handled by pre-checks and results in no reporting for TF notifications. Fixes #2386 --- .../worker/handlers/testing_farm.py | 15 +- packit_service/worker/helpers/testing_farm.py | 61 +------ tests/integration/test_pr_comment.py | 77 +++++++++ tests/unit/test_testing_farm.py | 150 +----------------- 4 files changed, 94 insertions(+), 209 deletions(-) diff --git a/packit_service/worker/handlers/testing_farm.py b/packit_service/worker/handlers/testing_farm.py index 02a96c2b9..b569391a6 100644 --- a/packit_service/worker/handlers/testing_farm.py +++ b/packit_service/worker/handlers/testing_farm.py @@ -260,13 +260,14 @@ def run_with_copr_builds(self, targets: List[str], failed: Dict): "The latest build has not finished yet, " "waiting until it finishes before running tests for it." ) - self.testing_farm_job_helper.report_status_to_tests_for_test_target( - state=BaseCommitStatus.pending, - description="The latest build has not finished yet, " - "waiting until it finishes before running tests for it.", - target=test_run.target, - url=get_copr_build_info_url(copr_build.id), - ) + if not self.job_config.manual_trigger: + self.testing_farm_job_helper.report_status_to_tests_for_test_target( + state=BaseCommitStatus.pending, + description="The latest build has not finished yet, " + "waiting until it finishes before running tests for it.", + target=test_run.target, + url=get_copr_build_info_url(copr_build.id), + ) continue # Only retry what's needed diff --git a/packit_service/worker/helpers/testing_farm.py b/packit_service/worker/helpers/testing_farm.py index 00cd91d4c..485215691 100644 --- a/packit_service/worker/helpers/testing_farm.py +++ b/packit_service/worker/helpers/testing_farm.py @@ -1298,45 +1298,6 @@ def test_target2build_target(self, test_target: str) -> str: """ return self.test_target2build_target_for_test_job(test_target, self.job_config) - def is_manual_job_being_triggered_by_label(self) -> bool: - """ - Is this job meant to be triggered manually - and is it being triggered by a matching - /packit test --labels label - command or has it one of the default_labels? - """ - if self.job_config.manual_trigger: - if self.job_config.labels and self.comment_arguments.labels: - return bool( - set(self.job_config.labels).intersection( - set(self.comment_arguments.labels) - ) - ) - if self.job_config.test_command.default_labels: - return bool( - set(self.job_config.labels).intersection( - set(self.job_config.test_command.default_labels) - ) - ) - return False - - def is_manual_job_being_triggered_by_identifier(self) -> bool: - """ - Is this job meant to be triggered manually - and is it being triggered by a matching - /packit test --identifier job-identifier - command or is it the default_identifier? - """ - if self.job_config.manual_trigger: - if self.job_config.identifier and self.comment_arguments.identifier: - return self.job_config.identifier == self.comment_arguments.identifier - if self.job_config.test_command.default_identifier: - return ( - self.job_config.identifier - == self.job_config.test_command.default_identifier - ) - return False - @property def build_targets_for_tests(self) -> Set[str]: """ @@ -1355,12 +1316,6 @@ def report_status_to_tests_for_chroot( links_to_external_services: Optional[Dict[str, str]] = None, update_feedback_time: Callable = None, ) -> None: - if self.job_config.manual_trigger and not ( - self.is_manual_job_being_triggered_by_label() - or self.is_manual_job_being_triggered_by_identifier() - ): - return - if chroot in self.build_targets_for_tests: test_targets = self.build_target2test_targets_for_test_job( chroot, self.job_config @@ -1386,12 +1341,6 @@ def report_status_to_tests_for_test_target( links_to_external_services: Optional[Dict[str, str]] = None, update_feedback_time: Callable = None, ) -> None: - if self.job_config.manual_trigger and not ( - self.is_manual_job_being_triggered_by_label() - or self.is_manual_job_being_triggered_by_identifier() - ): - return - if target in self.tests_targets: self._report( description=description, @@ -1412,12 +1361,6 @@ def report_status_to_tests( links_to_external_services: Optional[Dict[str, str]] = None, update_feedback_time: Callable = None, ) -> None: - if self.job_config.manual_trigger and not ( - self.is_manual_job_being_triggered_by_label() - or self.is_manual_job_being_triggered_by_identifier() - ): - return - self._report( description=description, state=state, @@ -1437,6 +1380,10 @@ def report_status_to_configured_job( links_to_external_services: Optional[Dict[str, str]] = None, update_feedback_time: Callable = None, ): + if self.job_config.manual_trigger and self.build_required(): + logger.debug("Skipping the reporting.") + return + self.report_status_to_tests( description=description, state=state, diff --git a/tests/integration/test_pr_comment.py b/tests/integration/test_pr_comment.py index 31a5f4584..df17329e1 100644 --- a/tests/integration/test_pr_comment.py +++ b/tests/integration/test_pr_comment.py @@ -320,6 +320,83 @@ def test_pr_comment_build_build_and_test_handler( assert "already handled" in first_dict_value(results["job"])["details"]["msg"] +@pytest.mark.parametrize( + "mock_pr_comment_functionality", + ( + [ + [ + { + "trigger": "pull_request", + "job": "copr_build", + "metadata": {"targets": "fedora-rawhide-x86_64"}, + }, + { + "trigger": "pull_request", + "job": "tests", + "manual_trigger": True, + "metadata": {"targets": "fedora-rawhide-x86_64"}, + }, + ] + ] + ), + indirect=True, +) +def test_pr_comment_build_build_and_test_handler_manual_test_reporting( + mock_pr_comment_functionality, pr_build_comment_event +): + flexmock(GithubProject).should_receive("is_private").and_return(False) + flexmock(CoprHelper).should_receive("get_valid_build_targets").and_return(set()) + flexmock(CoprBuildJobHelper).should_receive("report_status_to_build").with_args( + description=TASK_ACCEPTED, + state=BaseCommitStatus.pending, + url="", + markdown_content=None, + links_to_external_services=None, + update_feedback_time=object, + ).once() + # check that we are not reporting task accepted if manual_trigger is enabled for tests + 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, + ).never() + flexmock(CoprBuildJobHelper).should_receive( + "is_custom_copr_project_defined" + ).and_return(False).once() + flexmock(Signature).should_receive("apply_async").twice() + flexmock(Pushgateway).should_receive("push").times(4).and_return() + 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() + + processing_results = SteveJobs().process_message(pr_build_comment_event) + assert len(processing_results) == 2 + + copr_build_job = [ + item for item in processing_results if item["details"]["job"] == "copr_build" + ] + assert copr_build_job + + test_job = [ + item for item in processing_results if item["details"]["job"] == "tests" + ] + assert test_job + + event_dict, job, job_config, package_config = get_parameters_from_results(test_job) + assert json.dumps(event_dict) + results = run_testing_farm_handler( + package_config=package_config, + event=event_dict, + job_config=job_config, + ) + assert first_dict_value(results["job"])["success"] + + def test_pr_comment_production_build_handler(pr_production_build_comment_event): packit_yaml = str( { diff --git a/tests/unit/test_testing_farm.py b/tests/unit/test_testing_farm.py index 7be2c9507..44cb5cb1b 100644 --- a/tests/unit/test_testing_farm.py +++ b/tests/unit/test_testing_farm.py @@ -8,8 +8,6 @@ from celery.canvas import Signature from flexmock import flexmock -from packit.config.commands import TestCommandConfig - import packit_service.models import packit_service.service.urls as urls from packit.config import ( @@ -96,6 +94,7 @@ def test_testing_farm_response( ), JobConfig( type=JobType.tests, + manual_trigger=True, trigger=JobConfigTriggerType.pull_request, packages={ "package": CommonPackageConfig( @@ -108,7 +107,9 @@ def test_testing_farm_response( flexmock(PackageConfigGetter).should_receive( "get_package_config_from_repo" ).and_return(package_config) - config = flexmock(command_handler_work_dir=flexmock()) + config = flexmock( + command_handler_work_dir=flexmock(), comment_command_prefix="/packit" + ) flexmock(TFResultsHandler).should_receive("service_config").and_return(config) flexmock(TFResultsEvent).should_receive("db_project_object").and_return(None) config.should_receive("get_project").with_args( @@ -139,6 +140,7 @@ def test_testing_farm_response( job_config=JobConfig( type=JobType.tests, trigger=JobConfigTriggerType.pull_request, + manual_trigger=True, packages={ "package": CommonPackageConfig( identifier=None, @@ -1896,145 +1898,3 @@ def test_parse_comment_arguments( assert helper.comment_arguments.pr_argument == expected_pr_arg assert helper.comment_arguments.identifier == expected_identifier assert helper.comment_arguments.labels == expected_labels - - -@pytest.mark.parametrize( - "comment,job_identifier,job_labels,manual_trigger,test_command,show_status_check_times", - [ - ( - "/packit-dev test --identifier my-id-1 --labels label1,label2", - "my-id-1", - [], - True, - None, - 1, - ), - ( - "/packit-dev test --identifier my-id-1 --labels label1,label2", - "my-id-2", - [ - "label1", - ], - True, - None, - 1, - ), - ( - "/packit-dev test --identifier my-id-2 --labels label1,label2", - "my-id-1", - [ - "label3", - ], - True, - None, - 0, - ), - ( - "/packit-dev test --identifier my-id-2 --labels label1,label2", - "my-id-1", - [ - "label3", - ], - False, - None, - 1, - ), - ( - "/packit-dev test", - "my-id-2", - [ - "label1", - ], - True, - None, - 0, - ), - ( - "/packit-dev test", - "my-id-2", - [ - "label1", - ], - False, - None, - 1, - ), - ( - "/packit-dev test --identifier my-id-1 --labels label1,label2", - "my-id-1", - [], - False, - None, - 1, - ), - ( - "/packit-dev test --identifier my-id-1 --labels label1,label2", - "my-id-2", - [ - "label1", - ], - False, - None, - 1, - ), - ( - "/packit-dev test", - "my-id-2", - [ - "label1", - ], - True, - TestCommandConfig(default_labels=["label5", "label1"]), - 1, - ), - ( - "/packit-dev test", - "my-id-2", - [ - "label1", - ], - True, - TestCommandConfig(default_identifier="my-id-2"), - 1, - ), - ], -) -def test_manually_triggered_tests_statuses_report( - comment: str, - job_identifier: str, - job_labels: List[str], - manual_trigger, - test_command, - show_status_check_times, -): - job_config = JobConfig( - trigger=JobConfigTriggerType.pull_request, - type=JobType.tests, - manual_trigger=manual_trigger, - labels=job_labels, - packages={ - "package": CommonPackageConfig( - test_command=test_command, - identifier=job_identifier, - _targets=["test-target", "another-test-target"], - ) - }, - ) - metadata = flexmock(event_dict={"comment": comment}) - - git_project = flexmock() - - helper = TFJobHelper( - service_config=flexmock(comment_command_prefix="/packit-dev"), - package_config=flexmock(jobs=[]), - project=git_project, - metadata=metadata, - db_project_event=flexmock() - .should_receive("get_project_event_object") - .and_return(flexmock(job_config_trigger_type=JobConfigTriggerType.pull_request)) - .mock(), - job_config=job_config, - ) - - flexmock(helper).should_receive("_report").times(show_status_check_times) - helper.report_status_to_tests("a description", "a new status")