Skip to content
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

Trigger Zocalo and turn on feedback after all collections in MSP #850

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

DominicOram
Copy link
Contributor

Fixes #826 and fixes #848

Note, this is probably a breaking change for single rotation scans but given #847 I think that's ok.

Instructions to reviewer on how to test:

  1. Confirm new tests make sense and pass
  2. Confirm new docs make sense
  3. Confirm all other changes in Multipin tests 17/02 #826 have been solved or have been spun into new issues

Checks for reviewer

  • Would the PR title make sense to a user on a set of release notes

@DominicOram DominicOram changed the title Trigger Zocalo and turn on feedback after _all_ collections in MSP Trigger Zocalo and turn on feedback after all collections in MSP Feb 21, 2025
Copy link

codecov bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (76ea6f7) to head (35f9a10).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #850   +/-   ##
=======================================
  Coverage   87.33%   87.33%           
=======================================
  Files         104      104           
  Lines        6995     6997    +2     
=======================================
+ Hits         6109     6111    +2     
  Misses        886      886           
Components Coverage Δ
i24 SSX 73.95% <ø> (ø)
hyperion 96.33% <100.00%> (ø)
other 96.25% <100.00%> (+<0.01%) ⬆️

@olliesilvester olliesilvester self-assigned this Mar 3, 2025
Copy link
Contributor

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks. My 'requested changes' comment are the ones regarding the docs, other ones are optional

def start_dict(plan_name: str = "test_plan_name", env: str = "test_env"):
return {CONST.TRIGGER.ZOCALO: plan_name, "zocalo_environment": env}


class TestZocaloHandler:
def _setup_handler(self):
zocalo_handler = ZocaloCallback("test_plan_name", "test_env")
return zocalo_handler

def test_handler_doesnt_trigger_on_wrong_plan(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need an assert here now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can probably remove this test entirely in favour of test_zocalo_start_and_end_not_triggered_if_ispyb_ids_not_present

"mx_bluesky.common.external_interaction.callbacks.common.zocalo_callback.ZocaloTrigger",
autospec=True,
)
def test_handler_raises_on_the_end_of_a_plan_with_no_depositions(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could: Add some tests which assert this exception if the document's scan_points or ispyb_dcids aren't in a correct form

DominicOram and others added 2 commits March 4, 2025 16:56
Co-authored-by: olliesilvester <122091460+olliesilvester@users.noreply.github.com>
Co-authored-by: olliesilvester <122091460+olliesilvester@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi rotations: Wait for unstage to happen before changing transmission Multipin tests 17/02
2 participants