Skip to content

Conversation

@rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Oct 20, 2025

No description provided.

@rtuck99 rtuck99 marked this pull request as ready for review October 20, 2025 10:34
@rtuck99 rtuck99 requested a review from a team as a code owner October 20, 2025 10:34
@rtuck99 rtuck99 changed the title Add option to connect as mock devices feat: Add option to connect as mock devices Oct 20, 2025
@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.81%. Comparing base (6115b04) to head (722df18).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1245      +/-   ##
==========================================
+ Coverage   94.67%   94.81%   +0.13%     
==========================================
  Files          41       41              
  Lines        2611     2642      +31     
==========================================
+ Hits         2472     2505      +33     
+ Misses        139      137       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DiamondJoseph
Copy link
Contributor

Do you have a use case that requires this? To my mind this should be tracked in the dodal module, as the device_factory allows setting whether to connect sim or not, rather than having multiple potentially overlapping places to define this.

Copy link
Contributor

@tpoliaw tpoliaw left a comment

Choose a reason for hiding this comment

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

There's currently no way (that I'm aware of?) of marking all device factories as mocks in one change so I can see the motivation for being able to mock an entire beamline in one flag.

This is all about to change though if the device manager change works out so it might be easier to change there? Other than the weirdness it introduces around having mock plans, I'm not against the idea of it being here as well though.

"kind": "planFunctions",
"module": "dodal.plans"
"module": "dodal.plans",
"mock": false
Copy link
Contributor

Choose a reason for hiding this comment

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

Having a mock field on a plan module is odd. Can we split the Source type into a union where each sub type only has relevant fields?

@rtuck99
Copy link
Contributor Author

rtuck99 commented Oct 28, 2025

Do you have a use case that requires this? To my mind this should be tracked in the dodal module, as the device_factory allows setting whether to connect sim or not, rather than having multiple potentially overlapping places to define this.

The use case is the same as the parent issue

When hyperion moves to blueapi, we will be launching as standard blueapi plan + device modules, so without this option there will be no way to test it outside of connecting to real devices, if we want to be able to do integration testing.

I expect that this will come in handy when I start writing the hyperion-supervisor process, as then I will want to be able to verify that I can launch plans, monitor execution and do things like start and stop hyperion-blueapi plan execution when the baton is taken away, etc. etc.

@rtuck99
Copy link
Contributor Author

rtuck99 commented Oct 28, 2025

There's currently no way (that I'm aware of?) of marking all device factories as mocks in one change so I can see the motivation for being able to mock an entire beamline in one flag.

This is all about to change though if the device manager change works out so it might be easier to change there? Other than the weirdness it introduces around having mock plans, I'm not against the idea of it being here as well though.

I agree the configuration is slightly weird, I can investigate changing the Source type. If the device manager will let us do this more neatly that would be ok, but it does need to be something that can be set up via configuration so that I can do things like run hyperion-blueapi in a test container. So I think it would still require a change in blueapi since that is what drives loading of the beamline module.

I'm not sure what the plan is currently for getting the device manager into dodal - on its own the PR DiamondLightSource/dodal#1549 doesn't provide anything without accompanying changes in dodal device modules and blueapi, and it seems the related discussion DiamondLightSource/dodal#1574 is stalled at the moment.

type union of Source objects for sources
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.

4 participants