From 9c9ea42267fd128f2f58dfba2e651ee9e9145290 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 12 May 2026 17:09:03 -0700 Subject: [PATCH 1/2] feat(preprod): Add PR comments for snapshot base-build edge cases Align PR comment logic with status check logic so that all three base-build edge cases post comments instead of silently returning: no base_sha posts "Generated N snapshots", missing base posts "Waiting for base snapshots...", and timeout posts "No base snapshots found". Co-Authored-By: Claude Opus 4.6 --- .../preprod/vcs/pr_comments/snapshot_tasks.py | 102 +++++++++------ .../vcs/pr_comments/snapshot_templates.py | 80 +++++++++++- .../vcs/status_checks/snapshots/tasks.py | 9 ++ .../vcs/pr_comments/test_snapshot_tasks.py | 116 +++++++++++++++++- .../pr_comments/test_snapshot_templates.py | 79 ++++++++++++ .../vcs/status_checks/snapshots/test_tasks.py | 25 +++- 6 files changed, 354 insertions(+), 57 deletions(-) diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py index 6fc82cb741671d..de5fbffdec1d09 100644 --- a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py +++ b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py @@ -13,7 +13,12 @@ from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics from sentry.preprod.snapshots.utils import build_changes_map -from sentry.preprod.vcs.pr_comments.snapshot_templates import format_snapshot_pr_comment +from sentry.preprod.vcs.pr_comments.snapshot_templates import ( + format_missing_base_snapshot_pr_comment, + format_snapshot_pr_comment, + format_solo_snapshot_pr_comment, + format_waiting_for_base_snapshot_pr_comment, +) from sentry.preprod.vcs.pr_comments.tasks import find_existing_comment_id, save_pr_comment_result from sentry.shared_integrations.exceptions import ApiError from sentry.silo.base import SiloMode @@ -38,7 +43,10 @@ retry=Retry(times=3, delay=60), ) def create_preprod_snapshot_pr_comment_task( - preprod_artifact_id: int, caller: str | None = None, **kwargs: Any + preprod_artifact_id: int, + caller: str | None = None, + is_timeout_check: bool = False, + **kwargs: Any, ) -> None: try: artifact = PreprodArtifact.objects.select_related( @@ -155,47 +163,63 @@ def create_preprod_snapshot_pr_comment_task( base_artifact_map = PreprodArtifact.get_base_artifacts_for_commit(all_artifacts) - post_on_added = artifact.project.get_option(POST_ON_ADDED_OPTION_KEY, default=False) - post_on_removed = artifact.project.get_option(POST_ON_REMOVED_OPTION_KEY, default=True) - post_on_changed = artifact.project.get_option(POST_ON_CHANGED_OPTION_KEY, default=True) - post_on_renamed = artifact.project.get_option(POST_ON_RENAMED_OPTION_KEY, default=False) - changes_map = build_changes_map( - all_artifacts, - snapshot_metrics_map, - comparisons_map, - fail_on_added=post_on_added, - fail_on_removed=post_on_removed, - fail_on_changed=post_on_changed, - fail_on_renamed=post_on_renamed, - ) - - has_changes = any(changes_map.values()) - # Failed comparisons are absent from changes_map (which only tracks - # SUCCESS state), so check comparisons_map directly to avoid - # suppressing failure reports. - has_failures = any( - c.state == PreprodSnapshotComparison.State.FAILED for c in comparisons_map.values() - ) - if not has_changes and not has_failures: - logger.info( - "preprod.snapshot_pr_comments.create.skipped_no_diff", - extra={"artifact_id": artifact.id}, - ) - return - - comment_body = format_snapshot_pr_comment( - all_artifacts, - snapshot_metrics_map, - comparisons_map, - base_artifact_map, - changes_map, - approvals_map=approvals_map, - project=artifact.project, - ) + is_solo = not base_artifact_map existing_comment_id = find_existing_comment_id(all_for_pr, "snapshots") cc_id = cc.id + if is_solo: + if not commit_comparison.base_sha: + comment_body = format_solo_snapshot_pr_comment( + all_artifacts, snapshot_metrics_map, project=artifact.project + ) + elif not is_timeout_check: + comment_body = format_waiting_for_base_snapshot_pr_comment( + all_artifacts, snapshot_metrics_map, project=artifact.project + ) + else: + comment_body = format_missing_base_snapshot_pr_comment( + all_artifacts, snapshot_metrics_map, project=artifact.project + ) + else: + post_on_added = artifact.project.get_option(POST_ON_ADDED_OPTION_KEY, default=False) + post_on_removed = artifact.project.get_option(POST_ON_REMOVED_OPTION_KEY, default=True) + post_on_changed = artifact.project.get_option(POST_ON_CHANGED_OPTION_KEY, default=True) + post_on_renamed = artifact.project.get_option(POST_ON_RENAMED_OPTION_KEY, default=False) + changes_map = build_changes_map( + all_artifacts, + snapshot_metrics_map, + comparisons_map, + fail_on_added=post_on_added, + fail_on_removed=post_on_removed, + fail_on_changed=post_on_changed, + fail_on_renamed=post_on_renamed, + ) + + has_changes = any(changes_map.values()) + # Failed comparisons are absent from changes_map (which only tracks + # SUCCESS state), so check comparisons_map directly to avoid + # suppressing failure reports. + has_failures = any( + c.state == PreprodSnapshotComparison.State.FAILED for c in comparisons_map.values() + ) + if not has_changes and not has_failures: + logger.info( + "preprod.snapshot_pr_comments.create.skipped_no_diff", + extra={"artifact_id": artifact.id}, + ) + return + + comment_body = format_snapshot_pr_comment( + all_artifacts, + snapshot_metrics_map, + comparisons_map, + base_artifact_map, + changes_map, + approvals_map=approvals_map, + project=artifact.project, + ) + post_snapshot_pr_comment_task.delay( organization_id=organization.id, repo_name=commit_comparison.head_repo_name, diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py b/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py index a76f8aa833ea6a..f549522862b479 100644 --- a/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py +++ b/src/sentry/preprod/vcs/pr_comments/snapshot_templates.py @@ -25,7 +25,6 @@ def format_snapshot_pr_comment( *, project: Project, ) -> str: - """Format a PR comment for snapshot comparisons.""" if not artifacts: raise ValueError("Cannot format PR comment for empty artifact list") @@ -90,12 +89,8 @@ def format_snapshot_pr_comment( f" | {status} |" ) - settings_url = project.organization.absolute_url( - f"/settings/projects/{project.slug}/mobile-builds/", query="tab=snapshots" - ) - table = f"{_HEADER}\n\n{COMPARISON_TABLE_HEADER}" + "\n".join(table_rows) - settings_link = f"[⚙️ {project.name} Snapshot Settings]({settings_url})" + settings_link = _format_settings_link(project) return f"{table}\n\n{settings_link}" @@ -137,3 +132,76 @@ def _section_cell(count: int, section: str, artifact_url: str) -> str: if count > 0: return f"[{count}]({artifact_url}?selectedTypes={section})" return str(count) + + +def _format_settings_link(project: Project) -> str: + settings_url = project.organization.absolute_url( + f"/settings/projects/{project.slug}/mobile-builds/", query="tab=snapshots" + ) + return f"[⚙️ {project.name} Snapshot Settings]({settings_url})" + + +def _format_solo_table( + artifacts: list[PreprodArtifact], + snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], +) -> str: + table_rows = [] + for artifact in artifacts: + app_display, app_id = _app_display_info(artifact) + artifact_url = get_preprod_artifact_url(artifact, view_type="snapshots") + name_cell = _format_name_cell(app_display, app_id, artifact_url) + metrics = snapshot_metrics_map.get(artifact.id) + if metrics: + table_rows.append( + f"| {name_cell} | - | - | - | - | - | - | ✅ {metrics.image_count} uploaded |" + ) + else: + table_rows.append(f"| {name_cell} | - | - | - | - | - | - | {PROCESSING_STATUS} |") + return f"{_HEADER}\n\n{COMPARISON_TABLE_HEADER}" + "\n".join(table_rows) + + +_SOLO_MESSAGE = "Snapshot diffs will appear when we have a base upload to compare against. Make sure to upload snapshots from your main branch." +_WAITING_MESSAGE = "Waiting for base snapshots to finish uploading. This comment will update automatically within ~10 minutes or fail." +_MISSING_BASE_MESSAGE = "No base snapshots found to compare against. Make sure snapshots are uploaded from your main branch." + + +def _format_solo_comment( + artifacts: list[PreprodArtifact], + snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], + message: str, + *, + project: Project, +) -> str: + if not artifacts: + raise ValueError("Cannot format PR comment for empty artifact list") + table = _format_solo_table(artifacts, snapshot_metrics_map) + return f"{table}\n\n{message}\n\n{_format_settings_link(project)}" + + +def format_solo_snapshot_pr_comment( + artifacts: list[PreprodArtifact], + snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], + *, + project: Project, +) -> str: + return _format_solo_comment(artifacts, snapshot_metrics_map, _SOLO_MESSAGE, project=project) + + +def format_waiting_for_base_snapshot_pr_comment( + artifacts: list[PreprodArtifact], + snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], + *, + project: Project, +) -> str: + return _format_solo_comment(artifacts, snapshot_metrics_map, _WAITING_MESSAGE, project=project) + + +def format_missing_base_snapshot_pr_comment( + artifacts: list[PreprodArtifact], + snapshot_metrics_map: dict[int, PreprodSnapshotMetrics], + *, + project: Project, +) -> str: + return _format_solo_comment( + artifacts, snapshot_metrics_map, _MISSING_BASE_MESSAGE, project=project + ) diff --git a/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py b/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py index be538749f0c5ce..315ee36a2d1e2b 100644 --- a/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py +++ b/src/sentry/preprod/vcs/status_checks/snapshots/tasks.py @@ -16,6 +16,7 @@ from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics from sentry.preprod.snapshots.utils import build_changes_map from sentry.preprod.url_utils import get_preprod_artifact_url +from sentry.preprod.vcs.pr_comments.snapshot_tasks import create_preprod_snapshot_pr_comment_task from sentry.preprod.vcs.status_checks.snapshots.templates import ( format_first_snapshot_status_check_messages, format_generated_snapshot_status_check_messages, @@ -319,6 +320,14 @@ def create_preprod_snapshot_status_check_task( }, countdown=MISSING_BASE_GRACE_PERIOD_SECONDS, ) + create_preprod_snapshot_pr_comment_task.apply_async( + kwargs={ + "preprod_artifact_id": preprod_artifact_id, + "caller": "missing_base_timeout", + "is_timeout_check": True, + }, + countdown=MISSING_BASE_GRACE_PERIOD_SECONDS, + ) def _compute_snapshot_status( diff --git a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py index eeec784d9afa50..e515c1646dac0d 100644 --- a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py +++ b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py @@ -84,13 +84,15 @@ def _create_artifact_with_metrics( @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") def test_dispatches_subtask( - self, mock_format, mock_get_client, mock_build_changes_map, mock_delay + self, mock_get_base, mock_format, mock_get_client, mock_build_changes_map, mock_delay ): mock_get_client.return_value = Mock() mock_format.return_value = "## Sentry Snapshot Testing\n..." artifact, metrics = self._create_artifact_with_metrics() + mock_get_base.return_value = {artifact.id: Mock()} mock_build_changes_map.return_value = {artifact.id: True} with self.feature(self._feature): @@ -112,8 +114,9 @@ def test_dispatches_subtask( @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") def test_dispatches_with_existing_comment_id( - self, mock_format, mock_get_client, mock_build_changes_map, mock_delay + self, mock_get_base, mock_format, mock_get_client, mock_build_changes_map, mock_delay ): mock_get_client.return_value = Mock() mock_format.return_value = "updated body" @@ -134,6 +137,7 @@ def test_dispatches_with_existing_comment_id( artifact, metrics = self._create_artifact_with_metrics( commit_comparison=commit_comparison, ) + mock_get_base.return_value = {artifact.id: Mock()} mock_build_changes_map.return_value = {artifact.id: True} with self.feature(self._feature): @@ -265,8 +269,9 @@ def test_skips_nonexistent_artifact(self, mock_get_client): @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") def test_reads_fresh_extras_via_select_for_update( - self, mock_format, mock_get_client, mock_build_changes_map, mock_delay + self, mock_get_base, mock_format, mock_get_client, mock_build_changes_map, mock_delay ): mock_get_client.return_value = Mock() mock_format.return_value = "updated body" @@ -286,6 +291,7 @@ def test_reads_fresh_extras_via_select_for_update( artifact, metrics = self._create_artifact_with_metrics( commit_comparison=commit_comparison, ) + mock_get_base.return_value = {artifact.id: Mock()} mock_build_changes_map.return_value = {artifact.id: True} CommitComparison.objects.filter(id=commit_comparison.id).update( @@ -302,8 +308,9 @@ def test_reads_fresh_extras_via_select_for_update( @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") def test_updates_comment_from_previous_commit( - self, mock_format, mock_get_client, mock_build_changes_map, mock_delay + self, mock_get_base, mock_format, mock_get_client, mock_build_changes_map, mock_delay ): mock_get_client.return_value = Mock() mock_format.return_value = "updated body" @@ -336,6 +343,7 @@ def test_updates_comment_from_previous_commit( ) artifact, metrics = self._create_artifact_with_metrics(commit_comparison=cc_b) + mock_get_base.return_value = {artifact.id: Mock()} mock_build_changes_map.return_value = {artifact.id: True} with self.feature(self._feature): @@ -348,8 +356,9 @@ def test_updates_comment_from_previous_commit( @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") def test_passes_post_on_options_to_build_changes_map( - self, mock_format, mock_get_client, mock_build_changes_map, mock_delay + self, mock_get_base, mock_format, mock_get_client, mock_build_changes_map, mock_delay ): mock_get_client.return_value = Mock() mock_format.return_value = "body" @@ -360,6 +369,7 @@ def test_passes_post_on_options_to_build_changes_map( self.project.update_option("sentry:preprod_snapshot_pr_comments_post_on_renamed", True) artifact, metrics = self._create_artifact_with_metrics() + mock_get_base.return_value = {artifact.id: Mock()} mock_build_changes_map.return_value = {artifact.id: True} with self.feature(self._feature): @@ -376,8 +386,9 @@ def test_passes_post_on_options_to_build_changes_map( @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") def test_creates_when_no_sibling_has_comment_id( - self, mock_format, mock_get_client, mock_build_changes_map, mock_delay + self, mock_get_base, mock_format, mock_get_client, mock_build_changes_map, mock_delay ): mock_get_client.return_value = Mock() mock_format.return_value = "new body" @@ -407,6 +418,7 @@ def test_creates_when_no_sibling_has_comment_id( ) artifact, metrics = self._create_artifact_with_metrics(commit_comparison=cc_b) + mock_get_base.return_value = {artifact.id: Mock()} mock_build_changes_map.return_value = {artifact.id: True} with self.feature(self._feature): @@ -416,6 +428,98 @@ def test_creates_when_no_sibling_has_comment_id( assert mock_delay.call_args.kwargs["existing_comment_id"] is None +@cell_silo_test +class CreateSnapshotPrCommentSoloTest(CreatePreprodSnapshotPrCommentTaskTest): + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_solo_snapshot_pr_comment") + def test_no_base_sha_posts_solo_comment(self, mock_format_solo, mock_get_client, mock_delay): + mock_get_client.return_value = Mock() + mock_format_solo.return_value = "solo body" + + unique = str(uuid.uuid4()).replace("-", "")[:8] + cc = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha=unique.ljust(40, "a"), + base_sha=None, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + ) + + artifact, _ = self._create_artifact_with_metrics(commit_comparison=cc) + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_format_solo.assert_called_once() + mock_delay.assert_called_once() + assert mock_delay.call_args.kwargs["comment_body"] == "solo body" + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch( + "sentry.preprod.vcs.pr_comments.snapshot_tasks.format_waiting_for_base_snapshot_pr_comment" + ) + def test_base_sha_no_match_posts_waiting_comment( + self, mock_format_waiting, mock_get_client, mock_delay + ): + mock_get_client.return_value = Mock() + mock_format_waiting.return_value = "waiting body" + + artifact, _ = self._create_artifact_with_metrics() + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_format_waiting.assert_called_once() + mock_delay.assert_called_once() + assert mock_delay.call_args.kwargs["comment_body"] == "waiting body" + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_missing_base_snapshot_pr_comment") + def test_timeout_posts_missing_base_comment( + self, mock_format_missing, mock_get_client, mock_delay + ): + mock_get_client.return_value = Mock() + mock_format_missing.return_value = "missing body" + + artifact, _ = self._create_artifact_with_metrics() + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id, is_timeout_check=True) + + mock_format_missing.assert_called_once() + mock_delay.assert_called_once() + assert mock_delay.call_args.kwargs["comment_body"] == "missing body" + + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") + def test_timeout_with_base_arrived_runs_normal_path( + self, mock_get_base, mock_format_normal, mock_get_client, mock_build_changes_map, mock_delay + ): + mock_get_client.return_value = Mock() + mock_format_normal.return_value = "normal body" + + head_artifact, head_metrics = self._create_artifact_with_metrics() + mock_get_base.return_value = {head_artifact.id: Mock()} + mock_build_changes_map.return_value = {head_artifact.id: True} + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(head_artifact.id, is_timeout_check=True) + + mock_format_normal.assert_called_once() + mock_delay.assert_called_once() + assert mock_delay.call_args.kwargs["comment_body"] == "normal body" + + @cell_silo_test class PostSnapshotPrCommentTaskTest(TestCase): def setUp(self) -> None: diff --git a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py index 495950b64ae7ac..48d9d0633dd2dc 100644 --- a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py +++ b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py @@ -5,7 +5,10 @@ from sentry.preprod.models import PreprodArtifact, PreprodComparisonApproval from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics from sentry.preprod.vcs.pr_comments.snapshot_templates import ( + format_missing_base_snapshot_pr_comment, format_snapshot_pr_comment, + format_solo_snapshot_pr_comment, + format_waiting_for_base_snapshot_pr_comment, ) from sentry.testutils.cases import TestCase from sentry.testutils.silo import cell_silo_test @@ -424,3 +427,79 @@ def test_no_base_app_id_shown(self) -> None: ) assert "`com.example.myapp`" in result + + +@cell_silo_test +class FormatSoloPrCommentTest(SnapshotPrCommentTestBase): + def test_solo_shows_uploaded_count_and_first_upload_message(self) -> None: + artifact, metrics = self._create_artifact_with_metrics(app_name="My App", image_count=7) + + result = format_solo_snapshot_pr_comment( + [artifact], {artifact.id: metrics}, project=self.project + ) + + assert "## Sentry Snapshot Testing" in result + assert "7 uploaded" in result + assert "My App" in result + assert "Snapshot diffs will appear when we have a base upload to compare against" in result + assert "main branch" in result + assert f"/settings/projects/{self.project.slug}/mobile-builds/" in result + assert "tab=snapshots" in result + assert f"{self.project.name} Snapshot Settings" in result + + def test_solo_empty_artifacts_raises(self) -> None: + with pytest.raises(ValueError, match="Cannot format PR comment for empty artifact list"): + format_solo_snapshot_pr_comment([], {}, project=self.project) + + def test_solo_multiple_artifacts(self) -> None: + artifact1, metrics1 = self._create_artifact_with_metrics( + app_id="com.example.app1", build_number=1, image_count=3 + ) + artifact2, metrics2 = self._create_artifact_with_metrics( + app_id="com.example.app2", build_number=2, image_count=5 + ) + + result = format_solo_snapshot_pr_comment( + [artifact1, artifact2], + {artifact1.id: metrics1, artifact2.id: metrics2}, + project=self.project, + ) + + assert "com.example.app1" in result + assert "com.example.app2" in result + assert "3 uploaded" in result + assert "5 uploaded" in result + + def test_waiting_for_base_shows_waiting_message(self) -> None: + artifact, metrics = self._create_artifact_with_metrics(image_count=4) + + result = format_waiting_for_base_snapshot_pr_comment( + [artifact], {artifact.id: metrics}, project=self.project + ) + + assert "## Sentry Snapshot Testing" in result + assert "4 uploaded" in result + assert "Waiting for base snapshots to finish uploading" in result + assert "~10 minutes" in result + assert f"/settings/projects/{self.project.slug}/mobile-builds/" in result + + def test_waiting_for_base_empty_artifacts_raises(self) -> None: + with pytest.raises(ValueError, match="Cannot format PR comment for empty artifact list"): + format_waiting_for_base_snapshot_pr_comment([], {}, project=self.project) + + def test_missing_base_shows_failure_message(self) -> None: + artifact, metrics = self._create_artifact_with_metrics(image_count=2) + + result = format_missing_base_snapshot_pr_comment( + [artifact], {artifact.id: metrics}, project=self.project + ) + + assert "## Sentry Snapshot Testing" in result + assert "2 uploaded" in result + assert "No base snapshots found to compare against" in result + assert "main branch" in result + assert f"/settings/projects/{self.project.slug}/mobile-builds/" in result + + def test_missing_base_empty_artifacts_raises(self) -> None: + with pytest.raises(ValueError, match="Cannot format PR comment for empty artifact list"): + format_missing_base_snapshot_pr_comment([], {}, project=self.project) diff --git a/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py b/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py index fc89330fe061c0..48f173de791466 100644 --- a/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py +++ b/tests/sentry/preprod/vcs/status_checks/snapshots/test_tasks.py @@ -423,9 +423,15 @@ def _create_solo_head_artifact(self): @patch(f"{TASK_MODULE}.post_snapshot_status_check_task") @patch(f"{TASK_MODULE}.get_status_check_provider") @patch(f"{TASK_MODULE}.get_status_check_client") + @patch(f"{TASK_MODULE}.create_preprod_snapshot_pr_comment_task") @patch(f"{TASK_MODULE}.create_preprod_snapshot_status_check_task.apply_async") def test_missing_base_first_attempt_posts_in_progress( - self, mock_apply_async, mock_get_client, mock_get_provider, mock_post_task + self, + mock_status_apply_async, + mock_pr_comment_task, + mock_get_client, + mock_get_provider, + mock_post_task, ): mock_get_client.return_value = (Mock(), Mock()) mock_get_provider.return_value = Mock() @@ -441,11 +447,18 @@ def test_missing_base_first_attempt_posts_in_progress( call_kwargs = mock_post_task.delay.call_args[1] assert call_kwargs["status"] == StatusCheckStatus.IN_PROGRESS.value - mock_apply_async.assert_called_once() - async_kwargs = mock_apply_async.call_args[1] - assert async_kwargs["kwargs"]["is_timeout_check"] is True - assert async_kwargs["kwargs"]["preprod_artifact_id"] == artifact.id - assert async_kwargs["countdown"] == 600 + mock_status_apply_async.assert_called_once() + status_kwargs = mock_status_apply_async.call_args[1] + assert status_kwargs["kwargs"]["is_timeout_check"] is True + assert status_kwargs["kwargs"]["preprod_artifact_id"] == artifact.id + assert status_kwargs["countdown"] == 600 + + mock_pr_comment_task.apply_async.assert_called_once() + pr_kwargs = mock_pr_comment_task.apply_async.call_args[1] + assert pr_kwargs["kwargs"]["is_timeout_check"] is True + assert pr_kwargs["kwargs"]["preprod_artifact_id"] == artifact.id + assert pr_kwargs["kwargs"]["caller"] == "missing_base_timeout" + assert pr_kwargs["countdown"] == 600 @patch(f"{TASK_MODULE}.post_snapshot_status_check_task") @patch(f"{TASK_MODULE}.get_status_check_provider") From 7c3645216612353ef29efcd30b3a72da20a70e91 Mon Sep 17 00:00:00 2001 From: Nico Hinderling Date: Tue, 12 May 2026 17:21:57 -0700 Subject: [PATCH 2/2] fix(preprod): Handle first-upload and stale waiting comment edge cases First-upload with base_sha would post "Waiting..." but no timeout was scheduled, leaving the comment stuck. Now routes first uploads to the solo comment like the status check does. Also ensure existing "Waiting" comments get updated to the normal diff view when base arrives with no changes, instead of silently returning. Co-Authored-By: Claude Opus 4 --- .../preprod/vcs/pr_comments/snapshot_tasks.py | 17 ++++- .../vcs/pr_comments/test_snapshot_tasks.py | 75 +++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py index de5fbffdec1d09..37b218c81ff420 100644 --- a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py +++ b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py @@ -169,7 +169,20 @@ def create_preprod_snapshot_pr_comment_task( cc_id = cc.id if is_solo: - if not commit_comparison.base_sha: + app_ids = {a.app_id for a in all_artifacts if a.app_id} + has_previous_snapshots = ( + PreprodSnapshotMetrics.objects.filter( + preprod_artifact__project_id=artifact.project_id, + preprod_artifact__app_id__in=app_ids, + ) + .exclude(preprod_artifact__commit_comparison_id=commit_comparison.id) + .exists() + if app_ids + else False + ) + is_first_upload = not has_previous_snapshots + + if is_first_upload or not commit_comparison.base_sha: comment_body = format_solo_snapshot_pr_comment( all_artifacts, snapshot_metrics_map, project=artifact.project ) @@ -203,7 +216,7 @@ def create_preprod_snapshot_pr_comment_task( has_failures = any( c.state == PreprodSnapshotComparison.State.FAILED for c in comparisons_map.values() ) - if not has_changes and not has_failures: + if not has_changes and not has_failures and not existing_comment_id: logger.info( "preprod.snapshot_pr_comments.create.skipped_no_diff", extra={"artifact_id": artifact.id}, diff --git a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py index e515c1646dac0d..d53527ea469833 100644 --- a/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py +++ b/tests/sentry/preprod/vcs/pr_comments/test_snapshot_tasks.py @@ -430,6 +430,26 @@ def test_creates_when_no_sibling_has_comment_id( @cell_silo_test class CreateSnapshotPrCommentSoloTest(CreatePreprodSnapshotPrCommentTaskTest): + def _create_previous_snapshot(self, app_id: str = "com.example.app") -> None: + prev_cc = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="prev" + "0" * 36, + base_sha="prev" + "1" * 36, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="main", + base_ref="main", + pr_number=None, + ) + prev_artifact = self.create_preprod_artifact( + project=self.project, + state=PreprodArtifact.ArtifactState.PROCESSED, + app_id=app_id, + commit_comparison=prev_cc, + ) + PreprodSnapshotMetrics.objects.create(preprod_artifact=prev_artifact, image_count=5) + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_solo_snapshot_pr_comment") @@ -459,6 +479,24 @@ def test_no_base_sha_posts_solo_comment(self, mock_format_solo, mock_get_client, mock_delay.assert_called_once() assert mock_delay.call_args.kwargs["comment_body"] == "solo body" + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_solo_snapshot_pr_comment") + def test_first_upload_with_base_sha_posts_solo_comment( + self, mock_format_solo, mock_get_client, mock_delay + ): + mock_get_client.return_value = Mock() + mock_format_solo.return_value = "first upload body" + + artifact, _ = self._create_artifact_with_metrics() + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(artifact.id) + + mock_format_solo.assert_called_once() + mock_delay.assert_called_once() + assert mock_delay.call_args.kwargs["comment_body"] == "first upload body" + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") @patch( @@ -470,6 +508,7 @@ def test_base_sha_no_match_posts_waiting_comment( mock_get_client.return_value = Mock() mock_format_waiting.return_value = "waiting body" + self._create_previous_snapshot() artifact, _ = self._create_artifact_with_metrics() with self.feature(self._feature): @@ -488,6 +527,7 @@ def test_timeout_posts_missing_base_comment( mock_get_client.return_value = Mock() mock_format_missing.return_value = "missing body" + self._create_previous_snapshot() artifact, _ = self._create_artifact_with_metrics() with self.feature(self._feature): @@ -519,6 +559,41 @@ def test_timeout_with_base_arrived_runs_normal_path( mock_delay.assert_called_once() assert mock_delay.call_args.kwargs["comment_body"] == "normal body" + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.post_snapshot_pr_comment_task.delay") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.build_changes_map") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.get_commit_context_client") + @patch("sentry.preprod.vcs.pr_comments.snapshot_tasks.format_snapshot_pr_comment") + @patch("sentry.preprod.models.PreprodArtifact.get_base_artifacts_for_commit") + def test_updates_existing_comment_when_no_changes( + self, mock_get_base, mock_format, mock_get_client, mock_build_changes_map, mock_delay + ): + mock_get_client.return_value = Mock() + mock_format.return_value = "unchanged body" + + cc = CommitComparison.objects.create( + organization_id=self.organization.id, + head_sha="a" * 40, + base_sha="b" * 40, + provider="github", + head_repo_name="owner/repo", + base_repo_name="owner/repo", + head_ref="feature/test", + base_ref="main", + pr_number=42, + extras={"pr_comments": {"snapshots": {"success": True, "comment_id": "waiting_123"}}}, + ) + + head_artifact, _ = self._create_artifact_with_metrics(commit_comparison=cc) + mock_get_base.return_value = {head_artifact.id: Mock()} + mock_build_changes_map.return_value = {head_artifact.id: False} + + with self.feature(self._feature): + create_preprod_snapshot_pr_comment_task(head_artifact.id) + + mock_format.assert_called_once() + mock_delay.assert_called_once() + assert mock_delay.call_args.kwargs["existing_comment_id"] == "waiting_123" + @cell_silo_test class PostSnapshotPrCommentTaskTest(TestCase):