-
Notifications
You must be signed in to change notification settings - Fork 25
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
System tests with a real AreaDetector IOC #518
Comments
If you would like to run a set of tests against a SimDetector then the container already exists here: Source for the container is at: We could easily spin up an instance of this in GitHub actions and run some tests against it. I could help with getting that working. |
Discussed this with @DominicOram in relation to an Odin system test. To summarize:
|
Yep, Garry pointed me at the repos. I will do some of this as part of DiamondLightSource/dodal#700 but will probably spin out a new issue for the CI |
I like the idea of a separate |
@jwlodek agreed but I think running it just around releases is not often enough. Given containerised IOCs etc. there's no reason not to also run it in CI for at least every PR. |
This issue captures a discussion with @jwlodek @mrakitin @evalott100 @prjemian and others.
Despite our best efforts, updates to
ophyd-async
are still reasonably likely to break for someone somewhere, primarily because IOCs are fiddly, idiosyncratic, have no defined schemas and can vary per facility. @coretl actually identified this as a major risk to the project in a review a few weeks ago. This brittleness is particularly evident with AreaDetectors, so if we're going to put any work into mitigating this problem, that would be the low-hanging fruit to start with.@jwlodek's proposal was a set of system tests that run in CI against a containerised simulated areadetector. @gilesknap could probably comment on how easy such a thing would be to set up. The main flaw is that it would only pick up problems with ADCore, and not any specific detectors, but it would be a start.
Comment if I've missed anything :)
The text was updated successfully, but these errors were encountered: