DM-48197: Add additional efd connection checks to ensure tests only run where they can #288
Conversation
python/lsst/cp/pipe/utilsEfd.py
Outdated
|
|
||
| Raises | ||
| ------ | ||
| RuntimError : |
| # Initialize a client. | ||
| try: | ||
| self.client = CpEfdClient() | ||
| cls.client = CpEfdClient() | ||
| except Exception as e: |
There was a problem hiding this comment.
Is this still supposed to give up on any Exception? It appears as if it should run if you can connect to the EFD, no?
There was a problem hiding this comment.
If it can't connect to the EFD with any error it should skip the tests. I'm not sure what your question is here.
There was a problem hiding this comment.
My question would be: why do this at all, and not just use vcr to test the code exactly the same way, but with no dependency on the EFD at all? That way the tests always do run, and you don't have to ever worry about connections.
There was a problem hiding this comment.
If you're interested I can point you to a working example in summit_utils, it's used in a few places there for exactly this.
There was a problem hiding this comment.
All I meant was that you are raising a RuntimeError in checkConnection but here you catch any exception from CpEfdClient.__init__. But I suppose that's fine since it is reraising and not just continuing.
There was a problem hiding this comment.
Would you prefer that this just check for a RuntimeError?
There was a problem hiding this comment.
Eh, never mind. Maybe reconsider if/when you switch to vcr and maybe it'll just be moot.
erykoff
force-pushed
the
tickets/DM-48197
branch
2 times, most recently
from
0766781 to
3713cd3
Compare
No description provided.