Opened 13 years ago

Last modified 8 years ago

#2462 defect new

write tests for serial port code

Reported by: itamarst Owned by: Michael
Priority: normal Milestone:
Component: core Keywords: serialport, tests
Cc: Jean-Paul Calderone, therve, Lucas Taylor, jesstess, lvh, Michael Branch: branches/serialport-tests-2462
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

We should write tests for serial code.

Note we have two versions of serial code: one for fd-based reactors, one for win32eventreactor.

Options:

  1. Software that emulates a serial device (e.g. http://www.eltima.com/products/vspdxp/)
  2. Hardware loopback, probably with usb/rs-232 adapter so it'll work with newer buildslaves (I think this is one - http://www.cablesnmor.com/serial-loopback-plug.html)

Attachments (1)

2462-1.diff (2.1 KB) - added by Lucas Taylor 9 years ago.
read/write test

Download all attachments as: .zip

Change History (15)

comment:1 Changed 10 years ago by Glyph

Owner: changed from Glyph to Stephen Thorne

Any suggestions? You know stuff about serial ports, right?

comment:2 Changed 10 years ago by Jean-Paul Calderone

Keywords: serialport tests added

comment:3 Changed 9 years ago by Jean-Paul Calderone

Both #3690 and #4249 would benefit from the resolution of this ticket.

comment:5 Changed 9 years ago by Jean-Paul Calderone

That does look like a promising list. Certainly worth investigating further before we spend money on a hardware solution or some proprietary software.

Changed 9 years ago by Lucas Taylor

Attachment: 2462-1.diff added

read/write test

comment:6 Changed 9 years ago by Lucas Taylor

socat can be used for a pty loopback that should work for posix test cases:

$ socat PTY,link=/tmp/a-pty PTY,link=/tmp/a-pty

com0com looks promising for Windows, but I haven't had time to set it up. http://com0com.sourceforge.net/

What other tests ought to be created?

  • test_writeToClosed - Writing to a closed SerialPort should raise an exception

comment:7 Changed 9 years ago by Lucas Taylor

Cc: Lucas Taylor added

comment:8 Changed 9 years ago by jesstess

Cc: jesstess added

comment:9 Changed 9 years ago by Jean-Paul Calderone

socat can be used for a pty loopback that should work for posix test cases:

Creating a loopback pty isn't a big challenge. The main concern is whether a pty is sufficiently like a serial port to be a good test of the code in question.

comment:10 Changed 9 years ago by <automation>

Owner: Stephen Thorne deleted

comment:11 Changed 8 years ago by Itamar Turner-Trauring

Branch: branches/serialport-tests-2462

I moved the testing branch from #3690 here.

comment:12 Changed 8 years ago by Itamar Turner-Trauring

Cc: lvh Michael added
Owner: set to Michael

Here's the review comments this is blocked on, copied from #3690, written by lvh:

Hi! I'm a recovering serial port victim.

First off: this code is *definitely* going in the right direction, please carry on :)

There's a few style issues:

  1. Default value arguments should not have spaces before and after the =, so not def f(x = 1): ... but def f(x=1): ... -- both when defining and when calling. Many of these are existing problems, but this patch should fix them.
    1. In your patch, line 27 of _posixserialport, the __init__ method
    2. In your patch, line 31 of _posixserialport, self._serial = self.SerialClass(...)
    3. In your patch, line 44 of _win32serialport, SerialPort.__init__
  2. Attributes should be in mixedCase, even if they're a class. This means the SerialClass classattr should actually be serialClass (or maybe even serialFactory)
  3. Operators should have spaces around them.
    1. In your patch, line 44 of _win32serialport, SerialPort.doWrite, there's no spaces around n==0. I had to look up n was the number of bytes received. Perhaps it should be renamed bytesReceived. Perhaps that line should be if not bytesReceived:.
    2. In that same doWrite method, if len(self.outQueue)0: needs a space. I'd replace it with if self.outQueue:, though.
    3. in the _serial_win test method, 175, 177, 191: missing space around | operator
  4. Long import lines get several lines.
    1. In your patch, line 27 of test_serialport, the import should match the coding standard

In the tests, _serial_win, man, win32pipe.CreateNamedPipe and win32file.CreateFile make me want to cry. This is not your fault at all, but perhaps the way they are called could be a bit cleaner. Perhaps using kwargs and creating local variables for those things?

There's a few implementation questions:

  1. In your patch, line 110 of _win32serialport, except Exception as e: around a ReadFile call. Is that really the exception you want to catch?
  2. In doWrite, you only write the first thing you pop from the queue. Is that right? The method's docstring appears to suggest it should write everything. Also, if it's a queue and not a stack, why is it pop and not popleft?

Thanks in any case for your awesome contribution to this previously hairier piece of code :)

comment:13 Changed 8 years ago by Jean-Paul Calderone

FWIW, I think we should go ahead and do something with PTYs now, if that would help move this forward.

comment:14 Changed 8 years ago by Jean-Paul Calderone

Also, note that Combinator will not be able to correctly merge the branch associated with this ticket. It will need a manual svn merge command. I suggest someone do that first and then create a new branch with the changes using Combinator.

Note: See TracTickets for help on using tickets.