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

Merge EditableReportFile into ReportFile #548

Merged
merged 1 commit into from
Feb 28, 2025
Merged

Conversation

Swatinem
Copy link
Contributor

The main purpose of the EditableReportFile was to offer methods to delete labels/sessions, as well as to properly maintain uptodate versions of present_sessions and totals when doing mutations on the file.

All the methods were now moved to ReportFile, which has also gained new functionality related to making sure that caches are cleared when necessary, and re-computed on-demand, so that they are always correct.


I plan to do a similar thing for EditableReport as well in a followup, as it has a similar purpose in relation to maintaining uptodate totals.

This is part of codecov/engineering-team#3381

@Swatinem Swatinem requested a review from a team February 27, 2025 13:43
@Swatinem Swatinem self-assigned this Feb 27, 2025
Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.50%. Comparing base (b49f475) to head (4f96ce2).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #548      +/-   ##
==========================================
- Coverage   88.51%   88.50%   -0.02%     
==========================================
  Files         451      451              
  Lines       13127    13107      -20     
  Branches     1529     1528       -1     
==========================================
- Hits        11620    11600      -20     
  Misses       1184     1184              
  Partials      323      323              
Flag Coverage Δ
shared-docker-uploader 88.50% <100.00%> (-0.02%) ⬇️

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 Feb 27, 2025

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 Feb 27, 2025

❌ 16 Tests Failed:

Tests completed Failed Passed Skipped
1645 16 1629 1
View the top 3 failed test(s) by shortest run time
tests/unit/reports/test_editable.py::TestEditableReportHelpers::test_delete_labels_session_without_datapoints
Stack Traces | 0.002s run time
self = <test_editable.TestEditableReportHelpers object at 0x7fceaa30a0c0>

    def test_delete_labels_session_without_datapoints(self):
        line = ReportLine.create(
            1,
            None,
            [LineSession(0, 1), LineSession(1, 1), LineSession(2, 0)],
            datapoints=[
                CoverageDatapoint(1, 1, None, [5, 6]),
                CoverageDatapoint(1, 0, None, [7, 6]),
                CoverageDatapoint(2, 0, None, [10]),
            ],
        )
>       assert EditableReportFile.line_without_labels(
            line, [1], [5, 6, 10]
        ) == ReportLine.create(
            1,
            None,
            [LineSession(0, 1), LineSession(2, 0)],
            datapoints=[CoverageDatapoint(2, 0, None, [10])],
        )

.../unit/reports/test_editable.py:219: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'shared.reports.resources.ReportFile'>
line = ReportLine(coverage=1, type=None, sessions=[LineSession(id=0, coverage=1, branches=None, partials=None, complexity=Non...coverage_type=None, label_ids=[7, 6]), CoverageDatapoint(sessionid=2, coverage=0, coverage_type=None, label_ids=[10])])
session_ids_to_delete = [1], label_ids_to_delete = [5, 6, 10]

    @classmethod
    def line_without_labels(
        cls, line, session_ids_to_delete: set[int], label_ids_to_delete: set[int]
    ):
        new_datapoints = (
            [
                dp
                for dp in line.datapoints
                if dp.sessionid not in session_ids_to_delete
                or all(lb not in label_ids_to_delete for lb in dp.label_ids)
            ]
            if line.datapoints is not None
            else None
        )
        remaining_session_ids = set(dp.sessionid for dp in new_datapoints)
>       removed_session_ids = session_ids_to_delete - remaining_session_ids
E       TypeError: unsupported operand type(s) for -: 'list' and 'set'

shared/reports/resources.py:434: TypeError
tests/unit/reports/test_editable.py::TestEditableReportHelpers::test_line_without_labels
Stack Traces | 0.002s run time
self = <test_editable.TestEditableReportHelpers object at 0x7fceaa30a4b0>

    def test_line_without_labels(self):
        line = ReportLine.create(
            "2/2",
            None,
            [LineSession(1, 0), LineSession(0, 1)],
            datapoints=[
                CoverageDatapoint(1, 0, None, [5, 6]),
                CoverageDatapoint(1, 0, None, [7, 6]),
                CoverageDatapoint(0, 1, None, [5, 6]),
                CoverageDatapoint(0, "1/2", None, [5, 8]),
                CoverageDatapoint(0, "1/2", None, [9, 10]),
                CoverageDatapoint(0, 0, None, [10, 8]),
            ],
        )
