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.

Sign up for GitHub

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

Jump to bottom

feat: Add processing upload comment #140

Merged
merged 2 commits into from
Oct 25, 2023
Merged

feat: Add processing upload comment #140

merged 2 commits into from
Oct 25, 2023

Conversation

scott-codecov
Copy link
Contributor

@scott-codecov scott-codecov commented Oct 5, 2023 •
edited
Loading

codecov/engineering-team#553

We'd like to be able to make an early PR comment (as soon as we receive an upload but before we've processed it) indicating that we've received the upload but it is still processing. I'm reusing some of the "empty upload" functionality for this since it's similar in nature. The API directly enqueues a notify task for empty uploads and I'm thinking we do the same for this - the API will enqueue both a notify task with empty_upload="processing" as well as a normal upload task (which later triggers another notify w/ report results).

Here's the corresponding PR for codecov-api that enqueues this early notify task: codecov/codecov-api#179

scott-codecov
scott-codecov commented Oct 5, 2023
View reviewed changes
services/notification/notifiers/comment/__init__.py

def _create_processing_upload_message(self):
return [
"We're currently processing your upload. This comment will be updated when the results are available.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy TBD here

@codecov
Copy link

codecov bot commented Oct 5, 2023 •
edited
Loading

Codecov Report

Merging #140 (4e28e5b) into main (1d2e5a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #140   +/-   ##
=======================================
  Coverage   98.35%   98.35%           
=======================================
  Files         374      374           
  Lines       27947    27975   +28     
=======================================
+ Hits        27486    27514   +28     
  Misses        461      461
Flag Coverage Δ
integration 98.39% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 98.39% <100.00%> (+<0.01%) ⬆️
unit 98.39% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 96.79% <100.00%> (+<0.01%) ⬆️
OutsideTasks 98.18% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/decoration.py 100.00% <100.00%> (ø)
services/notification/notifiers/base.py 90.90% <100.00%> (+0.43%) ⬆️
...ervices/notification/notifiers/comment/__init__.py 99.45% <100.00%> (+0.01%) ⬆️
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (ø)
services/tests/test_decoration.py 96.22% <100.00%> (+0.08%) ⬆️

This change has been scanned for critical changes. Learn more

@codecov-qa
Copy link

codecov-qa bot commented Oct 5, 2023 •
edited
Loading

Codecov Report

Merging #140 (4e28e5b) into main (1d2e5a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #140   +/-   ##
=======================================
  Coverage   98.38%   98.39%           
=======================================
  Files         348      348           
  Lines       27451    27479   +28     
=======================================
+ Hits        27009    27037   +28     
  Misses        442      442
Flag Coverage Δ
integration 98.39% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall ?
unit 98.39% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 96.89% <100.00%> (+<0.01%) ⬆️
OutsideTasks 98.18% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/decoration.py 100.00% <100.00%> (ø)
services/notification/notifiers/base.py 90.69% <100.00%> (+0.45%) ⬆️
...ervices/notification/notifiers/comment/__init__.py 99.45% <100.00%> (+0.01%) ⬆️
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (ø)
services/tests/test_decoration.py 96.22% <100.00%> (+0.08%) ⬆️

@codecov-public-qa
Copy link

codecov-public-qa bot commented Oct 5, 2023 •
edited
Loading

Codecov Report

Merging #140 (4e28e5b) into main (1d2e5a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #140   +/-   ##
=======================================
  Coverage   98.38%   98.39%           
=======================================
  Files         348      348           
  Lines       27451    27479   +28     
=======================================
+ Hits        27009    27037   +28     
  Misses        442      442
Flag Coverage Δ
integration 98.39% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 98.39% <100.00%> (+<0.01%) ⬆️
unit 98.39% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 96.89% <100.00%> (+<0.01%) ⬆️
OutsideTasks 98.18% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/decoration.py 100.00% <100.00%> (ø)
services/notification/notifiers/base.py 90.69% <100.00%> (+0.45%) ⬆️
...ervices/notification/notifiers/comment/__init__.py 99.45% <100.00%> (+0.01%) ⬆️
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (ø)
services/tests/test_decoration.py 96.22% <100.00%> (+0.08%) ⬆️

@scott-codecov scott-codecov mentioned this pull request Oct 5, 2023
Merged
@scott-codecov
Copy link
Contributor Author

Hmm, I was testing this a little more and I don't think this architecture will really work. Say the early notify task gets stuck behind a slow task and the actual report notify happens first. Then this "processing" notification could happen 2nd and leave the PR comment in a bad state. 🤔

dana-yaish
dana-yaish approved these changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@dana-yaish dana-yaish left a comment

Choose a reason for hiding this comment

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

LGTM. One minor question, are we sure that this comment will get updated when the processing finishes? so we won't get 2 separate comments. It would be worth to check it with different behaviors of the comment, "default" and "new" for example

@dana-yaish
Copy link
Contributor

#140 (comment) maybe we can chain the notify task after the upload task just like we chain all 3 upload tasks together?

@scott-codecov
Copy link
Contributor Author

scott-codecov commented Oct 6, 2023 •
edited
Loading

we can chain the notify task after the upload

Great idea - thank you!

I suppose this could slightly delay the processing (since we wait for the notify to finish) but I think we typically delay a little bit anyway in order to wait for the coverage data to be uploaded.

@scott-codecov
Copy link
Contributor Author

@dana-yaish I implemented the chaining in codecov/codecov-api#179 Seems to be working nicely now. Thanks.

@codecov-staging
Copy link

codecov-staging bot commented Oct 25, 2023 •
edited
Loading

Codecov Report

Merging #140 (4e28e5b) into main (1d2e5a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #140   +/-   ##
=======================================
  Coverage   98.38%   98.39%           
=======================================
  Files         348      348           
  Lines       27451    27479   +28     
=======================================
+ Hits        27009    27037   +28     
  Misses        442      442
Flag Coverage Δ
integration 98.39% <100.00%> (+<0.01%) ⬆️
latest-uploader-overall 98.39% <100.00%> (+<0.01%) ⬆️
unit 98.39% <100.00%> (+<0.01%) ⬆️

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

Components Coverage Δ
NonTestCode 96.89% <100.00%> (+<0.01%) ⬆️
OutsideTasks 98.18% <100.00%> (+<0.01%) ⬆️
Files Coverage Δ
database/enums.py 100.00% <100.00%> (ø)
services/decoration.py 100.00% <100.00%> (ø)
services/notification/notifiers/base.py 90.69% <100.00%> (+0.45%) ⬆️
...ervices/notification/notifiers/comment/__init__.py 99.45% <100.00%> (+0.01%) ⬆️
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (ø)
services/tests/test_decoration.py 96.22% <100.00%> (+0.08%) ⬆️

@scott-codecov scott-codecov merged commit 9bc093f into main Oct 25, 2023
25 of 26 checks passed
@scott-codecov scott-codecov deleted the scott/processing-upload-comment branch October 25, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dana-yaish dana-yaish dana-yaish approved these changes

@giovanni-guidini giovanni-guidini Awaiting requested review from giovanni-guidini

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@scott-codecov @dana-yaish