fix: don't require BASE report for patch in comparison (#650)

services/comparison/__init__.py

@@ -3,8 +3,9 @@
 from dataclasses import dataclass
 from typing import Dict, List, Optional
 
+import sentry_sdk
 from shared.reports.changes import get_changes_using_rust, run_comparison_using_rust
-from shared.reports.types import Change
+from shared.reports.types import Change, ReportTotals
 from shared.torngit.exceptions import (
     TorngitClientGeneralError,
 )
@@ -202,6 +203,16 @@ async def get_changes(self) -> Optional[List[Change]]:
                         )
             return self._changes
 
+    @sentry_sdk.trace
+    async def get_patch_totals(self) -> ReportTotals | None:
+        """Returns the patch coverage for the comparison.
+
+        Patch coverage refers to looking at the coverage in HEAD report filtered by the git diff HEAD..BASE.
+        """
+        diff = await self.get_diff(use_original_base=True)
+        totals = self.head.report.apply_diff(diff)
+        return totals
+
     async def get_behind_by(self):
         async with self._behind_by_lock:
             if self._behind_by is None:

tasks/compute_comparison.py

@@ -1,10 +1,13 @@
 import logging
+from typing import Literal, TypedDict
 
+import sentry_sdk
 from asgiref.sync import async_to_sync
 from shared.celery_config import compute_comparison_task_name
 from shared.components import Component
 from shared.helpers.flag import Flag
 from shared.reports.readonly import ReadOnlyReport
+from shared.reports.types import ReportTotals
 from shared.torngit.exceptions import TorngitRateLimitError
 from shared.yaml import UserYaml
 
@@ -13,7 +16,6 @@
 from database.models import CompareCommit, CompareComponent, CompareFlag
 from database.models.reports import ReportLevelTotals, RepositoryFlag
 from helpers.github_installation import get_installation_name_for_owner_for_task
-from helpers.metrics import metrics
 from services.archive import ArchiveService
 from services.comparison import ComparisonContext, ComparisonProxy, FilteredComparison
 from services.comparison.types import Comparison, FullCommit
@@ -24,51 +26,89 @@
 log = logging.getLogger(__name__)
 
 
+ComputeComparisonTaskErrors = (
+    Literal["missing_head_report"]
+    | Literal["missing_base_report"]
+    | Literal["torngit_rate_limit"]
+)
+
+
+class ComputeComparisonTaskReturn(TypedDict):
+    success: bool
+    error: ComputeComparisonTaskErrors | None
+
+
 class ComputeComparisonTask(BaseCodecovTask, name=compute_comparison_task_name):
-    def run_impl(self, db_session, comparison_id, *args, **kwargs):
-        comparison = db_session.query(CompareCommit).get(comparison_id)
+    def run_impl(
+        self, db_session, comparison_id, *args, **kwargs
+    ) -> ComputeComparisonTaskReturn:
+        comparison: CompareCommit = db_session.query(CompareCommit).get(comparison_id)
         repo = comparison.compare_commit.repository
-        log_extra = dict(comparison_id=comparison_id, repoid=repo.repoid)
+        log_extra = dict(
+            comparison_id=comparison_id,
+            repoid=repo.repoid,
+            commit=comparison.compare_commit.commitid,
+        )
         log.info("Computing comparison", extra=log_extra)
-        current_yaml = self.get_yaml_commit(comparison.compare_commit)
+        current_yaml = get_repo_yaml(repo)
         installation_name_to_use = get_installation_name_for_owner_for_task(
             db_session, self.name, repo.owner
         )
 
-        with metrics.timer(f"{self.metrics_prefix}.get_comparison_proxy"):
-            comparison_proxy = self.get_comparison_proxy(
-                comparison, current_yaml, installation_name_to_use
-            )
+        comparison_proxy = self.get_comparison_proxy(
+            comparison, current_yaml, installation_name_to_use
+        )
+        if not comparison_proxy.has_head_report():
+            comparison.error = CompareCommitError.missing_head_report.value
+            comparison.state = CompareCommitState.error.value
+            log.warning("Comparison doesn't have HEAD report", extra=log_extra)
+            return {"successful": False, "error": "missing_head_report"}
+
+        # At this point we can calculate the patch coverage
+        # Because we have a HEAD report and a base commit to get the diff from
+        patch_totals = async_to_sync(comparison_proxy.get_patch_totals)()
+        comparison.patch_totals = self.minimal_totals(patch_totals)
+
         if not comparison_proxy.has_project_coverage_base_report():
             comparison.error = CompareCommitError.missing_base_report.value
