561 588 improve dodal and its use in artemis #595
Conversation
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Looks much nicer, particularly with the eiger moved around, thanks. Minor comments in code
| class FGSComposite: | ||
| """A device consisting of all the Devices required for a fast gridscan.""" | ||
|
|
||
| dcm: DCM |
There was a problem hiding this comment.
Should: The FGS doesn't use the DCM or OAV, and we have no simulator for them so I think we should remove them from here
| ) | ||
| fast_grid_scan_composite.wait_for_connection() | ||
| with patch("dodal.i03.dcm"): | ||
| with patch("dodal.i03.oav"): |
There was a problem hiding this comment.
Nit: Maybe add a comment here that we're patching it cos there's no simulator. We should remove the patch once we have merged DiamondLightSource/tickit-devices#27
There was a problem hiding this comment.
removed this since they are removed from fgs composite anyway
| @patch("dodal.i03.ApertureScatterguard") | ||
| @patch("dodal.i03.Backlight") | ||
| @patch("dodal.i03.DCM") | ||
| @patch("dodal.i03.EigerDetector") | ||
| @patch("dodal.i03.FastGridScan") | ||
| @patch("dodal.i03.OAV") | ||
| @patch("dodal.i03.S4SlitGaps") | ||
| @patch("dodal.i03.Smargon") | ||
| @patch("dodal.i03.Synchrotron") | ||
| @patch("dodal.i03.Undulator") | ||
| @patch("dodal.i03.Zebra") | ||
| @patch("artemis.experiment_plans.fast_grid_scan_plan.get_beamline_parameters") | ||
| def test_when_blueskyrunner_initiated_then_plans_are_setup_and_devices_connected( | ||
| mock_get_beamline_params, mock_fgs, mock_eiger | ||
| mock_get_beamline_params, | ||
| zebra, | ||
| undulator, | ||
| synchrotron, | ||
| smargon, | ||
| s4_slits, | ||
| oav, | ||
| fast_grid_scan, | ||
| eiger, | ||
| dcm, | ||
| backlight, | ||
| aperture_scatterguard, | ||
| ): | ||
| BlueskyRunner(MagicMock()) | ||
|
|
||
| mock_fgs.return_value.wait_for_connection.assert_called_once() | ||
| zebra.return_value.wait_for_connection.assert_called_once() | ||
| undulator.return_value.wait_for_connection.assert_called_once() | ||
| synchrotron.return_value.wait_for_connection.assert_called_once() | ||
| smargon.return_value.wait_for_connection.assert_called_once() | ||
| s4_slits.return_value.wait_for_connection.assert_called_once() | ||
| oav.return_value.wait_for_connection.assert_called_once() | ||
| fast_grid_scan.return_value.wait_for_connection.assert_called_once() | ||
| eiger.return_value.wait_for_connection.assert_not_called() # can't wait on eiger | ||
| dcm.return_value.wait_for_connection.assert_called_once() | ||
| backlight.return_value.wait_for_connection.assert_called_once() | ||
| aperture_scatterguard.return_value.wait_for_connection.assert_called_once() |
There was a problem hiding this comment.
Nit: Could we just patch dodal.i03 then use mock_i03.zebra.return_value.wait_for_connection.assert_called_once() etc. in the test, reduces complexity a bit
There was a problem hiding this comment.
if you mock dodal.i03 then the function which makes the devices doesn't exist
Codecov Report
@@ Coverage Diff @@
## main #595 +/- ##
==========================================
+ Coverage 80.35% 80.52% +0.16%
==========================================
Files 26 26
Lines 1405 1422 +17
==========================================
+ Hits 1129 1145 +16
- Misses 276 277 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
DominicOram
left a comment
DominicOram
left a comment
There was a problem hiding this comment.
Looks good, thank you!
Fixes #561 and fixes #588
Gets rid of FGSComposite in Dodal and reimplements it as a simple holder class in fast_grid_scan_plan with some config options
Dodal i03.py now manages device instantiation and this functionality is used in Artemis
Link to dodal PR (if required): #31
To test: