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

Failed check and status messaging in comment, part II #1023

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

nora-codecov
Copy link
Contributor

when user has:
patch checks notifier OR patch status notifier
AND
comment notifier
AND
has defined a target in their codecov yml
AND
the patch check/status fails
THEN
add some helper text to the comment notifier to point out that the check/status failed and that they can change their target.

codecov/engineering-team#1626

@nora-codecov nora-codecov requested a review from a team January 22, 2025 00:49
@nora-codecov nora-codecov changed the title add helper text to PR comment Failed check and status messaging in comment, part II Jan 22, 2025
@nora-codecov nora-codecov force-pushed the nora/1626-target-text branch from 9dc9f9b to b7348ce Compare January 22, 2025 00:51
Copy link

github-actions bot commented Jan 22, 2025

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

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.

LGTM

codecov.yml Outdated
@@ -7,7 +7,7 @@ component_management:
default_rules:
statuses:
- type: project
target: auto
target: 99
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this for testing? Do we want to leave it like so?

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 wanted to trigger the help text im adding in this pr, but i changed this in a separate commit after the first pr comment already happened, so i think the comment didn't update with my helper text. I'll manipulate it some more...

owner.plan == BillingPlan.team_monthly.value
or owner.plan == BillingPlan.team_yearly.value
):
owner_plan = PlanService(owner)
Copy link
Contributor

Choose a reason for hiding this comment

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

nice 👍

@codecov-qa
Copy link

codecov-qa bot commented Jan 22, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1 1 0 0
View the top 1 failed tests by shortest run time
services/notification/tests/unit/test_notification_service.py::services.notification.tests.unit.test_notification_service
Stack Traces | 0s run time
.../tests/unit/test_notification_service.py:52: in <module>
    class TestNotificationService(object):
.../tests/unit/test_notification_service.py:215: in TestNotificationService
    [GITHUB_APP_INSTALLATION_DEFAULT_NAME, "notifications-app"],
E   NameError: name 'GITHUB_APP_INSTALLATION_DEFAULT_NAME' is not defined

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

Copy link

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1 1 0 0
View the top 1 failed tests by shortest run time
services/notification/tests/unit/test_notification_service.py::::services.notification.tests.unit.test_notification_service
Stack Traces | 0s run time
No failure message available

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

@codecov codecov deleted a comment from codecov bot Jan 22, 2025
@codecov codecov deleted a comment from codecov-notifications bot Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.71%. Comparing base (254374e) to head (bcef3bc).
Report is 1 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ification/notifiers/checks/checks_with_fallback.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1023   +/-   ##
=======================================
  Coverage   97.71%   97.71%           
=======================================
  Files         458      458           
  Lines       37072    37138   +66     
=======================================
+ Hits        36224    36289   +65     
- Misses        848      849    +1     
Flag Coverage Δ
integration 42.55% <41.30%> (-0.04%) ⬇️
unit 90.29% <98.91%> (+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.

⚠️ Impact Analysis from Codecov is deprecated and will be sunset on Jan 31 2025. See more

@codecov-notifications
Copy link

codecov-notifications bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 98.91304% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ification/notifiers/checks/checks_with_fallback.py 80.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@nora-codecov nora-codecov force-pushed the nora/1626-target-text branch from 55997ae to 94a506d Compare January 23, 2025 21:52
@nora-codecov nora-codecov changed the base branch from nora/1626 to main January 23, 2025 21:59
@nora-codecov nora-codecov force-pushed the nora/1626-target-text branch from 94a506d to bcef3bc Compare January 23, 2025 22:16
@nora-codecov nora-codecov added this pull request to the merge queue Jan 23, 2025
Merged via the queue into main with commit e3229e5 Jan 23, 2025
26 of 27 checks passed
@nora-codecov nora-codecov deleted the nora/1626-target-text branch January 23, 2025 22:33
Copy link

sentry-io bot commented Jan 23, 2025

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ KeyError: 'layout' app.tasks.notify.Notify View Issue
  • ‼️ IntegrityError: (psycopg2.errors.UniqueViolation) duplicate key value violates unique constraint "commit_notifica... app.tasks.notify.Notify View Issue
  • ‼️ IndexError: list index out of range app.tasks.notify.Notify View Issue
  • ‼️ AttributeError: 'NoneType' object has no attribute 'repoid' app.tasks.notify.Notify View Issue
  • ‼️ ValueError: Unsupported plan app.tasks.notify.Notify View Issue

Did you find this useful? React with a 👍 or 👎

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.

3 participants