Add testing of rs232.SerialData #147
Conversation
feature that allows a handler to be used based on the url of the serial "device".
I think that the module is being loaded twice, once by test_rs232.py, and another time by serial.serial_for_url(). As a result, the buffers set by the test aren't visible to the BuffersSerial instance.
pocs/utils/rs232.py
Outdated
| self.process.daemon = True | ||
| self.process.name = "PANOPTES_{}".format(name) | ||
|
|
||
| # TODO(jamessynge): Consider eliminating this sleep period, or making |
There was a problem hiding this comment.
Looks like you did make this configurable. Is comment still necessary?
There was a problem hiding this comment.
Not if you're comfortable with the configurable behavior.
And can you tell me what problem you were working around with the 2 second delay?
pocs/utils/rs232.py
Outdated
| if self.is_threaded: | ||
| self._serial_io = TextIOWrapper(BufferedRWPair(self.ser, self.ser), | ||
| newline='\r\n', encoding='ascii', line_buffering=True) | ||
| self.ser = serial.serial_for_url(port, do_not_open=True) |
There was a problem hiding this comment.
I see the documentation says to consider using this but don't understand what the advantage is.
There was a problem hiding this comment.
Not sure what "this" refers to. If you mean do_not_open, the advantage is that you set all of the properties (e.g. baud rate and byte size) before opening the port. Depending on the device, it may require effectively closing and reopening to change properties (_reconfigure_port is called for this).
There was a problem hiding this comment.
Sorry, bad comment on my part. This was in reference to using serial_for_url. I reviewed this out of order, starting with this file, so after looking at how the tests are set up this makes more sense.
pocs/utils/rs232.py
Outdated
| self.process.daemon = True | ||
| self.process.name = "PANOPTES_{}".format(name) | ||
| # Properties have been set to reasonable values, ready to open the port. | ||
| self.ser.open() |
There was a problem hiding this comment.
How come we've lost the exception handling?
There was a problem hiding this comment.
Since we don't have any "open on write" behavior, this means that we have a completely useless SerialData instance if we ignore the failure. And since we use exceptions elsewhere for failures, I didn't see the point in hiding this failure. All the tests passed after making that change.
pocs/utils/rs232.py
Outdated
| except Exception as err: | ||
| self.ser = None | ||
| self.logger.critical('Could not set up serial port {} {}'.format(port, err)) | ||
| if self.is_threaded: |
There was a problem hiding this comment.
I understand this is not your focus here necessarily, but I see that pySerial now has support for threaded and asyncio. I haven't looked into them but perhaps we should consider?
There was a problem hiding this comment.
I too haven't looked at that. While probably worth looking into, I don't want to entangle these issues.
pocs/utils/rs232.py
Outdated
| def read(self, retry_limit=5, retry_delay=0.5): | ||
| """Reads next line of input using readline. | ||
|
|
||
| If no response is given, delay for retry_delay and then try to read |
There was a problem hiding this comment.
Better than previous docstring, but we have explicit Args section. Need to start enforcing docstring standards. I have a plugin for Sublime that fills out all the boilerplate.
http://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html#example-google
| self._ri = False | ||
| self._ri = False | ||
| self._ri = False | ||
| self._ri = False |
| if self.logger: | ||
| self.logger.info('ignored setBreak({!r})'.format(level)) | ||
|
|
||
| def setRTS(self, level=True): |
There was a problem hiding this comment.
All these method names are deprecated in favor of properties. Should we still be using?
pocs/tests/test_rs232.py
Outdated
| @@ -0,0 +1,81 @@ | |||
| import pytest | |||
| import serial # PySerial, from https://github.com/pyserial/pyserial | |||
There was a problem hiding this comment.
No need, removing.
pocs/tests/test_rs232.py
Outdated
| print(str(ser.ser)) | ||
| # Open is automatically called by SerialData. | ||
| assert ser.is_connected | ||
| # Not using threading. |
There was a problem hiding this comment.
threaded=True is the default on SerialData. Does this get unset in one of the serial_handler files above?
There was a problem hiding this comment.
My mistake, I misunderstood.
pocs/tests/test_rs232.py
Outdated
| # Not using threading. | ||
| assert not ser.is_listening | ||
|
|
||
| # no_op handler doesn't do any reading or writing. |
|
In meetings for a bit now, so quick comment: I don't think we are using the threaded anywhere so maybe look at scrapping it. More in a few. |
pocs/tests/test_rs232.py
Outdated
|
|
||
| # Assert how much is written, which unfortunately isn't consistent. |
There was a problem hiding this comment.
I guess that's a bit concerning. Do you know why this is?
…rial. This shows how we can develop mock serial devices for testing.
jamessynge
changed the title
|
Still need to review your comments. |
Codecov Report
@@ Coverage Diff @@
## develop #147 +/- ##
==========================================
- Coverage 84.42% 80.6% -3.82%
==========================================
Files 37 36 -1
Lines 2414 2527 +113
Branches 287 319 +32
==========================================
- Hits 2038 2037 -1
- Misses 294 386 +92
- Partials 82 104 +22
Continue to review full report at Codecov.
|
|
Superseded by a later PR |
The latest test, test_unthreaded_io, isn't working yet, probably because of the module being loaded twice (i.e. two separate paths), and thus the global variables in the module aren't shared between the two distinct loads of the module.