bluesky / ophyd-async

Hardware abstraction for bluesky written using asyncio
https://blueskyproject.io/ophyd-async
BSD 3-Clause "New" or "Revised" License
10 stars 23 forks source link

Add a shared parent mock for all signals in once device #336

Open DominicOram opened 4 months ago

DominicOram commented 4 months ago

As a developer I occasionally want to make sure that one signal is changed before another. For example, see test_retrieve_mock_and_assert in https://github.com/bluesky/ophyd-async/pull/335/files or for a more real world example see numerous tests in https://github.com/DiamondLightSource/dodal/blob/main/tests/devices/unit_tests/test_aperture_scatterguard.py. To do this you need to create a parent mock that all the signals are children of.

If ophyd_async by default made all the mock's in one device have the same parent this would remove a bit of boilerplate from downstream.

coretl commented 3 months ago

A suggestion for implementation:

This will mean that if you have a tree of devices where connect it called at the top level then you can call:

get_mock_put(mydevice).assert_has_calls(
        [
            call.child1.grandchild1(100, wait=True, timeout=ANY),
            call.child2.grandchild2(67, wait=True, timeout=ANY),
        ]
    )

I'm a little concerned that we might decide we want to check the ordering of puts and other methods like get_descriptor, so maybe we should consider this before making a mock heirarchy we rely upon?

For instance, would we ever want something like this?

get_mock(mydevice).assert_has_calls(
        [
            call.child1.grandchild1.put(100, wait=True, timeout=ANY),
            call.child1.grandchild1.get_descriptor(),
            call.child2.grandchild2.put(67, wait=True, timeout=ANY),
        ]
    )

@DominicOram thoughts?

coretl commented 2 months ago

@DominicOram any thoughts on this?

coretl commented 2 months ago

@dperl-dls any thoughts on this?

DominicOram commented 2 months ago

A suggestion for implementation:

Yes, I like this idea.

I'm a little concerned that we might...

This feels like feature creep at this point. I can't see a reason we would want to do this yet and it doesn't seem like the way we structure this hierarchy will have that much of an affect on how we want to do it in the future.

coretl commented 2 months ago

This feels like feature creep at this point. I can't see a reason we would want to do this yet and it doesn't seem like the way we structure this hierarchy will have that much of an affect on how we want to do it in the future.

Ok, let's go with the minimum change then, so

get_mock_put(motor.setpoint).assert_called_once_with(32.0, wait=True, timeout=ANY)

will still work, and so will:

get_mock_put(motor).assert_has_calls(
        [
            call.setpoint(32.0, wait=True, timeout=ANY),
            call.velocity(1.0, wait=True, timeout=ANY),
        ]
    )