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

Fix Hyperion system tests #183

Open
DominicOram opened this issue Aug 27, 2024 · 7 comments
Open

Fix Hyperion system tests #183

DominicOram opened this issue Aug 27, 2024 · 7 comments
Labels
Hyperion merge Issues needed to complete the hyperion merge

Comments

@DominicOram
Copy link
Contributor

With the move done in #172 many of the system tests are now not working. In reality many of the system tests have been not working for some time but have not been maintained. We should have a discussion about why this is. Is the maintenance overhead of the system tests worth the benefit they give us?

Acceptance Criteria

  • All system tests on Hyperion work
@DominicOram DominicOram added the Hyperion merge Issues needed to complete the hyperion merge label Aug 27, 2024
@DominicOram
Copy link
Contributor Author

My two cents are:

  • The ispyb interaction tests are definitely useful. They use an existing dev instance that is well maintained by the ispyb team and so require little overhead by us to maintain
  • The config service tests are useful, given that we already maintain the config service ourselves there is less context switching required for us to keep up the maintenance of a dev system
  • The system tests interacting with zocalo are useful. The maintenance overhead is low given that they just use some very simple python scripts that we bundle in our repo. I think there could also be a path here to working more closely with the analysis team to make them use a realistic dev version of zocalo.
  • The system tests against s03 are not useful given the overhead required to maintain them. They use systems that we are not very familiar with and so context switching to fix them is hard for us. We also do not have much buy-in from controls developers to help us regularly maintain them as they do not use s03 itself

My main concern here is that the continuous failure of s03 tests (and our collective reluctance to maintain them) is causing us to miss potentially useful errors in the tests that we can more easily maintain or causing us to not bother maintaining any of them. I propose we remove the tests against s03 and depreciate s03 as a concept in it's current form. However, I could be open to at least splitting the s03 tests off into a separate set of system tests.

Things that would change my mind:

  • Highlighting some specific instances where the s03 tests caught something that would have been a severe issue in prod
  • More buy-in from other teams on maintaining s03 or, more generally, maintaining device simulators. We have this for the eiger (thanks to @GDYendell) which I think would be good to try and run some system testing against. However, it's not clear that we have that for other devices or if there's a path to having it

@DominicOram
Copy link
Contributor Author

A similar discussion should be had with dodal

@olliesilvester
Copy link
Contributor

I agree with your comments about s03. I also think we should make clear which systems tests should 'just work' on a diamond workstation with no extra steps, as those are the most useful ones.

I've been ignoring most of the system tests because I keep expecting none of them to ever work, but like you say, some are actually very useful!

@DominicOram
Copy link
Contributor Author

See DiamondLightSource/dodal#777 for an example of a useful test that we're currently missing out on

@olliesilvester
Copy link
Contributor

@DominicOram Are you happy for me to write an issue, as a candidate for next sprint, to

  • Remove system tests which require S03
  • Fix system tests which don't require S03
  • Get the system tests automatically running once per day, ideally on a cluster

That issue can then close this one

@DominicOram
Copy link
Contributor Author

Go for it, maybe cut it into 3 issues though, as you have described.

@olliesilvester
Copy link
Contributor

See #632, #633, #634

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hyperion merge Issues needed to complete the hyperion merge
Projects
None yet
Development

No branches or pull requests

2 participants