Opened 13 years ago

Closed 9 years ago

#2838 defect closed fixed (fixed)

_dumbwin32proc.Process does not implement IProcessTransport completely

Reported by: Peaker Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords:
Cc: therve Branch: branches/win32-proc-missing-2838-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

writeToChild(childFD, data) is missing from the implementation of _dumbwin32proc

Attachments (3)

_dumwin32proc.2.py_diff (1.2 KB) - added by Michael Schneider 13 years ago.
_dumwin32proc.py_diff (1.9 KB) - added by Michael Schneider 13 years ago.
test_process.py_diff (3.3 KB) - added by Michael Schneider 13 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 13 years ago by Michael Schneider

Owner: changed from Glyph to Michael Schneider
Status: newassigned

Changed 13 years ago by Michael Schneider

Attachment: _dumwin32proc.2.py_diff added

comment:2 Changed 13 years ago by Michael Schneider

Keywords: review added
Owner: Michael Schneider deleted
Status: assignednew

comment:3 Changed 13 years ago by therve

Summary: _dumbwin32proc does not IProcessTransport make_dumbwin32proc.Process does not implement IProcessTransport completely

comment:4 Changed 13 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Michael Schneider
  • the new test should be named test_writeToChild
  • the SkipTest is not necessary, the skip is already made at class level
  • missing spaces after commas and around equal operator
  • assertRaises doesn't take a message argument
  • one thing that would be cool is to use zope.interface.verify.verifyClass in another test to check that the class isn't missing something else

Thanks!

Changed 13 years ago by Michael Schneider

Attachment: _dumwin32proc.py_diff added

comment:5 Changed 13 years ago by Michael Schneider

Keywords: review added
Owner: Michael Schneider deleted

Therve,

Thank you, I am slowly learning the twisted way :-)

Thanks for the tip about verify, I added a test case to validate that Process implements all functions defined in it's interfaces, and that test identified two additional functions.

getPeer and getHost.

I added those functions, and tests.

Changed 13 years ago by Michael Schneider

Attachment: test_process.py_diff added

comment:6 Changed 12 years ago by Glyph

Keywords: review removed
Owner: set to Michael Schneider

Please remove the local imports and move the inner classes to a lower level of indentation, plus, explain what you're faking and how much of it you're faking.

It looks like maybe you could fake a lot less stuff if you split the massive __init__ into (individually tested) pieces; at the very least, an initializer that sets up the appropriate state a "start" method.

It turns out that the UNIX stuff doesn't actually declare that it implements() IProcessTransport either, so if you could file a ticket for that it would be great, and probably easier to get started with :).

comment:7 Changed 12 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/win32-proc-missing-2838

(In [24103]) Branching to 'win32-proc-missing-2838'

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

(In [24104]) apply _dumwin32proc.py_diff and test_process.py_diff

refs #2838

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

I'll probably get back to this after #1291 is merged. Of course, feel free to submit more patches (but make them against the branch instead of against trunk).

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

Branch: branches/win32-proc-missing-2838branches/win32-proc-missing-2838-2

(In [32704]) Branching to 'win32-proc-missing-2838-2'

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

Keywords: review added
Owner: Michael Schneider deleted

The branch now only adds writeToChild, not getHost and getPeer. See #1124 for those.

Build results

comment:12 Changed 9 years ago by Glyph

Owner: set to Jean-Paul Calderone

Looks pretty good to me.

One thing to consider: on other platforms, writeToChild will raise a KeyError when an invalid file descriptor is passed. This error behavior is accidental, and possibly not the best thing that could be done, but I think it would be best to remain consistent with twisted.internet.process in this code as much as possible. Mostly I noticed this because I don't like docstrings that say "only XYZ is supported"; it reminds me of the C standard's "undefined behavior". If it's an error, we should detect it, and say what kind of error it is.

I can see that NotImplementedError might be a clearer thing for the maintainer of code calling this API to see. In either case, it should say what exception is raised.

Land at your discretion.

comment:13 Changed 9 years ago by Glyph

Keywords: review removed

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

Yea, good call, thanks. I changed it to KeyError, updated the interface documentation, and added a test for the exceptional case (r32730).

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

Resolution: fixed
Status: newclosed

(In [32731]) Merge win32-proc-missing-2838-2

Author: bigdog, exarkun Reviewer: therve, glyph Fixes: #2838

Implement IProcessTransport.writeToChild on Windows.

Note: See TracTickets for help on using tickets.