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

Provide a NoOpStatus #521

Closed
DominicOram opened this issue Aug 15, 2024 · 8 comments · Fixed by #540
Closed

Provide a NoOpStatus #521

DominicOram opened this issue Aug 15, 2024 · 8 comments · Fixed by #540
Assignees

Comments

@DominicOram
Copy link
Contributor

ophyd has a NullStatus (https://github.com/bluesky/ophyd/blob/050b98f34d68bd9577179accfdfca8f635cc0f31/ophyd/sim.py#L51) that immediately returns, this is very useful for testing as it's something you probably want to inject into many devices. We should provide something similar in ophyd-async where it basically just does AsyncStatus(asyncio.sleep(0.0)). It will also be nice to be able to:

  • Be able to provide an identifier (such as the name of the test) so that we can easily identify it later if needed
  • Be able to provide a flag that causes the status to be in error
@coretl
Copy link
Collaborator

coretl commented Aug 16, 2024

Can you provide a use case please? The closest we have at the moment is:

@AsyncStatus.wrap
async def kickoff(self) -> None:
    pass

Which could be (slightly) reduce if we had a NoOpStatus:

def kickoff(self) -> Status:
    return NoOpStatus()

@DominicOram
Copy link
Contributor Author

It's more for downstream testing e.g.

class MyDevice(Device):
    def set(self, value):
        # Some long running thing that checks against PVs etc

def test_my_plan():
    my_device.set = MagicMock(return_value=NullStatus())
    my_plan(my_device)

In this case the logic in set is too complicated for the mocked object (via DeviceCollector(mock=True)) to handle out the box but we also don't want to have to worry about internal details of it in our plan. For this test we just care that it works (or maybe doesn't work hence the requirement on being able to provide an error)

@coretl
Copy link
Collaborator

coretl commented Aug 16, 2024

Would it have to be a subclass of AsyncStatus so you can isinstance it, or would a factory function work? I'm slightly averse to big nested class heirarchies...

E.g.

@AsyncStatus.wrap
async def completed_status(exception: Optional[Exception] = None) -> None:
    if exception:
        raise exception

def test_my_plan():
    my_device.set = MagicMock(return_value=completed_status())
    my_plan(my_device)

@DominicOram
Copy link
Contributor Author

Yh, that works, I'm not fussed about the implementation, just the behaviour.

@olliesilvester olliesilvester self-assigned this Aug 22, 2024
@olliesilvester
Copy link
Contributor

I can pick this up. @DominicOram what did you mean by having an identifier? Won't it be obvious from whatever test is failing?

@DominicOram
Copy link
Contributor Author

It was a request by @dperl-dls. What we've seen in the past is issues where status' are hanging around at the end of tests and have not been sufficiently awaited/cancelled. When this happens it's very hard to tell which of the hundreds of tests produced the status. Personally I think in this case it might not be 100% necessary as it shouldn't really be possible to have the noop hanging around for any length of time.

@olliesilvester
Copy link
Contributor

I think this part may be addressed in https://github.com/bluesky/ophyd-async/pull/538/files#

@DominicOram
Copy link
Contributor Author

In which case with @dperl-dls away in the spirit of keeping things moving I'm happy to drop the requirement of having them named but he's welcome to open a new issue when he's back.

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 a pull request may close this issue.

3 participants