diff --git a/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py b/src/sentry/preprod/vcs/pr_comments/snapshot_tasks.py index 6fc82cb741671d..37b218c81ff420 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,46 +163,75 @@ 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, - ) + is_solo = not base_artifact_map - 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}, + existing_comment_id = find_existing_comment_id(all_for_pr, "snapshots") + cc_id = cc.id + + if is_solo: + 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 ) - return + is_first_upload = not has_previous_snapshots - 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, - ) + 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 + ) + 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, + ) - existing_comment_id = find_existing_comment_id(all_for_pr, "snapshots") - cc_id = cc.id + 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 and not existing_comment_id: + 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, 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..d53527ea469833 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,173 @@ 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): + 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") + 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_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( + "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" + + self._create_previous_snapshot() + 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" + + self._create_previous_snapshot() + 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" + + @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): 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")