-
Notifications
You must be signed in to change notification settings - Fork 24
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
EpicsMotorIOC #62
base: master
Are you sure you want to change the base?
EpicsMotorIOC #62
Conversation
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
==========================================
+ Coverage 52.16% 55.45% +3.28%
==========================================
Files 11 12 +1
Lines 692 743 +51
==========================================
+ Hits 361 412 +51
Misses 331 331
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to resolve the confusion and issues with the tests prior to merging see above comment chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klauer thanks for the review request, however I already have reviewed this a few times. The last concerns I had related to the location of the test files which @malitsky resolved, hence why I approved it.
Clearly their are still some outstanding comments from @tacaswell that require resolution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test should be split up into multiple tests and the clean up logic moved to a fixture.
The time.sleep
needs be removed.
The questions about how this behaves to semi-pathological input should be answered, but we can discuss if fixing that is in scope for this or not.
Removing approval pending resolution of @tacswell ‘s comments
PR updated with Tom's comments. |
fyi, @tacaswell and @awalter-bnl |
No description provided.