-
Notifications
You must be signed in to change notification settings - Fork 11
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 IV #1042
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
807bd68
to
339ffc4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM. Left a nit comment about enum.
I do have 1 question about this:
the helper text is a dictionary, and the helper-text-section will write all helper texts in this dictionary.
If adjust_base
failed then the regular project check also failed - by definition. Wouldn't that also create a helper text?
My question is: if there are 2 helper texts in the help text dict, will the comment have 2 messages about virtually the same thing?
I haven't seen a test for this case.
@@ -27,16 +27,24 @@ class StatusResult(TypedDict): | |||
|
|||
CUSTOM_TARGET_TEXT_PATCH_KEY = "custom_target_helper_text_patch" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With so many keys, maybe it's more organized to create an Enum for them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or a TypedDict
, where you have safety and autocompletion when writing keys, but you can iterate over only the values, or the k/v pairs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
yes and no! yes, you can have more than 1 helper text, but I think they are not about the same thing (therefore it is useful to have both). tested and attached a screenshot to this ticket. I designed the helper text system with this case in mind - each helper text should be about an independent issue, so that the comment can have one or many. However, if the user has defined a custom |
339ffc4
to
3e129cb
Compare
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
3e129cb
to
fc565b1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #1042 +/- ##
=======================================
Coverage 97.52% 97.52%
=======================================
Files 462 462
Lines 37902 37924 +22
=======================================
+ Hits 36963 36985 +22
Misses 939 939
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fc565b1
to
a379de0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for clarifications
when user has:
project checks notifier OR project status notifier
AND
comment notifier
AND
has defined
removed_code_behavior
asadjust_base
in their codecov.ymlAND
the project check/status fails
AND
the
adjusted_base
coverage does NOT allow the check/status to passTHEN
add some helper text to the comment notifier to point out that the check/status failed, with links to
rcb
andadjust_base
docs, and tell them the coverage on the head vs the adjusted base.codecov/engineering-team#1607