timeout related fixes and error propogation improvements#120
timeout related fixes and error propogation improvements#120amruthesht wants to merge 11 commits into
timeout related fixes and error propogation improvements#120Conversation
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?
|
@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??
There was a problem hiding this comment.
Didn't do anything 😅
| 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?
There was a problem hiding this comment.
Done, please have a look. Wasn't sure what the best way to do this was. Made a seperate class to test this buffer-specific scenario
| 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.
There was a problem hiding this comment.
Yes, agreed. Added a test that should do the job!
| 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.
|
@amruthesht @HeydenLabASU I think we still want this fix, right? |
|
@amruthesht can you finish the PR and then we can make a release #130? We need a release so that we have a zenodo DOI for the package as we want to include that in the paper. |
3 participants
Fixes #111
Changes made in this Pull Request:
timeoutdefualt value chnaged to600timeouttimeout <= 1PR Checklist