From 79e838ee1e7a8d094fffb4057d93f257764a33b9 Mon Sep 17 00:00:00 2001 From: Suejung Shin Date: Mon, 2 Dec 2024 17:14:16 -0800 Subject: [PATCH 1/4] fix: Filter components by id in addition to name --- api/internal/coverage/views.py | 2 +- .../commands/compare/interactors/fetch_impacted_files.py | 2 +- graphql_api/types/commit/commit.py | 8 ++++---- graphql_api/types/comparison/comparison.py | 2 +- services/components.py | 9 +++++++-- 5 files changed, 14 insertions(+), 9 deletions(-) diff --git a/api/internal/coverage/views.py b/api/internal/coverage/views.py index 1ef9e3863d..66998f9337 100644 --- a/api/internal/coverage/views.py +++ b/api/internal/coverage/views.py @@ -43,7 +43,7 @@ def get_object(self) -> ReportPaths: component_paths = [] if components: all_components = components_service.commit_components(commit, self.owner) - filtered_components = components_service.filter_components_by_name( + filtered_components = components_service.filter_components_by_name_or_id( all_components, components ) diff --git a/compare/commands/compare/interactors/fetch_impacted_files.py b/compare/commands/compare/interactors/fetch_impacted_files.py index 678e0c06ea..0e722d1d38 100644 --- a/compare/commands/compare/interactors/fetch_impacted_files.py +++ b/compare/commands/compare/interactors/fetch_impacted_files.py @@ -45,7 +45,7 @@ def _apply_filters( all_components = components.commit_components( comparison.head_commit, comparison.user ) - filtered_components = components.filter_components_by_name( + filtered_components = components.filter_components_by_name_or_id( all_components, components_filter ) for component in filtered_components: diff --git a/graphql_api/types/commit/commit.py b/graphql_api/types/commit/commit.py index 2fbc03b414..4cc7bf58ed 100644 --- a/graphql_api/types/commit/commit.py +++ b/graphql_api/types/commit/commit.py @@ -178,7 +178,7 @@ def resolve_path_contents(commit: Commit, info, path: str = None, filters=None): if component_filter: all_components = components_service.commit_components(commit, current_owner) - filtered_components = components_service.filter_components_by_name( + filtered_components = components_service.filter_components_by_name_or_id( all_components, component_filter ) @@ -268,7 +268,7 @@ def resolve_deprecated_path_contents( if component_filter: all_components = components_service.commit_components(commit, current_owner) - filtered_components = components_service.filter_components_by_name( + filtered_components = components_service.filter_components_by_name_or_id( all_components, component_filter ) @@ -394,7 +394,7 @@ def resolve_coverage_file(commit, info, path, flags=None, components=None): all_components = components_service.commit_components( commit, info.context["request"].current_owner ) - filtered_components = components_service.filter_components_by_name( + filtered_components = components_service.filter_components_by_name_or_id( all_components, components ) for fc in filtered_components: @@ -423,7 +423,7 @@ def resolve_coverage_components(commit: Commit, info, filters=None) -> List[Comp all_components = components_service.commit_components(commit, current_owner) if filters and filters.get("components"): - return components_service.filter_components_by_name( + return components_service.filter_components_by_name_or_id( all_components, filters["components"] ) diff --git a/graphql_api/types/comparison/comparison.py b/graphql_api/types/comparison/comparison.py index 70d1a0a714..36eeb53fa4 100644 --- a/graphql_api/types/comparison/comparison.py +++ b/graphql_api/types/comparison/comparison.py @@ -203,7 +203,7 @@ def resolve_component_comparisons( list_components = comparison_report.commit_comparison.component_comparisons.all() if filters and filters.get("components"): - components = components_service.filter_components_by_name( + components = components_service.filter_components_by_name_or_id( components, filters["components"] ) diff --git a/services/components.py b/services/components.py index 34460588e8..318e22e01b 100644 --- a/services/components.py +++ b/services/components.py @@ -39,15 +39,20 @@ def component_filtered_report( return filtered_report -def filter_components_by_name( +def filter_components_by_name_or_id( components: List[Component], terms: List[str] ) -> List[Component]: """ Given a list of Components and a list of strings (terms), return a new list of Components only including Components with names in terms (case insensitive) + OR component_id in terms (case insensitive) """ terms = [v.lower() for v in terms] - return list(filter(lambda c: c.name.lower() in terms, components)) + return [ + component + for component in components + if component.name.lower() in terms or component.component_id.lower() in terms + ] class ComponentComparison: From a4edd1f28e943a039ae79ac4c9f2774e7e44c72f Mon Sep 17 00:00:00 2001 From: Suejung Shin Date: Mon, 2 Dec 2024 17:48:45 -0800 Subject: [PATCH 2/4] add unit test --- services/tests/test_components.py | 34 ++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/services/tests/test_components.py b/services/tests/test_components.py index 2fa214bcd4..9efde6a8a7 100644 --- a/services/tests/test_components.py +++ b/services/tests/test_components.py @@ -16,7 +16,7 @@ from services.components import ( ComponentComparison, commit_components, - component_filtered_report, + component_filtered_report, filter_components_by_name_or_id, ) @@ -158,8 +158,8 @@ def test_base_report(self, base_report_mock): component_comparison = ComponentComparison(self.comparison, component_go) assert component_comparison.base_report.files == ["file_1.go"] assert ( - component_comparison.base_report.totals.coverage - == report.get("file_1.go").totals.coverage + component_comparison.base_report.totals.coverage + == report.get("file_1.go").totals.coverage ) @patch("services.comparison.Comparison.head_report", new_callable=PropertyMock) @@ -176,8 +176,8 @@ def test_head_report(self, head_report_mock): component_comparison = ComponentComparison(self.comparison, component_go) assert component_comparison.head_report.files == ["file_1.go"] assert ( - component_comparison.head_report.totals.coverage - == report.get("file_1.go").totals.coverage + component_comparison.head_report.totals.coverage + == report.get("file_1.go").totals.coverage ) @patch("services.comparison.Comparison.git_comparison", new_callable=PropertyMock) @@ -221,3 +221,27 @@ def test_patch_totals(self, head_report_mock, git_comparison_mock): # removed 1 tested line, added 1 tested and 1 untested line assert component_comparison.patch_totals.coverage == "50.00000" + + def test_filter_components_by_name_or_id(self): + components = [ + Component(name="ComponentA", component_id="123"), + Component(name="ComponentB", component_id="456"), + Component(name="ComponentC", component_id="789"), + ] + terms = ["comPOnentA", "123", "456"] + + filtered = filter_components_by_name_or_id(components, terms) + self.assertEqual(len(filtered), 2) + self.assertEqual(filtered[0].name, "ComponentA") + self.assertEqual(filtered[1].component_id, "456") + + def test_filter_components_by_name_or_id_no_matches(self): + components = [ + Component(name="ComponentA", component_id="123"), + Component(name="ComponentB", component_id="456"), + Component(name="ComponentC", component_id="789"), + ] + terms = ["nonexistent", "000"] + + filtered = filter_components_by_name_or_id(components, terms) + self.assertEqual(len(filtered), 0) From f003a0189fde909be0b862b3dfca1d2337ffa0e4 Mon Sep 17 00:00:00 2001 From: Suejung Shin Date: Mon, 2 Dec 2024 18:21:38 -0800 Subject: [PATCH 3/4] fix test --- services/tests/test_components.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/services/tests/test_components.py b/services/tests/test_components.py index 9efde6a8a7..2dbc36f1de 100644 --- a/services/tests/test_components.py +++ b/services/tests/test_components.py @@ -224,9 +224,9 @@ def test_patch_totals(self, head_report_mock, git_comparison_mock): def test_filter_components_by_name_or_id(self): components = [ - Component(name="ComponentA", component_id="123"), - Component(name="ComponentB", component_id="456"), - Component(name="ComponentC", component_id="789"), + Component(name="ComponentA", component_id="123", paths=[], flag_regexes=[], statuses=[]), + Component(name="ComponentB", component_id="456", paths=[], flag_regexes=[], statuses=[]), + Component(name="ComponentC", component_id="789", paths=[], flag_regexes=[], statuses=[]), ] terms = ["comPOnentA", "123", "456"] @@ -237,9 +237,9 @@ def test_filter_components_by_name_or_id(self): def test_filter_components_by_name_or_id_no_matches(self): components = [ - Component(name="ComponentA", component_id="123"), - Component(name="ComponentB", component_id="456"), - Component(name="ComponentC", component_id="789"), + Component(name="ComponentA", component_id="123", paths=[], flag_regexes=[], statuses=[]), + Component(name="ComponentB", component_id="456", paths=[], flag_regexes=[], statuses=[]), + Component(name="ComponentC", component_id="789", paths=[], flag_regexes=[], statuses=[]), ] terms = ["nonexistent", "000"] From a65bf875d5290d360264fa1243547d17ea77a9eb Mon Sep 17 00:00:00 2001 From: Suejung Shin Date: Mon, 2 Dec 2024 23:07:41 -0800 Subject: [PATCH 4/4] fix lint --- services/tests/test_components.py | 59 +++++++++++++++++++++++++------ 1 file changed, 48 insertions(+), 11 deletions(-) diff --git a/services/tests/test_components.py b/services/tests/test_components.py index 2dbc36f1de..402f881cd0 100644 --- a/services/tests/test_components.py +++ b/services/tests/test_components.py @@ -16,7 +16,8 @@ from services.components import ( ComponentComparison, commit_components, - component_filtered_report, filter_components_by_name_or_id, + component_filtered_report, + filter_components_by_name_or_id, ) @@ -158,8 +159,8 @@ def test_base_report(self, base_report_mock): component_comparison = ComponentComparison(self.comparison, component_go) assert component_comparison.base_report.files == ["file_1.go"] assert ( - component_comparison.base_report.totals.coverage - == report.get("file_1.go").totals.coverage + component_comparison.base_report.totals.coverage + == report.get("file_1.go").totals.coverage ) @patch("services.comparison.Comparison.head_report", new_callable=PropertyMock) @@ -176,8 +177,8 @@ def test_head_report(self, head_report_mock): component_comparison = ComponentComparison(self.comparison, component_go) assert component_comparison.head_report.files == ["file_1.go"] assert ( - component_comparison.head_report.totals.coverage - == report.get("file_1.go").totals.coverage + component_comparison.head_report.totals.coverage + == report.get("file_1.go").totals.coverage ) @patch("services.comparison.Comparison.git_comparison", new_callable=PropertyMock) @@ -224,9 +225,27 @@ def test_patch_totals(self, head_report_mock, git_comparison_mock): def test_filter_components_by_name_or_id(self): components = [ - Component(name="ComponentA", component_id="123", paths=[], flag_regexes=[], statuses=[]), - Component(name="ComponentB", component_id="456", paths=[], flag_regexes=[], statuses=[]), - Component(name="ComponentC", component_id="789", paths=[], flag_regexes=[], statuses=[]), + Component( + name="ComponentA", + component_id="123", + paths=[], + flag_regexes=[], + statuses=[], + ), + Component( + name="ComponentB", + component_id="456", + paths=[], + flag_regexes=[], + statuses=[], + ), + Component( + name="ComponentC", + component_id="789", + paths=[], + flag_regexes=[], + statuses=[], + ), ] terms = ["comPOnentA", "123", "456"] @@ -237,9 +256,27 @@ def test_filter_components_by_name_or_id(self): def test_filter_components_by_name_or_id_no_matches(self): components = [ - Component(name="ComponentA", component_id="123", paths=[], flag_regexes=[], statuses=[]), - Component(name="ComponentB", component_id="456", paths=[], flag_regexes=[], statuses=[]), - Component(name="ComponentC", component_id="789", paths=[], flag_regexes=[], statuses=[]), + Component( + name="ComponentA", + component_id="123", + paths=[], + flag_regexes=[], + statuses=[], + ), + Component( + name="ComponentB", + component_id="456", + paths=[], + flag_regexes=[], + statuses=[], + ), + Component( + name="ComponentC", + component_id="789", + paths=[], + flag_regexes=[], + statuses=[], + ), ] terms = ["nonexistent", "000"]