From c650bbe1ce54acb24ea9ec9062c2df6e4abdf81c Mon Sep 17 00:00:00 2001 From: joseph-sentry Date: Fri, 27 Dec 2024 15:20:41 -0500 Subject: [PATCH] fix: reorganize TA finishing and error commenting this commit makes it so there is a single flow for TA finishing where we will always attempt to comment if there is relevant data to comment and we will only persist data if there is new data (uploads) to persist in the new test results flow the error comment will no longer be used and there will be a single notify function that has the capability of choosing which section of the comment to show: if there are errors show them, if there are relevant failures show them --- services/test_results.py | 108 +++++++++++---------- services/tests/test_test_results.py | 13 +-- tasks/ta_finisher.py | 112 ++++++++++------------ tasks/test_results_finisher.py | 4 +- tasks/tests/unit/test_ta_finisher_task.py | 4 +- 5 files changed, 118 insertions(+), 123 deletions(-) diff --git a/services/test_results.py b/services/test_results.py index ce98e1e4b..a6428c93d 100644 --- a/services/test_results.py +++ b/services/test_results.py @@ -135,13 +135,18 @@ class FlakeInfo: count: int +@dataclass +class TACommentInDepthInfo: + failures: list[TestResultsNotificationFailure] + flaky_tests: dict[str, FlakeInfo] + + @dataclass class TestResultsNotificationPayload: failed: int passed: int skipped: int - failures: list[TestResultsNotificationFailure] - flaky_tests: dict[str, FlakeInfo] + info: TACommentInDepthInfo | None = None def wrap_in_details(summary: str, content: str): @@ -237,12 +242,12 @@ def messagify_flake( return make_quoted(f"{test_name}\n{flake_rate_section}\n{stack_trace}") -def specific_error_message(upload_errors: list[UploadError]) -> str: - title = f"### :x: {upload_errors[0].error_code}" +def specific_error_message(upload_error: UploadError) -> str: + title = f"### :x: {upload_error.error_code}" description = "\n".join( [ - f"Upload processing failed due to {upload_errors[0].error_code.lower()}. Please review the parser error message:", - f"`{upload_errors[0].error_params['error_message']}`", + f"Upload processing failed due to {upload_error.error_code.lower()}. Please review the parser error message:", + f"`{upload_error.error_params['error_message']}`", "For more help, visit our [troubleshooting guide](https://docs.codecov.com/docs/test-analytics#troubleshooting).", ] ) @@ -256,7 +261,7 @@ def specific_error_message(upload_errors: list[UploadError]) -> str: @dataclass class TestResultsNotifier(BaseNotifier): payload: TestResultsNotificationPayload | None = None - errors: list[UploadError] | None = None + error: UploadError | None = None def build_message(self) -> str: if self.payload is None: @@ -264,57 +269,62 @@ def build_message(self) -> str: message = [] - if self.errors is not None and len(self.errors) > 0: - message.append(specific_error_message(self.errors)) + if self.error: + message.append(specific_error_message(self.error)) + + if self.error and self.payload.info: message.append("---") - message.append(f"### :x: {self.payload.failed} Tests Failed:") + if self.payload.info: + message.append(f"### :x: {self.payload.failed} Tests Failed:") - completed = self.payload.failed + self.payload.passed + completed = self.payload.failed + self.payload.passed - message += [ - "| Tests completed | Failed | Passed | Skipped |", - "|---|---|---|---|", - f"| {completed} | {self.payload.failed} | {self.payload.passed} | {self.payload.skipped} |", - ] - - failures = sorted( - ( - failure - for failure in self.payload.failures - if failure.test_id not in self.payload.flaky_tests - ), - key=lambda x: (x.duration_seconds, x.display_name), - ) - if failures: - failure_content = [f"{messagify_failure(failure)}" for failure in failures] + message += [ + "| Tests completed | Failed | Passed | Skipped |", + "|---|---|---|---|", + f"| {completed} | {self.payload.failed} | {self.payload.passed} | {self.payload.skipped} |", + ] - top_3_failed_section = wrap_in_details( - f"View the top {min(3, len(failures))} failed tests by shortest run time", - "\n".join(failure_content), + failures = sorted( + ( + failure + for failure in self.payload.info.failures + if failure.test_id not in self.payload.info.flaky_tests + ), + key=lambda x: (x.duration_seconds, x.display_name), ) + if failures: + failure_content = [ + f"{messagify_failure(failure)}" for failure in failures + ] - message.append(top_3_failed_section) + top_3_failed_section = wrap_in_details( + f"View the top {min(3, len(failures))} failed tests by shortest run time", + "\n".join(failure_content), + ) - flaky_failures = [ - failure - for failure in self.payload.failures - if failure.test_id in self.payload.flaky_tests - ] - if flaky_failures: - flake_content = [ - f"{messagify_flake(flaky_failure, self.payload.flaky_tests[flaky_failure.test_id])}" - for flaky_failure in flaky_failures + message.append(top_3_failed_section) + + flaky_failures = [ + failure + for failure in self.payload.info.failures + if failure.test_id in self.payload.info.flaky_tests ] + if flaky_failures: + flake_content = [ + f"{messagify_flake(flaky_failure, self.payload.info.flaky_tests[flaky_failure.test_id])}" + for flaky_failure in flaky_failures + ] - flaky_section = wrap_in_details( - f"View the full list of {len(flaky_failures)} :snowflake: flaky tests", - "\n".join(flake_content), - ) + flaky_section = wrap_in_details( + f"View the full list of {len(flaky_failures)} :snowflake: flaky tests", + "\n".join(flake_content), + ) - message.append(flaky_section) + message.append(flaky_section) - message.append(generate_view_test_analytics_line(self.commit)) + message.append(generate_view_test_analytics_line(self.commit)) return "\n".join(message) def error_comment(self): @@ -324,12 +334,10 @@ def error_comment(self): """ message: str - if self.errors is None: + if self.error is None: message = ":x: We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format." else: - if len(self.errors) == 0: - raise ValueError("No errors passed to error_comment method") - message = specific_error_message(self.errors) + message = specific_error_message(self.error) pull = self.get_pull() if pull is None: diff --git a/services/tests/test_test_results.py b/services/tests/test_test_results.py index 0351ad409..0d5ab6b6b 100644 --- a/services/tests/test_test_results.py +++ b/services/tests/test_test_results.py @@ -12,6 +12,7 @@ from helpers.notifier import NotifierResult from services.test_results import ( FlakeInfo, + TACommentInDepthInfo, TestResultsNotificationFailure, TestResultsNotificationPayload, TestResultsNotifier, @@ -114,7 +115,8 @@ def test_build_message(): 1.0, "https://example.com/build_url", ) - payload = TestResultsNotificationPayload(1, 2, 3, [fail], dict()) + info = TACommentInDepthInfo(failures=[fail], flaky_tests={}) + payload = TestResultsNotificationPayload(1, 2, 3, info) commit = CommitFactory(branch="thing/thing") tn = TestResultsNotifier(commit, None, None, None, payload) res = tn.build_message() @@ -161,10 +163,9 @@ def test_build_message_with_flake(): 1.0, "https://example.com/build_url", ) - - payload = TestResultsNotificationPayload( - 1, 2, 3, [fail], {test_id: FlakeInfo(1, 3)} - ) + flaky_test = FlakeInfo(1, 3) + info = TACommentInDepthInfo(failures=[fail], flaky_tests={test_id: flaky_test}) + payload = TestResultsNotificationPayload(1, 2, 3, info) commit = CommitFactory(branch="test_branch") tn = TestResultsNotifier(commit, None, None, None, payload) res = tn.build_message() @@ -296,7 +297,7 @@ def test_specific_error_message(mocker): "error_message": "Error parsing JUnit XML in test.xml at 4:32: ParserError: No name found" }, ) - tn = TestResultsNotifier(CommitFactory(), None, errors=[error]) + tn = TestResultsNotifier(CommitFactory(), None, error=error) result = tn.error_comment() expected = """### :x: Unsupported file format diff --git a/tasks/ta_finisher.py b/tasks/ta_finisher.py index 21f04807d..5762a6214 100644 --- a/tasks/ta_finisher.py +++ b/tasks/ta_finisher.py @@ -2,7 +2,6 @@ from dataclasses import dataclass from typing import Any -import sentry_sdk from asgiref.sync import async_to_sync from shared.reports.types import UploadType from shared.typings.torngit import AdditionalData @@ -35,6 +34,7 @@ from services.ta_finishing import persist_intermediate_results from services.test_results import ( FlakeInfo, + TACommentInDepthInfo, TestResultsNotificationFailure, TestResultsNotificationPayload, TestResultsNotifier, @@ -72,7 +72,7 @@ def get_uploads(db_session: Session, commit: Commit) -> dict[int, Upload]: .filter( CommitReport.commit_id == commit.id, CommitReport.report_type == ReportType.TEST_RESULTS.value, - Upload.state.in_(["v2_processed", "v2_failed"]), + Upload.state == "v2_processed", ) .all() ) @@ -127,26 +127,6 @@ def get_totals( return totals -def handle_processor_fail( - commit: Commit, - commit_yaml: UserYaml, - upload_errors: list[UploadError] | None = None, -) -> dict[str, bool]: - # make an attempt to make test results comment - notifier = TestResultsNotifier(commit, commit_yaml, errors=upload_errors) - success, reason = notifier.error_comment() - if not success and reason == "torngit_error": - sentry_sdk.capture_message( - "Error posting error comment in test results finisher due to torngit error" - ) - - return { - "notify_attempted": False, - "notify_succeeded": False, - "queue_notify": True, - } - - def populate_failures( failures: list[TestResultsNotificationFailure], db_session: Session, @@ -242,7 +222,6 @@ def run_impl( repoid=repoid, commitid=commitid, commit_yaml=UserYaml.from_dict(commit_yaml), - previous_result=chord_result, **kwargs, ) if finisher_result["queue_notify"]: @@ -265,7 +244,6 @@ def process_impl_within_lock( repoid: int, commitid: str, commit_yaml: UserYaml, - previous_result: list[bool], **kwargs, ): log.info("Running test results finishers", extra=self.extra_dict) @@ -279,53 +257,51 @@ def process_impl_within_lock( commit_report = commit.commit_report(ReportType.TEST_RESULTS) totals = get_totals(commit_report, db_session) - uploads = get_uploads(db_session, commit) - - upload_errors = ( - db_session.query(UploadError) - .filter(UploadError.upload_id.in_(uploads.keys())) - .all() - ) - - if not any(previous_result): - return handle_processor_fail(commit, commit_yaml, upload_errors) + uploads = get_uploads( + db_session, commit + ) # processed uploads that have yet to be persisted repo = commit.repository branch = commit.branch flaky_test_set = get_flake_set(db_session, repoid) - persist_intermediate_results( - db_session, repoid, commitid, branch, uploads, flaky_test_set - ) + if len(uploads) > 0: + persist_intermediate_results( + db_session, repoid, commitid, branch, uploads, flaky_test_set + ) test_summary = get_test_summary_for_commit(db_session, repoid, commitid) totals.failed = test_summary.get("error", 0) + test_summary.get("failure", 0) totals.skipped = test_summary.get("skip", 0) totals.passed = test_summary.get("pass", 0) db_session.flush() - if not totals.failed: - return { - "notify_attempted": False, - "notify_succeeded": False, - "queue_notify": True, - } - escaper = StringEscaper(ESCAPE_FAILURE_MESSAGE_DEFN) - shorten_paths = commit_yaml.read_yaml_field( - "test_analytics", "shorten_paths", _else=True - ) + info = None + if totals.failed: + escaper = StringEscaper(ESCAPE_FAILURE_MESSAGE_DEFN) + shorten_paths = commit_yaml.read_yaml_field( + "test_analytics", "shorten_paths", _else=True + ) - failures = [] - populate_failures( - failures, - db_session, - repoid, - commitid, - shorten_paths, - uploads, - escaper, - ) + failures = [] + populate_failures( + failures, + db_session, + repoid, + commitid, + shorten_paths, + uploads, + escaper, + ) + + flaky_tests = dict() + if should_do_flaky_detection(repo, commit_yaml): + flaky_tests = get_flaky_tests(db_session, repoid, failures) + + failures = sorted(failures, key=lambda x: x.duration_seconds)[:3] + + info = TACommentInDepthInfo(failures, flaky_tests) additional_data: AdditionalData = {"upload_type": UploadType.TEST_RESULTS} repo_service = get_repo_provider_service(repo, additional_data=additional_data) @@ -340,13 +316,21 @@ def process_impl_within_lock( if seat_activation_result: return seat_activation_result - flaky_tests = dict() - if should_do_flaky_detection(repo, commit_yaml): - flaky_tests = get_flaky_tests(db_session, repoid, failures) + upload_error = ( + db_session.query(UploadError) + .filter(UploadError.upload_id.in_(uploads.keys())) + .first() + ) + + if not (info or upload_error): + return { + "notify_attempted": False, + "notify_succeeded": False, + "queue_notify": True, + } - failures = sorted(failures, key=lambda x: x.duration_seconds)[:3] payload = TestResultsNotificationPayload( - totals.failed, totals.passed, totals.skipped, failures, flaky_tests + totals.failed, totals.passed, totals.skipped, info ) notifier = TestResultsNotifier( commit, @@ -354,7 +338,7 @@ def process_impl_within_lock( payload=payload, _pull=pull, _repo_service=repo_service, - errors=upload_errors, + error=upload_error, ) notifier_result = notifier.notify() for upload in uploads.values(): @@ -373,7 +357,7 @@ def process_impl_within_lock( return { "notify_attempted": True, "notify_succeeded": success, - "queue_notify": False, + "queue_notify": not info, } def seat_activation( diff --git a/tasks/test_results_finisher.py b/tasks/test_results_finisher.py index 7b0a55160..634f70060 100644 --- a/tasks/test_results_finisher.py +++ b/tasks/test_results_finisher.py @@ -24,6 +24,7 @@ from services.seats import ShouldActivateSeat, determine_seat_activation from services.test_results import ( FlakeInfo, + TACommentInDepthInfo, TestResultsNotificationFailure, TestResultsNotificationPayload, TestResultsNotifier, @@ -288,8 +289,9 @@ def process_impl_within_lock( flaky_tests = self.get_flaky_tests(db_session, repoid, failures) failures = sorted(failures, key=lambda x: x.duration_seconds)[:3] + info = TACommentInDepthInfo(failures, flaky_tests) payload = TestResultsNotificationPayload( - failed_tests, passed_tests, skipped_tests, failures, flaky_tests + failed_tests, passed_tests, skipped_tests, info ) notifier = TestResultsNotifier( commit, commit_yaml, payload=payload, _pull=pull, _repo_service=repo_service diff --git a/tasks/tests/unit/test_ta_finisher_task.py b/tasks/tests/unit/test_ta_finisher_task.py index e5f33f6a5..9af5fce49 100644 --- a/tasks/tests/unit/test_ta_finisher_task.py +++ b/tasks/tests/unit/test_ta_finisher_task.py @@ -377,8 +377,8 @@ def test_test_analytics_error_comment( commit_yaml={"codecov": {"max_report_age": False}}, ) - assert result["notify_attempted"] is False - assert result["notify_succeeded"] is False + assert result["notify_attempted"] is True + assert result["notify_succeeded"] is True assert result["queue_notify"] is True mock_repo_provider_service.edit_comment.assert_called_once()