-        elif not comparison_proxy.has_head_report():
-            comparison.error = CompareCommitError.missing_head_report.value
+            log.warning(
+                "Comparison doesn't have BASE report",
+                extra={"base_commit": comparison.base_commit.commitid, **log_extra},
+            )
+            comparison.state = CompareCommitState.error.value
+            return {"successful": False, "error": "missing_base_report"}
         else:
             comparison.error = None
 
-        if comparison.error:
-            comparison.state = CompareCommitState.error.value
-            log.warn("Compute comparison failed, %s", comparison.error, extra=log_extra)
-            return {"successful": False}
         try:
-            with metrics.timer(f"{self.metrics_prefix}.serialize_impacted_files") as tm:
-                impacted_files = self.serialize_impacted_files(comparison_proxy)
+            impacted_files = self.serialize_impacted_files(comparison_proxy)
         except TorngitRateLimitError:
             log.warning(
                 "Unable to compute comparison due to rate limit error",
                 extra=dict(
                     comparison_id=comparison_id, repoid=comparison.compare_commit.repoid
                 ),
             )
-            return {"successful": False}
-        log.info("Files impact calculated", extra=dict(timing_ms=tm.ms, **log_extra))
-        with metrics.timer(f"{self.metrics_prefix}.store_results"):
-            path = self.store_results(comparison, impacted_files)
+            comparison.state = CompareCommitState.error.value
+            return {"successful": False, "error": "torngit_rate_limit"}
+
+        log.info("Files impact calculated", extra=log_extra)
+        path = self.store_results(comparison, impacted_files)
 
         comparison.report_storage_path = path
-        comparison.patch_totals = impacted_files.get("changes_summary").get(
+        calculated_patch_totals = impacted_files.get("changes_summary").get(
             "patch_totals"
         )
+        if calculated_patch_totals != comparison.patch_totals:
+            log.warning(
+                "Calculated patch totals differ!",
+                extra={
+                    **log_extra,
+                    "calculated_patch_totals": calculated_patch_totals,
+                    "patch_totals": comparison.patch_totals,
+                },
+            )
+
         comparison.state = CompareCommitState.processed.value
         log.info("Computing comparison successful", extra=log_extra)
         db_session.commit()
@@ -79,6 +119,21 @@ def run_impl(self, db_session, comparison_id, *args, **kwargs):
         db_session.commit()
         return {"successful": True}
 
+    def minimal_totals(self, totals: ReportTotals | None) -> dict:
+        if totals is None:
+            return {
+                "hits": 0,
+                "misses": 0,
+                "partials": 0,
+                "coverage": None,
+            }
+        return {
+            "hits": totals.hits,
+            "misses": totals.misses,
+            "partials": totals.partials,
+            "coverage": totals.coverage,
+        }
+
     def compute_flag_comparison(self, db_session, comparison, comparison_proxy):
         log_extra = dict(comparison_id=comparison.id)
         log.info("Computing flag comparisons", extra=log_extra)
@@ -93,6 +148,7 @@ def compute_flag_comparison(self, db_session, comparison, comparison_proxy):
             comparison_proxy,
         )
 
