Skip to content

Commit 15b3213

Browse files
committed
ref(preprod): Remove solo format, use single function for all cases
Remove format_snapshot_pr_comment_solo in favor of handling the no-base case within format_snapshot_pr_comment. When there's no base artifact, the same comparison table is shown with "N uploaded" in the status column, matching how size analysis handles missing bases.
1 parent 09c6816 commit 15b3213

File tree

2 files changed

+37
-56
lines changed

2 files changed

+37
-56
lines changed

src/sentry/preprod/vcs/pr_comments/snapshot_templates.py

Lines changed: 9 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,15 @@ def format_snapshot_pr_comment(
3333
continue
3434

3535
comparison = comparisons_map.get(metrics.id)
36+
has_base = artifact.id in base_artifact_map
37+
38+
if not comparison and not has_base:
39+
# No base to compare against — show snapshot count only
40+
table_rows.append(
41+
f"| {name_cell} | - | - | - | - | - | \u2705 {metrics.image_count} uploaded |"
42+
)
43+
continue
44+
3645
if not comparison:
3746
table_rows.append(f"| {name_cell} | - | - | - | - | - | {_PROCESSING_STATUS} |")
3847
continue
@@ -81,33 +90,6 @@ def format_snapshot_pr_comment(
8190
return f"{_HEADER}\n\n{table_header}" + "\n".join(table_rows)
8291

8392

84-
def format_snapshot_pr_comment_solo(
85-
artifacts: list[PreprodArtifact],
86-
snapshot_metrics_map: dict[int, PreprodSnapshotMetrics],
87-
) -> str:
88-
"""Format a PR comment for snapshots without a base comparison."""
89-
if not artifacts:
90-
raise ValueError("Cannot format PR comment for empty artifact list")
91-
92-
table_rows = []
93-
94-
for artifact in artifacts:
95-
app_display, app_id = _app_display_info(artifact)
96-
artifact_url = get_preprod_artifact_url(artifact, view_type="snapshots")
97-
name_cell = _format_name_cell(app_display, app_id, artifact_url)
98-
99-
metrics = snapshot_metrics_map.get(artifact.id)
100-
if not metrics:
101-
table_rows.append(f"| {name_cell} | - | {_PROCESSING_STATUS} |")
102-
continue
103-
104-
table_rows.append(f"| {name_cell} | {metrics.image_count} | \u2705 Uploaded |")
105-
106-
table_header = "| Name | Snapshots | Status |\n| :--- | :---: | :---: |\n"
107-
108-
return f"{_HEADER}\n\n{table_header}" + "\n".join(table_rows)
109-
110-
11193
def _name_cell(
11294
artifact: PreprodArtifact,
11395
snapshot_metrics_map: dict[int, PreprodSnapshotMetrics],

tests/sentry/preprod/vcs/pr_comments/test_snapshot_templates.py

Lines changed: 28 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
from sentry.preprod.snapshots.models import PreprodSnapshotComparison, PreprodSnapshotMetrics
77
from sentry.preprod.vcs.pr_comments.snapshot_templates import (
88
format_snapshot_pr_comment,
9-
format_snapshot_pr_comment_solo,
109
)
1110
from sentry.testutils.cases import TestCase
1211
from sentry.testutils.silo import cell_silo_test
@@ -79,10 +78,6 @@ def test_empty_artifacts_raises(self) -> None:
7978
with pytest.raises(ValueError, match="Cannot format PR comment for empty artifact list"):
8079
format_snapshot_pr_comment([], {}, {}, {}, {})
8180

82-
def test_empty_solo_artifacts_raises(self) -> None:
83-
with pytest.raises(ValueError, match="Cannot format PR comment for empty artifact list"):
84-
format_snapshot_pr_comment_solo([], {})
85-
8681

8782
@cell_silo_test
8883
class FormatSnapshotPrCommentProcessingTest(SnapshotPrCommentTestBase):
@@ -98,10 +93,17 @@ def test_no_metrics_shows_processing(self) -> None:
9893
assert "Processing" in result
9994
assert "com.example.app" in result
10095

101-
def test_no_comparison_shows_processing(self) -> None:
102-
artifact, metrics = self._create_artifact_with_metrics()
96+
def test_no_comparison_with_base_shows_processing(self) -> None:
97+
head_artifact, head_metrics = self._create_artifact_with_metrics()
98+
base_artifact, _ = self._create_artifact_with_metrics(app_id="com.example.base")
10399

104-
result = format_snapshot_pr_comment([artifact], {artifact.id: metrics}, {}, {}, {})
100+
result = format_snapshot_pr_comment(
101+
[head_artifact],
102+
{head_artifact.id: head_metrics},
103+
{},
104+
{head_artifact.id: base_artifact},
105+
{},
106+
)
105107

106108
assert "Processing" in result
107109

@@ -117,7 +119,7 @@ def test_pending_comparison_shows_processing(self) -> None:
117119
[head_artifact],
118120
{head_artifact.id: head_metrics},
119121
{head_metrics.id: comparison},
120-
{},
122+
{head_artifact.id: base_artifact},
121123
{},
122124
)
123125

@@ -135,7 +137,7 @@ def test_processing_comparison_shows_processing(self) -> None:
135137
[head_artifact],
136138
{head_artifact.id: head_metrics},
137139
{head_metrics.id: comparison},
138-
{},
140+
{head_artifact.id: base_artifact},
139141
{},
140142
)
141143

@@ -156,7 +158,7 @@ def test_failed_comparison_shows_failure(self) -> None:
156158
[head_artifact],
157159
{head_artifact.id: head_metrics},
158160
{head_metrics.id: comparison},
159-
{},
161+
{head_artifact.id: base_artifact},
160162
{},
161163
)
162164

