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

feat: make comparison session agnostic by only comparing the totals #410

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

daniel-codecov
Copy link
Contributor

@daniel-codecov daniel-codecov commented Apr 24, 2024

For the parallel upload processing verification experiment, I am changing how the outputs of the parallel vs serial flow are compared.

Instead of comparing exactly the report json and chunks.txt of our reports, I compare the top level totals of coverage, and the file level totals for each file instead. This way, even if the way we assign session ids to uploads in the parallel flow differs from the serial flow (which would've caused a diff in the old comparison method), the coverage totals should hopefully still be the same indicating that the processing was successful.

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@@ -486,7 +486,17 @@ def merge_report(cumulative_report, obj):
incremental_report = obj["report"]
parallel_idx = obj["parallel_idx"]

assert len(incremental_report.sessions) == 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

from the logs, i noticed sometimes an incremental report has 0 sessions, which is weird

@codecov-notifications
Copy link

codecov-notifications bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes are missing coverage. Please review.

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
- Coverage   97.52%   97.48%   -0.05%     
==========================================
  Files         395      395              
  Lines       33355    33369      +14     
==========================================
  Hits        32530    32530              
- Misses        825      839      +14     
Flag Coverage Δ
integration 97.48% <0.00%> (-0.05%) ⬇️
latest-uploader-overall 97.48% <0.00%> (-0.05%) ⬇️
unit 97.48% <0.00%> (-0.05%) ⬇️

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

Components Coverage Δ
NonTestCode 94.84% <0.00%> (-0.10%) ⬇️
OutsideTasks 97.59% <ø> (ø)
Files Coverage Δ
tasks/upload_finisher.py 70.74% <0.00%> (-0.38%) ⬇️
tasks/parallel_verification.py 22.72% <0.00%> (-5.58%) ⬇️

Copy link

codecov-public-qa bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 97.48%. Comparing base (303521d) to head (7032820).

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
- Coverage   97.52%   97.48%   -0.05%     
==========================================
  Files         395      395              
  Lines       33355    33369      +14     
==========================================
  Hits        32530    32530              
- Misses        825      839      +14     
Flag Coverage Δ
integration 97.48% <0.00%> (-0.05%) ⬇️
latest-uploader-overall 97.48% <0.00%> (-0.05%) ⬇️
unit 97.48% <0.00%> (-0.05%) ⬇️

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

Components Coverage Δ
NonTestCode 94.84% <0.00%> (-0.10%) ⬇️
OutsideTasks 97.59% <ø> (ø)
Files Coverage Δ
tasks/upload_finisher.py 70.74% <0.00%> (-0.38%) ⬇️
tasks/parallel_verification.py 22.72% <0.00%> (-5.58%) ⬇️

@codecov-qa
Copy link

codecov-qa bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 97.48%. Comparing base (303521d) to head (7032820).

✅ All tests successful. No failed tests found ☺️

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
- Coverage   97.52%   97.48%   -0.05%     
==========================================
  Files         395      395              
  Lines       33355    33369      +14     
==========================================
  Hits        32530    32530              
- Misses        825      839      +14     
Flag Coverage Δ
integration 97.48% <0.00%> (-0.05%) ⬇️
latest-uploader-overall 97.48% <0.00%> (-0.05%) ⬇️
unit 97.48% <0.00%> (-0.05%) ⬇️

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

Components Coverage Δ
NonTestCode 94.84% <0.00%> (-0.10%) ⬇️
OutsideTasks 97.59% <ø> (ø)
Files Coverage Δ
tasks/upload_finisher.py 70.74% <0.00%> (-0.38%) ⬇️
tasks/parallel_verification.py 22.72% <0.00%> (-5.58%) ⬇️

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

Attention: Patch coverage is 0% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 97.51%. Comparing base (303521d) to head (7032820).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #410      +/-   ##
==========================================
- Coverage   97.55%   97.51%   -0.05%     
==========================================
  Files         426      426              
  Lines       34046    34060      +14     
==========================================
  Hits        33212    33212              
- Misses        834      848      +14     
Flag Coverage Δ
integration 97.48% <0.00%> (-0.05%) ⬇️
latest-uploader-overall 97.48% <0.00%> (-0.05%) ⬇️
unit 97.48% <0.00%> (-0.05%) ⬇️

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

Components Coverage Δ
NonTestCode 94.86% <0.00%> (-0.10%) ⬇️
OutsideTasks 97.59% <ø> (ø)
Files Coverage Δ
tasks/upload_finisher.py 70.89% <0.00%> (-0.38%) ⬇️
tasks/parallel_verification.py 22.72% <0.00%> (-5.58%) ⬇️
Related Entrypoints
run/app.tasks.upload.UploadFinisher

joseph-sentry
joseph-sentry previously approved these changes Apr 24, 2024
Copy link
Contributor

@joseph-sentry joseph-sentry left a comment

Choose a reason for hiding this comment

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

looks good just left one comment to confirm an assumption

# file level totals comparison
for filename, file_summary in parallel_report._files.items():
parallel_file_level_totals = file_summary.file_totals
serial_file_level_totals = serial_report._files[filename].file_totals
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we're assuming that parallel report and serial report should contain the same files but in the case that parallel report contains a file that serial report does not this will crash, just in case that's a possibility we should handle this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I was originally relying on the check here: https://github.com/codecov/worker/pull/410/files#diff-49dba3d15152574626bb8cc6bb4d690c86c34d1d5d9be202e012f70f27be9b5dR110

but that wouldn't run if this throws an error. Thanks

@daniel-codecov daniel-codecov force-pushed the daniel/use-totals-for-parallel-verification branch from cc40771 to 7032820 Compare April 24, 2024 17:20
@daniel-codecov daniel-codecov added this pull request to the merge queue Apr 24, 2024
Merged via the queue into main with commit a89553d Apr 24, 2024
18 of 30 checks passed
@daniel-codecov daniel-codecov deleted the daniel/use-totals-for-parallel-verification branch April 24, 2024 17:36
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.

None yet

2 participants