Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Filter file comparisons by flags #129

Merged
merged 4 commits into from
Sep 14, 2023
Merged

Filter file comparisons by flags #129

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2cf907e
filter file comparisons by flag
adrian-codecov Sep 13, 2023
c54acca
Merge branch 'main' of github.com:codecov/codecov-api into 453-add-fl…
adrian-codecov Sep 13, 2023
7d09189
Merge branch 'main' of github.com:codecov/codecov-api into 453-add-fl…
adrian-codecov Sep 14, 2023
e71b2e4
Adjust code based on pr feedback
adrian-codecov Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions compare/commands/compare/compare.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,20 @@
from typing import Union

from shared.reports.resources import Report

from codecov.commands.base import BaseCommand
from services.comparison import Comparison, ComparisonReport, PullRequestComparison

from .interactors.fetch_impacted_files import FetchImpactedFiles


class CompareCommands(BaseCommand):
def fetch_impacted_files(self, comparsion, filters):
return self.get_interactor(FetchImpactedFiles).execute(comparsion, filters)
def fetch_impacted_files(
self,
comparison_report: ComparisonReport,
comparison: Union[PullRequestComparison, Comparison],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just Comparison is fine here since PullRequestComparison inherits from Comparison and we're not using anything specific to PullRequestComparison.

filters,
):
return self.get_interactor(FetchImpactedFiles).execute(
comparison_report, comparison, filters
)
37 changes: 33 additions & 4 deletions compare/commands/compare/interactors/fetch_impacted_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
import enum
from typing import List, Optional, Union

from shared.reports.resources import Report

from codecov.commands.base import BaseInteractor
from services.comparison import ImpactedFile
from services.comparison import (
Comparison,
ComparisonReport,
ImpactedFile,
PullRequestComparison,
)
from services.report import files_belonging_to_flags


class ImpactedFileParameter(enum.Enum):
Expand All @@ -13,13 +22,28 @@ class ImpactedFileParameter(enum.Enum):


class FetchImpactedFiles(BaseInteractor):
def _apply_filters(self, impacted_files, filters):
def _apply_filters(
self,
impacted_files: Optional[List[ImpactedFile]],
comparison: Union[PullRequestComparison, Comparison],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here - can just be Comparison

filters,
):
parameter = filters.get("ordering", {}).get("parameter")
direction = filters.get("ordering", {}).get("direction")
if parameter and direction:
impacted_files = self.sort_impacted_files(
impacted_files, parameter, direction
)
flags = filters.get("flags", [])
if flags and comparison:
head_commit_report = comparison.head_report
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brought the comparison all the way down here to only compute the head_report if the flags are provided

if set(flags) & set(head_commit_report.flags):
files = files_belonging_to_flags(
commit_report=head_commit_report, flags=flags
)
impacted_files = list(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might read a little easier:

impacted_files = [
    file
    for file in impacted_files
    if file.head_name in files
]

filter(lambda x: x.head_name in files, impacted_files)
)
return impacted_files

def get_attribute(
Expand Down Expand Up @@ -65,7 +89,12 @@ def sort_impacted_files(self, impacted_files, parameter, direction):
# Merge both lists together
return files_with_coverage + files_without_coverage

def execute(self, comparison_report, filters):
def execute(
self,
comparison_report: ComparisonReport,
comparison: Union[PullRequestComparison, Comparison],
filters,
):
if filters is None:
return comparison_report.impacted_files

Expand All @@ -79,4 +108,4 @@ def execute(self, comparison_report, filters):
else:
impacted_files = comparison_report.impacted_files

return self._apply_filters(impacted_files, filters)
return self._apply_filters(impacted_files, comparison, filters)
131 changes: 111 additions & 20 deletions compare/commands/compare/interactors/tests/test_fetch_impacted_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,18 @@
import enum
from unittest.mock import patch
from unittest.mock import PropertyMock, patch

from django.contrib.auth.models import AnonymousUser
from django.test import TransactionTestCase
from shared.reports.resources import Report, ReportFile, ReportLine
from shared.utils.sessions import Session

from codecov_auth.tests.factories import OwnerFactory
from compare.commands.compare.interactors.fetch_impacted_files import (
ImpactedFileParameter,
)
from compare.tests.factories import CommitComparisonFactory
from core.tests.factories import CommitFactory
from services.comparison import ComparisonReport
from core.tests.factories import CommitFactory, PullFactory, RepositoryFactory
from services.comparison import Comparison, ComparisonReport, PullRequestComparison

from ..fetch_impacted_files import FetchImpactedFiles

Expand Down Expand Up @@ -274,7 +276,7 @@ def setUp(self):
compare_commit=self.commit,
report_storage_path="v4/test.json",
)
self.comparison = ComparisonReport(self.commit_comparison)
self.comparison_report = ComparisonReport(self.commit_comparison)