@@ -220,7 +222,6 @@ def test_changes_show_needs_approval(self) -> None:
220222
)
221223

222224
assert "Needs approval" in result
223-
# Counts with changes should be linked
224225
assert "?section=added" in result
225226
assert "?section=removed" in result
226227
assert "?section=changed" in result
@@ -344,29 +345,25 @@ def test_table_header_present(self) -> None:
344345

345346

346347
@cell_silo_test
347-
class FormatSnapshotPrCommentSoloTest(SnapshotPrCommentTestBase):
348-
def test_solo_upload(self) -> None:
348+
class FormatSnapshotPrCommentNoBaseTest(SnapshotPrCommentTestBase):
349+
def test_no_base_shows_uploaded_with_count(self) -> None:
349350
artifact, metrics = self._create_artifact_with_metrics(app_name="My App", image_count=15)
350351

351-
result = format_snapshot_pr_comment_solo([artifact], {artifact.id: metrics})
352+
result = format_snapshot_pr_comment([artifact], {artifact.id: metrics}, {}, {}, {})
352353

353354
assert "## Sentry Snapshot Testing" in result
354-
assert "| Name | Snapshots | Status |" in result
355+
assert "15 uploaded" in result
355356
assert "My App" in result
356-
assert "| 15 |" in result
357-
assert "Uploaded" in result
357+
assert "`com.example.app`" in result
358358

359-
def test_solo_no_metrics_shows_processing(self) -> None:
360-
artifact = self.create_preprod_artifact(
361-
project=self.project,
362-
app_id="com.example.app",
363-
)
359+
def test_no_base_uses_same_table_header(self) -> None:
360+
artifact, metrics = self._create_artifact_with_metrics()
364361

365-
result = format_snapshot_pr_comment_solo([artifact], {})
362+
result = format_snapshot_pr_comment([artifact], {artifact.id: metrics}, {}, {}, {})
366363

367-
assert "Processing" in result
364+
assert "| Name | Added | Removed | Modified | Renamed | Unchanged | Status |" in result
368365

369-
def test_solo_multiple_artifacts(self) -> None:
366+
def test_no_base_multiple_artifacts(self) -> None:
370367
artifacts = []
371368
metrics_map: dict[int, PreprodSnapshotMetrics] = {}
372369

@@ -377,14 +374,16 @@ def test_solo_multiple_artifacts(self) -> None:
377374
artifacts.append(artifact)
378375
metrics_map[artifact.id] = metrics
379376

380-
result = format_snapshot_pr_comment_solo(artifacts, metrics_map)
377+
result = format_snapshot_pr_comment(artifacts, metrics_map, {}, {}, {})
381378

382379
assert "com.example.app0" in result
383380
assert "com.example.app1" in result
381+
assert "5 uploaded" in result
382+
assert "6 uploaded" in result
384383

385-
def test_app_id_shown_in_name_cell(self) -> None:
384+
def test_no_base_app_id_shown(self) -> None:
386385
artifact, metrics = self._create_artifact_with_metrics(app_id="com.example.myapp")
387386

388-
result = format_snapshot_pr_comment_solo([artifact], {artifact.id: metrics})
387+
result = format_snapshot_pr_comment([artifact], {artifact.id: metrics}, {}, {}, {})
389388

390389
assert "`com.example.myapp`" in result

0 commit comments

Comments
 (0)