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 the Report._files/_chunks #553

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Merge the Report._files/_chunks #553

wants to merge 4 commits into from

Conversation

Swatinem
Copy link
Contributor

@Swatinem Swatinem commented Mar 4, 2025

Instead of maintaining two separate fields, this merges the previous _files which was a dict from filename to totals, and the _chunks which was a list of split chunks, themselves optionally a fully parsed ReportFile.

Maintaining only a single structure for the list of files makes a ton of internals of the Report simpler.
It also solves the problem of file_totals getting out of sync with the ReportFile.totals.

Instead of maintaining a FileSummary dataclass, and a separate ReportFile, this merges these two structures together, and makes sure that a Report always has ReportFiles.

Though the new ReportFile has a bit more logic around making parsing of its actual chunk contents on-demand only when needed, and avoid having to parse things when not strictly necessary.


I think the ~40% perf regression is reasonable, as it is doing more work now for a "shallow" parse. The slowness comes from the ReportFile constructor being slow. Part of that is because it has to support various arguments and argument types which are primarily only used within tests. In real usage, we either parse ReportFiles from json/chunks, or start with an empty file that has lines added/replaced into it.

@Swatinem Swatinem self-assigned this Mar 4, 2025
@Swatinem Swatinem force-pushed the swatinem/one-file branch 2 times, most recently from 4974cef to f6d9418 Compare March 5, 2025 10:21
Base automatically changed from swatinem/mv-reportfile to main March 5, 2025 13:51
@Swatinem Swatinem force-pushed the swatinem/one-file branch from f6d9418 to 054211b Compare March 10, 2025 08:25
Copy link

overwatch-beta bot commented Mar 10, 2025

✅ Sentry found no issues in your recent changes ✅

@Swatinem Swatinem force-pushed the swatinem/one-file branch from 054211b to 7bac317 Compare March 10, 2025 09:18
Copy link

codspeed-hq bot commented Mar 10, 2025

CodSpeed Performance Report

Merging #553 will degrade performances by 41.83%

Comparing swatinem/one-file (b1c9a14) with main (e30c1c7)

Summary

⚡ 1 improvements
❌ 2 regressions
✅ 6 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
test_parse_shallow 7.2 ms 12.3 ms -41.83%
test_report_carryforward 6.1 ms 2.9 ms ×2.1
test_report_serialize 6 ms 7.1 ms -15.04%

@Swatinem Swatinem force-pushed the swatinem/one-file branch 4 times, most recently from f3d14c7 to 83f9302 Compare March 11, 2025 11:35
Copy link

codecov bot commented Mar 11, 2025

Codecov Report

Attention: Patch coverage is 96.15385% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.64%. Comparing base (e30c1c7) to head (b1c9a14).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
shared/reports/resources.py 93.33% 1 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
- Coverage   88.69%   88.64%   -0.06%     
==========================================
  Files         462      462              
  Lines       13159    13092      -67     
  Branches     1507     1492      -15     
==========================================
- Hits        11672    11605      -67     
- Misses       1173     1176       +3     
+ Partials      314      311       -3     
Flag Coverage Δ
shared-docker-uploader 88.64% <96.15%> (-0.06%) ⬇️

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 Mar 11, 2025

Codecov Report

Attention: Patch coverage is 96.12403% with 5 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
shared/reports/resources.py 93.42% 1 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Swatinem Swatinem force-pushed the swatinem/one-file branch from 83f9302 to 10dd46e Compare March 11, 2025 12:15
@Swatinem Swatinem marked this pull request as ready for review March 11, 2025 15:29
@Swatinem Swatinem requested a review from a team March 11, 2025 15:30
Copy link
Contributor

@giovanni-guidini giovanni-guidini left a comment

Choose a reason for hiding this comment

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

Very good change!

Instead of maintaining two separate fields, this merges the previous `_files` which was a dict from filename to `totals`, and the `_chunks` which was either a fully parsed `ReportFile`, or just a shallow `list` of lines.

Maintaining only a single structure for the list of files makes a ton of internals of the `Report` simpler.

It also solves the problem of `file_totals` getting out of sync with the `ReportFile.totals`.
@Swatinem Swatinem force-pushed the swatinem/one-file branch from 8136c05 to b1c9a14 Compare March 14, 2025 11:30
Copy link

sentry-autofix bot commented Mar 14, 2025

✅ Sentry found no issues in your recent changes ✅

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