timeout related fixes and error propogation improvements
#120
Conversation
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
amruthesht
force-pushed
the
socket-timeout
branch
from
079696d to
0596e06
Compare
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
👍 on the timeout=600.
I have technical comments on the tests, please see inline.
@HeydenLabASU do you want to review and give your blessing?
@ljwoods2 do you have comments?
imdclient/tests/test_imdclient.py
Outdated
| server = InThreadIMDServer(universe.trajectory) | ||
| server.set_imdsessioninfo(imdsinfo) | ||
| server.handshake_sequence("localhost", first_frame=False) | ||
| client = IMDClient( | ||
| f"localhost", | ||
| server.port, | ||
| universe.trajectory.n_atoms, | ||
| timeout=timeout_val, | ||
| ) | ||
| server.join_accept_thread() | ||
|
|
||
| # Sleep for less than timeout before sending frames | ||
| time.sleep(timeout_val - 1) | ||
| server.send_frames(0) |
There was a problem hiding this comment.
This code is repeated - does it make sense to turn it into a fixture or at least a method that can be used by multiple tests?
There was a problem hiding this comment.
Yes, I can make a function that does the "server setup and connection" part of the code
imdclient/tests/test_imdclient.py
Outdated
| server = InThreadIMDServer(universe.trajectory) | ||
| server.set_imdsessioninfo(imdsinfo) | ||
| server.handshake_sequence("localhost", first_frame=False) | ||
| client = IMDClient( | ||
| f"localhost", | ||
| server.port, | ||
| universe.trajectory.n_atoms, | ||
| timeout=timeout_val, | ||
| ) | ||
| server.join_accept_thread() | ||
|
|
||
| # Sleep for longer than timeout without sending any frames | ||
| time.sleep(timeout_val + 1) |
|
@orbeckst super backed up atm, will need ~1 week to get to this. apologies, feel free to move forward without me if you're hoping to push a release |
|
@amruthesht please resolve the conflict that's holding up the CI. Thanks. |
amruthesht
force-pushed
the
socket-timeout
branch
from
00b7edb to
51dd426
Compare
orbeckst
left a comment
orbeckst
left a comment
There was a problem hiding this comment.
The refactoring of the client/server in the tests looks good to me (and all tests continue to pass).
Coverage is low because you added many more cases for exceptions and we're not testing all these different fail cases separately. There also appears to be some code (InThread client?) that is not really tested at all (?)
I don't think it's worthwhile right now to create test cases for each separate failure case (unless it's easy to do, e.g. with mocking) but we should be clear what is untested code. Could you please
- check the [coverage]((https://app.codecov.io/gh/Becksteinlab/imdclient/pull/120) for the code.
- mark untested code blocks with
#pragma: nocover - raise issues for larger code blocks that should be tested eventually
| logger.debug( | ||
| f"InThreadIMDServer: Listening on {host}:{self._bound_port}" | ||
| ) |
There was a problem hiding this comment.
Did black want to reformat this??
| raise EOFError | ||
| raise EOFError("Consumer has finished") | ||
|
|
||
| return self._empty_q.get() |
There was a problem hiding this comment.
This return is never tested according to coverage. Do you know why and does it matter?
| try: | ||
| return self._producer._get_imdframe() | ||
| except EOFError: | ||
| except EOFError as e: | ||
| logger.debug(f"IMDClient: Single-threaded connection ended") | ||
| self._disconnect() | ||
| raise EOFError | ||
| raise EOFError from e |
There was a problem hiding this comment.
This block is never tested. Do we not test the single-threaded client?
I think we should do test it at least with a simple test. Otherwise we're advertising code for which we don't even know if it runs correctly.
| try: | ||
| self._parse_imdframe() | ||
| except EOFError as e: | ||
| raise EOFError | ||
| logger.debug(f"IMDProducer: No more frames to read: {e}") | ||
| raise EOFError from e | ||
| except Exception as e: | ||
| raise RuntimeError("An unexpected error occurred") from e |
There was a problem hiding this comment.
None of these lines are covered.
Fixes #111
Changes made in this Pull Request:
timeoutdefualt value chnaged to600timeouttimeout <= 1PR Checklist