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

Add message when upload count differs #500

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

giovanni-guidini
Copy link
Contributor

These changes include 2 related things:

  • refactor: create MessagesToUserSectionWriter

Moving the "install github app" message to a dedicated section writer.
I'm doing this because I want to use this new section writer to add the
"number of uploads in your commit differs from base" message afterwards

  • feat: add warning if number of uploads differ

Introduces a message when the number of uploads differ and there is a significant coverage drop.
The idea is that we can inform users that possibly some upload is missing, and that is the cause of the coverage drop.

I had to re-work part of the logic that counts the upload diff because the sessions in a report is a dict object, not a list. Thankfully we have many tests :E

Moving the "install github app" message to a dedicated section writer.
I'm doing this because I want to use this new section writer to add the
"number of uploads in your commit differs from base" message afterwards
@giovanni-guidini giovanni-guidini requested a review from a team June 12, 2024 13:32
@codecov-notifications
Copy link

codecov-notifications bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
+ Coverage   97.26%   97.46%   +0.19%     
==========================================
  Files         414      417       +3     
  Lines       34479    34796     +317     
==========================================
+ Hits        33536    33913     +377     
+ Misses        943      883      -60     
Flag Coverage Δ
integration 97.46% <100.00%> (+0.19%) ⬆️
latest-uploader-overall 97.46% <100.00%> (+0.19%) ⬆️
unit 97.46% <100.00%> (+0.19%) ⬆️

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

Components Coverage Δ
NonTestCode 94.47% <100.00%> (+0.05%) ⬆️
OutsideTasks 97.73% <100.00%> (+0.18%) ⬆️
Files Coverage Δ
services/comparison/__init__.py 97.24% <100.00%> (-0.02%) ⬇️
...son/tests/unit/test_reports_uploaded_count_diff.py 100.00% <ø> (ø)
.../notification/notifiers/mixins/message/__init__.py 100.00% <100.00%> (ø)
...s/notification/notifiers/mixins/message/helpers.py 93.15% <100.00%> (+1.15%) ⬆️
.../notification/notifiers/mixins/message/sections.py 97.34% <100.00%> (+0.31%) ⬆️
...ion/notifiers/mixins/message/tests/test_helpers.py 100.00% <100.00%> (ø)
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (+0.77%) ⬆️
tasks/tests/integration/test_notify_task.py 100.00% <ø> (ø)

... and 17 files with indirect coverage changes

Copy link

codecov bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.50%. Comparing base (099911a) to head (5f90fa3).
Report is 3 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
+ Coverage   97.29%   97.50%   +0.21%     
==========================================
  Files         445      448       +3     
  Lines       35208    36128     +920     
==========================================
+ Hits        34255    35228     +973     
+ Misses        953      900      -53     
Flag Coverage Δ
integration 97.46% <100.00%> (+0.19%) ⬆️
latest-uploader-overall 97.46% <100.00%> (+0.19%) ⬆️
unit 97.46% <100.00%> (+0.19%) ⬆️

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

Components Coverage Δ
NonTestCode 94.64% <100.00%> (+0.16%) ⬆️
OutsideTasks 97.73% <100.00%> (+0.18%) ⬆️
Files Coverage Δ
services/comparison/__init__.py 97.26% <100.00%> (-0.02%) ⬇️
...son/tests/unit/test_reports_uploaded_count_diff.py 100.00% <ø> (ø)
.../notification/notifiers/mixins/message/__init__.py 100.00% <100.00%> (ø)
...s/notification/notifiers/mixins/message/helpers.py 93.15% <100.00%> (+1.15%) ⬆️
.../notification/notifiers/mixins/message/sections.py 96.55% <100.00%> (+0.39%) ⬆️
...ion/notifiers/mixins/message/tests/test_helpers.py 100.00% <100.00%> (ø)
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (+0.72%) ⬆️
tasks/tests/integration/test_notify_task.py 100.00% <ø> (ø)

... and 17 files with indirect coverage changes

Related Entrypoints
run/app.tasks.notify.Notify
run/app.tasks.compute_comparison.ComputeComparison

@codecov-qa
Copy link

codecov-qa bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.46%. Comparing base (099911a) to head (5f90fa3).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
+ Coverage   97.26%   97.46%   +0.19%     
==========================================
  Files         414      417       +3     
  Lines       34479    34796     +317     
