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

Conversation

rohitvinnakota-codecov
Copy link
Contributor

@rohitvinnakota-codecov rohitvinnakota-codecov commented Dec 9, 2024

Closes https://github.com/orgs/codecov/projects/31/views/11?pane=issue&itemId=88922866&issue=codecov%7Cengineering-team%7C3030

We choose segments with indirectly changed lines even if a user queries for intended changes only if they have diff changes. With this PR, we will exclude those lines from each segment.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.02%. Comparing base (dde7496) to head (9d03cb1).

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1037   +/-   ##
=======================================
  Coverage   96.02%   96.02%           
=======================================
  Files         828      828           
  Lines       19434    19450   +16     
=======================================
+ Hits        18661    18677   +16     
  Misses        773      773           
Flag Coverage Δ
unit 92.30% <100.00%> (+<0.01%) ⬆️
unit-latest-uploader 92.30% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-notifications
Copy link

codecov-notifications bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@codecov-qa
Copy link

codecov-qa bot commented Dec 9, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2666 1 2665 6
View the top 1 failed tests by shortest run time
services/tests/test_comparison.py::ComparisonReportTest::test_remove_unintended_changes
Stack Traces | 0.049s run time
self = <services.tests.test_comparison.ComparisonReportTest testMethod=test_remove_unintended_changes>

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

services/tests/test_comparison.py:1798: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
services/comparison.py:487: in remove_unintended_changes
    base_cov = line.coverage["base"]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <services.comparison.LineComparison object at 0x7fac78badc40>

    @property
    def coverage(self):
        return {
            "base": None
            if self.added or not self.base_line
>           else line_type(self.base_line[0]),
            "head": None
            if self.removed or not self.head_line
            else line_type(self.head_line[0]),
        }
E       TypeError: 'int' object is not subscriptable

services/comparison.py:337: TypeError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Copy link

codecov-public-qa bot commented Dec 9, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
2666 1 2665 6
View the top 1 failed tests by shortest run time
services/tests/test_comparison.py::ComparisonReportTest::test_remove_unintended_changes
Stack Traces | 0.049s run time
self = <services.tests.test_comparison.ComparisonReportTest testMethod=test_remove_unintended_changes>

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

services/tests/test_comparison.py:1798: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
services/comparison.py:487: in remove_unintended_changes
    base_cov = line.coverage["base"]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <services.comparison.LineComparison object at 0x7fac78badc40>

    @property
    def coverage(self):
        return {
            "base": None
            if self.added or not self.base_line
>           else line_type(self.base_line[0]),
            "head": None
            if self.removed or not self.head_line
            else line_type(self.head_line[0]),
        }
E       TypeError: 'int' object is not subscriptable

services/comparison.py:337: TypeError

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

Copy link
Contributor

github-actions bot commented Dec 9, 2024

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

@rohitvinnakota-codecov rohitvinnakota-codecov marked this pull request as ready for review December 18, 2024 21:29
@rohitvinnakota-codecov rohitvinnakota-codecov requested a review from a team as a code owner December 18, 2024 21:29
@rohitvinnakota-codecov rohitvinnakota-codecov changed the title [fix] Only count directly changed lines as misses in files changed [fix] Only count directly changed lines as misses when intended changes is specified Dec 18, 2024
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

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good. Just want to confirm that these indirect changes will still show in the indirect changes tab?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

Copy link
Contributor

Choose a reason for hiding this comment

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

Solid lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants