fix: address feedback

this commit changes the int conversions of Outcome to str conversions. moves the db query for latest test instances for a given commit to a separate function and cleans up some unused vars Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
codecov · Feb 1, 2024 · 5d625ed · 5d625ed
1 parent c5d4d3e
commit 5d625ed
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 51 deletions.
diff --git a/services/test_results.py b/services/test_results.py
@@ -3,10 +3,11 @@
 from typing import Mapping, Sequence
 
 from shared.torngit.exceptions import TorngitClientError
-from test_results_parser import Outcome, Testrun
+from sqlalchemy import desc
+from test_results_parser import Outcome
 
 from database.enums import ReportType
-from database.models import Commit, CommitReport, RepositoryFlag, Upload
+from database.models import Commit, CommitReport, RepositoryFlag, TestInstance, Upload
 from services.report import BaseReportService
 from services.repository import (
     fetch_and_update_pull_request_information_from_commit,
@@ -170,10 +171,9 @@ def build_message(self, url, test_instances):
         failures = dict()
 
         for test_instance in test_instances:
-            if (
-                test_instance.outcome == Outcome.Failure
-                or test_instance.outcome == Outcome.Error
-            ):
+            if test_instance.outcome == str(
+                Outcome.Failure
+            ) or test_instance.outcome == str(Outcome.Error):
                 failed_tests += 1
                 job_code = test_instance.upload.job_code
                 flag_names = sorted(test_instance.upload.flag_names)
@@ -183,9 +183,9 @@ def build_message(self, url, test_instances):
                 failures[
                     f"{test_instance.test.testsuite}::{test_instance.test.name}{suffix}"
                 ] = test_instance.failure_message
-            elif test_instance.outcome == Outcome.Skip:
+            elif test_instance.outcome == str(Outcome.Skip):
                 skipped_tests += 1
-            elif test_instance.outcome == Outcome.Pass:
+            elif test_instance.outcome == str(Outcome.Pass):
                 passed_tests += 1
 
         results = f"Completed {len(test_instances)} tests with **`{failed_tests} failed`**, {passed_tests} passed and {skipped_tests} skipped."
@@ -222,3 +222,31 @@ def insert_breaks(self, table_value):
             for i in range(0, len(table_value), line_size)
         ]
         return "<br>".join(lines)
+
+
+def latest_test_instances_for_a_given_commit(db_session, commit_id):
+    """
+    This will result in a SQL query that looks something like this:
+
+    SELECT DISTINCT ON (rt.test_id) rt.id, rt.external_id, rt.created_at, rt.updated_at, rt.test_id, rt.duration_seconds, rt.outcome, rt.upload_id, rt.failure_message
+        FROM reports_testinstance rt JOIN reports_upload ru ON ru.id = rt.upload_id JOIN reports_commitreport rc ON rc.id = ru.report_id
+        WHERE rc.commit_id = <commit_id> ORDER BY rt.test_id, ru.created_at desc
+
+    The goal of this query is to return: "the latest test instance for each unique test based on upload creation time"
+
+    The `DISTINCT ON` test_id with the order by test_id, enforces that we are only fetching one test instance for each test
+
+    The ordering of the upload.create_at desc enforces that we get the latest test instance for that unique test
+    """
+    return (
+        db_session.query(TestInstance)
+        .join(Upload)
+        .join(CommitReport)
+        .filter(
+            CommitReport.commit_id == commit_id,
+        )
+        .order_by(TestInstance.test_id)
+        .order_by(desc(Upload.created_at))
+        .distinct(TestInstance.test_id)
+        .all()
+    )
diff --git a/tasks/notify.py b/tasks/notify.py
@@ -15,7 +15,7 @@
 from test_results_parser import Outcome
 
 from app import celery_app
-from database.enums import CommitErrorTypes, Decoration
+from database.enums import CommitErrorTypes, Decoration, ReportType
 from database.models import Commit, Pull
 from database.models.core import GITHUB_APP_INSTALLATION_DEFAULT_NAME
 from helpers.checkpoint_logger import from_kwargs as checkpoints_from_kwargs
@@ -37,6 +37,7 @@
     fetch_and_update_pull_request_information_from_commit,
     get_repo_provider_service,
 )
+from services.test_results import latest_test_instances_for_a_given_commit
 from services.yaml import get_current_yaml, read_yaml_field
 from tasks.base import BaseCodecovTask
 from tasks.upload_processor import UPLOAD_PROCESSING_LOCK_NAME
