diff --git a/database/models/core.py b/database/models/core.py index b91830578..693d46cd3 100644 --- a/database/models/core.py +++ b/database/models/core.py @@ -2,6 +2,7 @@ import string import uuid from datetime import datetime +from functools import cached_property from sqlalchemy import Column, ForeignKey, Index, UniqueConstraint, types from sqlalchemy.dialects import postgresql @@ -297,7 +298,9 @@ class Pull(CodecovBaseModel): behind_by_commit = Column(types.Text) author = relationship(Owner) - repository = relationship(Repository, backref=backref("pulls", cascade="delete")) + repository = relationship( + Repository, backref=backref("pulls", cascade="delete", lazy="dynamic") + ) __table_args__ = (Index("pulls_repoid_pullid", "repoid", "pullid", unique=True),) @@ -355,6 +358,13 @@ def should_write_to_storage(self) -> bool: repoid=self.repository.repoid, ) + @cached_property + def is_first_pull(self): + first_pull = ( + self.repository.pulls.with_entities(Pull.id_).order_by(Pull.id_).first() + ) + return first_pull.id_ == self.id_ + _flare = Column("flare", postgresql.JSON) _flare_storage_path = Column("flare_storage_path", types.Text, nullable=True) flare = ArchiveField( diff --git a/database/tests/factories/core.py b/database/tests/factories/core.py index 25d7ca1c4..d82b3af95 100644 --- a/database/tests/factories/core.py +++ b/database/tests/factories/core.py @@ -128,6 +128,7 @@ class Meta: "s": 1, } ) + _report_json_storage_path = None _report_json = factory.LazyFunction( lambda: { "files": { diff --git a/dockerscripts/Dockerfile.circle b/dockerscripts/Dockerfile.circle index 61375c394..4f6eacf08 100644 --- a/dockerscripts/Dockerfile.circle +++ b/dockerscripts/Dockerfile.circle @@ -1,9 +1,7 @@ # syntax=docker/dockerfile:1.3 ARG REQUIREMENTS_IMAGE -FROM busybox:1.34.1 as berglas -WORKDIR /tmp -ADD https://storage.googleapis.com/berglas/0.5.0/linux_amd64/berglas berglas -RUN echo "99dc4b3146d19b7e36cc6d339eeefa6a959a6d86143b7317e722d09a3b57ca45 berglas" | sha256sum -c +ARG BERGLAS_VERSION=1.0.3 +FROM us-docker.pkg.dev/berglas/berglas/berglas:$BERGLAS_VERSION as berglas FROM $REQUIREMENTS_IMAGE @@ -12,7 +10,7 @@ ARG RELEASE_VERSION ENV RELEASE_VERSION $RELEASE_VERSION -COPY --chmod=755 --from=berglas /tmp/berglas /usr/local/bin/berglas +COPY --chmod=755 --from=berglas /bin/berglas /usr/local/bin/berglas USER codecov diff --git a/requirements.in b/requirements.in index ccbee6ceb..25cddc4d7 100644 --- a/requirements.in +++ b/requirements.in @@ -1,4 +1,4 @@ -git+ssh://git@github.com/codecov/shared.git@f0174635ccaebc3463a5641b9ad640a46b5fd472#egg=shared +git+ssh://git@github.com/codecov/shared.git@bef9260a4b6218b5ce4b5b9152a71d7e6d63a954#egg=shared git+ssh://git@github.com/codecov/opentelem-python.git@v0.0.4a1#egg=codecovopentelem boto3 celery diff --git a/requirements.txt b/requirements.txt index cac8f38ed..3e4d57f1a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,5 +1,5 @@ # -# This file is autogenerated by pip-compile with Python 3.10 +# This file is autogenerated by pip-compile with Python 3.8 # by the following command: # # pip-compile requirements.in @@ -251,7 +251,7 @@ s3transfer==0.3.4 # via boto3 sentry-sdk==1.19.1 # via -r requirements.in -shared @ git+ssh://git@github.com/codecov/shared.git@f0174635ccaebc3463a5641b9ad640a46b5fd472 +shared @ git+ssh://git@github.com/codecov/shared.git@bef9260a4b6218b5ce4b5b9152a71d7e6d63a954 # via -r requirements.in six==1.15.0 # via diff --git a/services/notification/__init__.py b/services/notification/__init__.py index 190dfbd89..35c5266c6 100644 --- a/services/notification/__init__.py +++ b/services/notification/__init__.py @@ -107,14 +107,17 @@ def get_notifiers_instances(self) -> Iterator[AbstractBaseNotifier]: decoration_type=self.decoration_type, ) - yield CodecovSlackAppNotifier( - repository=self.repository, - title="codecov-slack-app", - notifier_yaml_settings={}, - notifier_site_settings={}, - current_yaml=self.current_yaml, - decoration_type=self.decoration_type, - ) + # yield notifier if slack_app field is True, nonexistent, or a non-empty dict + slack_app_yaml_field = read_yaml_field(self.current_yaml, ("slack_app",), True) + if slack_app_yaml_field: + yield CodecovSlackAppNotifier( + repository=self.repository, + title="codecov-slack-app", + notifier_yaml_settings=slack_app_yaml_field, + notifier_site_settings={}, + current_yaml=self.current_yaml, + decoration_type=self.decoration_type, + ) comment_yaml_field = read_yaml_field(self.current_yaml, ("comment",)) if comment_yaml_field: diff --git a/services/notification/notifiers/codecov_slack_app.py b/services/notification/notifiers/codecov_slack_app.py index c711d3124..bf592c295 100644 --- a/services/notification/notifiers/codecov_slack_app.py +++ b/services/notification/notifiers/codecov_slack_app.py @@ -26,7 +26,16 @@ def notification_type(self) -> Notification: return Notification.codecov_slack_app def is_enabled(self) -> bool: - return True + # if yaml settings are a dict, then check the enabled key and return that + # the enabled field should always exist if the yaml settings are a dict because otherwise it would fail the validation + + # else if the yaml settings is a boolean then just return that + + # in any case, self.notifier_yaml_settings should either be a bool or a dict always and should never be None + if isinstance(self.notifier_yaml_settings, dict): + return self.notifier_yaml_settings.get("enabled", False) + elif isinstance(self.notifier_yaml_settings, bool): + return self.notifier_yaml_settings def store_results(self, comparison: Comparison, result: NotificationResult): pass diff --git a/services/notification/notifiers/comment/__init__.py b/services/notification/notifiers/comment/__init__.py index 2cd2ab7a3..b65c6c70f 100644 --- a/services/notification/notifiers/comment/__init__.py +++ b/services/notification/notifiers/comment/__init__.py @@ -8,6 +8,7 @@ ) from database.enums import Notification +from database.models import Pull from helpers.metrics import metrics from services.comparison.types import Comparison from services.license import requires_license @@ -358,11 +359,22 @@ async def build_message(self, comparison: Comparison) -> List[str]: return self._create_empty_upload_message() if self.should_use_upload_limit_decoration(): return self._create_reached_upload_limit_message(comparison) + if comparison.pull.is_first_pull: + return self._create_welcome_message() pull_dict = comparison.enriched_pull.provider_pull return await self.create_message( comparison, pull_dict, self.notifier_yaml_settings ) + def _create_welcome_message(self): + return [ + "## Welcome to [Codecov](https://codecov.io) :tada:", + "", + "Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.", + "", + "Thanks for integrating Codecov - We've got you covered :open_umbrella:", + ] + def _create_empty_upload_message(self): if self.is_passing_empty_upload(): return [ diff --git a/services/notification/notifiers/mixins/message/__init__.py b/services/notification/notifiers/mixins/message/__init__.py index 2bb333f61..0df9debf8 100644 --- a/services/notification/notifiers/mixins/message/__init__.py +++ b/services/notification/notifiers/mixins/message/__init__.py @@ -156,8 +156,6 @@ async def create_message( section_writer, ) - await self._write_feedback_link(comparison, settings, write) - return [m for m in message if m is not None] async def _possibly_write_gh_app_login_announcement( @@ -174,26 +172,6 @@ async def _possibly_write_gh_app_login_announcement( write(f":exclamation: {message_to_display}") write("") - async def _write_feedback_link( - self, comparison: ComparisonProxy, settings: dict, write: Callable - ): - hide_project_coverage = settings.get("hide_project_coverage", False) - if hide_project_coverage: - write( - ":loudspeaker: Thoughts on this report? [Let us know!]({0}).".format( - "https://about.codecov.io/pull-request-comment-report/" - ) - ) - else: - repo_service = comparison.repository_service.service - write( - ":loudspeaker: Have feedback on the report? [Share it here]({0}).".format( - "https://gitlab.com/codecov-open-source/codecov-user-feedback/-/issues/4" - if repo_service == "gitlab" - else "https://about.codecov.io/codecov-pr-comment-feedback/" - ) - ) - async def write_section_to_msg( self, comparison, changes, diff, links, write, section_writer, behind_by=None ): diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py index fb10fe4a5..4754c8203 100644 --- a/services/notification/notifiers/mixins/message/sections.py +++ b/services/notification/notifiers/mixins/message/sections.py @@ -81,13 +81,28 @@ async def write_section(*args, **kwargs): class NewFooterSectionWriter(BaseSectionWriter): async def do_write_section(self, comparison, diff, changes, links, behind_by=None): hide_project_coverage = self.settings.get("hide_project_coverage", False) - if not hide_project_coverage: + if hide_project_coverage: + yield ("") + yield ( + ":loudspeaker: Thoughts on this report? [Let us know!]({0}).".format( + "https://about.codecov.io/pull-request-comment-report/" + ) + ) + else: + repo_service = comparison.repository_service.service yield ("") yield ( "[:umbrella: View full report in Codecov by Sentry]({0}?src=pr&el=continue). ".format( links["pull"] ) ) + yield ( + ":loudspeaker: Have feedback on the report? [Share it here]({0}).".format( + "https://about.codecov.io/codecov-pr-comment-feedback/" + if repo_service == "github" + else "https://gitlab.com/codecov-open-source/codecov-user-feedback/-/issues/4" + ) + ) class NewHeaderSectionWriter(BaseSectionWriter): diff --git a/services/notification/notifiers/tests/integration/test_comment.py b/services/notification/notifiers/tests/integration/test_comment.py index 279697b20..0af653359 100644 --- a/services/notification/notifiers/tests/integration/test_comment.py +++ b/services/notification/notifiers/tests/integration/test_comment.py @@ -1,4 +1,4 @@ -from unittest.mock import patch +from unittest.mock import PropertyMock, patch import pytest from shared.reports.readonly import ReadOnlyReport @@ -10,6 +10,15 @@ from services.notification.notifiers.comment import CommentNotifier +@pytest.fixture +def is_not_first_pull(mocker): + mocker.patch( + "database.models.core.Pull.is_first_pull", + return_value=False, + new_callable=PropertyMock, + ) + + @pytest.fixture def codecove2e_comparison(dbsession, request, sample_report, small_report): repository = RepositoryFactory.create( @@ -319,6 +328,7 @@ def sample_comparison_for_limited_upload( ) +@pytest.mark.usefixtures("is_not_first_pull") class TestCommentNotifierIntegration(object): @pytest.mark.asyncio async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration): @@ -380,7 +390,6 @@ async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration): "> `Δ = absolute (impact)`, `ø = not affected`, `? = missing data`", "> Powered by [Codecov](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=footer). Last update [5b174c2...5601846](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", "", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(result.data_sent["message"], message): assert exp == res @@ -524,7 +533,6 @@ async def test_notify_gitlab( "> `Δ = absolute (impact)`, `ø = not affected`, `? = missing data`", "> Powered by [Codecov](https://app.codecov.io/gl/joseph-sentry/example-python/pull/1?src=pr&el=footer). Last update [0fc784a...0b6a213](https://app.codecov.io/gl/joseph-sentry/example-python/pull/1?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", "", - f":loudspeaker: Have feedback on the report? [Share it here](https://gitlab.com/codecov-open-source/codecov-user-feedback/-/issues/4).", ] for exp, res in zip(result.data_sent["message"], message): assert exp == res @@ -592,8 +600,8 @@ async def test_notify_new_layout( "", "", "[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=continue). ", - "", ":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", + "", ] for exp, res in zip(result.data_sent["message"], message): assert exp == res @@ -672,8 +680,8 @@ async def test_notify_with_components( "", "", "[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=continue). ", - "", ":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", + "", ] for exp, res in zip(result.data_sent["message"], message): assert exp == res diff --git a/services/notification/notifiers/tests/unit/test_checks.py b/services/notification/notifiers/tests/unit/test_checks.py index 4491a5cef..d3704e661 100644 --- a/services/notification/notifiers/tests/unit/test_checks.py +++ b/services/notification/notifiers/tests/unit/test_checks.py @@ -1516,7 +1516,6 @@ async def test_build_default_payload( f"", f"... and [1 file with indirect coverage changes](test.example.br/gh/test_build_default_payload/{repo.name}/pull/{sample_comparison.pull.pullid}/indirect-changes?src=pr&el=tree-more)", f"", - ":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] ), }, @@ -1559,7 +1558,6 @@ async def test_build_default_payload_with_flags( f"", f"... and [1 file with indirect coverage changes](test.example.br/gh/test_build_default_payload_with_flags/{repo.name}/pull/{sample_comparison.pull.pullid}/indirect-changes?src=pr&el=tree-more)", f"", - ":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] ), }, @@ -1610,7 +1608,6 @@ async def test_build_default_payload_with_flags_and_footer( f"> `Δ = absolute (impact)`, `ø = not affected`, `? = missing data`", f"> Powered by [Codecov](test.example.br/gh/{test_name}/{repo.name}/pull/{sample_comparison.pull.pullid}?src=pr&el=footer). Last update [{base_commit.commitid[:7]}...{head_commit.commitid[:7]}](test.example.br/gh/{test_name}/{repo.name}/pull/{sample_comparison.pull.pullid}?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - ":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] ), }, diff --git a/services/notification/notifiers/tests/unit/test_codecov_slack_app.py b/services/notification/notifiers/tests/unit/test_codecov_slack_app.py index 27d5e8e3e..d63ba2a81 100644 --- a/services/notification/notifiers/tests/unit/test_codecov_slack_app.py +++ b/services/notification/notifiers/tests/unit/test_codecov_slack_app.py @@ -11,19 +11,30 @@ def test_is_enabled(self, dbsession, mock_configuration, sample_comparison): notifier = CodecovSlackAppNotifier( repository=sample_comparison.head.commit.repository, title="title", - notifier_yaml_settings={}, + notifier_yaml_settings={"enabled": True}, notifier_site_settings=True, - current_yaml={}, + current_yaml={"slack_app": {"enabled": True}}, ) assert notifier.is_enabled() == True + def test_is_enable_false(self, dbsession, mock_configuration, sample_comparison): + notifier = CodecovSlackAppNotifier( + repository=sample_comparison.head.commit.repository, + title="title", + notifier_yaml_settings={"enabled": False}, + notifier_site_settings=True, + current_yaml={"slack_app": {"enabled": False}}, + ) + + assert notifier.is_enabled() is False + def test_notification_type(self, dbsession, mock_configuration, sample_comparison): notifier = CodecovSlackAppNotifier( repository=sample_comparison.head.commit.repository, title="title", - notifier_yaml_settings={}, + notifier_yaml_settings={"enabled": True}, notifier_site_settings=True, - current_yaml={}, + current_yaml={"slack_app": {"enabled": True}}, ) assert notifier.notification_type == Notification.codecov_slack_app @@ -36,9 +47,9 @@ async def test_notify( notifier = CodecovSlackAppNotifier( repository=sample_comparison.head.commit.repository, title="title", - notifier_yaml_settings={}, + notifier_yaml_settings={"enabled": True}, notifier_site_settings=True, - current_yaml={}, + current_yaml={"slack_app": {"enabled": True}}, ) result = await notifier.notify(sample_comparison) assert result.notification_successful == True @@ -54,9 +65,9 @@ async def test_notify_failure( notifier = CodecovSlackAppNotifier( repository=sample_comparison.head.commit.repository, title="title", - notifier_yaml_settings={}, + notifier_yaml_settings={"enabled": True}, notifier_site_settings=True, - current_yaml={}, + current_yaml={"slack_app": {"enabled": True}}, ) result = await notifier.notify(sample_comparison) assert result.notification_successful == False @@ -74,9 +85,9 @@ async def test_notify_request_being_called( notifier = CodecovSlackAppNotifier( repository=sample_comparison.head.commit.repository, title="title", - notifier_yaml_settings={}, + notifier_yaml_settings={"enabled": True}, notifier_site_settings=True, - current_yaml={}, + current_yaml={"slack_app": {"enabled": True}}, ) result = await notifier.notify(sample_comparison) assert result.notification_successful == True diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py index e65d67bed..e44f83413 100644 --- a/services/notification/notifiers/tests/unit/test_comment.py +++ b/services/notification/notifiers/tests/unit/test_comment.py @@ -1,5 +1,5 @@ from decimal import Decimal -from unittest.mock import patch +from unittest.mock import PropertyMock, patch import pytest from shared.reports.readonly import ReadOnlyReport @@ -36,6 +36,15 @@ from services.yaml.reader import get_components_from_yaml +@pytest.fixture +def is_not_first_pull(mocker): + mocker.patch( + "database.models.core.Pull.is_first_pull", + return_value=False, + new_callable=PropertyMock, + ) + + @pytest.fixture def sample_comparison_bunch_empty_flags(request, dbsession, mocker): """ @@ -500,6 +509,7 @@ async def test__possibly_write_gh_app_login_announcement_enterprise( assert mock_write.call_count == 0 +@pytest.mark.usefixtures("is_not_first_pull") class TestCommentNotifier(object): @pytest.mark.asyncio async def test_is_enabled_settings_individual_settings_false(self, dbsession): @@ -703,7 +713,6 @@ async def test_create_message_files_section( f"| [file\\_1.go](https://app.codecov.io/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree#diff-ZmlsZV8xLmdv) | `62.50% <ø> (+12.50%)` | `10.00 <0.00> (-1.00)` | :arrow_up: |", f"| [file\\_2.py](https://app.codecov.io/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree#diff-ZmlsZV8yLnB5) | `50.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] res = await notifier.create_message(comparison, pull_dict, {"layout": "files"}) print(res) @@ -864,7 +873,6 @@ async def test_create_message_files_section_with_critical_files( f"| [file\\_2.py](https://app.codecov.io/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree#diff-ZmlsZV8yLnB5) **Critical** | `50.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |", f"", "", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] res = await notifier.build_message(comparison) assert expected_result == res @@ -938,7 +946,6 @@ async def test_build_message( f"[{sample_comparison.base.commit.commitid[:7]}...{sample_comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): assert exp == res @@ -982,7 +989,6 @@ async def test_build_message_flags_empty_coverage( "", "Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.", "", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] li = 0 print("\n".join(result)) @@ -1073,7 +1079,6 @@ async def test_build_message_more_sections( f"", f"", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): assert exp == res @@ -1333,7 +1338,6 @@ async def test_build_message_hide_complexity( f"[{sample_comparison.base.commit.commitid[:7]}...{sample_comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): assert exp == res @@ -1405,7 +1409,6 @@ async def test_build_message_no_base_report( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): assert exp == res @@ -1474,7 +1477,6 @@ async def test_build_message_no_base_commit( f"[cdf9aa4...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): assert exp == res @@ -1548,7 +1550,6 @@ async def test_build_message_no_change( f"[{sample_comparison_no_change.base.commit.commitid[:7]}...{sample_comparison_no_change.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) li = 0 @@ -1627,7 +1628,6 @@ async def test_build_message_negative_change( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) for exp, res in zip(expected_result, result): @@ -1692,7 +1692,6 @@ async def test_build_message_negative_change_tricky_rounding( f"- Misses 871 874 +3 ", f"```", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) li = 0 @@ -1747,7 +1746,6 @@ async def test_build_message_negative_change_tricky_rounding_newheader( f"Patch coverage has no change and project coverage change: **`-0.04%`** :warning:", f"> Comparison is base [(`{comparison.base.commit.commitid[:7]}`)](test.example.br/gh/{repository.slug}/commit/{comparison.base.commit.commitid}?el=desc) 88.58% compared to head [(`{comparison.head.commit.commitid[:7]}`)](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=desc) 88.54%.", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) li = 0 @@ -1823,7 +1821,6 @@ async def test_build_message_show_carriedforward_flags_no_cf_coverage( f"[{sample_comparison.base.commit.commitid[:7]}...{sample_comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): assert exp == res @@ -1888,7 +1885,6 @@ async def test_build_message_with_without_flags( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) li = 0 @@ -1958,7 +1954,6 @@ async def test_build_message_with_without_flags( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) li = 0 @@ -2035,7 +2030,6 @@ async def test_build_message_show_carriedforward_flags_has_cf_coverage( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) li = 0 @@ -2110,7 +2104,6 @@ async def test_build_message_hide_carriedforward_flags_has_cf_coverage( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] print(result) li = 0 @@ -2218,7 +2211,6 @@ async def test_build_message_no_flags( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): assert exp == res @@ -3236,7 +3228,6 @@ async def test_message_hide_details_github( f"[![Impacted file tree graph](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/graphs/tree.svg?width=650&height=150&src=pr&token={repository.image_token})](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree)", f"", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] li = 0 for exp, res in zip(expected_result, result): @@ -3269,7 +3260,6 @@ async def test_message_announcements_only( "", ":mega: message", "", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): if exp == ":mega: message": @@ -3307,7 +3297,6 @@ async def test_message_hide_details_bitbucket( f"", f"[![Impacted file tree graph](test.example.br/gh/{repository.slug}/pull/{pull.pullid}/graphs/tree.svg?width=650&height=150&src=pr&token={repository.image_token})](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree)", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] li = 0 for exp, res in zip(expected_result, result): @@ -3813,6 +3802,51 @@ async def test_footer_section_writer_in_github(self, mocker): assert res == [ "", "[:umbrella: View full report in Codecov by Sentry](pull.link?src=pr&el=continue). ", + ":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", + ] + + @pytest.mark.asyncio + async def test_footer_section_writer_in_gitlab(self, mocker): + writer = NewFooterSectionWriter( + mocker.MagicMock(), + mocker.MagicMock(), + mocker.MagicMock(), + settings={}, + current_yaml=mocker.MagicMock(), + ) + mock_comparison = mocker.MagicMock() + mock_comparison.repository_service.service = "gitlab" + res = list( + await writer.write_section( + mock_comparison, {}, [], links={"pull": "pull.link"} + ) + ) + assert res == [ + "", + "[:umbrella: View full report in Codecov by Sentry](pull.link?src=pr&el=continue). ", + ":loudspeaker: Have feedback on the report? [Share it here](https://gitlab.com/codecov-open-source/codecov-user-feedback/-/issues/4).", + ] + + @pytest.mark.asyncio + async def test_footer_section_writer_in_bitbucket(self, mocker): + writer = NewFooterSectionWriter( + mocker.MagicMock(), + mocker.MagicMock(), + mocker.MagicMock(), + settings={}, + current_yaml=mocker.MagicMock(), + ) + mock_comparison = mocker.MagicMock() + mock_comparison.repository_service.service = "bitbucket" + res = list( + await writer.write_section( + mock_comparison, {}, [], links={"pull": "pull.link"} + ) + ) + assert res == [ + "", + "[:umbrella: View full report in Codecov by Sentry](pull.link?src=pr&el=continue). ", + ":loudspeaker: Have feedback on the report? [Share it here](https://gitlab.com/codecov-open-source/codecov-user-feedback/-/issues/4).", ] @pytest.mark.asyncio @@ -3834,9 +3868,13 @@ async def test_footer_section_writer_with_project_cov_hidden(self, mocker): mock_comparison, {}, [], links={"pull": "pull.link"} ) ) - assert res == [] + assert res == [ + "", + ":loudspeaker: Thoughts on this report? [Let us know!](https://about.codecov.io/pull-request-comment-report/).", + ] +@pytest.mark.usefixtures("is_not_first_pull") class TestCommentNotifierInNewLayout(object): @pytest.mark.asyncio async def test_create_message_files_section_with_critical_files_new_layout( @@ -3993,7 +4031,6 @@ async def test_create_message_files_section_with_critical_files_new_layout( f"| [file\\_2.py](https://app.codecov.io/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=tree#diff-ZmlsZV8yLnB5) **Critical** | `50.00% <ø> (ø)` | `0.00 <0.00> (ø)` | |", f"", "", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] res = await notifier.build_message(comparison) assert expected_result == res @@ -4057,8 +4094,8 @@ async def test_build_message_no_base_commit_new_layout( f"", f"", f"[:umbrella: View full report in Codecov by Sentry](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=continue). ", - f"", f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", + f"", ] for exp, res in zip(expected_result, result): assert exp == res @@ -4127,8 +4164,8 @@ async def test_build_message_no_base_report_new_layout( f"", f"", f"[:umbrella: View full report in Codecov by Sentry](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=continue). ", - f"", f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", + f"", ] for exp, res in zip(expected_result, result): assert exp == res @@ -4168,6 +4205,7 @@ async def test_build_message_no_project_coverage( f"", f"", f":loudspeaker: Thoughts on this report? [Let us know!](https://about.codecov.io/pull-request-comment-report/).", + f"", ] for exp, res in zip(expected_result, result): assert exp == res @@ -4234,8 +4272,8 @@ async def test_build_message_head_and_pull_head_differ_new_layout( f"", f"", f"[:umbrella: View full report in Codecov by Sentry](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=continue). ", - f"", f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", + f"", ] for exp, res in zip(expected_result, result): assert exp == res @@ -4314,8 +4352,8 @@ async def test_build_message_head_and_pull_head_differ_with_components( f"", f"", f"[:umbrella: View full report in Codecov by Sentry](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=continue). ", - f"", f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", + f"", ] for exp, res in zip(expected_result, result): assert exp == res @@ -4385,7 +4423,6 @@ async def test_build_message_no_patch_or_proj_change( f"[{comparison.base.commit.commitid[:7]}...{comparison.head.commit.commitid[:7]}](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?src=pr&el=lastupdated). " f"Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", f"", - f":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ] for exp, res in zip(expected_result, result): @@ -4689,3 +4726,29 @@ async def test_write_message_component_section_no_base( "| [py_files](urlurl/components?src=pr&el=component) | `50.00% <0.00%> (?)` | |", ] assert message == expected + + +class TestCommentNotifierWelcome: + @pytest.mark.asyncio + async def test_build_message( + self, dbsession, mock_configuration, mock_repo_provider, sample_comparison + ): + mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br" + notifier = CommentNotifier( + repository=sample_comparison.head.commit.repository, + title="title", + notifier_yaml_settings={"layout": "reach, diff, flags, files, footer"}, + notifier_site_settings=True, + current_yaml={}, + ) + result = await notifier.build_message(sample_comparison) + expected_result = [ + "## Welcome to [Codecov](https://codecov.io) :tada:", + "", + "Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.", + "", + "Thanks for integrating Codecov - We've got you covered :open_umbrella:", + ] + for exp, res in zip(expected_result, result): + assert exp == res + assert result == expected_result diff --git a/services/yaml/tests/test_yaml.py b/services/yaml/tests/test_yaml.py index 1a39f56ac..f55814320 100644 --- a/services/yaml/tests/test_yaml.py +++ b/services/yaml/tests/test_yaml.py @@ -64,6 +64,7 @@ def test_get_final_yaml_no_thing_set_at_all(self, mocker, mock_configuration): "show_carryforward_flags": False, }, "github_checks": {"annotations": True}, + "slack_app": True, } result = UserYaml.get_final_yaml(owner_yaml={}, repo_yaml={}, commit_yaml={}) assert expected_result == result.to_dict() diff --git a/tasks/__init__.py b/tasks/__init__.py index 73b18d92f..6d769f502 100644 --- a/tasks/__init__.py +++ b/tasks/__init__.py @@ -1,5 +1,6 @@ from app import celery_app from tasks.add_to_sendgrid_list import add_to_sendgrid_list_task +from tasks.backfill_commit_data_to_storage import backfill_commit_data_to_storage_task from tasks.brolly_stats_rollup import brolly_stats_rollup_task from tasks.commit_update import commit_update_task from tasks.compute_comparison import compute_comparison_task diff --git a/tasks/backfill_commit_data_to_storage.py b/tasks/backfill_commit_data_to_storage.py new file mode 100644 index 000000000..685f2fe92 --- /dev/null +++ b/tasks/backfill_commit_data_to_storage.py @@ -0,0 +1,153 @@ +import logging +from enum import Enum +from typing import List, Optional, TypedDict + +from shared.utils.enums import TaskConfigGroup +from sqlalchemy.orm.session import Session + +from app import celery_app +from database.models.core import Commit +from database.models.reports import CommitReport, ReportDetails +from services.report import ReportService +from services.yaml import get_repo_yaml +from tasks.base import BaseCodecovTask + +log = logging.getLogger(__name__) + + +class BackfillError(Enum): + commit_not_found = "commit_not_found" + missing_data = "missing_data" + + +class BackfillResult(TypedDict): + success: bool + errors: List[BackfillError] + + +class BackfillCommitDataToStorageTask(BaseCodecovTask): + name = f"app.tasks.{TaskConfigGroup.archive}.BackfillCommitDataToStorage" + + async def run_async( + self, + db_session: Session, + *, + commitid: int, + **kwargs, + ) -> BackfillResult: + commit: Optional[Commit] = db_session.query(Commit).get(commitid) + if commit is None: + log.error("Commit not found.", extra=dict(commitid=commitid)) + return {"success": False, "errors": [BackfillError.commit_not_found.value]} + # Handle the report_json to storage + report_json_backfill_result = self.handle_report_json(db_session, commit) + # Handle report related classes + report_classes_backfill_result = self.handle_all_report_rows(db_session, commit) + # We can leave to BaseCodeovTask to commit the changes to DB + return { + "success": report_json_backfill_result["success"] + and report_classes_backfill_result["success"], + "errors": report_classes_backfill_result.get("errors") + + report_json_backfill_result.get("errors"), + } + + def handle_report_json(self, dbsession: Session, commit: Commit) -> BackfillResult: + if commit._report_json_storage_path is not None: + if commit._report_json is not None: + log.warning( + "Both _report_json AND _report_json_storage_path are set. Leaving as is.", + extra=dict(commitid=commit.id, commit=commit.commitid), + ) + log.debug( + "Commit info already in storage", + extra=dict(commitid=commit.id, commit=commit.commitid), + ) + return {"success": True, "errors": []} + if commit._report_json is not None: + # write to storage and clears out db field + commit.report_json = commit._report_json + return {"success": True, "errors": []} + log.warning( + "Neither _report_json nor _report_json_storage_path are set. Nothing to do.", + extra=dict(commitid=commit.id, commit=commit.commitid), + ) + return {"success": False, "errors": [BackfillError.missing_data.value]} + + def handle_all_report_rows( + self, db_session: Session, commit: Commit + ) -> BackfillResult: + report_rows = ( + db_session.query(CommitReport).filter_by(commit_id=commit.id_).all() + ) + if report_rows == []: + new_report_row = CommitReport(commit_id=commit.id_, code=None) + db_session.add(new_report_row) + db_session.flush() + report_rows = [new_report_row] + aggregate_results = dict(success=True, errors=[]) + for row in report_rows: + result = self.handle_single_report_row(db_session, commit, row) + aggregate_results["success"] = ( + aggregate_results["success"] and result["success"] + ) + aggregate_results["errors"].extend(result["errors"]) + return aggregate_results + + def handle_single_report_row( + self, db_session: Session, commit: Commit, report_row: CommitReport + ) -> BackfillResult: + report_details = ( + db_session.query(ReportDetails).filter_by(report_id=report_row.id_).first() + ) + if report_details is None: + report_details = ReportDetails( + report_id=report_row.id_, + _files_array=[], + report=report_row, + ) + db_session.add(report_details) + db_session.flush() + + repo_yaml = get_repo_yaml(commit.repository) + report_service = ReportService(current_yaml=repo_yaml) + actual_report = ( + report_service.get_existing_report_for_commit_from_legacy_data( + commit, report_code=None + ) + ) + if actual_report is not None: + report_service.save_report(commit, actual_report) + if report_details._files_array_storage_path is not None: + if report_details._files_array is not None: + log.warning( + "Both _files_array_storage_path AND _files_array are set. Leaving as is.", + extra=dict( + commitid=commit.id, + commit=commit.commitid, + commit_report=report_row.id_, + report_details=report_details.id_, + ), + ) + return {"success": True, "errors": []} + if report_details._files_array is not None: + # write to storage and clears out db field + report_details.files_array = report_details._files_array + return {"success": True, "errors": []} + log.warning( + "Neither _files_array_storage_path nor _files_array are set. Nothing to do.", + extra=dict( + commitid=commit.id, + commit=commit.commitid, + commit_report=report_row.id_, + report_details=report_details.id_, + ), + ) + return {"success": False, "errors": [BackfillError.missing_data.value]} + + +RegisteredBackfillCommitDataToStorageTask = celery_app.register_task( + BackfillCommitDataToStorageTask() +) +backfill_commit_data_to_storage_task = celery_app.tasks[ + RegisteredBackfillCommitDataToStorageTask.name +] diff --git a/tasks/label_analysis.py b/tasks/label_analysis.py index 26ddfc4a3..88e9b6809 100644 --- a/tasks/label_analysis.py +++ b/tasks/label_analysis.py @@ -62,9 +62,16 @@ async def run_async(self, db_session, request_id, *args, **kwargs): "Starting label analysis request", extra=dict( request_id=request_id, + external_id=label_analysis_request.external_id, commit=label_analysis_request.head_commit.commitid, ), ) + + if label_analysis_request.state_id == LabelAnalysisRequestState.FINISHED.db_id: + # Indicates that this request has been calculated already + # We might need to update the requested labels + return self._handle_larq_already_calculated(label_analysis_request) + try: lines_relevant_to_diff = await self._get_lines_relevant_to_diff( label_analysis_request @@ -100,6 +107,7 @@ async def run_async(self, db_session, request_id, *args, **kwargs): extra=dict( request_id=request_id, commit=label_analysis_request.head_commit.commitid, + external_id=label_analysis_request.external_id, ), ) label_analysis_request.result = None @@ -124,6 +132,8 @@ async def run_async(self, db_session, request_id, *args, **kwargs): has_relevant_lines=(lines_relevant_to_diff is not None), has_base_report=(base_report is not None), commit=label_analysis_request.head_commit.commitid, + external_id=label_analysis_request.external_id, + request_id=request_id, ), ) label_analysis_request.state_id = LabelAnalysisRequestState.FINISHED.db_id @@ -153,6 +163,44 @@ def add_processing_error( self.errors.append(error.to_representation()) self.dbsession.add(error) + def _handle_larq_already_calculated(self, larq: LabelAnalysisRequest): + # This means we already calculated everything + # Except possibly the absent labels + log.info( + "Label analysis request was already calculated", + extra=dict( + request_id=larq.id, + external_id=larq.external_id, + commit=larq.head_commit.commitid, + ), + ) + if larq.requested_labels: + saved_result = larq.result + all_saved_labels = set( + saved_result.get("present_report_labels", []) + + saved_result.get("present_diff_labels", []) + + saved_result.get("global_level_labels", []) + ) + executable_lines_saved_labels = set( + saved_result.get("present_diff_labels", []) + ) + global_saved_labels = set(saved_result.get("global_level_labels", [])) + result = self.calculate_final_result( + requested_labels=larq.requested_labels, + existing_labels=( + all_saved_labels, + executable_lines_saved_labels, + global_saved_labels, + ), + commit_sha=larq.head_commit.commitid, + ) + larq.result = result # Save the new result + return {**result, "success": True, "errors": []} + # No requested labels mean we don't have any new information + # So we don't need to calculate again + # This shouldn't actually happen + return {**larq.result, "success": True, "errors": []} + def _get_requested_labels(self, label_analysis_request: LabelAnalysisRequest): if label_analysis_request.requested_labels: return label_analysis_request.requested_labels @@ -186,6 +234,8 @@ async def _get_lines_relevant_to_diff( extra=dict( lines_relevant_to_diff=executable_lines_relevant_to_diff, commit=label_analysis_request.head_commit.commitid, + external_id=label_analysis_request.external_id, + request_id=label_analysis_request.id_, ), ) return executable_lines_relevant_to_diff @@ -210,6 +260,7 @@ async def _get_parsed_git_diff( "Label analysis failed to parse git diff", extra=dict( request_id=label_analysis_request.id, + external_id=label_analysis_request.external_id, commit=label_analysis_request.head_commit.commitid, ), ) diff --git a/tasks/tests/integration/test_notify_task.py b/tasks/tests/integration/test_notify_task.py index f2483301d..c5c083a10 100644 --- a/tasks/tests/integration/test_notify_task.py +++ b/tasks/tests/integration/test_notify_task.py @@ -4,7 +4,7 @@ import pytest from database.models import Pull -from database.tests.factories import CommitFactory, RepositoryFactory +from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory from services.archive import ArchiveService from services.notification.notifiers.base import NotificationResult from tasks.notify import NotifyTask @@ -807,6 +807,8 @@ async def test_simple_call_status_and_notifiers( repository=repository, author=repository.owner, ) + # create another pull so that we don't trigger the 1st time comment message + dbsession.add(PullFactory.create(repository=repository, pullid=8)) commit = CommitFactory.create( message="", pullid=9, @@ -1191,7 +1193,6 @@ async def test_simple_call_status_and_notifiers( "> `Δ = absolute (impact)`, `ø = not affected`, `? = missing data`", "> Powered by [Codecov](https://myexamplewebsite.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=footer). Last update [5b174c2...5601846](https://myexamplewebsite.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).", "", - ":loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/).", ], "commentid": None, "pullid": 9, diff --git a/tasks/tests/unit/test_backfill_commit_data_to_storage_task.py b/tasks/tests/unit/test_backfill_commit_data_to_storage_task.py new file mode 100644 index 000000000..0e4d7b520 --- /dev/null +++ b/tasks/tests/unit/test_backfill_commit_data_to_storage_task.py @@ -0,0 +1,292 @@ +from unittest.mock import call, patch + +import pytest + +from database.models.core import Commit +from database.models.reports import CommitReport, ReportDetails +from database.tests.factories.core import CommitFactory, ReportDetailsFactory +from tasks.backfill_commit_data_to_storage import ( + BackfillCommitDataToStorageTask, + BackfillError, +) + + +class TestBackfillCommitDataToStorageTask(object): + @patch("database.utils.ArchiveService") + def test_handle_report_json( + self, mock_archive_service, dbsession, mock_configuration + ): + mock_configuration.set_params( + { + "setup": { + "save_report_data_in_storage": { + "report_details_files_array": "general_access", + "commit_report": "general_access", + }, + } + } + ) + commit = CommitFactory() + dbsession.add(commit) + assert commit._report_json is not None + assert commit._report_json_storage_path is None + task = BackfillCommitDataToStorageTask() + result = task.handle_report_json(dbsession, commit) + assert result == {"success": True, "errors": []} + assert commit._report_json is None + assert commit._report_json_storage_path is not None + mock_archive_service.return_value.write_json_data_to_storage.assert_called() + + @patch("database.utils.ArchiveService") + def test_handle_report_json_alredy_in_storage( + self, mock_archive_service, dbsession + ): + commit = CommitFactory() + dbsession.add(commit) + commit._report_json = None + commit._report_json_storage_path = "path/to/sotorage" + task = BackfillCommitDataToStorageTask() + result = task.handle_report_json(dbsession, commit) + assert result == {"success": True, "errors": []} + assert commit._report_json is None + assert commit._report_json_storage_path == "path/to/sotorage" + mock_archive_service.return_value.write_json_data_to_storage.assert_not_called() + + def test_handle_report_json_missing_data(self, dbsession): + commit = CommitFactory() + dbsession.add(commit) + commit._report_json = None + commit._report_json_storage_path = None + task = BackfillCommitDataToStorageTask() + result = task.handle_report_json(dbsession, commit) + assert result == {"success": False, "errors": ["missing_data"]} + assert commit._report_json is None + assert commit._report_json_storage_path is None + + @patch( + "tasks.backfill_commit_data_to_storage.BackfillCommitDataToStorageTask.handle_single_report_row" + ) + def test_all_report_rows(self, mock_handle_single_row, dbsession): + def mock_handle_single_row_return_side_effect(db_session, commit, report_row): + if report_row.code is None: + return {"success": True, "errors": []} + if report_row.code is "local": + return {"success": False, "errors": [BackfillError.missing_data.value]} + + mock_handle_single_row.side_effect = mock_handle_single_row_return_side_effect + commit = CommitFactory() + dbsession.add(commit) + report_default = CommitReport(commit=commit, code=None) + report_code = CommitReport(commit=commit, code="local") + dbsession.add(report_default) + dbsession.add(report_code) + task = BackfillCommitDataToStorageTask() + result = task.handle_all_report_rows(dbsession, commit) + assert result == {"success": False, "errors": ["missing_data"]} + mock_handle_single_row.assert_has_calls( + [ + call(dbsession, commit, report_default), + call(dbsession, commit, report_code), + ] + ) + + @patch( + "tasks.backfill_commit_data_to_storage.BackfillCommitDataToStorageTask.handle_single_report_row" + ) + def test_all_report_rows_no_CommitReport(self, mock_handle_single_row, dbsession): + commit = CommitFactory() + dbsession.add(commit) + + def mock_handle_single_row_return_side_effect( + db_session, received_commit, report_row + ): + assert received_commit == commit + assert isinstance(report_row, CommitReport) + if report_row.code is None: + return {"success": True, "errors": []} + + mock_handle_single_row.side_effect = mock_handle_single_row_return_side_effect + + assert commit.reports_list == [] + task = BackfillCommitDataToStorageTask() + result = task.handle_all_report_rows(dbsession, commit) + assert result == {"success": True, "errors": []} + assert mock_handle_single_row.call_count == 1 + + @patch("database.utils.ArchiveService") + def test_handle_single_report_row( + self, mock_archive_service, dbsession, mock_configuration + ): + mock_configuration.set_params( + { + "setup": { + "save_report_data_in_storage": { + "report_details_files_array": "general_access", + "commit_report": "general_access", + }, + } + } + ) + commit = CommitFactory() + dbsession.add(commit) + report_row = CommitReport(commit=commit, code=None) + dbsession.add(report_row) + report_details = ReportDetailsFactory( + report=report_row, report_id=report_row.id_ + ) + dbsession.add(report_details) + dbsession.flush() + + report_details._files_array = [ + { + "filename": "test_file_1.py", + "file_index": 2, + "file_totals": [1, 10, 8, 2, 5, "80.00000", 6, 7, 9, 8, 20, 40, 13], + "session_totals": [[0, 10, 8, 2, 0, "80.00000", 0, 0, 0, 0, 0, 0, 0]], + "diff_totals": [0, 2, 1, 1, 0, "50.00000", 0, 0, 0, 0, 0, 0, 0], + }, + { + "filename": "test_file_2.py", + "file_index": 0, + "file_totals": [1, 3, 2, 1, 0, "66.66667", 0, 0, 0, 0, 0, 0, 0], + "session_totals": [[0, 3, 2, 1, 0, "66.66667", 0, 0, 0, 0, 0, 0, 0]], + "diff_totals": None, + }, + ] + report_details._files_array_storage_path = None + assert ( + dbsession.query(ReportDetails).filter_by(report_id=report_row.id_).first() + is not None + ) + + task = BackfillCommitDataToStorageTask() + result = task.handle_single_report_row(dbsession, commit, report_row) + assert result == {"success": True, "errors": []} + assert report_details._files_array is None + assert report_details._files_array_storage_path is not None + mock_archive_service.return_value.write_json_data_to_storage.assert_called() + + @patch("database.utils.ArchiveService") + def test_handle_single_report_row_ReportDetails_missing_data( + self, mock_archive_service, dbsession + ): + commit = CommitFactory() + dbsession.add(commit) + report_row = CommitReport(commit=commit, code=None) + dbsession.add(report_row) + report_details = ReportDetailsFactory( + report=report_row, report_id=report_row.id_ + ) + dbsession.add(report_details) + dbsession.flush() + + report_details._files_array = None + report_details._files_array_storage_path = None + assert ( + dbsession.query(ReportDetails).filter_by(report_id=report_row.id_).first() + is not None + ) + + task = BackfillCommitDataToStorageTask() + result = task.handle_single_report_row(dbsession, commit, report_row) + assert result == {"success": False, "errors": ["missing_data"]} + assert report_details._files_array is None + assert report_details._files_array_storage_path is None + mock_archive_service.return_value.write_json_data_to_storage.assert_not_called() + + @patch("tasks.backfill_commit_data_to_storage.ReportService.save_report") + @patch( + "tasks.backfill_commit_data_to_storage.ReportService.get_existing_report_for_commit_from_legacy_data", + return_value="the existing report", + ) + def test_handle_single_report_row_create_ReportDetails( + self, mock_get_existing_report, mock_save_report, dbsession, mock_configuration + ): + mock_configuration.set_params( + { + "setup": { + "save_report_data_in_storage": { + "report_details_files_array": "general_access", + "commit_report": "general_access", + }, + } + } + ) + + commit = CommitFactory() + dbsession.add(commit) + report_row = CommitReport(commit=commit, code=None) + dbsession.add(report_row) + dbsession.flush() + + def mock_save_report_side_effect(received_commit, actual_report): + assert received_commit == commit + assert actual_report == "the existing report" + commit.report.details._files_array_storage_path = "path/to/storage" + commit.report.details._files_array = None + + mock_save_report.side_effect = mock_save_report_side_effect + + assert ( + dbsession.query(ReportDetails).filter_by(report_id=report_row.id_).first() + is None + ) + + task = BackfillCommitDataToStorageTask() + result = task.handle_single_report_row(dbsession, commit, report_row) + assert result == {"success": True, "errors": []} + report_details = ( + dbsession.query(ReportDetails).filter_by(report_id=report_row.id_).first() + ) + assert report_details is not None + assert isinstance(report_details, ReportDetails) + assert report_details._files_array is None + assert report_details._files_array_storage_path == "path/to/storage" + mock_get_existing_report.assert_called_with(commit, report_code=None) + mock_save_report.assert_called_with(commit, "the existing report") + + @patch( + "tasks.backfill_commit_data_to_storage.BackfillCommitDataToStorageTask.handle_report_json" + ) + @patch( + "tasks.backfill_commit_data_to_storage.BackfillCommitDataToStorageTask.handle_all_report_rows" + ) + @pytest.mark.asyncio + async def test_run( + self, mock_handle_all_report_rows, mock_handle_report_json, dbsession + ): + commit = CommitFactory() + dbsession.add(commit) + dbsession.flush() + + mock_handle_all_report_rows.return_value = {"success": True, "errors": []} + mock_handle_report_json.return_value = { + "success": False, + "errors": [BackfillError.missing_data.value], + } + + assert dbsession.query(Commit).get(commit.id_) is not None + task = BackfillCommitDataToStorageTask() + result = await task.run_async(dbsession, commitid=commit.id_) + assert result == {"success": False, "errors": ["missing_data"]} + + @patch( + "tasks.backfill_commit_data_to_storage.BackfillCommitDataToStorageTask.handle_report_json" + ) + @patch( + "tasks.backfill_commit_data_to_storage.BackfillCommitDataToStorageTask.handle_all_report_rows" + ) + @pytest.mark.asyncio + async def test_run_missing_commit( + self, mock_handle_all_report_rows, mock_handle_report_json, dbsession + ): + mock_handle_all_report_rows.return_value = {"success": True, "errors": []} + mock_handle_report_json.return_value = { + "success": False, + "errors": [BackfillError.missing_data.value], + } + + assert dbsession.query(Commit).get(-1) is None + task = BackfillCommitDataToStorageTask() + result = await task.run_async(dbsession, commitid=-1) + assert result == {"success": False, "errors": ["commit_not_found"]} diff --git a/tasks/tests/unit/test_label_analysis.py b/tasks/tests/unit/test_label_analysis.py index cc06d2f16..204294a56 100644 --- a/tasks/tests/unit/test_label_analysis.py +++ b/tasks/tests/unit/test_label_analysis.py @@ -406,7 +406,7 @@ def sample_report_with_labels(): @pytest.mark.asyncio -async def test_simple_call_without_requested_labels( +async def test_simple_call_without_requested_labels_then_with_requested_labels( dbsession, mock_storage, mocker, sample_report_with_labels, mock_repo_provider ): mocker.patch.object( @@ -502,6 +502,29 @@ async def test_simple_call_without_requested_labels( "present_report_labels": expected_present_report_labels, "global_level_labels": ["applejuice", "justjuice", "orangejuice"], } + # Now we call the task again, this time with the requested labels. + # This illustrates what should happen if we patch the labels after calculating + # And trigger the task again to save the new results + larf.requested_labels = ["tangerine", "pear", "banana", "apple"] + dbsession.flush() + res = await task.run_async(dbsession, larf.id) + expected_present_diff_labels = ["banana"] + expected_present_report_labels = ["apple", "banana"] + expected_absent_labels = ["pear", "tangerine"] + assert res == { + "absent_labels": expected_absent_labels, + "present_diff_labels": expected_present_diff_labels, + "present_report_labels": expected_present_report_labels, + "success": True, + "global_level_labels": [], + "errors": [], + } + assert larf.result == { + "absent_labels": expected_absent_labels, + "present_diff_labels": expected_present_diff_labels, + "present_report_labels": expected_present_report_labels, + "global_level_labels": [], + } @pytest.mark.asyncio