From 8a35dbf2ac047ee01a145cfbcd90fdc0a6af87be Mon Sep 17 00:00:00 2001 From: Hiroshi Nishio Date: Mon, 23 Feb 2026 16:44:47 -0800 Subject: [PATCH] Fix all-None coverage treated as 100% in schedule handler Files never measured by the coverage tool (all three metrics None) were incorrectly skipped as "fully covered". Now only files with at least one actual 100% metric and the rest None/100% are skipped. --- schemas/supabase/types.py | 56 +-------- .../users/test_get_user_public_email.py | 108 +++++++++++++++++- services/webhook/schedule_handler.py | 15 ++- services/webhook/test_new_pr_handler.py | 8 +- services/webhook/test_schedule_handler.py | 102 +++++++++++++++++ 5 files changed, 225 insertions(+), 64 deletions(-) diff --git a/schemas/supabase/types.py b/schemas/supabase/types.py index e6617fa5..3051a19e 100644 --- a/schemas/supabase/types.py +++ b/schemas/supabase/types.py @@ -197,35 +197,6 @@ class InstallationsInsert(TypedDict): uninstalled_by: NotRequired[str | None] -class JiraGithubLinks(TypedDict): - id: int - jira_site_id: str - jira_site_name: str - jira_project_id: int - jira_project_name: str - github_owner_id: int - github_owner_name: str - github_repo_id: int - github_repo_name: str - created_at: datetime.datetime | None - updated_at: datetime.datetime | None - created_by: int - updated_by: int | None - - -class JiraGithubLinksInsert(TypedDict): - jira_site_id: str - jira_site_name: str - jira_project_id: int - jira_project_name: str - github_owner_id: int - github_owner_name: str - github_repo_id: int - github_repo_name: str - created_by: int - updated_by: NotRequired[int | None] - - class LlmRequests(TypedDict): id: int usage_id: int | None @@ -284,31 +255,6 @@ class NpmTokensInsert(TypedDict): updated_by: str -class OauthTokens(TypedDict): - id: int - user_id: int - service_name: str - access_token: str - refresh_token: str | None - scope: str - expires_at: datetime.datetime - created_at: datetime.datetime - updated_at: datetime.datetime - created_by: int - updated_by: int | None - - -class OauthTokensInsert(TypedDict): - user_id: int - service_name: str - access_token: str - refresh_token: NotRequired[str | None] - scope: str - expires_at: datetime.datetime - created_by: int - updated_by: NotRequired[int | None] - - class Owners(TypedDict): created_at: datetime.datetime owner_id: int @@ -603,6 +549,7 @@ class Users(TypedDict): created_by: str | None user_rules: str display_name: str + deleted_at: datetime.datetime | None class UsersInsert(TypedDict): @@ -612,6 +559,7 @@ class UsersInsert(TypedDict): created_by: NotRequired[str | None] user_rules: str display_name: str + deleted_at: NotRequired[datetime.datetime | None] class WebhookDeliveries(TypedDict): diff --git a/services/github/users/test_get_user_public_email.py b/services/github/users/test_get_user_public_email.py index 88636b43..82172f1d 100644 --- a/services/github/users/test_get_user_public_email.py +++ b/services/github/users/test_get_user_public_email.py @@ -409,8 +409,10 @@ def test_get_user_public_info_title_cases_uppercase_name(sample_username, sample assert result.display_name == "Hiroshi" -def test_get_user_public_info_preserves_mixed_case_name(sample_username, sample_token): - """Test that mixed-case names are preserved (e.g., 'Wes Nishio' stays 'Wes Nishio').""" +def test_get_user_public_info_title_cases_already_correct_name( + sample_username, sample_token +): + """Test that already title-cased names stay the same (e.g., 'Wes Nishio' stays 'Wes Nishio').""" mock_response = MagicMock() mock_response.raise_for_status.return_value = None mock_response.json.return_value = { @@ -429,6 +431,108 @@ def test_get_user_public_info_preserves_mixed_case_name(sample_username, sample_ assert result.display_name == "Wes Nishio" +def test_get_user_public_info_title_cases_mixed_case_name( + sample_username, sample_token +): + """Test that mixed-case names like 'Naman joshi' are title-cased to 'Naman Joshi'.""" + mock_response = MagicMock() + mock_response.raise_for_status.return_value = None + mock_response.json.return_value = { + "email": "naman@example.com", + "name": "Naman joshi", + } + + with patch( + "services.github.users.get_user_public_email.requests.get", + return_value=mock_response, + ): + result = get_user_public_info( + username=sample_username, + token=sample_token, + ) + assert result.display_name == "Naman Joshi" + + +def test_get_user_public_info_strips_unicode_bold(sample_username, sample_token): + """Test that Unicode bold characters are normalized to plain ASCII and title-cased.""" + mock_response = MagicMock() + mock_response.raise_for_status.return_value = None + mock_response.json.return_value = { + "email": "one@example.com", + "name": "\U0001d40e\U0001d427\U0001d41e \U0001d405\U0001d422\U0001d427\U0001d41e \U0001d412\U0001d42d\U0001d41a\U0001d42b\U0001d42c\U0001d42d\U0001d42e\U0001d41f\U0001d41f", + } + + with patch( + "services.github.users.get_user_public_email.requests.get", + return_value=mock_response, + ): + result = get_user_public_info( + username=sample_username, + token=sample_token, + ) + assert result.display_name == "One Fine Starstuff" + + +def test_get_user_public_info_replaces_dots_with_spaces(sample_username, sample_token): + """Test that dots are replaced with spaces (e.g., 'Yang.qu' -> 'Yang Qu').""" + mock_response = MagicMock() + mock_response.raise_for_status.return_value = None + mock_response.json.return_value = { + "email": "yang@example.com", + "name": "Yang.qu", + } + + with patch( + "services.github.users.get_user_public_email.requests.get", + return_value=mock_response, + ): + result = get_user_public_info( + username=sample_username, + token=sample_token, + ) + assert result.display_name == "Yang Qu" + + +def test_get_user_public_info_preserves_accented_latin(sample_username, sample_token): + """Test that accented Latin characters are preserved and title-cased.""" + mock_response = MagicMock() + mock_response.raise_for_status.return_value = None + mock_response.json.return_value = { + "email": "andre@example.com", + "name": "andré goulart", + } + + with patch( + "services.github.users.get_user_public_email.requests.get", + return_value=mock_response, + ): + result = get_user_public_info( + username=sample_username, + token=sample_token, + ) + assert result.display_name == "André Goulart" + + +def test_get_user_public_info_preserves_cjk_characters(sample_username, sample_token): + """Test that CJK characters pass through unchanged.""" + mock_response = MagicMock() + mock_response.raise_for_status.return_value = None + mock_response.json.return_value = { + "email": "user@example.com", + "name": "高森松太郎", + } + + with patch( + "services.github.users.get_user_public_email.requests.get", + return_value=mock_response, + ): + result = get_user_public_info( + username=sample_username, + token=sample_token, + ) + assert result.display_name == "高森松太郎" + + def test_get_user_public_info_empty_name_returns_empty_display_name( sample_username, sample_token ): diff --git a/services/webhook/schedule_handler.py b/services/webhook/schedule_handler.py index 0d8ccee0..22c42c00 100644 --- a/services/webhook/schedule_handler.py +++ b/services/webhook/schedule_handler.py @@ -164,8 +164,14 @@ def schedule_handler(event: EventBridgeSchedulerEvent): func, branch, ) - # None means "not measured" by the coverage tool (e.g. PHP doesn't report branch data) - all_complete = all(v is None or v == 100.0 for v in (stmt, func, branch)) + # None means "not measured" by the coverage tool (e.g. PHP doesn't report branch data). + # If ALL three are None, the file was never measured at all — treat as uncovered. + # E.g. finishp.php: stmt=None, func=None, branch=None → candidate + # If only some are None, those metrics are N/A for the language — ignore them. + # E.g. some PHP file: stmt=100, func=100, branch=None → skip (fully covered) + metrics = (stmt, func, branch) + all_none = all(v is None for v in metrics) + all_complete = not all_none and all(v is None or v == 100.0 for v in metrics) if all_complete: logger.info("Skipping %s: all 3 metrics at 100%%", item["full_path"]) elif item.get("is_excluded_from_testing"): @@ -175,11 +181,12 @@ def schedule_handler(event: EventBridgeSchedulerEvent): files_needing_tests.append(item) # Sort: prioritize untouched files (0% coverage) first, then by file_size, coverage, path + # None (unmeasured) is treated as 0 for sorting purposes files_needing_tests.sort( key=lambda x: ( - 1 if cast(float, x["statement_coverage"]) > 0 else 0, + 1 if (x["statement_coverage"] or 0) > 0 else 0, cast(int, x["file_size"]), - cast(float, x["statement_coverage"]), + x["statement_coverage"] or 0, cast(str, x["full_path"]), ) ) diff --git a/services/webhook/test_new_pr_handler.py b/services/webhook/test_new_pr_handler.py index 59422686..4f23b966 100644 --- a/services/webhook/test_new_pr_handler.py +++ b/services/webhook/test_new_pr_handler.py @@ -1605,7 +1605,7 @@ async def test_new_pr_handler_token_accumulation( "token": "github_token_123", "new_branch": "gitauto/dashboard-20250101-120000-Ab1C", "sender_email": "test@example.com", - "sender_display_name": "Test Sender", + "sender_display_name": "Test Sender", "github_urls": { "issues": "https://api.github.com/repos/test_owner/test_repo/issues", "pulls": "https://api.github.com/repos/test_owner/test_repo/pulls", @@ -1776,7 +1776,7 @@ async def test_restrict_edit_to_target_test_file_only_passed_to_chat_with_agent( "token": "github_token_123", "new_branch": "gitauto/dashboard-20250101-120000-Ab1C", "sender_email": "test@example.com", - "sender_display_name": "Test Sender", + "sender_display_name": "Test Sender", "github_urls": {}, "is_automation": False, "clone_url": "https://github.com/test_owner/test_repo.git", @@ -1932,7 +1932,7 @@ async def test_few_test_files_include_contents_in_prompt( "token": "github_token_123", "new_branch": "gitauto/dashboard-20250101-120000-Ab1C", "sender_email": "test@example.com", - "sender_display_name": "Test Sender", + "sender_display_name": "Test Sender", "github_urls": {}, "is_automation": False, "clone_url": "https://github.com/test_owner/test_repo.git", @@ -2080,7 +2080,7 @@ async def test_many_test_files_include_paths_only_in_prompt( "token": "github_token_123", "new_branch": "gitauto/dashboard-20250101-120000-Ab1C", "sender_email": "test@example.com", - "sender_display_name": "Test Sender", + "sender_display_name": "Test Sender", "github_urls": {}, "is_automation": False, "clone_url": "https://github.com/test_owner/test_repo.git", diff --git a/services/webhook/test_schedule_handler.py b/services/webhook/test_schedule_handler.py index 17f21950..478f5809 100644 --- a/services/webhook/test_schedule_handler.py +++ b/services/webhook/test_schedule_handler.py @@ -719,3 +719,105 @@ def test_schedule_handler_skips_none_coverage_as_fully_covered( # All files skipped (100% stmt + 100% func + None branch = fully covered), no PR created assert result["status"] == "skipped" + + +@patch("services.webhook.schedule_handler.add_labels") +@patch("services.webhook.schedule_handler.create_pull_request") +@patch("services.webhook.schedule_handler.create_empty_commit") +@patch("services.webhook.schedule_handler.create_remote_branch") +@patch("services.webhook.schedule_handler.get_latest_remote_commit_sha") +@patch("services.webhook.schedule_handler.generate_branch_name") +@patch("services.webhook.schedule_handler.get_open_pull_requests") +@patch("services.webhook.schedule_handler.evaluate_condition") +@patch("services.webhook.schedule_handler.should_skip_test") +@patch("services.webhook.schedule_handler.is_schedule_paused") +@patch("services.webhook.schedule_handler.get_installation_access_token") +@patch("services.webhook.schedule_handler.get_repository") +@patch("services.webhook.schedule_handler.check_availability") +@patch("services.webhook.schedule_handler.get_default_branch") +@patch("services.webhook.schedule_handler.get_file_tree") +@patch("services.webhook.schedule_handler.get_all_coverages") +@patch("services.webhook.schedule_handler.get_raw_content") +def test_schedule_handler_all_none_coverage_treated_as_candidate( + mock_get_raw_content, + mock_get_all_coverages, + mock_get_file_tree, + mock_get_default_branch, + mock_check_availability, + mock_get_repository, + mock_get_token, + mock_is_paused, + mock_should_skip_test, + mock_evaluate_condition, + mock_get_open_pull_requests, + mock_generate_branch_name, + mock_get_latest_sha, + mock_create_remote_branch, + mock_create_empty_commit, + mock_create_pr, + mock_add_labels, + mock_event, +): + """Files with all three coverage metrics as None (never measured) should be + treated as candidates, not skipped as fully covered. + E.g. web/pickup/finishp.php: stmt=None, func=None, branch=None.""" + mock_get_token.return_value = "test-token" + mock_is_paused.return_value = False + mock_get_repository.return_value = {"trigger_on_schedule": True} + mock_check_availability.return_value = { + "can_proceed": True, + "billing_type": "exception", + "requests_left": None, + "credit_balance_usd": 0, + "period_end_date": None, + "user_message": "", + "log_message": "Exception owner - unlimited access.", + } + mock_get_default_branch.return_value = ("main", None) + mock_get_file_tree.return_value = [ + {"path": "web/pickup/finishp.php", "type": "blob", "size": 500}, + ] + mock_get_all_coverages.return_value = [ + { + "id": 1, + "full_path": "web/pickup/finishp.php", + "owner_id": 123, + "repo_id": 456, + "branch_name": "main", + "created_by": "test-user", + "updated_by": "test-user", + "level": "file", + "file_size": 500, + "statement_coverage": None, + "function_coverage": None, + "branch_coverage": None, + "line_coverage": None, + "package_name": None, + "language": "php", + "uncovered_lines": None, + "uncovered_functions": None, + "uncovered_branches": None, + "created_at": "2024-01-01", + "updated_at": "2024-01-01", + "github_issue_url": None, + "is_excluded_from_testing": False, + }, + ] + mock_get_raw_content.return_value = ( + "execute();" + ) + mock_should_skip_test.return_value = False + mock_evaluate_condition.return_value = EvaluationResult(True, "has testable logic") + mock_get_open_pull_requests.return_value = [] + mock_generate_branch_name.return_value = "gitauto/schedule-20240101-120000-ABCD" + mock_get_latest_sha.return_value = "abc123" + mock_create_pr.return_value = ("https://github.com/test/repo/pull/1", 1) + + with patch("services.webhook.schedule_handler.insert_coverages"): + result = schedule_handler(mock_event) + + # File with all-None coverage should become a candidate and get a PR created + assert result["status"] == "success" + mock_create_pr.assert_called_once() + call_kwargs = mock_create_pr.call_args.kwargs + assert "web/pickup/finishp.php" in call_kwargs["title"]