From d3ca22318970df30ec16dc68b43ab51be60dd4e9 Mon Sep 17 00:00:00 2001 From: Dana Date: Thu, 21 Sep 2023 19:19:13 +0300 Subject: [PATCH 1/2] create new files section --- .../notifiers/mixins/message/__init__.py | 2 +- .../notifiers/mixins/message/helpers.py | 31 +--- .../notifiers/mixins/message/sections.py | 144 ++++++++++++------ .../notifiers/tests/unit/test_comment.py | 9 +- 4 files changed, 111 insertions(+), 75 deletions(-) diff --git a/services/notification/notifiers/mixins/message/__init__.py b/services/notification/notifiers/mixins/message/__init__.py index 0df9debf8..4fb98398c 100644 --- a/services/notification/notifiers/mixins/message/__init__.py +++ b/services/notification/notifiers/mixins/message/__init__.py @@ -190,7 +190,7 @@ def get_middle_layout_section_names(self, settings): return [ section for section in sections - if section not in ["header", "newheader", "newfooter"] + if section not in ["header", "newheader", "newfooter", "newfiles"] ] def get_upper_section_name(self, settings): diff --git a/services/notification/notifiers/mixins/message/helpers.py b/services/notification/notifiers/mixins/message/helpers.py index d9792fb3a..23debe4b4 100644 --- a/services/notification/notifiers/mixins/message/helpers.py +++ b/services/notification/notifiers/mixins/message/helpers.py @@ -149,32 +149,17 @@ def make_patch_only_metrics(before, after, relative, show_complexity, yaml, pull return "".join(("|", coverage, missing_line_str)) -def get_metrics_function(hide_project_coverage): - if hide_project_coverage: - metrics = make_patch_only_metrics - else: - metrics = make_metrics - return metrics - +def get_table_header(show_complexity): -def get_table_header(hide_project_coverage, show_complexity): - if not hide_project_coverage: - table_header = ( - "| Coverage \u0394 |" - + (" Complexity \u0394 |" if show_complexity else "") - + " |" - ) - else: - table_header = "| Patch % | Lines |" - - return table_header + return ( + "| Coverage \u0394 |" + + (" Complexity \u0394 |" if show_complexity else "") + + " |" + ) -def get_table_layout(hide_project_coverage, show_complexity): - if hide_project_coverage: - return "|---|---|---|" - else: - return "|---|---|---|" + ("---|" if show_complexity else "") +def get_table_layout(show_complexity): + return "|---|---|---|" + ("---|" if show_complexity else "") def format_number_to_str( diff --git a/services/notification/notifiers/mixins/message/sections.py b/services/notification/notifiers/mixins/message/sections.py index 89bb88643..9da7f012d 100644 --- a/services/notification/notifiers/mixins/message/sections.py +++ b/services/notification/notifiers/mixins/message/sections.py @@ -15,10 +15,10 @@ diff_to_string, ellipsis, escape_markdown, - get_metrics_function, get_table_header, get_table_layout, make_metrics, + make_patch_only_metrics, sort_by_importance, ) from services.urls import get_commit_url_from_commit_sha, get_pull_graph_url @@ -50,6 +50,8 @@ def get_section_class_from_layout_name(layout_name): return NewFooterSectionWriter if layout_name.startswith("component"): return ComponentsSectionWriter + if layout_name == "newfiles": + return NewFilesSectionWriter class BaseSectionWriter(object): @@ -451,20 +453,18 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non yield ("```") -class FileSectionWriter(BaseSectionWriter): +class NewFilesSectionWriter(BaseSectionWriter): async def do_write_section(self, comparison, diff, changes, links, behind_by=None): # create list of files changed in diff base_report = comparison.base.report head_report = comparison.head.report if base_report is None: base_report = Report() - hide_project_coverage = self.settings.get("hide_project_coverage", False) - make_metrics_fn = get_metrics_function(hide_project_coverage) files_in_diff = [ ( _diff["type"], path, - make_metrics_fn( + make_patch_only_metrics( get_totals_from_file_in_reports(base_report, path) or False, get_totals_from_file_in_reports(head_report, path) or False, _diff["totals"], @@ -482,8 +482,8 @@ async def do_write_section(self, comparison, diff, changes, links, behind_by=Non c.path for c in changes or [] ) if files_in_diff: - table_header = get_table_header(hide_project_coverage, self.show_complexity) - table_layout = get_table_layout(hide_project_coverage, self.show_complexity) + table_header = "| Patch % | Lines |" + table_layout = "|---|---|---|" # get limit of results to show limit = int(self.layout.split(":")[1] if ":" in self.layout else 10) @@ -508,56 +508,106 @@ def tree_cell(typ, path, metrics, _=None): file_tags=" **Critical**" if path in files_in_critical else "", ) - if not hide_project_coverage: + remaining_files = 0 + printed_files = 0 + changed_files = sorted( + files_in_diff, key=lambda a: a[3] or Decimal("0"), reverse=True + ) + changed_files_with_missing_lines = [f for f in changed_files if f[3] > 0] + if changed_files_with_missing_lines: yield ( "| [Files]({0}?src=pr&el=tree) {1}".format( links["pull"], table_header ) ) yield (table_layout) - for line in starmap( - tree_cell, - sorted(files_in_diff, key=lambda a: a[3] or Decimal("0"))[:limit], - ): - yield (line) - - remaining = len(files_in_diff) - limit - if remaining > 0: - yield ( - "| ... and [{n} more]({href}?src=pr&el=tree-more) | |".format( - n=remaining, href=links["pull"] - ) + for file in changed_files_with_missing_lines: + if printed_files == limit: + remaining_files += 1 + else: + printed_files += 1 + yield (tree_cell(file[0], file[1], file[2])) + if remaining_files: + yield ( + "| ... and [{n} more]({href}?src=pr&el=tree-more) | |".format( + n=remaining_files, href=links["pull"] ) - else: - remaining_files = 0 - printed_files = 0 - changed_files = sorted( - files_in_diff, key=lambda a: a[3] or Decimal("0"), reverse=True ) - changed_files_with_missing_lines = [ - f for f in changed_files if f[3] > 0 - ] - if changed_files_with_missing_lines: - yield ( - "| [Files]({0}?src=pr&el=tree) {1}".format( - links["pull"], table_header - ) + + +class FileSectionWriter(BaseSectionWriter): + async def do_write_section(self, comparison, diff, changes, links, behind_by=None): + # create list of files changed in diff + base_report = comparison.base.report + head_report = comparison.head.report + if base_report is None: + base_report = Report() + files_in_diff = [ + ( + _diff["type"], + path, + make_metrics( + get_totals_from_file_in_reports(base_report, path) or False, + get_totals_from_file_in_reports(head_report, path) or False, + _diff["totals"], + self.show_complexity, + self.current_yaml, + links["pull"], + ), + int(_diff["totals"].misses + _diff["totals"].partials), + ) + for path, _diff in (diff["files"] if diff else {}).items() + if _diff.get("totals") + ] + + all_files = set(f[1] for f in files_in_diff or []) | set( + c.path for c in changes or [] + ) + if files_in_diff: + table_header = get_table_header(self.show_complexity) + table_layout = get_table_layout(self.show_complexity) + + # get limit of results to show + limit = int(self.layout.split(":")[1] if ":" in self.layout else 10) + mentioned = [] + files_in_critical = set() + if self.settings.get("show_critical_paths", False): + overlay = comparison.get_overlay(OverlayType.line_execution_count) + files_in_critical = set( + await overlay.search_files_for_critical_changes(all_files) + ) + + def tree_cell(typ, path, metrics, _=None): + if path not in mentioned: + # mentioned: for files that are in diff and changes + mentioned.append(path) + return "| {rm}[{path}]({compare}?src=pr&el=tree#diff-{hash}){rm}{file_tags} {metrics}".format( + rm="~~" if typ == "deleted" else "", + path=escape_markdown(ellipsis(path, 50, False)), + compare=links["pull"], + hash=b64encode(path.encode()).decode(), + metrics=metrics, + file_tags=" **Critical**" if path in files_in_critical else "", ) - yield (table_layout) - for file in changed_files_with_missing_lines: - if printed_files == limit: - remaining_files += 1 - else: - printed_files += 1 - yield (tree_cell(file[0], file[1], file[2])) - - if remaining_files: - yield ( - "| ... and [{n} more]({href}?src=pr&el=tree-more) | |".format( - n=remaining_files, href=links["pull"] - ) + + yield ( + "| [Files]({0}?src=pr&el=tree) {1}".format(links["pull"], table_header) + ) + yield (table_layout) + for line in starmap( + tree_cell, + sorted(files_in_diff, key=lambda a: a[3] or Decimal("0"))[:limit], + ): + yield (line) + remaining = len(files_in_diff) - limit + if remaining > 0: + yield ( + "| ... and [{n} more]({href}?src=pr&el=tree-more) | |".format( + n=remaining, href=links["pull"] ) - if changes and not hide_project_coverage: + ) + + if changes: len_changes_not_in_diff = len(all_files or []) - len(files_in_diff or []) if files_in_diff and len_changes_not_in_diff > 0: yield ("") diff --git a/services/notification/notifiers/tests/unit/test_comment.py b/services/notification/notifiers/tests/unit/test_comment.py index c3d05fe48..85cf4d5c3 100644 --- a/services/notification/notifiers/tests/unit/test_comment.py +++ b/services/notification/notifiers/tests/unit/test_comment.py @@ -29,6 +29,7 @@ ComponentsSectionWriter, FileSectionWriter, ImpactedEntrypointsSectionWriter, + NewFilesSectionWriter, NewFooterSectionWriter, NewHeaderSectionWriter, ) @@ -3488,7 +3489,7 @@ async def test_file_with_critical(self, sample_comparison, mocker): @pytest.mark.asyncio async def test_filesection_hide_project_cov(self, sample_comparison, mocker): - section_writer = FileSectionWriter( + section_writer = NewFilesSectionWriter( sample_comparison.head.commit.repository, "layout", show_complexity=False, @@ -3572,7 +3573,7 @@ async def test_filesection_hide_project_cov(self, sample_comparison, mocker): async def test_filesection_hide_project_cov_with_changed_files_but_no_missing_lines( self, sample_comparison, mocker ): - section_writer = FileSectionWriter( + section_writer = NewFilesSectionWriter( sample_comparison.head.commit.repository, "layout", show_complexity=False, @@ -3651,7 +3652,7 @@ async def test_filesection_hide_project_cov_with_changed_files_but_no_missing_li async def test_filesection_hide_project_cov_no_files_changed( self, sample_comparison, mocker ): - section_writer = FileSectionWriter( + section_writer = NewFilesSectionWriter( sample_comparison.head.commit.repository, "layout", show_complexity=False, @@ -4391,7 +4392,7 @@ async def test_build_message_no_project_coverage( repository=comparison.head.commit.repository, title="title", notifier_yaml_settings={ - "layout": "newheader, files, newfooter", + "layout": "newheader, newfiles, newfooter", "hide_project_coverage": True, }, notifier_site_settings=True, From 5a7cba518c6e023059f9442305357fa96e913041 Mon Sep 17 00:00:00 2001 From: Dana Date: Fri, 22 Sep 2023 14:49:37 +0300 Subject: [PATCH 2/2] extracting newfiles from middle sections and including it in upper ones: --- .../notifiers/mixins/message/__init__.py | 43 +++++++++++-------- 1 file changed, 26 insertions(+), 17 deletions(-) diff --git a/services/notification/notifiers/mixins/message/__init__.py b/services/notification/notifiers/mixins/message/__init__.py index 4fb98398c..49ed9a78d 100644 --- a/services/notification/notifiers/mixins/message/__init__.py +++ b/services/notification/notifiers/mixins/message/__init__.py @@ -72,19 +72,22 @@ async def create_message( write = message.append # note: since we're using append, calling write("") will add a newline to the message - upper_section_name = self.get_upper_section_name(settings) - section_writer_class = get_section_class_from_layout_name(upper_section_name) - section_writer = section_writer_class( - self.repository, - upper_section_name, - show_complexity, - settings, - current_yaml, - ) - - await self.write_section_to_msg( - comparison, changes, diff, links, write, section_writer, behind_by - ) + upper_section_names = self.get_upper_section_names(settings) + for upper_section_name in upper_section_names: + section_writer_class = get_section_class_from_layout_name( + upper_section_name + ) + section_writer = section_writer_class( + self.repository, + upper_section_name, + show_complexity, + settings, + current_yaml, + ) + + await self.write_section_to_msg( + comparison, changes, diff, links, write, section_writer, behind_by + ) is_compact_message = should_message_be_compact(comparison, settings) @@ -193,10 +196,16 @@ def get_middle_layout_section_names(self, settings): if section not in ["header", "newheader", "newfooter", "newfiles"] ] - def get_upper_section_name(self, settings): - if "newheader" in settings["layout"]: - return "newheader" - return "header" + def get_upper_section_names(self, settings): + sections = list(map(lambda l: l.strip(), (settings["layout"] or "").split(","))) + if "newheader" not in sections and "header" not in sections: + sections.insert(0, "header") + + return [ + section + for section in sections + if section in ["header", "newheader", "newfiles"] + ] def get_lower_section_name(self, settings): if "newfooter" in settings["layout"]: