878 grid scans should move dectector in and open shutter#978
Conversation
Remove wait_for_det_to_finish_moving now that group wait handles it
…_move_dectector_in_and_open_shutter
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #978 +/- ##
==========================================
+ Coverage 93.48% 93.71% +0.23%
==========================================
Files 54 56 +2
Lines 2623 2673 +50
==========================================
+ Hits 2452 2505 +53
+ Misses 171 168 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
This is great! This must be the most well-tested little function we've got. I look forward to using your simulator in other tests.
- yes, you're using the right parameter for
detector_distance - I think your
yield from bps.wait("ready_for_data_collection")is in the right place - the mock values look okay to me
Although this test is complex and kind of like an integration test as far as components of hyperion are concerned, the distinction between unit/system tests here is basically whether we need to connect to real EPICS PVs (whether from a simulator or the real hardware ones) in order to run the test. If it can run self-sufficiently, it should be grouped with unit tests even if conceptually more complex.
| magic_mock.smargon = smargon | ||
| magic_mock.detector_motion = detector_motion | ||
| scan = make_fake_device(FastGridScan)("prefix", name="fake_fgs") | ||
| # scan.scan_invalid.sim_put(True) |
There was a problem hiding this comment.
Should: please remove commented code
| @@ -0,0 +1,161 @@ | |||
| from typing import Callable, Generator, Sequence | |||
|
|
|||
| from bluesky import Msg | |||
There was a problem hiding this comment.
Should: import this from bluesky.utils instead
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
| from bluesky import Msg |
There was a problem hiding this comment.
Should: please import this from bluesky.utils
| ) | ||
| @patch( | ||
| "hyperion.external_interaction.callbacks.xray_centre.zocalo_callback.XrayCentreZocaloCallback.wait_for_results", | ||
| lambda self, x: ([0, 0, 0], [1, 1, 1]), |
There was a problem hiding this comment.
Nit: it would be more idiomatic to make the arguments _, __ as they're not used
| "hyperion.external_interaction.callbacks.xray_centre.zocalo_callback.XrayCentreZocaloCallback.wait_for_results", | ||
| lambda self, x: ([0, 0, 0], [1, 1, 1]), | ||
| ) | ||
| def test_when_pin_centre_xray_centre_called_then_detector_positioned( |
There was a problem hiding this comment.
This test is fantastic! The whole reason plans are built from generators is for us to be able to introspect them like this, and we've never so far bothered. Thanks for writing this.
There was a problem hiding this comment.
@rtuck99 - seconding this, it's really good, thank you!
| and msg.kwargs["group"] == "ready_for_data_collection", | ||
| ) | ||
| ) | ||
| assert messages_from_where( |
There was a problem hiding this comment.
Nit: Is this supposed to just assert it's not an empty list? if so, the assertion in message_from_where() will already take care of that - maybe give it an error message saying nothing matched the predicate?
| and msg.kwargs["subplan_name"] == "do_fgs", | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: maybe it would be better for this function to live with the run engine simulator? it seems reusable
| "event", | ||
| { | ||
| "data": { | ||
| "oav_snapshot_last_saved_path": "xxx", |
There was a problem hiding this comment.
these can probably just be something like "/tmp/image1.png", "/tmp/image2.png", "/tmp/image3.png"
| yield from bps.abs_set(eiger.do_arm, 1, group=group) | ||
|
|
||
| yield from set_detector_z_position(detector_motion, detector_distance, group) | ||
| yield from set_shutter(detector_motion, ShutterState.OPEN, group) |
There was a problem hiding this comment.
Should we wait for these moves to have completed before doing the next plan? We have the groups here but they're never waited for
There was a problem hiding this comment.
The group is already waited for in flyscan_xray_centre_plan, the unit test calls all the subplans and asserts that this group is waited for before the main fast grid scan starts
4 participants
Fixes #878
Also adds some new testing infrastructure that allows lightweight integration test.
Link to dodal PR (if required): https://github.com/DiamondLightSource/dodal/pull/237
Please check:
wait_for_det_to_finishshouldn't break anything and that theready_for_data_collectionis waited at the correct location