feat(test results): error message PR comment if no successful parse (#…

…358) * feat(test results): error message PR comment if no successful parse Signed-off-by: joseph-sentry <[email protected]> * fix: except LockRetry instead of LockError Signed-off-by: joseph-sentry <[email protected]> * feat: add error message to comment when test results parsing fails This commit: - adds the TestResultsProcessingError enum - adds the enum as a nullable field in the test results total - in the test results finisher, when there are no succesful parses, it writes to the error field in the totals - it immediately writes the error message to the comment, then queues up a notify - in the notify step, it will read from the totals to check if there was a test result processing error, if there was, let the user know by displaying it in the coverage comment, similary to how we show succesful parsing of test results * feat: add error message to PR comment when test results parsing fails The logic for doing this is as follows: If there are no successful test results processing tasks this chord then we check if there were previously test instances that were processed for this commit. If there are previous test instances on this commit, there is no error, and we just exit this finisher without doing anything. If there are no previous test instances on this commit, then we should notify the user that there is an error. So we will begin by doing a test results error comment which contains no info about coverage, then we will queue up the notify task so it can create a coverage comment that still contains this error's information. If there are successful test results processing tasks, then we should get rid of the error on the test results totals for this commit. Signed-off-by: joseph-sentry <[email protected]> * fix: fix test_results_error comment logic If there has been any success in processing for a given commit, it's associated test result totals should have the error set to None, otherwise it should be NO_SUCCESS. The condition for all_tests_passed is: if the error is None and failed = 0 in the totals then all tests passed. In the comment, if there's an error, show the error comment, else if all tests passed show the all tests passed message. If neither of these are true and the user HAS uploaded test results, then it means that there must have been failures, or they somehow skipped all their tests. Signed-off-by: joseph-sentry <[email protected]> * fix: apply logic to team plan and add test Signed-off-by: joseph-sentry <[email protected]> --------- Signed-off-by: joseph-sentry <[email protected]>
codecov · May 2, 2024 · 32d2884 · 32d2884
1 parent d4496ed
commit 32d2884
Show file tree

Hide file tree

Showing 13 changed files with 1,415 additions and 28 deletions.
diff --git a/database/enums.py b/database/enums.py
@@ -68,3 +68,7 @@ class FlakeSymptomType(Enum):
     FAILED_IN_DEFAULT_BRANCH = "failed_in_default_branch"
     CONSECUTIVE_DIFF_OUTCOMES = "consecutive_diff_outcomes"
     UNRELATED_MATCHING_FAILURES = "unrelated_matching_failures"
+
+
+class TestResultsProcessingError(Enum):
+    NO_SUCCESS = "no_success"
diff --git a/database/models/reports.py b/database/models/reports.py
@@ -312,3 +312,4 @@ class TestResultReportTotals(CodecovBaseModel, MixinBaseClass):
     passed = Column(types.Integer)
     skipped = Column(types.Integer)
     failed = Column(types.Integer)
+    error = Column(types.String(100), nullable=True)
diff --git a/services/comparison/__init__.py b/services/comparison/__init__.py
@@ -12,7 +12,7 @@
     TorngitObjectNotFoundError,
 )
 
-from database.enums import CompareCommitState
+from database.enums import CompareCommitState, TestResultsProcessingError
 from database.models import CompareCommit
 from helpers.metrics import metrics
 from services.archive import ArchiveService
@@ -29,6 +29,7 @@ class NotificationContext(object):
     """Extra information not necessarily related to coverage that may affect notifications"""
 
     all_tests_passed: bool
+    test_results_error: TestResultsProcessingError | None = None
 
 
 class ComparisonProxy(object):
@@ -255,6 +256,9 @@ async def get_behind_by(self):
     def all_tests_passed(self):
         return self.context is not None and self.context.all_tests_passed
 
+    def test_results_error(self):
+        return self.context is not None and self.context.test_results_error
+
     async def get_existing_statuses(self):
         async with self._existing_statuses_lock:
             if self._existing_statuses is None:

diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py
@@ -102,8 +102,13 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non
 
 class NewHeaderSectionWriter(BaseSectionWriter):
     def _possibly_include_test_result_setup_confirmation(self, comparison):
-        if comparison.all_tests_passed():
+        if comparison.test_results_error():
             yield ("")
+            yield (
+                ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format."
+            )
+        elif comparison.all_tests_passed():
+            yield ""
             yield (":white_check_mark: All tests successful. No failed tests found.")
 
     async def do_write_section(self, comparison, diff, changes, links, behind_by=None):

