Failed check and status messaging in comment, part I

[nora-shap]

The goal of this epic is to communicate details about check or status failures in the other notifiers, like pr comment.

This is part 1, which is mostly new data structures and variables, and handling of notifications.

Right now we collect all notifications and execute them in any order, they are all independent and they don't know about each other. In this pr, I split notifications into 2 categories (checks/status notifications, or other notifications). Now the check/status notifications go first, because their outcome will impact the messaging in the other types of notifications.

The specific condition I will tackle next: add custom helper text for users that have specified a target in their yaml. If their check/status fails, we will add a message to the pr comment telling them that the check/status failed, with advice that mentions target and links them to the target docs. There are screenshots in the tix linked below.

What are check/status notifications? Check out the best stack overflow answer I've ever seen

codecov/engineering-team#1605
codecov/engineering-team#1626
Epic here: codecov/engineering-team#1133

[codecov-notifications]

Codecov Report

Attention: Patch coverage is 95.77465% with 3 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
services/notification/notifiers/status/patch.py	75.00%	2 Missing ⚠️
services/notification/__init__.py	93.33%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[codecov]

Codecov Report

Attention: Patch coverage is 95.77465% with 3 lines in your changes missing coverage. Please review.

Project coverage is 97.75%. Comparing base (905d703) to head (9cdbe99).
Report is 10 commits behind head on main.

❌ We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.

Files with missing lines	Patch %	Lines
services/notification/notifiers/status/patch.py	75.00%	2 Missing ⚠️
services/notification/__init__.py	93.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1014   +/-   ##
=======================================
  Coverage   97.75%   97.75%           
=======================================
  Files         451      451           
  Lines       36812    36862   +50     
=======================================
+ Hits        35986    36035   +49     
- Misses        826      827    +1     

Flag	Coverage Δ
integration	42.55% <77.46%> (+0.06%)	⬆️
unit	90.22% <87.32%> (-0.01%)	⬇️

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

[github-actions]

✅ All tests successful. No failed tests were found.

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

[Swatinem]

I love the well defined return types now

[Swatinem]

also looking at the mockup for this, is it reasonable to include the numbers in this message as well, like:

… has failed because the patch coverage (X%) is below the target coverage (Y%).

[nora-shap]

That's not in the design, but I think it makes sense - having the %s makes the message better, will add

[nora-shap]

also decided I'm not going to add the helper text to this message, so the resulting message for check or status will be unchanged

[sentry-io]

Suspect Issues

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

• ‼️ AttributeError: 'NoneType' object has no attribute 'get' app.tasks.notify.Notify View Issue

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