I24 serial: beamline test fixes #774
Conversation
pyproject.toml
Outdated
| @@ -46,7 +46,7 @@ dependencies = [ | |||
| "ophyd == 1.9.0", | |||
| "ophyd-async >= 0.8a5", | |||
| "bluesky >= 1.13", | |||
| "dls-dodal @ git+https://github.com/DiamondLightSource/dodal.git@cb12746cb3f658a6d4571a0cd870133a17980bb7", | |||
| "dls-dodal == 1.39.0", | |||
There was a problem hiding this comment.
Should: Better not to have the pin in that I did for the release i.e. not to include that commit.
There was a problem hiding this comment.
Sigh, I didn't even notice this had made it into the PR. Good spot, thanks!
| @@ -626,6 +629,8 @@ def kickoff_and_complete_collection(pmac: PMAC, parameters: FixedTargetParameter | |||
| pmac.collection_time, total_collection_time, group="setup_pmac" | |||
| ) | |||
| yield from bps.wait(group="setup_pmac") # Make sure the soft signals are set | |||
| _sig = yield from bps.rd(pmac.collection_time) | |||
| SSX_LOGGER.warning(f"This was set for collection time {_sig}") | |||
There was a problem hiding this comment.
Should: I suspect this may have been left in after debugging? In theory it should just be the same as what's set and logged above?
There was a problem hiding this comment.
Yep, it was just to check that it had been correctly set
| filename_prefix = parameters.filename | ||
| filename_prefix = cagetstring(Eiger.pv.filenameRBV) |
There was a problem hiding this comment.
Should: We should remove the first set of filename_prefix if we're immediately going to overwrite it
There was a problem hiding this comment.
Could: A test for this would be nice
There was a problem hiding this comment.
Facepalm, I though I had removed it, thanks. I'm actually wondering if it would be better to pass as an argument since we have to do the same cagetstring before starting dcid in the collection plan...
| "group": { | ||
| "experimentType": self.parameters.ispyb_experiment_type.value | ||
| }, |
There was a problem hiding this comment.
Should: We should add a test that would have caught this. There are already tests that patch out the request.post below but they just assert it's called, we should assert it's called with reasonable values
| @@ -707,7 +712,7 @@ def run_fixed_target_plan( | |||
| parameters.collection_directory.mkdir(parents=True, exist_ok=True) | |||
|
|
|||
| if parameters.chip_map: | |||
| upload_chip_map_to_geobrick(pmac, parameters.chip_map) | |||
| yield from upload_chip_map_to_geobrick(pmac, parameters.chip_map) | |||
There was a problem hiding this comment.
Should: We should have a test that would have caught this
| @@ -433,7 +435,8 @@ def start_i24( | |||
|
|
|||
| # DCID process depends on detector PVs being set up already | |||
| SSX_LOGGER.debug("Start DCID process") | |||
| filetemplate = f"{parameters.filename}.nxs" | |||
| complete_filename = cagetstring(pv.eiger_ODfilenameRBV) | |||
| filetemplate = f"{complete_filename}.nxs" | |||
There was a problem hiding this comment.
Could: A test for this would be nice
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #774 +/- ##
==========================================
+ Coverage 86.94% 87.38% +0.44%
==========================================
Files 102 102
Lines 6969 6970 +1
==========================================
+ Hits 6059 6091 +32
+ Misses 910 879 -31
|
| @patch( | ||
| "mx_bluesky.beamlines.i24.serial.fixed_target.i24ssx_Chip_Manager_py3v1.PARAM_FILE_PATH_FT" | ||
| ) | ||
| def test_fiducial(fake_param_path, patch_read, patch_mtr, pmac, RE): |
There was a problem hiding this comment.
Could: This isn't a great test name. Test fiducial does what?
Testing version 1.4.4