# helper to execute the interactor
def execute(self, owner, *args):
Expand All @@ -287,7 +289,8 @@ def test_impacted_file_sort_function(self, read_file):
parameter = ImpactedFileParameter.CHANGE_COVERAGE
direction = OrderingDirection.ASC
filters = {"ordering": {"parameter": parameter, "direction": direction}}
sorted_files = self.execute(None, self.comparison, filters)
comparison = None
sorted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in sorted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -296,7 +299,8 @@ def test_impacted_file_sort_function_no_misses(self, read_file):
parameter = ImpactedFileParameter.MISSES_COUNT
direction = OrderingDirection.ASC
filters = {"ordering": {"parameter": parameter, "direction": direction}}
sorted_files = self.execute(None, self.comparison, filters)
comparison = None
sorted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in sorted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -305,9 +309,10 @@ def test_impacted_file_sort_function_error(self, read_file):
parameter = "something else"
direction = OrderingDirection.DESC
filters = {"ordering": {"parameter": parameter, "direction": direction}}
comparison = None

with self.assertRaises(ValueError) as ctx:
self.execute(None, self.comparison, filters)
self.execute(None, self.comparison_report, comparison, filters)
self.assertEqual(
"invalid impacted file parameter: something else", str(ctx.exception)
)
Expand All @@ -321,7 +326,8 @@ def test_impacted_files_filtered_by_change_coverage_ascending(self, read_file):
"parameter": ImpactedFileParameter.CHANGE_COVERAGE,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -333,7 +339,8 @@ def test_impacted_files_filtered_by_change_coverage_descending(self, read_file):
"parameter": ImpactedFileParameter.CHANGE_COVERAGE,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileB", "fileA"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -345,7 +352,8 @@ def test_impacted_files_filtered_by_head_coverage_ascending(self, read_file):
"parameter": ImpactedFileParameter.HEAD_COVERAGE,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -357,7 +365,8 @@ def test_impacted_files_filtered_by_patch_coverage_ascending(self, read_file):
"parameter": ImpactedFileParameter.PATCH_COVERAGE,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -369,7 +378,8 @@ def test_impacted_files_filtered_by_patch_coverage_descending(self, read_file):
"parameter": ImpactedFileParameter.PATCH_COVERAGE,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileB", "fileA"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -381,7 +391,8 @@ def test_impacted_files_filtered_by_head_coverage_descending(self, read_file):
"parameter": ImpactedFileParameter.HEAD_COVERAGE,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileB", "fileA"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -393,7 +404,8 @@ def test_impacted_files_filtered_by_head_name_ascending(self, read_file):
"parameter": ImpactedFileParameter.FILE_NAME,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -405,7 +417,8 @@ def test_impacted_files_filtered_by_head_name_descending(self, read_file):
"parameter": ImpactedFileParameter.FILE_NAME,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileB", "fileA"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -417,7 +430,8 @@ def test_impacted_files_filtered_by_misses_count_ascending(self, read_file):
"parameter": ImpactedFileParameter.MISSES_COUNT,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -429,14 +443,16 @@ def test_impacted_files_filtered_by_misses_count_descending(self, read_file):
"parameter": ImpactedFileParameter.MISSES_COUNT,
}
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileB", "fileA"]

