From 69051cb5af1ea86c1d24505c6663931b9d73edfc Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Sat, 10 Jan 2026 02:55:09 +0200 Subject: [PATCH 1/4] fix(welcome-msg): conditional display of Container Operations and Pre-commit sections Container Operations section now only appears when container config is enabled. Pre-commit Checks line now only appears when pre-commit is enabled. This ensures the welcome message matches the repository's actual capabilities. Fixes #972 --- .../libs/handlers/pull_request_handler.py | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 1776e906..a1b43c35 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -252,7 +252,7 @@ def _prepare_welcome_comment(self) -> str: * **Reviewer Assignment**: Reviewers are automatically assigned based on the OWNERS file in the repository root * **Size Labeling**: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes {issue_creation_note} -* **Pre-commit Checks**: [pre-commit](https://pre-commit.ci/) runs automatically if `.pre-commit-config.yaml` exists +{self._prepare_pre_commit_welcome_line}\ * **Branch Labeling**: Branch-specific labels are applied to track the target branch * **Auto-verification**: Auto-verified users have their PRs automatically marked as verified @@ -277,12 +277,7 @@ def _prepare_welcome_comment(self) -> str: #### Testing & Validation {self._prepare_retest_welcome_comment} - -#### Container Operations -* `/build-and-push-container` - Build and push container image (tagged with PR number) - * Supports additional build arguments: `/build-and-push-container --build-arg KEY=value` - -#### Cherry-pick Operations +{self._prepare_container_operations_welcome_section}#### Cherry-pick Operations * `/cherry-pick ` - Schedule cherry-pick to target branch when PR is merged * Multiple branches: `/cherry-pick branch1 branch2 branch3` @@ -366,6 +361,26 @@ def _prepare_retest_welcome_comment(self) -> str: return " * No retest actions are configured for this repository" if not retest_msg else retest_msg + @property + def _prepare_pre_commit_welcome_line(self) -> str: + if self.github_webhook.pre_commit: + return ( + "* **Pre-commit Checks**: [pre-commit](https://pre-commit.ci/) runs automatically " + "if `.pre-commit-config.yaml` exists\n" + ) + return "" + + @property + def _prepare_container_operations_welcome_section(self) -> str: + if self.github_webhook.build_and_push_container: + return """ +#### Container Operations +* `/build-and-push-container` - Build and push container image (tagged with PR number) + * Supports additional build arguments: `/build-and-push-container --build-arg KEY=value` + +""" + return "" + async def label_all_opened_pull_requests_merge_state_after_merged(self) -> None: """ Labels pull requests based on their mergeable state. From 24244fb7a0efc2e8878b7f8c436dd2ac21fae258 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Sun, 11 Jan 2026 09:48:07 +0200 Subject: [PATCH 2/4] fix(welcome-msg): improve container section formatting and add tests - Remove leading newline from container operations section - Add regression tests for welcome message newline structure - Add tests for format_duration to improve coverage to 90% Addresses CodeRabbit review feedback --- .../libs/handlers/pull_request_handler.py | 7 +- webhook_server/tests/test_app_utils.py | 44 +++- .../test_prepare_retest_welcome_comment.py | 223 ++++++++++++++++++ 3 files changed, 270 insertions(+), 4 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 7824301b..65880da2 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -277,7 +277,8 @@ def _prepare_welcome_comment(self) -> str: #### Testing & Validation {self._prepare_retest_welcome_comment} -{self._prepare_container_operations_welcome_section}#### Cherry-pick Operations +{self._prepare_container_operations_welcome_section}\ +#### Cherry-pick Operations * `/cherry-pick ` - Schedule cherry-pick to target branch when PR is merged * Multiple branches: `/cherry-pick branch1 branch2 branch3` @@ -379,7 +380,7 @@ def _prepare_container_operations_welcome_section(self) -> str: * Supports additional build arguments: `/build-and-push-container --build-arg KEY=value` """ - return "" + return "\n" async def label_all_opened_pull_requests_merge_state_after_merged(self) -> None: """ @@ -769,7 +770,7 @@ async def _compare_branches(self, base_ref: str, head_ref_full: str) -> dict[str NOTE: This API does NOT return conflict information (mergeable/mergeable_state). """ try: - _headers, data = await asyncio.to_thread( + _, data = await asyncio.to_thread( self.repository._requester.requestJsonAndCheck, "GET", f"{self.repository.url}/compare/{base_ref}...{head_ref_full}", diff --git a/webhook_server/tests/test_app_utils.py b/webhook_server/tests/test_app_utils.py index 7a169d59..deb5185b 100644 --- a/webhook_server/tests/test_app_utils.py +++ b/webhook_server/tests/test_app_utils.py @@ -7,7 +7,7 @@ import pytest from fastapi import HTTPException -from webhook_server.utils.app_utils import parse_datetime_string, verify_signature +from webhook_server.utils.app_utils import format_duration, parse_datetime_string, verify_signature class TestVerifySignature: @@ -90,3 +90,45 @@ def test_parse_datetime_string_empty_string(self) -> None: result = parse_datetime_string(datetime_str, "test_field") # Empty string is falsy, so it returns None (same as None input) assert result is None + + +class TestFormatDuration: + """Test suite for format_duration function.""" + + def test_format_duration_milliseconds_only(self) -> None: + """Test format_duration with less than 1 second.""" + assert format_duration(500) == "500ms" + assert format_duration(0) == "0ms" + assert format_duration(999) == "999ms" + + def test_format_duration_seconds_only(self) -> None: + """Test format_duration with exact seconds (no remaining ms).""" + assert format_duration(1000) == "1s" + assert format_duration(5000) == "5s" + assert format_duration(59000) == "59s" + + def test_format_duration_seconds_with_milliseconds(self) -> None: + """Test format_duration with seconds and remaining milliseconds.""" + assert format_duration(1500) == "1s500ms" + assert format_duration(5123) == "5s123ms" + + def test_format_duration_minutes_only(self) -> None: + """Test format_duration with exact minutes (no remaining seconds).""" + assert format_duration(60000) == "1m" + assert format_duration(120000) == "2m" + assert format_duration(3540000) == "59m" + + def test_format_duration_minutes_with_seconds(self) -> None: + """Test format_duration with minutes and remaining seconds.""" + assert format_duration(65000) == "1m5s" + assert format_duration(125000) == "2m5s" + + def test_format_duration_hours_only(self) -> None: + """Test format_duration with exact hours (no remaining minutes).""" + assert format_duration(3600000) == "1h" + assert format_duration(7200000) == "2h" + + def test_format_duration_hours_with_minutes(self) -> None: + """Test format_duration with hours and remaining minutes.""" + assert format_duration(3660000) == "1h1m" + assert format_duration(5700000) == "1h35m" diff --git a/webhook_server/tests/test_prepare_retest_welcome_comment.py b/webhook_server/tests/test_prepare_retest_welcome_comment.py index 6f0a7934..62f465db 100644 --- a/webhook_server/tests/test_prepare_retest_welcome_comment.py +++ b/webhook_server/tests/test_prepare_retest_welcome_comment.py @@ -1,3 +1,5 @@ +import re + import pytest from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler @@ -95,3 +97,224 @@ def test_prepare_retest_welcome_comment( ) assert pull_request_handler._prepare_retest_welcome_comment == expected + + +class TestWelcomeMessageNewlineStructure: + """Regression tests for welcome message markdown formatting. + + These tests ensure that conditional sections in the welcome message + maintain proper newline structure to prevent headers from being + glued to previous content. + """ + + def _create_handler(self, process_github_webhook, owners_file_handler, **config): + """Create a PullRequestHandler with the specified configuration.""" + # Set default values for all config options + defaults = { + "tox": False, + "build_and_push_container": False, + "pypi": False, + "pre_commit": False, + "conventional_title": False, + "parent_committer": "test-user", + "auto_verified_and_merged_users": [], + "create_issue_for_new_pr": False, + "issue_url_for_welcome_msg": "https://github.com/test/repo/issues/1", + "minimum_lgtm": 1, + "pull_request": None, + } + defaults.update(config) + + for key, value in defaults.items(): + setattr(process_github_webhook, key, value) + + # Mock the owners file handler attributes needed for welcome message generation + owners_file_handler.all_pull_request_approvers = ["approver1", "approver2"] + owners_file_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"] + + return PullRequestHandler(github_webhook=process_github_webhook, owners_file_handler=owners_file_handler) + + def test_container_section_enabled_has_header_on_own_line(self, process_github_webhook, owners_file_handler): + """When container operations are enabled, the header must start on its own line.""" + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=True, + tox=True, + ) + + welcome_msg = handler._prepare_welcome_comment() + + # Container Operations header should be on its own line (preceded by newline) + assert "\n#### Container Operations\n" in welcome_msg, ( + "#### Container Operations header should be on its own line" + ) + + def test_cherry_pick_section_has_header_on_own_line_when_container_enabled( + self, process_github_webhook, owners_file_handler + ): + """When container operations are enabled, Cherry-pick header must still be on its own line.""" + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=True, + tox=True, + ) + + welcome_msg = handler._prepare_welcome_comment() + + # Cherry-pick Operations header should be on its own line + assert "\n#### Cherry-pick Operations\n" in welcome_msg, ( + "#### Cherry-pick Operations header should be on its own line" + ) + + def test_cherry_pick_section_has_header_on_own_line_when_container_disabled( + self, process_github_webhook, owners_file_handler + ): + """When container operations are disabled, Cherry-pick header must still be on its own line. + + This is a critical regression test - when the container section is disabled, + the Cherry-pick header should not be glued to the previous section. + """ + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=False, + tox=True, + ) + + welcome_msg = handler._prepare_welcome_comment() + + # Cherry-pick Operations header should be on its own line, not glued to previous content + assert "\n#### Cherry-pick Operations\n" in welcome_msg, ( + "#### Cherry-pick Operations header should be on its own line even when container section is disabled" + ) + # Ensure it's not glued to retest section content + assert "all`#### Cherry-pick" not in welcome_msg, "Cherry-pick header should not be glued to retest section" + + def test_no_excessive_blank_lines_between_sections(self, process_github_webhook, owners_file_handler): + """Sections should not have more than 2 consecutive blank lines.""" + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=True, + tox=True, + pre_commit=True, + ) + + welcome_msg = handler._prepare_welcome_comment() + + # Check for triple or more blank lines (more than 2 consecutive newlines with only whitespace) + triple_blank_pattern = re.compile(r"\n\s*\n\s*\n\s*\n") + match = triple_blank_pattern.search(welcome_msg) + assert match is None, ( + f"Found excessive blank lines in welcome message at position {match.start() if match else 'N/A'}" + ) + + def test_all_level_4_headers_on_own_lines(self, process_github_webhook, owners_file_handler): + """All #### headers in the welcome message should start on their own line.""" + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=True, + tox=True, + pre_commit=True, + conventional_title=True, + ) + + welcome_msg = handler._prepare_welcome_comment() + + # Find all #### headers + header_pattern = re.compile(r"#### [A-Za-z]") + headers = list(header_pattern.finditer(welcome_msg)) + + for match in headers: + pos = match.start() + # Header should be at start of message or preceded by newline + if pos > 0: + char_before = welcome_msg[pos - 1] + assert char_before == "\n", ( + f"Header '{match.group()}' at position {pos} is not on its own line. " + f"Preceded by: '{repr(welcome_msg[max(0, pos - 20) : pos])}'" + ) + + @pytest.mark.parametrize( + "build_and_push_container,tox,expected_container_section", + [ + pytest.param(True, True, True, id="container_enabled"), + pytest.param(False, True, False, id="container_disabled"), + pytest.param(True, False, True, id="container_enabled_no_tox"), + pytest.param(False, False, False, id="all_disabled"), + ], + ) + def test_section_presence_matches_config( + self, + process_github_webhook, + owners_file_handler, + build_and_push_container, + tox, + expected_container_section, + ): + """Container section should only appear when build_and_push_container is enabled.""" + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=build_and_push_container, + tox=tox, + ) + + welcome_msg = handler._prepare_welcome_comment() + + container_section_present = "#### Container Operations" in welcome_msg + assert container_section_present == expected_container_section, ( + f"Container section presence ({container_section_present}) does not match " + f"expected ({expected_container_section}) for build_and_push_container={build_and_push_container}" + ) + + # Cherry-pick section should always be present + assert "#### Cherry-pick Operations" in welcome_msg, "Cherry-pick Operations section should always be present" + + def test_testing_validation_section_structure(self, process_github_webhook, owners_file_handler): + """The Testing & Validation section should have proper structure.""" + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=True, + tox=True, + ) + + welcome_msg = handler._prepare_welcome_comment() + + # Find the Testing & Validation section + assert "#### Testing & Validation" in welcome_msg + + # The order should be: Testing & Validation -> (retest content) -> Container Operations -> Cherry-pick + testing_pos = welcome_msg.find("#### Testing & Validation") + container_pos = welcome_msg.find("#### Container Operations") + cherry_pick_pos = welcome_msg.find("#### Cherry-pick Operations") + + assert testing_pos < container_pos < cherry_pick_pos, ( + "Sections should be in order: Testing & Validation -> Container Operations -> Cherry-pick" + ) + + def test_retest_content_between_testing_header_and_container_section( + self, process_github_webhook, owners_file_handler + ): + """Retest commands should appear between Testing & Validation header and Container section.""" + handler = self._create_handler( + process_github_webhook, + owners_file_handler, + build_and_push_container=True, + tox=True, + ) + + welcome_msg = handler._prepare_welcome_comment() + + testing_pos = welcome_msg.find("#### Testing & Validation") + container_pos = welcome_msg.find("#### Container Operations") + + # Get content between the two headers + section_between = welcome_msg[testing_pos:container_pos] + + # Should contain retest commands + assert "/retest tox" in section_between, "Retest tox command should be in Testing & Validation section" From ca0ffa27f6ef6b1741bb194e7028c208aa59eb68 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Sun, 11 Jan 2026 10:24:10 +0200 Subject: [PATCH 3/4] refactor(tests): add type hints to _create_handler test helper - Add type annotations for process_github_webhook, owners_file_handler, and config params - Use TYPE_CHECKING block for type-only imports - Add PEP 563 postponed evaluation with __future__ annotations Addresses CodeRabbit review feedback --- .../tests/test_prepare_retest_welcome_comment.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/webhook_server/tests/test_prepare_retest_welcome_comment.py b/webhook_server/tests/test_prepare_retest_welcome_comment.py index 62f465db..b4c8865f 100644 --- a/webhook_server/tests/test_prepare_retest_welcome_comment.py +++ b/webhook_server/tests/test_prepare_retest_welcome_comment.py @@ -1,9 +1,16 @@ +from __future__ import annotations + import re +from typing import TYPE_CHECKING, Any import pytest from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler +if TYPE_CHECKING: + from webhook_server.libs.github_api import GithubWebhook + from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler + class TestPrepareRetestWelcomeMsg: @pytest.mark.parametrize( @@ -107,10 +114,12 @@ class TestWelcomeMessageNewlineStructure: glued to previous content. """ - def _create_handler(self, process_github_webhook, owners_file_handler, **config): + def _create_handler( + self, process_github_webhook: GithubWebhook, owners_file_handler: OwnersFileHandler, **config: Any + ) -> PullRequestHandler: """Create a PullRequestHandler with the specified configuration.""" # Set default values for all config options - defaults = { + defaults: dict[str, Any] = { "tox": False, "build_and_push_container": False, "pypi": False, From f5516bd0489c75e3c88bab37c57c502b8a3bbd42 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Sun, 11 Jan 2026 10:39:14 +0200 Subject: [PATCH 4/4] refactor(tests): improve type safety and code style in welcome comment tests - Define HandlerConfig TypedDict for _create_handler kwargs - Use Unpack for type-safe **kwargs handling - Replace repr() with f-string !r conversion (RUF010) - Use tuple format in pytest.mark.parametrize (PT006) Addresses CodeRabbit review feedback --- .../test_prepare_retest_welcome_comment.py | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/webhook_server/tests/test_prepare_retest_welcome_comment.py b/webhook_server/tests/test_prepare_retest_welcome_comment.py index b4c8865f..c47a9da7 100644 --- a/webhook_server/tests/test_prepare_retest_welcome_comment.py +++ b/webhook_server/tests/test_prepare_retest_welcome_comment.py @@ -1,17 +1,35 @@ from __future__ import annotations import re -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, TypedDict, Unpack import pytest from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler if TYPE_CHECKING: + from github.PullRequest import PullRequest + from webhook_server.libs.github_api import GithubWebhook from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler +class HandlerConfig(TypedDict, total=False): + """Configuration options for _create_handler.""" + + tox: bool + build_and_push_container: bool + pypi: bool + pre_commit: bool + conventional_title: bool + parent_committer: str + auto_verified_and_merged_users: list[str] + create_issue_for_new_pr: bool + issue_url_for_welcome_msg: str + minimum_lgtm: int + pull_request: PullRequest | None + + class TestPrepareRetestWelcomeMsg: @pytest.mark.parametrize( "tox, build_and_push_container, pypi, pre_commit, conventional_title, expected", @@ -115,11 +133,14 @@ class TestWelcomeMessageNewlineStructure: """ def _create_handler( - self, process_github_webhook: GithubWebhook, owners_file_handler: OwnersFileHandler, **config: Any + self, + process_github_webhook: GithubWebhook, + owners_file_handler: OwnersFileHandler, + **config: Unpack[HandlerConfig], ) -> PullRequestHandler: """Create a PullRequestHandler with the specified configuration.""" # Set default values for all config options - defaults: dict[str, Any] = { + defaults: HandlerConfig = { "tox": False, "build_and_push_container": False, "pypi": False, @@ -244,11 +265,11 @@ def test_all_level_4_headers_on_own_lines(self, process_github_webhook, owners_f char_before = welcome_msg[pos - 1] assert char_before == "\n", ( f"Header '{match.group()}' at position {pos} is not on its own line. " - f"Preceded by: '{repr(welcome_msg[max(0, pos - 20) : pos])}'" + f"Preceded by: {welcome_msg[max(0, pos - 20) : pos]!r}" ) @pytest.mark.parametrize( - "build_and_push_container,tox,expected_container_section", + ("build_and_push_container", "tox", "expected_container_section"), [ pytest.param(True, True, True, id="container_enabled"), pytest.param(False, True, False, id="container_disabled"),