+    @sentry_sdk.trace
     def create_or_update_flag_comparisons(
         self,
         db_session,
@@ -193,6 +249,7 @@ def store_flag_comparison(
         db_session.add(flag_comparison)
         db_session.flush()
 
+    @sentry_sdk.trace
     def compute_component_comparisons(
         self, db_session, comparison: CompareCommit, comparison_proxy: ComparisonProxy
     ):
@@ -255,9 +312,7 @@ def compute_component_comparison(
         db_session.add(component_comparison)
         db_session.flush()
 
-    def get_yaml_commit(self, commit):
-        return get_repo_yaml(commit.repository)
-
+    @sentry_sdk.trace
     def get_comparison_proxy(
         self, comparison, current_yaml, installation_name_to_use: str | None = None
     ):
@@ -289,9 +344,11 @@ def get_comparison_proxy(
             ),
         )
 
+    @sentry_sdk.trace
     def serialize_impacted_files(self, comparison_proxy):
         return async_to_sync(comparison_proxy.get_impacted_files)()
 
+    @sentry_sdk.trace
     def store_results(self, comparison, impacted_files):
         repository = comparison.compare_commit.repository
         storage_service = ArchiveService(repository)

tasks/tests/unit/test_compute_comparison.py

@@ -2,6 +2,7 @@
 
 from shared.reports.readonly import ReadOnlyReport
 from shared.reports.resources import Report
+from shared.reports.types import ReportTotals
 from shared.torngit.exceptions import TorngitRateLimitError
 from shared.yaml import UserYaml
 
@@ -311,20 +312,44 @@ def test_update_existing_flag_comparisons(
         assert compare_flag_records[0].repositoryflag_id == repositoryflag.id_
         assert compare_flag_records[0].patch_totals is not None
 
-    def test_set_state_to_error_missing_base_report(self, dbsession, mocker):
+    def test_set_state_to_error_missing_base_report(
+        self, dbsession, mocker, sample_report
+    ):
         comparison = CompareCommitFactory.create()
+        # We need a head report, but no base report
+        head_commit = comparison.compare_commit
+        mocker.patch.object(
+            ReportService,
+            "get_existing_report_for_commit",
+            side_effect=lambda commit,
+            *args,
+            **kwargs: ReadOnlyReport.create_from_report(sample_report)
+            if commit == head_commit
+            else None,
+        )
+        patch_totals = ReportTotals(
+            files=3, lines=200, hits=100, misses=100, coverage="50%"
+        )
+        mocker.patch(
+            "tasks.compute_comparison.ComparisonProxy.get_patch_totals",
+            return_value=patch_totals,
+        )
         dbsession.add(comparison)
         dbsession.flush()
         task = ComputeComparisonTask()
         mocker.patch.object(
             ReadOnlyReport, "should_load_rust_version", return_value=True
         )
-        mocker.patch.object(
-            ReportService, "get_existing_report_for_commit", return_value=None
-        )
-        task.run_impl(dbsession, comparison.id)
+        result = task.run_impl(dbsession, comparison.id)
         dbsession.flush()
+        assert result == {"successful": False, "error": "missing_base_report"}
         assert comparison.state == CompareCommitState.error.value
+        assert comparison.patch_totals == {
+            "hits": 100,
+            "misses": 100,
+            "partials": 0,
+            "coverage": "50%",
+        }
         assert comparison.error == CompareCommitError.missing_base_report.value
 
     def test_set_state_to_error_missing_head_report(
@@ -351,6 +376,13 @@ def test_run_task_ratelimit_error(self, dbsession, mocker, sample_report):
         comparison = CompareCommitFactory.create()
         dbsession.add(comparison)
         dbsession.flush()
+        patch_totals = ReportTotals(
+            files=3, lines=200, hits=100, misses=100, coverage="50%"
+        )
+        mocker.patch(
+            "tasks.compute_comparison.ComparisonProxy.get_patch_totals",
+            return_value=patch_totals,
+        )
         mocker.patch.object(
             ComputeComparisonTask,
             "serialize_impacted_files",
@@ -363,9 +395,15 @@ def test_run_task_ratelimit_error(self, dbsession, mocker, sample_report):
             return_value=ReadOnlyReport.create_from_report(sample_report),
         )
         res = task.run_impl(dbsession, comparison.id)
-        assert res == {"successful": False}
+        assert res == {"successful": False, "error": "torngit_rate_limit"}
         dbsession.flush()
-        assert comparison.state == CompareCommitState.pending.value
+        assert comparison.state == CompareCommitState.error.value
+        assert comparison.patch_totals == {
+            "hits": 100,
+            "misses": 100,
+            "partials": 0,
+            "coverage": "50%",
+        }
         assert comparison.error is None
 
     def test_compute_component_comparisons(