From 6d6efa79dd265abd8454d384ec20d46176e34f28 Mon Sep 17 00:00:00 2001 From: Matthew Foster Walsh <15671892+mfosterw@users.noreply.github.com> Date: Mon, 19 Feb 2024 19:11:45 -0500 Subject: [PATCH] Improve test coverage --- democrasite/webiscite/tests/factories.py | 2 +- democrasite/webiscite/tests/test_drf_views.py | 34 +++++++++++-------- democrasite/webiscite/tests/test_models.py | 22 ++++++++++-- democrasite/webiscite/tests/test_webhooks.py | 19 ++++++----- democrasite/webiscite/webhooks.py | 31 +++++++++-------- 5 files changed, 68 insertions(+), 40 deletions(-) diff --git a/democrasite/webiscite/tests/factories.py b/democrasite/webiscite/tests/factories.py index b8d6e9c..84b83db 100644 --- a/democrasite/webiscite/tests/factories.py +++ b/democrasite/webiscite/tests/factories.py @@ -9,7 +9,7 @@ class PullRequestFactory(DjangoModelFactory): number = Sequence(lambda n: -n) # Use negative numbers to represent fake PRs title = Faker("text", max_nb_chars=50) author_name = Faker("user_name") - state = Faker("random_element", elements=["open", "closed"]) + state = "open" additions = Faker("random_int") deletions = Faker("random_int") sha = Faker("pystr", min_chars=40, max_chars=40) diff --git a/democrasite/webiscite/tests/test_drf_views.py b/democrasite/webiscite/tests/test_drf_views.py index 10ba2b6..b84d171 100644 --- a/democrasite/webiscite/tests/test_drf_views.py +++ b/democrasite/webiscite/tests/test_drf_views.py @@ -1,34 +1,40 @@ +import pytest from django.utils.timezone import get_current_timezone from rest_framework.test import APIRequestFactory +from democrasite.webiscite.api.serializers import PullRequestSerializer +from democrasite.webiscite.tests.factories import PullRequestFactory + from ..api.views import BillViewSet, PullRequestViewSet -from ..models import Bill, PullRequest +from ..models import Bill + + +class TestPullRequestSerializer: + def test_serializer_read_only(self): + serializer = PullRequestSerializer(PullRequestFactory()) + with pytest.raises(NotImplementedError): + serializer.create({}) + with pytest.raises(NotImplementedError): + assert serializer.update({}, {}) class TestPullRequestViewSet: def test_viewset_fields(self, api_rf: APIRequestFactory, user): - pr = PullRequest.objects.create( - number=-1, - title="Test PR", - additions=0, - deletions=0, - sha="123", - author_name=user.username, - ) + pull_request = PullRequestFactory() view = PullRequestViewSet.as_view(actions={"get": "retrieve"}) request = api_rf.get("/fake-url/") request.user = user - response = view(request, pk=pr.pk) + response = view(request, pk=pull_request.number) assert ( response.data.items() >= { # Subset of the response data - "title": pr.title, - "sha": pr.sha, - "time_created": pr.time_created.astimezone(get_current_timezone()).isoformat(), - "url": f"http://testserver/api/pull-requests/{pr.pk}/", + "title": pull_request.title, + "sha": pull_request.sha, + "time_created": pull_request.time_created.astimezone(get_current_timezone()).isoformat(), + "url": f"http://testserver/api/pull-requests/{pull_request.number}/", }.items() ) diff --git a/democrasite/webiscite/tests/test_models.py b/democrasite/webiscite/tests/test_models.py index 8ab8ff7..d000fbc 100644 --- a/democrasite/webiscite/tests/test_models.py +++ b/democrasite/webiscite/tests/test_models.py @@ -8,8 +8,16 @@ from django_celery_beat.models import PeriodicTask from factory import Faker -from ..models import Bill, PullRequest -from .factories import BillFactory, GithubPullRequestFactory +from ..models import Bill, PullRequest, Vote +from .factories import BillFactory, GithubPullRequestFactory, PullRequestFactory + + +class TestVote: + """Test class for all tests related to the Vote model""" + + def test_vote_str(self, bill: Bill, user: Any): + bill.vote(user, True) + assert str(Vote.objects.get(user=user, support=True)) == f"{user} for {bill}" class TestPullRequest: @@ -36,6 +44,16 @@ def test_close(self, bill: Bill): assert pull_request.state == "closed" assert not pull_request.bill_set.filter(state=Bill.States.OPEN).exists() + def test_close_no_bill(self): + pull_request = PullRequestFactory() + assert pull_request.state == "open" + + pull_request.close() + + pull_request.refresh_from_db() + assert pull_request.state == "closed" + assert not pull_request.bill_set.exists() + class TestBill: """Test class for all tests related to the Bill model""" diff --git a/democrasite/webiscite/tests/test_webhooks.py b/democrasite/webiscite/tests/test_webhooks.py index cb9ba4f..cdc6b1b 100644 --- a/democrasite/webiscite/tests/test_webhooks.py +++ b/democrasite/webiscite/tests/test_webhooks.py @@ -10,8 +10,6 @@ from django.test import RequestFactory from django.utils.encoding import force_bytes -from democrasite.webiscite.tests.factories import PullRequestFactory - from ..models import Bill, PullRequest from ..webhooks import GithubWebhookView, PullRequestHandler, github_webhook_view @@ -108,14 +106,19 @@ def test_pr_handler_get_response(self, pr_handler: PullRequestHandler, bill: Bil @patch.object(Bill, "create_from_pr") def test_opened(self, mock_create, pr_handler: PullRequestHandler): - pull_request = PullRequestFactory() - pr = {"number": pull_request.number, "title": "Test PR", "diff_url": "http://test.com/diff"} - mock_create.return_value = (pull_request, None) + # This just calls the create_from_pr method + response = pr_handler.opened({}) + + mock_create.assert_called_once_with({}) + assert response == mock_create.return_value - response = pr_handler.opened(pr) + @patch.object(Bill, "create_from_pr") + def test_reopened(self, mock_create, pr_handler: PullRequestHandler): + # This does the exact same thing + response = pr_handler.reopened({}) - mock_create.assert_called_once_with(pr) - assert response == (pull_request, None) + mock_create.assert_called_once_with({}) + assert response == mock_create.return_value def test_closed_no_pr(self, pr_handler: PullRequestHandler): response = pr_handler.closed({"number": 1}) diff --git a/democrasite/webiscite/webhooks.py b/democrasite/webiscite/webhooks.py index 9197783..2cc8002 100644 --- a/democrasite/webiscite/webhooks.py +++ b/democrasite/webiscite/webhooks.py @@ -8,7 +8,7 @@ from logging import getLogger from typing import Any -import requests +# import requests from django.conf import settings from django.contrib.auth import get_user_model from django.http import HttpRequest, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, JsonResponse, request @@ -130,23 +130,24 @@ def _validate_signature(header_signature: str, request_body: bytes) -> HttpRespo return None - @staticmethod - def _validate_remote_addr(remote_addr: str) -> str: - """Validate the remote address of a request from a webhook + # Unused because I'm worried it will take too long + # @staticmethod + # def _validate_remote_addr(remote_addr: str) -> str: + # """Validate the remote address of a request from a webhook - Args: - request: The request from the webhook + # Args: + # request: The request from the webhook - Returns: - str: Error message if the remote address is invalid, otherwise an empty string - """ - # Get the list of IP addresses that GitHub uses to send webhooks - # This will slow the response but since it's not a frequent request, it's acceptable - webhook_allowed_hosts = requests.get("https://api.github.com/meta", timeout=5).json()["hooks"] - if remote_addr not in webhook_allowed_hosts: - return "Invalid remote address for GitHub webhook request" + # Returns: + # str: Error message if the remote address is invalid, otherwise an empty string + # """ + # # Get the list of IP addresses that GitHub uses to send webhooks + # # This will slow the response but since it's not a frequent request, it's acceptable + # webhook_allowed_hosts = requests.get("https://api.github.com/meta", timeout=5).json()["hooks"] + # if remote_addr not in webhook_allowed_hosts: + # return "Invalid remote address for GitHub webhook request" - return "" + # return "" def validate_request(self, headers: request.HttpHeaders, body: bytes) -> HttpResponse | None: return self._validate_header(headers) or self._validate_signature(headers["x-hub-signature-256"], body)