>       assert EditableReportFile.line_without_labels(
            line, [1], [5]
        ) == ReportLine.create(
            "2/2",
            None,
            [LineSession(1, 0), LineSession(0, 1)],
            datapoints=[
                CoverageDatapoint(1, 0, None, [7, 6]),
                CoverageDatapoint(0, 1, None, [5, 6]),
                CoverageDatapoint(0, "1/2", None, [5, 8]),
                CoverageDatapoint(0, "1/2", None, [9, 10]),
                CoverageDatapoint(0, 0, None, [10, 8]),
            ],
        )

.../unit/reports/test_editable.py:145: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'shared.reports.resources.ReportFile'>
line = ReportLine(coverage='2/2', type=None, sessions=[LineSession(id=1, coverage=0, branches=None, partials=None, complexity...rage_type=None, label_ids=[9, 10]), CoverageDatapoint(sessionid=0, coverage=0, coverage_type=None, label_ids=[10, 8])])
session_ids_to_delete = [1], label_ids_to_delete = [5]

    @classmethod
    def line_without_labels(
        cls, line, session_ids_to_delete: set[int], label_ids_to_delete: set[int]
    ):
        new_datapoints = (
            [
                dp
                for dp in line.datapoints
                if dp.sessionid not in session_ids_to_delete
                or all(lb not in label_ids_to_delete for lb in dp.label_ids)
            ]
            if line.datapoints is not None
            else None
        )
        remaining_session_ids = set(dp.sessionid for dp in new_datapoints)
>       removed_session_ids = session_ids_to_delete - remaining_session_ids
E       TypeError: unsupported operand type(s) for -: 'list' and 'set'

shared/reports/resources.py:434: TypeError
tests/unit/test_report.py::test_encode_chunk[chunk1-{}\n]
Stack Traces | 0.002s run time
chunk = <ReportFile name=name.ply lines=0>, res = '{}\n'

    @pytest.mark.unit
    @pytest.mark.parametrize(
        "chunk, res",
        [
            (None, "null"),
            (ReportFile(name="name.ply"), "{}\n"),
            (
                [ReportLine.create(2), ReportLine.create(1)],
                "[[2,null,null,null,null,null],[1,null,null,null,null,null]]",
            ),
        ],
    )
    def test_encode_chunk(chunk, res):
>       assert _encode_chunk(chunk) == res
E       assert '{"present_sessions":[]}\n' == '{}\n'
E         
E         - {}
E         + {"present_sessions":[]}

tests/unit/test_report.py:938: AssertionError

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Copy link

❌ 16 Tests Failed:

Tests completed Failed Passed Skipped
1645 16 1629 1
View the top 3 failed tests by shortest run time
tests/integration/test_report_file.py::::test_encode[r1-null\n[1]\n[2]]
Stack Traces | 0.002s run time
r = <ReportFile name=name.py lines=2>, encoded_val = 'null\n[1]\n[2]'

    @pytest.mark.integration
    @pytest.mark.parametrize(
        "r, encoded_val",
        [
            (ReportFile("name.py"), "{}\n"),
            (
                ReportFile("name.py", lines=[ReportLine.create(1), ReportLine.create(2)]),
                "null\n[1]\n[2]",
            ),
        ],
    )
    def test_encode(r, encoded_val):
>       assert r._encode() == encoded_val

tests/integration/test_report_file.py:370: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
shared/reports/resources.py:145: in _encode
    details = orjson.dumps(self.details, option=orjson_option)
shared/reports/resources.py:132: in details
    self._details["present_sessions"] = sorted(self._present_sessions)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <ReportFile name=name.py lines=2>

    @property
    def _present_sessions(self):
        if self.__present_sessions is None:
            self.__present_sessions = set()
            for _, line in self.lines:
>               self.__present_sessions.update(int(s.id) for s in line.sessions)
E               TypeError: 'NoneType' object is not iterable

shared/reports/resources.py:127: TypeError
tests/unit/reports/test_editable.py::TestEditableReportHelpers::test_line_without_labels
Stack Traces | 0.002s run time
self = <test_editable.TestEditableReportHelpers object at 0x7fceaa30a4b0>

    def test_line_without_labels(self):
        line = ReportLine.create(
            "2/2",
            None,
            [LineSession(1, 0), LineSession(0, 1)],
            datapoints=[
                CoverageDatapoint(1, 0, None, [5, 6]),
                CoverageDatapoint(1, 0, None, [7, 6]),
                CoverageDatapoint(0, 1, None, [5, 6]),
                CoverageDatapoint(0, "1/2", None, [5, 8]),
                CoverageDatapoint(0, "1/2", None, [9, 10]),
                CoverageDatapoint(0, 0, None, [10, 8]),
            ],
        )
