Conversation
…needed test module since they were having side effects on the event loop
… nosetests with cover html
|
Go ahead and delete the extra files that don't belong and update the PR. To remove files that shouldn't be in the repo, add them (or their patterns) to the |
|
Ran the clean alias you sent me. Looks like .gitignore was already set for python, and the files were just old local files. |
apollo/smoothie.py
Outdated
| counter = 0 | ||
| while not is_gcode_done: | ||
| counter += 1 | ||
| if counter > 9: |
There was a problem hiding this comment.
Is there a reason the constant 9 is hardcoded? Otherwise it should be in the settings file.
There was a problem hiding this comment.
agreed. there because needed a number to test run it. will put in settings file
apollo/smoothie.py
Outdated
| yield from self.write_and_drain(delimited_gcode) | ||
|
|
||
| # while True needs an escape to avoid infinite loop(s) - how many times does this get called??? | ||
| # counter used to prevent infinite loop, recorded in logger to see if this ever happens |
There was a problem hiding this comment.
These comments seem like developer notes, consider removing?
There was a problem hiding this comment.
They are developer notes. I wanted to make sure it was seen while pairing.
|
Also please remove the coverage files from the repo; there's no need for them to be persisted/tracked in the repo |
|
I'll make the changes and resubmit |
apollo/mock_smoothie.py
Outdated
| @@ -0,0 +1,89 @@ | |||
| import asyncio | |||
There was a problem hiding this comment.
I'm not sure but is this module still being used?
|
I'm a bit confused about the coverage files showing up because it looks like they were covered in gitignore. I'll make sure they're not included |
…g/settings.py for max reads in apollo/smoothie.py
…send_feedback if in halt state (need to double check halt state response)
|
Not entirely sure about the cover files showing up but I removed them manually and pushed the change.. we might have to add another regex to the gitignore file.. |
tests/test_smoothie_com.py
Outdated
|
|
||
| def test_send_G0(self): | ||
| """ TEST SENDING G0 """ | ||
| print('\n\n','='*20,'TESTING G0','-'*20,'\n') |
There was a problem hiding this comment.
print statements should be removed from production code, or at least when a PR is submitted, unless it's a feature...
There was a problem hiding this comment.
I'm a little confused as to when we're pushing code for testing and when PR should be submitted. I have some code to test, not production ready, but seems like I have to do a PR for it to get tested. Should I just go ahead with testing and then do PR?
…/mock_smoothie.py, I think it is worth keeping there for time being
…end_feedback_gcode
…nts tests/test_smoothie_com.py, deleted tests/mock/mock_smoothie.py
…_ for apollo/smoothie.py
|
I think I covered the requests. If not all covered, it's definitely much closer to ready. The automated tests do not cover everything that running the quick tests at the bottom of main and entering specific commands does, like testing the response_handler, but they're not terrible now. Logging should probably be made better too in tests/test_smoothie_com.py, but in the interest of time I just pulled the print statements out. |
| try: | ||
| response = (yield from self._read()) | ||
| logger.info('Smoothie send_feedback_gcode response(1): {}'.format(response)) | ||
| except: |
There was a problem hiding this comment.
blanket except statements aren't good practice because they swallow all exceptions. We should know what sort of exceptions our code will be throwing. So preferrably,
except SomeException as e
and maybe multiple rows of exception handling is needed...
| read_attempts += 1 | ||
| if read_attempts > Config.MAX_SEND_READS: | ||
| raise SmoothieMaxReadsException(gcode) | ||
| break |
There was a problem hiding this comment.
This break is unreachable because the raise will always terminate execution..
| if not isinstance(gcode, str): | ||
| logger.error('Gcode must be string') | ||
| raise TypeError('Gcode must be string') | ||
| return None |
There was a problem hiding this comment.
raise ends program execution, the return None won't be reached...
There was a problem hiding this comment.
Thanks, I was wondering about that
On Jun 14, 2016 12:28 PM, "SimplyAhmazing" notifications@github.com wrote:
In apollo/smoothie.py
#4 (comment):@@ -74,6 +74,10 @@ def send(self, gcode, response_handler=None):
back as the second argument if you want it to handle responses
from the SmoothieBoard.
"""
if not isinstance(gcode, str):logger.error('Gcode must be string')raise TypeError('Gcode must be string')return Noneraise ends program execution, the return None won't be reached...
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/OpenTrons/sandbox-driver/pull/4/files/8fdfbb476120cead0d128b223619bfaa99c99235..46bc44f425710aa6b5dca6e0ddc6ddeec83eaddc#r67005688,
or mute the thread
https://github.com/notifications/unsubscribe/AEzzl2JjkVc3pXFfdCQSkt_wnIrigebEks5qLtaRgaJpZM4IzA3_
.
4 participants
This bunch of changes adds support for specific GCode commands to apollo/smoothie.py and testing for those GCode commands. There is also a user-defined Exception, but it has not received testing. A couple other exceptions do not have test coverage yet. I'm also wondering if we should have testing for receiving "bad data" as opposed to good data/what is expected from Smoothieboard. I guess we'll also see what happens there when we get to integration testing. There are some extra files that snuck in and should be deleted, not merged. tests/test_smoothie_com_two.py is one. I think the rest are from running python3 setup.py install. It'd be nice to know the best way to keep those files out. The code definitely still needs some commenting.