-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
fix: AlertReportCronScheduler tests #19288
fix: AlertReportCronScheduler tests #19288
Conversation
91a95df
to
36ddb37
Compare
Codecov Report
@@ Coverage Diff @@
## master #19288 +/- ##
=======================================
Coverage 66.59% 66.60%
=======================================
Files 1682 1682
Lines 64314 64317 +3
Branches 6559 6559
=======================================
+ Hits 42832 42837 +5
+ Misses 19781 19779 -2
Partials 1701 1701
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
36ddb37
to
b6a472b
Compare
b6a472b
to
380abe3
Compare
@eschutho would love to get feedback from someone closer to this feature, in case there's any risk I'm not spotting here. |
|
||
const handlePressEnter = useCallback(() => { | ||
onChange(inputRef.current?.input.value || ''); | ||
}, [onChange]); |
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.
@diegomedina248 do you think we need useCallback
for simple Input component props?
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.
It doesn't hurt nor improve in this case, but I believe it's cleaner
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.
It's a bit more code and a bit more computation imo, which is my only concern with not much performance impact, but it's not a big deal. I'll cc @rusackas if he has any opinions.
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.
but to your comment of it being cleaner, @diegomedina248 I meant to also ask in what way?
Looks great.. thanks for the tests @diegomedina248! |
SUMMARY
Tests for the
AlertReportCronScheduler
component were skipped when the component was rewritten.This PR introduces tests for the component using the react testing library, plus some optimizations in the component.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
None (other than the test execution results)
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION