-
Notifications
You must be signed in to change notification settings - Fork 2
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
Read aperture properly rather than private method #506
base: main
Are you sure you want to change the base?
Conversation
@@ -239,8 +239,7 @@ async def test_rotation_plan_moves_aperture_correctly( | |||
run_full_rotation_plan.aperture_scatterguard | |||
) | |||
assert ( | |||
await aperture_scatterguard.get_current_aperture_position() | |||
== ApertureValue.SMALL | |||
await aperture_scatterguard.selected_aperture.get_value() == ApertureValue.SMALL |
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.
selected_aperture
is the sole HintedSignal of aperture_scatterguard
, you should be able to just aperture_scatterguard.read()
?
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.
I'm not sure I see the advantage of this as it's more verbose and now makes the test depend on the fact the signal is hinted but don't care so much so have changed it. Are you suggesting it because read()
is the more externally facing API over get_value()
?
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.
Looking at the change to the test I think I was just wrong? I thought that
selected_aperture.read() == ApertureValue.SMALL
And then we could ensure that the selected_aperture was always readable and always gave the ApertureValue as its output.
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.
Yh, I think even reading the one signal it will still return a dict with the name
and value
.
But the test shouldn't care if it's readable or hinted. The test is "does doing this plan change the aperture in the expected way". We have other tests that are "can I read the selected aperture in the way I expect" like
mx-bluesky/tests/unit_tests/hyperion/experiment_plans/test_flyscan_xray_centre_plan.py
Line 301 in 1dc26a5
"aperture_scatterguard-selected_aperture": ApertureValue.SMALL, |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
===========================================
+ Coverage 57.08% 77.61% +20.53%
===========================================
Files 28 89 +61
Lines 3199 6704 +3505
===========================================
+ Hits 1826 5203 +3377
- Misses 1373 1501 +128 ☔ 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.
Sorry re: confusion with the scatterguard read: I think it was neater before my previously requested changes but either way the change is more correct than before and either is compatible with the patched version of dodal
See DiamondLightSource/dodal#782
Requires dodal version DiamondLightSource/dodal#789
To test: