Skip to content

Commit 7f2f082

Browse files
authored
Add build_config handling to size comparison (#99558)
Adds `build_configuration` handling for comparisons. Also adds an error response to to the compare post endpoint to block invalid compares, but we'll make sure to not allow builds to be compared in the frontend if they can't be, just an extra level of safety Resolves EME-250
1 parent 4993e2f commit 7f2f082

File tree

4 files changed

+180
-0
lines changed

4 files changed

+180
-0
lines changed

src/sentry/preprod/api/endpoints/size_analysis/project_preprod_size_analysis_compare.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,11 @@ def post(
273273
extra={"head_artifact_id": head_artifact_id, "base_artifact_id": base_artifact_id},
274274
)
275275

276+
if head_artifact.build_configuration != base_artifact.build_configuration:
277+
return Response(
278+
{"error": "Head and base build configurations must be the same."}, status=400
279+
)
280+
276281
head_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter(
277282
preprod_artifact_id__in=[head_artifact.id],
278283
preprod_artifact__project=project,

src/sentry/preprod/size_analysis/tasks.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,13 @@ def compare_preprod_artifact_size_analysis(
6161
# Run all comparisons with artifact as head
6262
base_artifact = artifact.get_base_artifact_for_commit().first()
6363
if base_artifact:
64+
if artifact.build_configuration != base_artifact.build_configuration:
65+
logger.info(
66+
"preprod.size_analysis.compare.artifact_different_build_configurations",
67+
extra={"head_artifact_id": artifact_id, "base_artifact_id": base_artifact.id},
68+
)
69+
return
70+
6471
base_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter(
6572
preprod_artifact_id__in=[base_artifact.id],
6673
preprod_artifact__project__organization_id=org_id,
@@ -112,6 +119,13 @@ def compare_preprod_artifact_size_analysis(
112119
# Also run comparisons with artifact as base
113120
head_artifacts = artifact.get_head_artifacts_for_commit()
114121
for head_artifact in head_artifacts:
122+
if head_artifact.build_configuration != artifact.build_configuration:
123+
logger.info(
124+
"preprod.size_analysis.compare.head_artifact_different_build_configurations",
125+
extra={"head_artifact_id": head_artifact.id, "base_artifact_id": artifact_id},
126+
)
127+
continue
128+
115129
head_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter(
116130
preprod_artifact_id__in=[head_artifact.id],
117131
preprod_artifact__project__organization_id=org_id,
@@ -195,6 +209,14 @@ def manual_size_analysis_comparison(
195209
)
196210
return
197211

212+
# Should never be hit as we block this in manual compare endpoint, but safety check just in case
213+
if head_artifact.build_configuration != base_artifact.build_configuration:
214+
logger.info(
215+
"preprod.size_analysis.compare.manual.different_build_configurations",
216+
extra={"head_artifact_id": head_artifact.id, "base_artifact_id": base_artifact.id},
217+
)
218+
return
219+
198220
head_size_metrics_qs = PreprodArtifactSizeMetrics.objects.filter(
199221
preprod_artifact_id__in=[head_artifact.id],
200222
preprod_artifact__project__organization_id=org_id,

tests/sentry/preprod/api/endpoints/size_analysis/test_project_preprod_size_analysis_compare.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
PreprodArtifact,
99
PreprodArtifactSizeComparison,
1010
PreprodArtifactSizeMetrics,
11+
PreprodBuildConfiguration,
1112
)
1213
from sentry.testutils.cases import APITestCase
1314

@@ -640,3 +641,24 @@ def test_get_comparison_multiple_metrics(self):
640641
assert watch_comparison_data["head_size_metric_id"] == head_watch_metric.id
641642
assert watch_comparison_data["base_size_metric_id"] == base_watch_metric.id
642643
assert watch_comparison_data["comparison_id"] == watch_comparison.id
644+
645+
@override_settings(SENTRY_FEATURES={"organizations:preprod-frontend-routes": True})
646+
def test_post_comparison_different_build_configurations(self):
647+
"""Test POST endpoint returns 400 when artifacts have different build configurations"""
648+
# Create a build configuration for the base artifact
649+
debug_config = PreprodBuildConfiguration.objects.create(project=self.project, name="debug")
650+
651+
# Update base artifact to have different build configuration
652+
self.base_artifact.build_configuration = debug_config
653+
self.base_artifact.save()
654+
655+
# Head artifact will have None/default, base will have debug config
656+
response = self.get_error_response(
657+
self.organization.slug,
658+
self.project.slug,
659+
self.head_artifact.id,
660+
self.base_artifact.id,
661+
method="post",
662+
status_code=400,
663+
)
664+
assert response.data["error"] == "Head and base build configurations must be the same."

tests/sentry/preprod/size_analysis/test_size_analysis_tasks.py

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
PreprodArtifact,
1010
PreprodArtifactSizeComparison,
1111
PreprodArtifactSizeMetrics,
12+
PreprodBuildConfiguration,
1213
)
1314
from sentry.preprod.size_analysis.tasks import (
1415
_run_size_analysis_comparison,
@@ -371,6 +372,136 @@ def test_compare_preprod_artifact_size_analysis_cannot_compare_metrics(self):
371372
# Should not call _run_size_analysis_comparison due to incompatible metrics
372373
mock_run_comparison.assert_not_called()
373374

375+
def test_compare_preprod_artifact_size_analysis_different_build_configurations_as_head(self):
376+
"""Test compare_preprod_artifact_size_analysis with different build configurations when artifact is head."""
377+
# Create commit comparisons
378+
head_commit = CommitComparison.objects.create(
379+
organization_id=self.organization.id,
380+
head_sha="a" * 40,
381+
base_sha="b" * 40,
382+
provider="github",
383+
head_repo_name="owner/repo",
384+
base_repo_name="owner/repo",
385+
)
386+
base_commit = CommitComparison.objects.create(
387+
organization_id=self.organization.id,
388+
head_sha="b" * 40,
389+
base_sha="c" * 40,
390+
provider="github",
391+
head_repo_name="owner/repo",
392+
base_repo_name="owner/repo",
393+
)
394+
395+
# Create build configurations
396+
debug_config = PreprodBuildConfiguration.objects.create(project=self.project, name="debug")
397+
release_config = PreprodBuildConfiguration.objects.create(
398+
project=self.project, name="release"
399+
)
400+
401+
# Create artifacts with different build configurations
402+
head_artifact = PreprodArtifact.objects.create(
403+
project=self.project,
404+
commit_comparison=head_commit,
405+
app_id="com.example.app",
406+
build_version="1.0.0",
407+
build_number=1,
408+
build_configuration=debug_config,
409+
state=PreprodArtifact.ArtifactState.PROCESSED,
410+
)
411+
base_artifact = PreprodArtifact.objects.create(
412+
project=self.project,
413+
commit_comparison=base_commit,
414+
app_id="com.example.app",
415+
build_version="1.0.0",
416+
build_number=1,
417+
build_configuration=release_config,
418+
state=PreprodArtifact.ArtifactState.PROCESSED,
419+
)
420+
421+
with (
422+
patch(
423+
"sentry.preprod.size_analysis.tasks._run_size_analysis_comparison"
424+
) as mock_run_comparison,
425+
patch("sentry.preprod.size_analysis.tasks.logger") as mock_logger,
426+
):
427+
compare_preprod_artifact_size_analysis(
428+
project_id=self.project.id,
429+
org_id=self.organization.id,
430+
artifact_id=head_artifact.id,
431+
)
432+
433+
# Should log different build configurations and not run comparison
434+
mock_logger.info.assert_called_with(
435+
"preprod.size_analysis.compare.artifact_different_build_configurations",
436+
extra={"head_artifact_id": head_artifact.id, "base_artifact_id": base_artifact.id},
437+
)
438+
mock_run_comparison.assert_not_called()
439+
440+
def test_compare_preprod_artifact_size_analysis_different_build_configurations_as_base(self):
441+
"""Test compare_preprod_artifact_size_analysis with different build configurations when artifact is base."""
442+
# Create commit comparison
443+
base_commit = CommitComparison.objects.create(
444+
organization_id=self.organization.id,
445+
head_sha="a" * 40,
446+
base_sha="b" * 40,
447+
provider="github",
448+
head_repo_name="owner/repo",
449+
base_repo_name="owner/repo",
450+
)
451+
head_commit = CommitComparison.objects.create(
452+
organization_id=self.organization.id,
453+
head_sha="c" * 40,
454+
base_sha="a" * 40,
455+
provider="github",
456+
head_repo_name="owner/repo",
457+
base_repo_name="owner/repo",
458+
)
459+
460+
# Create build configurations
461+
debug_config = PreprodBuildConfiguration.objects.create(project=self.project, name="debug")
462+
release_config = PreprodBuildConfiguration.objects.create(
463+
project=self.project, name="release"
464+
)
465+
466+
# Create artifacts with different build configurations
467+
base_artifact = PreprodArtifact.objects.create(
468+
project=self.project,
469+
commit_comparison=base_commit,
470+
app_id="com.example.app",
471+
build_version="1.0.0",
472+
build_number=1,
473+
build_configuration=debug_config,
474+
state=PreprodArtifact.ArtifactState.PROCESSED,
475+
)
476+
head_artifact = PreprodArtifact.objects.create(
477+
project=self.project,
478+
commit_comparison=head_commit,
479+
app_id="com.example.app",
480+
build_version="1.0.0",
481+
build_number=1,
482+
build_configuration=release_config,
483+
state=PreprodArtifact.ArtifactState.PROCESSED,
484+
)
485+
486+
with (
487+
patch(
488+
"sentry.preprod.size_analysis.tasks._run_size_analysis_comparison"
489+
) as mock_run_comparison,
490+
patch("sentry.preprod.size_analysis.tasks.logger") as mock_logger,
491+
):
492+
compare_preprod_artifact_size_analysis(
493+
project_id=self.project.id,
494+
org_id=self.organization.id,
495+
artifact_id=base_artifact.id,
496+
)
497+
498+
# Should log different build configurations and not run comparison
499+
mock_logger.info.assert_called_with(
500+
"preprod.size_analysis.compare.head_artifact_different_build_configurations",
501+
extra={"head_artifact_id": head_artifact.id, "base_artifact_id": base_artifact.id},
502+
)
503+
mock_run_comparison.assert_not_called()
504+
374505

375506
class ManualSizeAnalysisComparisonTest(TestCase):
376507
def setUp(self):

0 commit comments

Comments
 (0)