-
Notifications
You must be signed in to change notification settings - Fork 8
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
805 move p99 detectors into dodal #807
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #807 +/- ##
==========================================
+ Coverage 95.12% 95.21% +0.08%
==========================================
Files 120 124 +4
Lines 4901 4993 +92
==========================================
+ Hits 4662 4754 +92
Misses 239 239 ☔ View full report in Codecov by Sentry. |
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.
Some comments in code. Additionally, I would recommend more tests that are smaller and more descriptive. For example:
test_get_deadtime_returns_expected
<- just test deadtimetest_given_trigger_number_of_0_then_number_of_images_set_to_max
test_given_no_live_time_then_esposure_time_not_set
The advantage of this is it very clearly lays out what is expected of the device and if one of them fail in the future it should be obvious to someone what went wrong. In general I would expect a device like this to have maybe a dozen small tests like this.
Fixes #805
Instructions to reviewer on how to test:
Checks for reviewer
dodal connect ${BEAMLINE}