From 625919a0c8ff0e7d16ffeb8befa4fb095e56025d Mon Sep 17 00:00:00 2001 From: primetheus <865381+primetheus@users.noreply.github.com> Date: Fri, 20 Mar 2026 13:07:17 -0400 Subject: [PATCH 1/2] fix: test failures, lint errors, and formatting issues - Fix webhook handler tests posting to / instead of /webhooks/github/ - Remove redundant init_app() calls (constructor already calls it) - Remove unused imports (json, APIRouter, Request, mock_open) - Remove unused local variables (result, client, github_app) - Fix type comparison using isinstance() instead of type() == - Run black formatter on core.py - Run ruff autofix for import sorting --- src/githubapp/core.py | 1 - tests/test_core.py | 73 ++++++++++++++++++------------------- tests/test_oauth.py | 1 - tests/test_rate_limiting.py | 4 +- 4 files changed, 38 insertions(+), 41 deletions(-) diff --git a/src/githubapp/core.py b/src/githubapp/core.py index 754ef80..34517e3 100644 --- a/src/githubapp/core.py +++ b/src/githubapp/core.py @@ -16,7 +16,6 @@ from .session import SessionManager from contextlib import asynccontextmanager - LOG = logging.getLogger(__name__) STATUS_FUNC_CALLED = "HIT" diff --git a/tests/test_core.py b/tests/test_core.py index 6d96d02..036ea5f 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1,8 +1,7 @@ import pytest -import json import time -from unittest.mock import patch, Mock, mock_open -from fastapi import APIRouter, Request, FastAPI +from unittest.mock import patch, Mock +from fastapi import FastAPI from fastapi.testclient import TestClient from githubapp import GitHubApp from githubapp.core import ( @@ -352,7 +351,7 @@ def test_client_with_payload_installation(self, mock_ghapi, mock_get_token): github_app = GitHubApp() github_app.payload = {"installation": {"id": 456}} - result = github_app.client() + github_app.client() mock_get_token.assert_called_once_with(456) @@ -363,27 +362,25 @@ def test_extract_payload_valid_json(self): github_app = GitHubApp(app) github_app.init_app(app) - with TestClient(app) as client: + with TestClient(app): # simplified; no real assertion here pass def test_handle_request_missing_content_type(self): app = FastAPI() - github_app = GitHubApp(app) - github_app.init_app(app) + GitHubApp(app) with TestClient(app) as client: - response = client.post("/", json={"test": "data"}) + response = client.post("/webhooks/github/", json={"test": "data"}) assert response.status_code == 400 def test_handle_request_missing_github_event_header(self): app = FastAPI() - github_app = GitHubApp(app) - github_app.init_app(app) + GitHubApp(app) with TestClient(app) as client: response = client.post( - "/", + "/webhooks/github/", json={"installation": {"id": 123}}, headers={"Content-Type": "application/json"}, ) @@ -395,9 +392,8 @@ def test_handle_request_valid_webhook(self): app, github_app_id=123, github_app_key=b"test_key", - github_app_secret=False, # Disable signature verification for testing + github_app_secret=False, ) - github_app.init_app(app) @github_app.on("issues.opened") def test_handler(): @@ -405,7 +401,7 @@ def test_handler(): with TestClient(app) as client: response = client.post( - "/", + "/webhooks/github/", json={ "action": "opened", "installation": {"id": 123}, @@ -427,9 +423,8 @@ def test_handle_request_call_async_hook_function(self): app, github_app_id=123, github_app_key=b"test_key", - github_app_secret=False, # Disable signature verification for testing + github_app_secret=False, ) - github_app.init_app(app) @github_app.on("issues.opened") async def async_test_handler(): @@ -437,7 +432,7 @@ async def async_test_handler(): with TestClient(app) as client: response = client.post( - "/", + "/webhooks/github/", json={ "action": "opened", "installation": {"id": 123}, @@ -457,25 +452,23 @@ async def async_test_handler(): class TestGitHubAppWebhookSignatureVerification: def test_signature_verification_disabled(self): app = FastAPI() - github_app = GitHubApp( - app, github_app_secret=False # Explicitly disable verification - ) - # Test that webhooks work without signature headers when verification is disabled + GitHubApp(app, github_app_secret=False) + # Webhooks work without signature headers when verification is disabled def test_signature_verification_sha256_valid(self): app = FastAPI() - github_app = GitHubApp(app, github_app_secret=b"test_secret") - # Test that valid SHA256 signatures are accepted + GitHubApp(app, github_app_secret=b"test_secret") + # Valid SHA256 signatures are accepted def test_signature_verification_sha256_invalid(self): app = FastAPI() - github_app = GitHubApp(app, github_app_secret=b"test_secret") - # Test that invalid SHA256 signatures are rejected + GitHubApp(app, github_app_secret=b"test_secret") + # Invalid SHA256 signatures are rejected def test_signature_verification_sha1_fallback(self): app = FastAPI() - github_app = GitHubApp(app, github_app_secret=b"test_secret") - # Test that SHA1 signatures work when SHA256 is not present + GitHubApp(app, github_app_secret=b"test_secret") + # SHA1 signatures work when SHA256 is not present class TestGitHubAppIntegration: @@ -483,9 +476,11 @@ def test_full_webhook_flow(self): """Test complete webhook handling flow""" app = FastAPI() github_app = GitHubApp( - app, github_app_id=123, github_app_key=b"test_key", github_app_secret=False + app, + github_app_id=123, + github_app_key=b"test_key", + github_app_secret=False, ) - github_app.init_app(app) results = [] @@ -501,7 +496,7 @@ def handle_opened_issue(): with TestClient(app) as client: response = client.post( - "/", + "/webhooks/github/", json={ "action": "opened", "installation": {"id": 123}, @@ -523,14 +518,16 @@ def handle_opened_issue(): def test_no_matching_handlers(self): """Test webhook with no matching handlers""" app = FastAPI() - github_app = GitHubApp( - app, github_app_id=123, github_app_key=b"test_key", github_app_secret=False + GitHubApp( + app, + github_app_id=123, + github_app_key=b"test_key", + github_app_secret=False, ) - github_app.init_app(app) with TestClient(app) as client: response = client.post( - "/", + "/webhooks/github/", json={ "action": "closed", "installation": {"id": 123}, @@ -551,9 +548,11 @@ def test_handler_exception_returns_500(self): """Test that exceptions in handlers return 500""" app = FastAPI() github_app = GitHubApp( - app, github_app_id=123, github_app_key=b"test_key", github_app_secret=False + app, + github_app_id=123, + github_app_key=b"test_key", + github_app_secret=False, ) - github_app.init_app(app) @github_app.on("issues.opened") def failing_handler(): @@ -561,7 +560,7 @@ def failing_handler(): with TestClient(app) as client: response = client.post( - "/", + "/webhooks/github/", json={ "action": "opened", "installation": {"id": 123}, diff --git a/tests/test_oauth.py b/tests/test_oauth.py index ac4a582..24bd8af 100644 --- a/tests/test_oauth.py +++ b/tests/test_oauth.py @@ -1,4 +1,3 @@ -import json from urllib.parse import urlparse, parse_qs import pytest diff --git a/tests/test_rate_limiting.py b/tests/test_rate_limiting.py index f07e248..0e69819 100644 --- a/tests/test_rate_limiting.py +++ b/tests/test_rate_limiting.py @@ -1,5 +1,5 @@ import pytest -from unittest.mock import Mock, patch, MagicMock +from unittest.mock import Mock, patch import time from githubapp import GitHubApp, with_rate_limit_handling @@ -133,7 +133,7 @@ def test_get_client_alias(self): client2 = self.app.get_client(123) # Both should return the same type of object - assert type(client1) == type(client2) + assert isinstance(client1, type(client2)) def test_decorator_restores_original_methods(self): """Test that the decorator properly restores original methods after execution.""" From fa26d85ae31f1eccd456abaf4cf1a170da33f1df Mon Sep 17 00:00:00 2001 From: primetheus <865381+primetheus@users.noreply.github.com> Date: Fri, 20 Mar 2026 13:19:10 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20remove=20empty=20tests,=20use=20=5F=5Fclass=5F=5F?= =?UTF-8?q?=20is=20for=20type=20comparison?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove empty signature verification tests (no assertions, always pass) - Remove empty extract_payload test (no assertions, always pass) - Use client1.__class__ is client2.__class__ for exact type comparison --- tests/test_core.py | 31 ------------------------------- tests/test_rate_limiting.py | 2 +- 2 files changed, 1 insertion(+), 32 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 036ea5f..5b53e24 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -357,15 +357,6 @@ def test_client_with_payload_installation(self, mock_ghapi, mock_get_token): class TestGitHubAppWebhookHandling: - def test_extract_payload_valid_json(self): - app = FastAPI() - github_app = GitHubApp(app) - github_app.init_app(app) - - with TestClient(app): - # simplified; no real assertion here - pass - def test_handle_request_missing_content_type(self): app = FastAPI() GitHubApp(app) @@ -449,28 +440,6 @@ async def async_test_handler(): assert "async_test_handler" in data["calls"] -class TestGitHubAppWebhookSignatureVerification: - def test_signature_verification_disabled(self): - app = FastAPI() - GitHubApp(app, github_app_secret=False) - # Webhooks work without signature headers when verification is disabled - - def test_signature_verification_sha256_valid(self): - app = FastAPI() - GitHubApp(app, github_app_secret=b"test_secret") - # Valid SHA256 signatures are accepted - - def test_signature_verification_sha256_invalid(self): - app = FastAPI() - GitHubApp(app, github_app_secret=b"test_secret") - # Invalid SHA256 signatures are rejected - - def test_signature_verification_sha1_fallback(self): - app = FastAPI() - GitHubApp(app, github_app_secret=b"test_secret") - # SHA1 signatures work when SHA256 is not present - - class TestGitHubAppIntegration: def test_full_webhook_flow(self): """Test complete webhook handling flow""" diff --git a/tests/test_rate_limiting.py b/tests/test_rate_limiting.py index 0e69819..788bbca 100644 --- a/tests/test_rate_limiting.py +++ b/tests/test_rate_limiting.py @@ -133,7 +133,7 @@ def test_get_client_alias(self): client2 = self.app.get_client(123) # Both should return the same type of object - assert isinstance(client1, type(client2)) + assert client1.__class__ is client2.__class__ def test_decorator_restores_original_methods(self): """Test that the decorator properly restores original methods after execution."""