==========================================
+ Hits        33536    33913     +377     
+ Misses        943      883      -60     
Flag Coverage Δ
integration 97.46% <100.00%> (+0.19%) ⬆️
latest-uploader-overall 97.46% <100.00%> (+0.19%) ⬆️
unit 97.46% <100.00%> (+0.19%) ⬆️

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

Components Coverage Δ
NonTestCode 94.47% <100.00%> (+0.05%) ⬆️
OutsideTasks 97.73% <100.00%> (+0.18%) ⬆️
Files Coverage Δ
services/comparison/__init__.py 97.24% <100.00%> (-0.02%) ⬇️
...son/tests/unit/test_reports_uploaded_count_diff.py 100.00% <ø> (ø)
.../notification/notifiers/mixins/message/__init__.py 100.00% <100.00%> (ø)
...s/notification/notifiers/mixins/message/helpers.py 93.15% <100.00%> (+1.15%) ⬆️
.../notification/notifiers/mixins/message/sections.py 97.34% <100.00%> (+0.31%) ⬆️
...ion/notifiers/mixins/message/tests/test_helpers.py 100.00% <100.00%> (ø)
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (+0.77%) ⬆️
tasks/tests/integration/test_notify_task.py 100.00% <ø> (ø)

... and 17 files with indirect coverage changes

Copy link

codecov-public-qa bot commented Jun 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.46%. Comparing base (099911a) to head (5f90fa3).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found ☺️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
+ Coverage   97.26%   97.46%   +0.19%     
==========================================
  Files         414      417       +3     
  Lines       34479    34796     +317     
==========================================
+ Hits        33536    33913     +377     
+ Misses        943      883      -60     
Flag Coverage Δ
integration 97.46% <100.00%> (+0.19%) ⬆️
latest-uploader-overall 97.46% <100.00%> (+0.19%) ⬆️
unit 97.46% <100.00%> (+0.19%) ⬆️

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

Components Coverage Δ
NonTestCode 94.47% <100.00%> (+0.05%) ⬆️
OutsideTasks 97.73% <100.00%> (+0.18%) ⬆️
Files Coverage Δ
services/comparison/__init__.py 97.24% <100.00%> (-0.02%) ⬇️
...son/tests/unit/test_reports_uploaded_count_diff.py 100.00% <ø> (ø)
.../notification/notifiers/mixins/message/__init__.py 100.00% <100.00%> (ø)
...s/notification/notifiers/mixins/message/helpers.py 93.15% <100.00%> (+1.15%) ⬆️
.../notification/notifiers/mixins/message/sections.py 97.34% <100.00%> (+0.31%) ⬆️
...ion/notifiers/mixins/message/tests/test_helpers.py 100.00% <100.00%> (ø)
.../notification/notifiers/tests/unit/test_comment.py 100.00% <100.00%> (+0.77%) ⬆️
tasks/tests/integration/test_notify_task.py 100.00% <ø> (ø)

... and 17 files with indirect coverage changes

Introduces a message when the number of uploads differ and there is a significant coverage drop.
The idea is that we can inform users that possibly some upload is missing, and that is the cause of the coverage drop.

I had to re-work part of the logic that counts the upload diff because the sessions in a report is a `dict` object, not a `list`. Thankfully we have many tests :E
@giovanni-guidini giovanni-guidini force-pushed the gio/upload-count-differs-message branch from 0fea51c to 2ee8a66 Compare June 14, 2024 13:02
Copy link
Contributor

@michelletran-codecov michelletran-codecov left a comment

Choose a reason for hiding this comment

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

Nice refactor! Just a small suggestion and question.

services/comparison/__init__.py Outdated Show resolved Hide resolved
Comment on lines +24 to +25
if "enabled" in project_status_details:
return project_status_details["enabled"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the YAML documentation for status checks, it doesn't look like enabled is a valid field for project status details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take the code as the source of truth

Maybe it's a deprecated config, but there's no mention of it being deprecated

Co-authored-by: michelletran-codecov <[email protected]>
@giovanni-guidini giovanni-guidini added this pull request to the merge queue Jun 18, 2024
Merged via the queue into main with commit 4382f51 Jun 18, 2024
29 checks passed
@giovanni-guidini giovanni-guidini deleted the gio/upload-count-differs-message branch June 18, 2024 07:52
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