Skip to content

Commit

Permalink
feat: update bundle_analysis comment requirement (#532)
Browse files Browse the repository at this point in the history
  • Loading branch information
@giovanni-guidini
giovanni-guidini authored Jul 4, 2024
1 parent 68281f2 commit d47749f
Show file tree
Hide file tree
Showing 5 changed files with 172 additions and 48 deletions.
2 changes: 1 addition & 1 deletion requirements.in
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
https://github.com/codecov/shared/archive/390e62c4858ee41ffd035e5d7fe77c71a179d76a.tar.gz#egg=shared
https://github.com/codecov/shared/archive/3b44680b87559e7fe69b3e9645172a99f6bdbf98.tar.gz#egg=shared
https://github.com/codecov/opentelem-python/archive/refs/tags/v0.0.4a1.tar.gz#egg=codecovopentelem
https://github.com/codecov/test-results-parser/archive/1507de2241601d678e514c08b38426e48bb6d47d.tar.gz#egg=test-results-parser
boto3>=1.34
Expand Down
4 changes: 2 additions & 2 deletions requirements.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ sentry-sdk==1.40.0
# via
# -r requirements.in
# shared
shared @ https://github.com/codecov/shared/archive/390e62c4858ee41ffd035e5d7fe77c71a179d76a.tar.gz
shared @ https://github.com/codecov/shared/archive/3b44680b87559e7fe69b3e9645172a99f6bdbf98.tar.gz
# via -r requirements.in
six==1.16.0
# via
Expand Down Expand Up @@ -432,7 +432,7 @@ tzdata==2024.1
# via celery
tzlocal==5.2
# via timeconvert
urllib3==1.26.18
urllib3==1.26.19
# via
# -r requirements.in
# botocore
Expand Down
93 changes: 62 additions & 31 deletions services/bundle_analysis.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import tempfile
from dataclasses import dataclass
from functools import cached_property
from typing import Any, Dict, Iterator, List, Optional, Tuple
from typing import Any, Dict, Iterator, List, Literal, Optional, Tuple

import sentry_sdk
from sentry_sdk import metrics as sentry_metrics
Expand Down Expand Up @@ -507,6 +507,45 @@ def bundle_analysis_loader(self):
def bundle_report(self) -> Optional[BundleAnalysisReport]:
return self.bundle_analysis_loader.load(self.commit_report.external_id)

def _has_required_changes(
self, comparison: BundleAnalysisComparison, pull: EnrichedPull
) -> bool:
"""Verifies if the notifier should notify according to the configured required_changes.
Changes are defined by the user in UserYaml. Default is "always notify".
Required changes are bypassed if a comment already exists, because we should update the comment.
"""
if pull.database_pull.bundle_analysis_commentid:
log.info(
"Skipping required_changes verification because comment already exists",
extra=dict(pullid=pull.database_pull.id, commitid=self.commit.commitid),
)
return True

required_changes: bool | Literal["bundle_increase"] = read_yaml_field(
self.current_yaml, ("comment", "require_bundle_changes"), False
)
changes_threshold: int = read_yaml_field(
self.current_yaml, ("comment", "bundle_change_threshold"), 0
)
match required_changes:
case False:
return True
case True:
return abs(comparison.total_size_delta) > changes_threshold
case "bundle_increase":
return (
comparison.total_size_delta > 0
and comparison.total_size_delta > changes_threshold
)
case _:
log.warning(
"Unknown value for required_changes",
extra=dict(
pull=pull.database_pull.id, commitid=self.commit.commitid
),
)
return True

async def notify(self) -> bool:
if self.commit_report is None:
log.warning(
Expand Down Expand Up @@ -541,45 +580,37 @@ async def notify(self) -> bool:

pullid = pull.database_pull.pullid
bundle_comparison = ComparisonLoader(pull).get_comparison()
skip_comment_without_size_changes = (
bundle_comparison.total_size_delta == 0
and read_yaml_field(
self.current_yaml, ("comment", "require_bundle_changes"), False
)
)

if skip_comment_without_size_changes:
if not self._has_required_changes(bundle_comparison, pull):
# Skips the comment and returns successful notification
log.info(
"Skipping bundle PR comment without size change",
"Not enough changes to notify bundle PR comment",
extra=dict(
commitid=self.commit.commitid,
pullid=pullid,
),
)
return True
else:
message = self._build_message(pull=pull, comparison=bundle_comparison)
try:
comment_id = pull.database_pull.bundle_analysis_commentid
if comment_id:
await self.repository_service.edit_comment(
pullid, comment_id, message
)
else:
res = await self.repository_service.post_comment(pullid, message)
pull.database_pull.bundle_analysis_commentid = res["id"]
return True
except TorngitClientError:
log.error(
"Error creating/updating PR comment",
extra=dict(
commitid=self.commit.commitid,
report_key=self.commit_report.external_id,
pullid=pullid,
),
)
return False

message = self._build_message(pull=pull, comparison=bundle_comparison)
try:
comment_id = pull.database_pull.bundle_analysis_commentid
if comment_id:
await self.repository_service.edit_comment(pullid, comment_id, message)
else:
res = await self.repository_service.post_comment(pullid, message)
pull.database_pull.bundle_analysis_commentid = res["id"]
return True
except TorngitClientError:
log.error(
"Error creating/updating PR comment",
extra=dict(
commitid=self.commit.commitid,
report_key=self.commit_report.external_id,
pullid=pullid,
),
)
return False

def _build_message(
self, pull: EnrichedPull, comparison: BundleAnalysisComparison
Expand Down
119 changes: 106 additions & 13 deletions services/tests/test_bundle_analysis.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,47 @@ def test_bytes_readable():


@pytest.mark.asyncio
@pytest.mark.parametrize(
"config, should_notify",
[
pytest.param({}, True, id="default_config"),
pytest.param(
{"comment": {"require_bundle_changes": False}},
True,
id="no_required_changes",
),
pytest.param(
{"comment": {"require_bundle_changes": True}}, True, id="required_changes"
),
pytest.param(
{"comment": {"require_bundle_changes": "bundle_increase"}},
True,
id="required_increase",
),
pytest.param(
{
"comment": {
"require_bundle_changes": "bundle_increase",
"bundle_change_threshold": 1000,
}
},
True,
id="required_increase_with_small_threshold",
),
pytest.param(
{
"comment": {
"require_bundle_changes": "bundle_increase",
"bundle_change_threshold": 1000000,
}
},
False,
id="required_increase_with_big_threshold",
),
],
)
async def test_bundle_analysis_notify(
dbsession, mocker, mock_storage, mock_repo_provider
config, should_notify, dbsession, mocker, mock_storage, mock_repo_provider
):
base_commit = CommitFactory()
dbsession.add(base_commit)
Expand All @@ -59,7 +98,7 @@ async def test_bundle_analysis_notify(
dbsession.add(pull)
dbsession.commit()

notifier = Notifier(head_commit, UserYaml.from_dict({}))
notifier = Notifier(head_commit, UserYaml.from_dict(config))

repo_key = ArchiveService.get_archive_hash(base_commit.repository)
mock_storage.write_file(
Expand Down Expand Up @@ -123,20 +162,69 @@ async def test_bundle_analysis_notify(

success = await notifier.notify()
assert success == True
mock_repo_provider.post_comment.assert_called_once_with(
pull.pullid, expected_message_increase
)
if should_notify:
assert pull.bundle_analysis_commentid is not None
mock_repo_provider.post_comment.assert_called_once_with(
pull.pullid, expected_message_increase
)
else:
assert pull.bundle_analysis_commentid is None
mock_repo_provider.post_comment.assert_not_called()

success = await notifier.notify()
assert success == True
mock_repo_provider.edit_comment.assert_called_once_with(
pull.pullid, "test-comment-id", expected_message_increase
)
if should_notify:
assert pull.bundle_analysis_commentid is not None
mock_repo_provider.edit_comment.assert_called_once_with(
pull.pullid, "test-comment-id", expected_message_increase
)
else:
assert pull.bundle_analysis_commentid is None
mock_repo_provider.edit_comment.assert_not_called()


@pytest.mark.asyncio
@pytest.mark.parametrize(
"config, should_notify",
[
pytest.param({}, True, id="default_config"),
pytest.param(
{"comment": {"require_bundle_changes": False}},
True,
id="no_required_changes",
),
pytest.param(
{"comment": {"require_bundle_changes": True}}, True, id="required_changes"
),
pytest.param(
{"comment": {"require_bundle_changes": "bundle_increase"}},
False,
id="required_increase",
),
pytest.param(
{
"comment": {
"require_bundle_changes": True,
"bundle_change_threshold": 1000,
}
},
True,
id="required_increase_with_small_threshold",
),
pytest.param(
{
"comment": {
"require_bundle_changes": True,
"bundle_change_threshold": 1000000,
}
},
False,
id="required_increase_with_big_threshold",
),
],
)
async def test_bundle_analysis_notify_size_decrease(
dbsession, mocker, mock_storage, mock_repo_provider
config, should_notify, dbsession, mocker, mock_storage, mock_repo_provider
):
base_commit = CommitFactory()
dbsession.add(base_commit)
Expand All @@ -161,7 +249,7 @@ async def test_bundle_analysis_notify_size_decrease(
dbsession.add(pull)
dbsession.commit()

notifier = Notifier(head_commit, UserYaml.from_dict({}))
notifier = Notifier(head_commit, UserYaml.from_dict(config))

repo_key = ArchiveService.get_archive_hash(base_commit.repository)
mock_storage.write_file(
Expand Down Expand Up @@ -216,9 +304,14 @@ async def test_bundle_analysis_notify_size_decrease(

success = await notifier.notify()
assert success == True
mock_repo_provider.post_comment.assert_called_once_with(
pull.pullid, expected_message_decrease
)
if should_notify:
assert pull.bundle_analysis_commentid is not None
mock_repo_provider.post_comment.assert_called_once_with(
pull.pullid, expected_message_decrease
)
else:
assert pull.bundle_analysis_commentid is None
mock_repo_provider.post_comment.assert_not_called()


@pytest.mark.asyncio
Expand Down
2 changes: 1 addition & 1 deletion services/yaml/tests/test_yaml.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def test_get_final_yaml_no_thing_set_at_all(self, mocker, mock_configuration):
mock_configuration, "load_yaml_file", side_effect=FileNotFoundError()
)
expected_result = {
"codecov": {"require_ci_to_pass": True},
"codecov": {"require_ci_to_pass": True, "notify": {"wait_for_ci": True}},
"coverage": {
"precision": 2,
"round": "down",
Expand Down

0 comments on commit d47749f

Please sign in to comment.