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.

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

[fix] Only count directly changed lines as misses when intended changes is specified #1037

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
4 changes: 3 additions & 1 deletion graphql_api/tests/test_impacted_file.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import hashlib
from dataclasses import dataclass
from dataclasses import dataclass, field
from typing import Callable
from unittest.mock import PropertyMock, patch

from django.test import TransactionTestCase
Expand Down Expand Up @@ -211,6 +212,7 @@
class MockSegment:
has_diff_changes: bool = False
has_unintended_changes: bool = False
remove_unintended_changes: Callable[[], None] = field(default=lambda: None)


class MockFileComparison(object):
Expand Down
8 changes: 6 additions & 2 deletions graphql_api/types/impacted_file/impacted_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,12 @@ def resolve_segments(
# segments with no diff changes and at least 1 unintended change
segments = [segment for segment in segments if segment.has_unintended_changes]
elif filters.get("has_unintended_changes") is False:
# segments with at least 1 diff change
segments = [segment for segment in segments if segment.has_diff_changes]
new_segments = []
for segment in segments:
if segment.has_diff_changes:
segment.remove_unintended_changes()
new_segments.append(segment)
segments = new_segments

return SegmentComparisons(results=segments)

Expand Down
9 changes: 9 additions & 0 deletions services/comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,15 @@ def has_unintended_changes(self):
return True
return False

def remove_unintended_changes(self):
filtered = []
for line in self._lines:
base_cov = line.coverage["base"]
head_cov = line.coverage["head"]
if (line.added or line.removed) or (base_cov == head_cov):
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic says if line is in the diff or coverage on the line has not changed, then it's unintended (coverage change on non-diff line) and we should filter it out. Correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct yep

filtered.append(line)
self._lines = filtered


class FileComparison:
def __init__(
Expand Down
16 changes: 16 additions & 0 deletions services/tests/test_comparison.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
LineComparison,
MissingComparisonReport,
PullRequestComparison,
Segment,
)

# Pulled from shared.django_apps.core.tests.factories.CommitFactory files.
Expand Down Expand Up @@ -1784,6 +1785,21 @@ def test_file_has_changes(self):
)
assert file.has_changes is True

def test_remove_unintended_changes(self):
lines = [
LineComparison([1, "", [], 0, 0], [1, "", [], 0, 0], 1, 1, "line1", False),
LineComparison([1, "", [], 0, 0], [0, "", [], 0, 0], 2, 2, "line2", False),
LineComparison(None, [0, "", [], 0, 0], None, 3, "+line3", True),
LineComparison([0, "", [], 0, 0], None, 4, None, "-line4", True),
LineComparison([1, "", [], 0, 0], [0, "", [], 0, 0], 5, 5, "line5", False),
]

segment = Segment(lines)
segment.remove_unintended_changes()

assert len(segment.lines) == 3
assert [line.value for line in segment.lines] == ["line1", "+line3", "-line4"]


class CommitComparisonTests(TestCase):
def setUp(self):
Expand Down
Loading