diff --git a/services/notification/notifiers/mixins/message/writers.py b/services/notification/notifiers/mixins/message/writers.py
@@ -46,7 +46,12 @@ def header_lines(self, comparison: ComparisonProxy, diff, settings) -> List[str]
 
         hide_project_coverage = settings.get("hide_project_coverage", False)
         if hide_project_coverage:
-            if comparison.all_tests_passed():
+            if comparison.test_results_error():
+                lines.append("")
+                lines.append(
+                    ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format."
+                )
+            elif comparison.all_tests_passed():
                 lines.append("")
                 lines.append(
                     ":white_check_mark: All tests successful. No failed tests found."

diff --git a/.../cassetes/test_comment/TestCommentNotifierIntegration/test_notify_test_results_error.yaml b/.../cassetes/test_comment/TestCommentNotifierIntegration/test_notify_test_results_error.yaml
diff --git a/services/notification/notifiers/tests/integration/test_comment.py b/services/notification/notifiers/tests/integration/test_comment.py
@@ -3,6 +3,7 @@
 import pytest
 from shared.reports.readonly import ReadOnlyReport
 
+from database.enums import TestResultsProcessingError
 from database.tests.factories import CommitFactory, PullFactory, RepositoryFactory
 from services.comparison import ComparisonProxy, NotificationContext
 from services.comparison.types import Comparison, EnrichedPull, FullCommit
@@ -337,7 +338,9 @@ def sample_comparison_for_limited_upload(
 class TestCommentNotifierIntegration(object):
     @pytest.mark.asyncio
     async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration):
-        sample_comparison.context = NotificationContext(all_tests_passed=True)
+        sample_comparison.context = NotificationContext(
+            all_tests_passed=True, test_results_error=None
+        )
         mock_configuration._params["setup"] = {
             "codecov_url": None,
             "codecov_dashboard_url": None,
@@ -405,6 +408,81 @@ async def test_notify(self, sample_comparison, codecov_vcr, mock_configuration):
         assert result.data_sent == {"commentid": None, "message": message, "pullid": 9}
         assert result.data_received == {"id": 1699669247}
 
+    @pytest.mark.asyncio
+    async def test_notify_test_results_error(
+        self, sample_comparison, codecov_vcr, mock_configuration
+    ):
+        sample_comparison.context = NotificationContext(
+            all_tests_passed=False,
+            test_results_error=TestResultsProcessingError.NO_SUCCESS,
+        )
+        mock_configuration._params["setup"] = {
+            "codecov_url": None,
+            "codecov_dashboard_url": None,
+        }
+        comparison = sample_comparison
+        notifier = CommentNotifier(
+            repository=comparison.head.commit.repository,
+            title="title",
+            notifier_yaml_settings={"layout": "reach, diff, flags, files, footer"},
+            notifier_site_settings=True,
+            current_yaml={},
+        )
+        result = await notifier.notify(comparison)
+        assert result.notification_attempted
+        assert result.notification_successful
+        assert result.explanation is None
+        message = [
+            "## [Codecov](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?dropdown=coverage&src=pr&el=h1) Report",
+            "All modified and coverable lines are covered by tests :white_check_mark:",
+            "> Project coverage is 60.00%. Comparing base [(`5b174c2`)](https://app.codecov.io/gh/joseph-sentry/codecov-demo/commit/5b174c2b40d501a70c479e91025d5109b1ad5c1b?dropdown=coverage&el=desc) to head [(`5601846`)](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?dropdown=coverage&src=pr&el=desc).",
+            "> Report is 2 commits behind head on main.",
+            "",
+            ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.",
+            "",
+            ":exclamation: Your organization needs to install the [Codecov GitHub app](https://github.com/apps/codecov/installations/select_target) to enable full functionality.",
+            "",
+            "[![Impacted file tree graph](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9/graphs/tree.svg?width=650&height=150&src=pr&token=abcdefghij)](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?src=pr&el=tree)",
+            "",
+            "```diff",
+            "@@              Coverage Diff              @@",
+            "##               main       #9       +/-   ##",
+            "=============================================",
+            "+ Coverage     50.00%   60.00%   +10.00%     ",
+            "+ Complexity       11       10        -1     ",
+            "=============================================",
+            "  Files             2        2               ",
+            "  Lines             6       10        +4     ",
+            "  Branches          0        1        +1     ",
+            "=============================================",
+            "+ Hits              3        6        +3     ",
+            "  Misses            3        3               ",
+            "- Partials          0        1        +1     ",
+            "```",
+            "",
+            "| [Flag](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9/flags?src=pr&el=flags) | Coverage Δ | Complexity Δ | |",
+            "|---|---|---|---|",
+            "| [integration](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9/flags?src=pr&el=flag) | `?` | `?` | |",
+            "| [unit](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9/flags?src=pr&el=flag) | `100.00% <ø> (?)` | `0.00 <ø> (?)` | |",
+            "",
+            "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.",
+            "",
+            "[see 2 files with indirect coverage changes](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9/indirect-changes?src=pr&el=tree-more)",
+            "",
+            "------",
+            "",
+            "[Continue to review full report in Codecov by Sentry](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?dropdown=coverage&src=pr&el=continue).",
+            "> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)",
+            "> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`",
+            "> Powered by [Codecov](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?dropdown=coverage&src=pr&el=footer). Last update [5b174c2...5601846](https://app.codecov.io/gh/joseph-sentry/codecov-demo/pull/9?dropdown=coverage&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).",
+            "",
+        ]
+        for exp, res in zip(result.data_sent["message"], message):
+            assert exp == res
+        assert result.data_sent["message"] == message
+        assert result.data_sent == {"commentid": None, "message": message, "pullid": 9}
+        assert result.data_received == {"id": 1699669247}
+
     @pytest.mark.asyncio
     async def test_notify_upgrade(
         self, dbsession, sample_comparison_for_upgrade, codecov_vcr, mock_configuration

diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py
@@ -16,6 +16,7 @@
 from shared.utils.sessions import Session
 from shared.yaml import UserYaml
 
+from database.enums import TestResultsProcessingError
 from database.models.core import GithubAppInstallation, Pull
 from database.tests.factories import RepositoryFactory
 from database.tests.factories.core import CommitFactory, OwnerFactory, PullFactory
@@ -4056,6 +4057,40 @@ async def test_new_header_section_writer_test_results_setup(
             ":white_check_mark: All tests successful. No failed tests found.",
         ]
 
+    @pytest.mark.asyncio
+    async def test_new_header_section_writer_test_results_error(
+        self, mocker, sample_comparison
+    ):
+        sample_comparison.context = NotificationContext(
+            all_tests_passed=False,
+            test_results_error=TestResultsProcessingError.NO_SUCCESS,
+        )
+        writer = NewHeaderSectionWriter(
+            mocker.MagicMock(),
+            mocker.MagicMock(),
+            show_complexity=mocker.MagicMock(),
+            settings={},
+            current_yaml=mocker.MagicMock(),
+        )
+        mocker.patch(
+            "services.notification.notifiers.mixins.message.sections.round_number",
+            return_value=Decimal(0),
+        )
+        res = list(
+            await writer.write_section(
+                sample_comparison,
+                None,
+                None,
+                links={"pull": "urlurl", "base": "urlurl"},
+            )
+        )
+        assert res == [
+            "All modified and coverable lines are covered by tests :white_check_mark:",
+            f"> Project coverage is 0%. Comparing base [(`{sample_comparison.project_coverage_base.commit.commitid[:7]}`)](urlurl?dropdown=coverage&el=desc) to head [(`{sample_comparison.head.commit.commitid[:7]}`)](urlurl?dropdown=coverage&src=pr&el=desc).",
+            "",
+            ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.",
+        ]
+
     @pytest.mark.asyncio
     async def test_new_header_section_writer_no_project_coverage(
         self, mocker, sample_comparison
@@ -4113,6 +4148,39 @@ async def test_new_header_section_writer_no_project_coverage_test_results_setup(
             ":white_check_mark: All tests successful. No failed tests found.",
         ]
 
+    @pytest.mark.asyncio
+    async def test_new_header_section_writer_no_project_coverage_test_results_error(
+        self, mocker, sample_comparison
+    ):
+        sample_comparison.context = NotificationContext(
+            all_tests_passed=False,
+            test_results_error=TestResultsProcessingError.NO_SUCCESS,
+        )
+        writer = NewHeaderSectionWriter(
+            mocker.MagicMock(),
+            mocker.MagicMock(),
+            show_complexity=mocker.MagicMock(),
+            settings={"hide_project_coverage": True},
+            current_yaml=mocker.MagicMock(),
+        )
+        mocker.patch(
+            "services.notification.notifiers.mixins.message.sections.round_number",
+            return_value=Decimal(0),
+        )
+        res = list(
+            await writer.write_section(
+                sample_comparison,
+                None,
+                None,
+                links={"pull": "urlurl", "base": "urlurl"},
+            )
+        )
+        assert res == [
+            "All modified and coverable lines are covered by tests :white_check_mark:",
+            "",
+            ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.",
+        ]
+
 
 class TestAnnouncementsSectionWriter(object):
     @pytest.mark.asyncio
@@ -4864,6 +4932,88 @@ async def test_build_message_team_plan_customer_all_lines_covered(
         ]
         assert result == expected_result
 
+    @pytest.mark.asyncio
+    async def test_build_message_team_plan_customer_all_lines_covered_test_results_error(
+        self,
+        dbsession,
+        mock_configuration,
+        mock_repo_provider,
+        sample_comparison_coverage_carriedforward,
+    ):
+        mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
+        sample_comparison_coverage_carriedforward.context = NotificationContext(
+            all_tests_passed=False,
+            test_results_error=TestResultsProcessingError.NO_SUCCESS,
+        )
+        comparison = sample_comparison_coverage_carriedforward
+        comparison.repository_service.service = "github"
+        # relevant part of this test
+        comparison.head.commit.repository.owner.plan = "users-teamm"
+        notifier = CommentNotifier(
+            repository=comparison.head.commit.repository,
+            title="title",
+            notifier_yaml_settings={
+                # Irrelevant but they don't overwrite Owner's plan
+                "layout": "newheader, reach, diff, flags, components, newfooter",
+                "hide_project_coverage": True,
+            },
+            notifier_site_settings=True,
+            current_yaml={},
+        )
+        pull = comparison.pull
+        repository = sample_comparison_coverage_carriedforward.head.commit.repository
+        result = await notifier.build_message(comparison)
+
+        expected_result = [
+            f"## [Codecov](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?dropdown=coverage&src=pr&el=h1) Report",
+            "All modified and coverable lines are covered by tests :white_check_mark:",
+            "",
+            ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.",
+            "",
+            ":loudspeaker: Thoughts on this report? [Let us know!](https://github.com/codecov/feedback/issues/255)",
+        ]
+        assert result == expected_result
+
+    @pytest.mark.asyncio
+    async def test_build_message_team_plan_customer_all_lines_covered(
+        self,
+        dbsession,
+        mock_configuration,
+        mock_repo_provider,
+        sample_comparison_coverage_carriedforward,
+    ):
+        mock_configuration.params["setup"]["codecov_dashboard_url"] = "test.example.br"
+        sample_comparison_coverage_carriedforward.context = NotificationContext(
+            all_tests_passed=False,
+            test_results_error=None,
+        )
+        comparison = sample_comparison_coverage_carriedforward
+        comparison.repository_service.service = "github"
+        # relevant part of this test
+        comparison.head.commit.repository.owner.plan = "users-teamm"
+        notifier = CommentNotifier(
+            repository=comparison.head.commit.repository,
+            title="title",
+            notifier_yaml_settings={
+                # Irrelevant but they don't overwrite Owner's plan
+                "layout": "newheader, reach, diff, flags, components, newfooter",
+                "hide_project_coverage": True,
+            },
+            notifier_site_settings=True,
+            current_yaml={},
+        )
+        pull = comparison.pull
+        repository = sample_comparison_coverage_carriedforward.head.commit.repository
+        result = await notifier.build_message(comparison)
+
+        expected_result = [
+            f"## [Codecov](test.example.br/gh/{repository.slug}/pull/{pull.pullid}?dropdown=coverage&src=pr&el=h1) Report",
+            "All modified and coverable lines are covered by tests :white_check_mark:",
+            "",
+            ":loudspeaker: Thoughts on this report? [Let us know!](https://github.com/codecov/feedback/issues/255)",
+        ]
+        assert result == expected_result
+
     @pytest.mark.asyncio
     async def test_build_message_no_patch_or_proj_change(
         self,

diff --git a/services/test_results.py b/services/test_results.py
@@ -316,6 +316,29 @@ def insert_breaks(self, table_value):
 
         return "<br>".join(lines)
 
+    async def error_comment(self):
+        self.repo_service = get_repo_provider_service(
+            self.commit.repository, self.commit
+        )
+
+        await self.get_pull()
+        if self.pull is None:
+            log.info(
+                "Not notifying since there is no pull request associated with this commit",
+                extra=dict(
+                    commitid=self.commit.commitid,
+                ),
+            )
+            return False, "no_pull"
+
+        message = ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format."
+
+        sent_to_provider = await self.send_to_provider(message)
+        if sent_to_provider == False:
+            return (False, "torngit_error")
+
+        return (True, "comment_posted")
+
 
 def latest_test_instances_for_a_given_commit(db_session, commit_id):
     """