@@ -141,21 +142,12 @@ async def run_async_within_lock(
         commit = commits_query.first()
 
         # check if there were any test failures
-        latest_test_instances = (
-            db_session.query(TestInstance)
-            .join(Upload)
-            .join(CommitReport)
-            .filter(
-                CommitReport.commit_id == commit.id_,
-            )
-            .order_by(TestInstance.test_id)
-            .order_by(desc(Upload.created_at))
-            .distinct(TestInstance.test_id)
-            .all()
+        latest_test_instances = latest_test_instances_for_a_given_commit(
+            db_session, commit.id_
         )
 
         if any(
-            [test.outcome == int(Outcome.Failure) for test in latest_test_instances]
+            [test.outcome == str(Outcome.Failure) for test in latest_test_instances]
         ):
             return {
                 "notify_attempted": False,

diff --git a/tasks/test_results_finisher.py b/tasks/test_results_finisher.py
@@ -2,14 +2,16 @@
 from typing import Any, Dict
 
 from shared.yaml import UserYaml
-from sqlalchemy import desc
 from test_results_parser import Outcome
 
 from app import celery_app
 from database.enums import ReportType
 from database.models import Commit, CommitReport, Test, TestInstance, Upload
 from services.lock_manager import LockManager, LockRetry, LockType
-from services.test_results import TestResultsNotifier
+from services.test_results import (
+    TestResultsNotifier,
+    latest_test_instances_for_a_given_commit,
+)
 from tasks.base import BaseCodecovTask
 from tasks.notify import notify_task_name
 
@@ -95,16 +97,8 @@ async def process_async_within_lock(
             # every processor errored, nothing to notify on
             return {"notify_attempted": False, "notify_succeeded": False}
 
-        test_instances = (
-            db_session.query(TestInstance)
-            .join(Upload)
-            .join(CommitReport)
-            .join(Commit)
-            .filter(Commit.id_ == commit.id_)
-            .order_by(TestInstance.test_id)
-            .order_by(desc(Upload.created_at))
-            .distinct(TestInstance.test_id)
-            .all()
+        test_instances = latest_test_instances_for_a_given_commit(
+            db_session, commit.id_
         )
 
         if self.check_if_no_failures(test_instances):
@@ -116,7 +110,6 @@ async def process_async_within_lock(
             )
             return {"notify_attempted": False, "notify_succeeded": False}
 
-        success = None
         notifier = TestResultsNotifier(commit, commit_yaml, test_instances)
         success = await notifier.notify()
 
@@ -143,7 +136,7 @@ def check_if_no_success(self, previous_result):
 
     def check_if_no_failures(self, testrun_list):
         return all(
-            [instance.outcome != int(Outcome.Failure) for instance in testrun_list]
+            [instance.outcome != str(Outcome.Failure) for instance in testrun_list]
         )
 
 

diff --git a/tasks/tests/unit/test_test_results_finisher.py b/tasks/tests/unit/test_test_results_finisher.py
@@ -51,7 +51,7 @@ async def test_upload_finisher_task_call(
         dbsession.flush()
 
         upload2 = UploadFactory.create()
-        upload2.created_at = upload2.created_at + datetime.timedelta(0, 3)
+        upload2.created_at = upload1.created_at + datetime.timedelta(0, 3)
         dbsession.add(upload2)
         dbsession.flush()
 
@@ -124,7 +124,7 @@ async def test_upload_finisher_task_call(
 
         test_instance1 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="bad",
             duration_seconds=1,
             upload_id=upload_id1,
@@ -134,7 +134,7 @@ async def test_upload_finisher_task_call(
 
         test_instance2 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="not that bad",
             duration_seconds=1,
             upload_id=upload_id2,
@@ -144,7 +144,7 @@ async def test_upload_finisher_task_call(
 
         test_instance3 = TestInstance(
             test_id=test_id2,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="okay i guess",
             duration_seconds=2,
             upload_id=upload_id1,
@@ -278,7 +278,7 @@ async def test_upload_finisher_task_call_no_failures(
 
         test_instance1 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Pass),
+            outcome=str(Outcome.Pass),
             failure_message="bad",
             duration_seconds=1,
             upload_id=upload_id1,
@@ -288,7 +288,7 @@ async def test_upload_finisher_task_call_no_failures(
 
         test_instance2 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Pass),
+            outcome=str(Outcome.Pass),
             failure_message="not that bad",
             duration_seconds=1,
             upload_id=upload_id2,
@@ -298,7 +298,7 @@ async def test_upload_finisher_task_call_no_failures(
 
         test_instance3 = TestInstance(
             test_id=test_id2,
-            outcome=int(Outcome.Pass),
+            outcome=str(Outcome.Pass),
             failure_message="okay i guess",
             duration_seconds=2,
             upload_id=upload_id1,
@@ -436,7 +436,7 @@ async def test_upload_finisher_task_call_no_success(
 
         test_instance1 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Pass),
+            outcome=str(Outcome.Pass),
             failure_message="bad",
             duration_seconds=1,
             upload_id=upload_id1,
@@ -446,7 +446,7 @@ async def test_upload_finisher_task_call_no_success(
 
         test_instance2 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Pass),
+            outcome=str(Outcome.Pass),
             failure_message="not that bad",
             duration_seconds=1,
             upload_id=upload_id2,
@@ -456,7 +456,7 @@ async def test_upload_finisher_task_call_no_success(
 
         test_instance3 = TestInstance(
             test_id=test_id2,
-            outcome=int(Outcome.Pass),
+            outcome=str(Outcome.Pass),
             failure_message="okay i guess",
             duration_seconds=2,
             upload_id=upload_id1,
@@ -588,7 +588,7 @@ async def test_upload_finisher_task_call_existing_comment(
 
         test_instance1 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="bad",
             duration_seconds=1,
             upload_id=upload_id1,
@@ -598,7 +598,7 @@ async def test_upload_finisher_task_call_existing_comment(
 
         test_instance2 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="not that bad",
             duration_seconds=1,
             upload_id=upload_id2,
@@ -608,7 +608,7 @@ async def test_upload_finisher_task_call_existing_comment(
 
         test_instance3 = TestInstance(
             test_id=test_id2,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="okay i guess",
             duration_seconds=2,
             upload_id=upload_id1,
@@ -745,7 +745,7 @@ async def test_upload_finisher_task_call_comment_fails(
 
         test_instance1 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="bad",
             duration_seconds=1,
             upload_id=upload_id1,
@@ -755,7 +755,7 @@ async def test_upload_finisher_task_call_comment_fails(
 
         test_instance2 = TestInstance(
             test_id=test_id1,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="not that bad",
             duration_seconds=1,
             upload_id=upload_id2,
@@ -765,7 +765,7 @@ async def test_upload_finisher_task_call_comment_fails(
 
         test_instance3 = TestInstance(
             test_id=test_id2,
-            outcome=int(Outcome.Failure),
+            outcome=str(Outcome.Failure),
             failure_message="okay i guess",
             duration_seconds=2,
             upload_id=upload_id1,