@patch("services.archive.ArchiveService.read_file")
def test_impacted_files_without_filters(self, read_file):
read_file.return_value = mock_data_from_archive
filters = {}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -449,7 +465,8 @@ def test_impacted_files_filtered_by_unintended_changes(self, read_file):
"parameter": ImpactedFileParameter.FILE_NAME,
},
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.archive.ArchiveService.read_file")
Expand All @@ -464,5 +481,79 @@ def test_impacted_files_filtered_by_unintended_changes_set_to_false(
"parameter": ImpactedFileParameter.FILE_NAME,
},
}
impacted_files = self.execute(None, self.comparison, filters)
comparison = None
impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert [file.head_name for file in impacted_files] == ["fileA", "fileB"]

@patch("services.comparison.Comparison.head_report", new_callable=PropertyMock)
@patch("services.archive.ArchiveService.read_file")
def test_impacted_files_filtered_by_flags_and_commit_comparison_for_pull(
self, read_file, build_report_from_commit_mock
):
read_file.return_value = mocked_files_with_direct_and_indirect_changes

commit_report = Report()
session_a_id, _ = commit_report.add_session(Session(flags=["flag-123"]))
session_b_id, _ = commit_report.add_session(Session(flags=["flag-456"]))
file_a = ReportFile("fileA")
file_a.append(1, ReportLine.create(coverage=1, sessions=[[session_a_id, 1]]))
commit_report.append(file_a)
file_b = ReportFile("fileB")
file_b.append(1, ReportLine.create(coverage=1, sessions=[[session_b_id, 1]]))
commit_report.append(file_b)
build_report_from_commit_mock.return_value = commit_report

flags = ["flag-123"]
filters = {"flags": flags}

owner = OwnerFactory()
repo = RepositoryFactory(author=owner)
base, head, compared_to = (
CommitFactory(repository=repo),
CommitFactory(repository=repo),
CommitFactory(repository=repo),
)
pull = PullFactory(
repository=repo,
base=base.commitid,
head=head.commitid,
compared_to=compared_to.commitid,
)
comparison = PullRequestComparison(user=owner, pull=pull)

impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert len(impacted_files) == 1
assert impacted_files[0].head_name == "fileA"

@patch("services.comparison.Comparison.head_report", new_callable=PropertyMock)
@patch("services.archive.ArchiveService.read_file")
def test_impacted_files_filtered_by_flags_and_commit_comparison_for_parent_commit(
self, read_file, build_report_from_commit_mock
):
read_file.return_value = mocked_files_with_direct_and_indirect_changes

commit_report = Report()
session_a_id, _ = commit_report.add_session(Session(flags=["flag-123"]))
session_b_id, _ = commit_report.add_session(Session(flags=["flag-456"]))
file_a = ReportFile("fileA")
file_a.append(1, ReportLine.create(coverage=1, sessions=[[session_a_id, 1]]))
commit_report.append(file_a)
file_b = ReportFile("fileB")
file_b.append(1, ReportLine.create(coverage=1, sessions=[[session_b_id, 1]]))
commit_report.append(file_b)
build_report_from_commit_mock.return_value = commit_report

flags = ["flag-123"]
filters = {"flags": flags}

owner = OwnerFactory()
repo = RepositoryFactory(author=owner)
base, head = (
CommitFactory(repository=repo),
CommitFactory(repository=repo),
)
comparison = Comparison(user=owner, base_commit=base, head_commit=head)

impacted_files = self.execute(None, self.comparison_report, comparison, filters)
assert len(impacted_files) == 1
assert impacted_files[0].head_name == "fileA"
2 changes: 1 addition & 1 deletion graphql_api/types/commit/commit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def resolve_path_contents(commit: Commit, info, path: str = None, filters=None):
filters = {}
search_value = filters.get("search_value")
display_type = filters.get("display_type")
flags = filters.get("flags") or []
flags = filters.get("flags", [])

if flags and not set(flags) & set(commit_report.flags):
return UnknownFlags()
Expand Down
Loading
Loading