tests: handle skipped tests (bug 1897504)

- fix landing job cancel tests
- remove unneeded phabricator exception test
- return json response for 403/404

src/lando/api/legacy/api/landing_jobs.py

@@ -2,7 +2,7 @@
 import logging
 
 from django import forms
-from django.http import Http404, HttpRequest, JsonResponse
+from django.http import HttpRequest, JsonResponse
 
 from lando.main.auth import require_authenticated_user
 from lando.main.models.landing_job import LandingJob, LandingJobAction, LandingJobStatus
@@ -59,12 +59,16 @@ def put(request: HttpRequest, landing_job_id: int) -> JsonResponse:
         try:
             landing_job = LandingJob.objects.get(pk=landing_job_id)
         except LandingJob.DoesNotExist:
-            raise Http404(f"A landing job with ID {landing_job_id} was not found.")
+            return JsonResponse(
+                {"detail": f"A landing job with ID {landing_job_id} was not found."},
+                status=404,
+            )
 
     ldap_username = request.user.email
     if landing_job.requester_email != ldap_username:
-        raise PermissionError(
-            f"User not authorized to update landing job {landing_job_id}"
+        return JsonResponse(
+            {"detail": f"User not authorized to update landing job {landing_job_id}"},
+            status=403,
         )
 
     if status != "CANCELLED":

src/lando/api/tests/test_hooks.py

@@ -4,10 +4,6 @@
     TreeStatusCommunicationException,
     TreeStatusError,
 )
-from lando.utils.phabricator import (
-    PhabricatorAPIException,
-    PhabricatorCommunicationException,
-)
 
 
 @pytest.mark.django_db
@@ -44,29 +40,6 @@ def test_app_wide_headers_csp_report_uri(app, client):
     assert "report-uri /__cspreport__" in (response.headers["Content-Security-Policy"])
 
 
-@pytest.mark.skip
-def test_phabricator_api_exception_handled(db, app, client):
-    # We need to tell Flask to handle exceptions as if it were in a production
-    # environment.
-    app.config["PROPAGATE_EXCEPTIONS"] = False
-
-    @app.route("/__testing__/phab_exception1")
-    def phab_exception1():
-        raise PhabricatorAPIException("OOPS!")
-
-    @app.route("/__testing__/phab_exception2")
-    def phab_exception2():
-        raise PhabricatorCommunicationException("OOPS!")
-
-    response = client.get("__testing__/phab_exception1")
-    assert response.status_code == 500
-    assert response.json["title"] == "Phabricator Error"
-
-    response = client.get("__testing__/phab_exception2")
-    assert response.status_code == 500
-    assert response.json["title"] == "Phabricator Error"
-
-
 @pytest.mark.skip
 def test_treestatus_exception_handled(db, app, client):
     # We need to tell Flask to handle exceptions as if it were in a production

src/lando/api/tests/test_landing_job.py

@@ -75,27 +75,33 @@ def test_cancel_landing_job_fails_in_progress(
     assert job.status == LandingJobStatus.IN_PROGRESS
 
 
-@pytest.mark.skip
-def test_cancel_landing_job_fails_not_owner(db, client, landing_job, mock_permissions):
+def test_cancel_landing_job_fails_not_owner(
+    db, authenticated_client, landing_job, mock_permissions
+):
     """Test trying to cancel a job that is created by a different user."""
     job = landing_job(LandingJobStatus.SUBMITTED, "[email protected]")
-    response = client.put(
-        f"/landing_jobs/{job.id}",
-        json={"status": LandingJobStatus.CANCELLED.value},
+    response = authenticated_client.put(
+        f"/landing_jobs/{job.id}/",
+        json.dumps({"status": LandingJobStatus.CANCELLED.value}),
         permissions=mock_permissions,
     )
 
     assert response.status_code == 403
-    assert response.json["detail"] == ("User not authorized to update landing job 1")
+    assert response.json()["detail"] == (
+        f"User not authorized to update landing job {job.id}"
+    )
+
+    job.refresh_from_db()
     assert job.status == LandingJobStatus.SUBMITTED
 
 
-@pytest.mark.skip
-def test_cancel_landing_job_fails_not_found(db, client, landing_job, mock_permissions):
+def test_cancel_landing_job_fails_not_found(
+    db, authenticated_client, landing_job, mock_permissions
+):
     """Test trying to cancel a job that does not exist."""
-    response = client.put(
-        "/landing_jobs/1",
-        json={"status": LandingJobStatus.CANCELLED.value},
+    response = authenticated_client.put(
+        "/landing_jobs/1/",
+        json.dumps({"status": LandingJobStatus.CANCELLED.value}),
         permissions=mock_permissions,
     )