>       assert EditableReportFile.line_without_labels(
            line, [1], [5]
        ) == ReportLine.create(
            "2/2",
            None,
            [LineSession(1, 0), LineSession(0, 1)],
            datapoints=[
                CoverageDatapoint(1, 0, None, [7, 6]),
                CoverageDatapoint(0, 1, None, [5, 6]),
                CoverageDatapoint(0, "1/2", None, [5, 8]),
                CoverageDatapoint(0, "1/2", None, [9, 10]),
                CoverageDatapoint(0, 0, None, [10, 8]),
            ],
        )

.../unit/reports/test_editable.py:145: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

cls = <class 'shared.reports.resources.ReportFile'>
line = ReportLine(coverage='2/2', type=None, sessions=[LineSession(id=1, coverage=0, branches=None, partials=None, complexity...rage_type=None, label_ids=[9, 10]), CoverageDatapoint(sessionid=0, coverage=0, coverage_type=None, label_ids=[10, 8])])
session_ids_to_delete = [1], label_ids_to_delete = [5]

    @classmethod
    def line_without_labels(
        cls, line, session_ids_to_delete: set[int], label_ids_to_delete: set[int]
    ):
        new_datapoints = (
            [
                dp
                for dp in line.datapoints
                if dp.sessionid not in session_ids_to_delete
                or all(lb not in label_ids_to_delete for lb in dp.label_ids)
            ]
            if line.datapoints is not None
            else None
        )
        remaining_session_ids = set(dp.sessionid for dp in new_datapoints)
>       removed_session_ids = session_ids_to_delete - remaining_session_ids
E       TypeError: unsupported operand type(s) for -: 'list' and 'set'

shared/reports/resources.py:434: TypeError
tests/unit/test_report.py::::test_encode_chunk[chunk1-{}\n]
Stack Traces | 0.002s run time
chunk = <ReportFile name=name.ply lines=0>, res = '{}\n'

    @pytest.mark.unit
    @pytest.mark.parametrize(
        "chunk, res",
        [
            (None, "null"),
            (ReportFile(name="name.ply"), "{}\n"),
            (
                [ReportLine.create(2), ReportLine.create(1)],
                "[[2,null,null,null,null,null],[1,null,null,null,null,null]]",
            ),
        ],
    )
    def test_encode_chunk(chunk, res):
>       assert _encode_chunk(chunk) == res
E       assert '{"present_sessions":[]}\n' == '{}\n'
E         
E         - {}
E         + {"present_sessions":[]}

tests/unit/test_report.py:938: AssertionError

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

Copy link

codspeed-hq bot commented Feb 27, 2025

CodSpeed Performance Report

Merging #548 will create unknown performance changes

Comparing swatinem/no-editablefile (4f96ce2) with main (b49f475)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.\

@@ -424,6 +454,38 @@ def line_without_labels(
sessions=new_sessions,
)

def delete_labels(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth documenting somehow that these functionalities used to belong to an EditableReport and that should be used only in particular contexts? I guess we're not making that distinction any more since we're merging Report related classes, so I guess it's fair game to use any property/methods since they're moved to the ReportFile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be fair game to use any method now.
In reality, all the mutating methods were already part of the previous interface anyway.

The main purpose of the `EditableReportFile` was to offer methods to delete labels/sessions, as well as to properly maintain uptodate versions of `present_sessions` and `totals` when doing mutations on the file.

All the methods were now moved to `ReportFile`, which has also gained new functionality related to making sure that caches are cleared when necessary, and re-computed on-demand, so that they are always correct.

---

I plan to do a similar thing for `EditableReport` as well in a followup, as it has a similar purpose in relation to maintaining uptodate `totals`.
@Swatinem Swatinem force-pushed the swatinem/no-editablefile branch from 676f04d to 4f96ce2 Compare February 28, 2025 09:39
@Swatinem Swatinem added this pull request to the merge queue Feb 28, 2025
Merged via the queue into main with commit bc3e338 Feb 28, 2025
12 checks passed
@Swatinem Swatinem deleted the swatinem/no-editablefile branch February 28, 2025 10:40
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