Opened 23 months ago

Last modified 15 months ago

#6064 defect new

Write tests for `twisted.internet.protocol.FileWrapper`

Reported by: itamar Owned by: kaizhang
Priority: normal Milestone:
Component: core Keywords:
Cc: kylerzhang11@… Branch: branches/FileWrapper-test-6064
(diff, github, buildbot, log)
Author: tomprince Launchpad Bug:

Description

twisted.internet.protocol.FileWrapper has no tests. We should write some, and may as well make them pass on Python 3 while we're t it.

Attachments (1)

6064.patch (5.3 KB) - added by kkpattern 16 months ago.
Add tests for twisted.internet.protocol.FileWrapper.

Download all attachments as: .zip

Change History (8)

comment:1 follow-up: Changed 23 months ago by itamar

Alternatively, it could be deprecated and its internal users switched to twisted.test.proto_helpers.StringTransport.

comment:2 in reply to: ↑ 1 Changed 16 months ago by kkpattern

Replying to itamar:

Alternatively, it could be deprecated and its internal users switched to twisted.test.proto_helpers.StringTransport.

Hi, since I'm applying for working on Twisted through Google Summer of Code, I think writing tests for twisted.internet.protocol.FileWrapper could be a good opportunity for me to get familiar with developing in Twisted. So I want to work on this ticket as my second patch for Twisted. I hope it's OK.

Changed 16 months ago by kkpattern

Add tests for twisted.internet.protocol.FileWrapper.

comment:3 Changed 16 months ago by kkpattern

  • Cc kylerzhang11@… added
  • Keywords review added

comment:4 Changed 15 months ago by tomprince

  • Author set to tomprince
  • Branch set to branches/FileWrapper-test-6064

(In [38731]) Branching to FileWrapper-test-6064.

comment:5 Changed 15 months ago by tomprince

(In [38732]) Apply 6064.patch from kkpattern.

Refs: #6064

comment:6 Changed 15 months ago by tom.prince

  • Keywords review removed
  • Owner set to kaizhang

There are a few docstring changes, and a couple of places where the test try to match the existing behavior too closely (but understandab ly so, given the lack of documentation. But overall, this looks good. Thanks for your contribution.

  1. FakeFile: The docstrings should have epytext markup for instance variables and parameters.
  2. *Old* versions of python didn't have constants for true or false, and there is some twisted code that dates from then. FileWrapper.closed is a boolean value, so should be tested as such.
  3. assertConnectionLost and assertProducerUnregisted should specify what "succesfully" means.
  4. You don't test any of the exception cases.
  5. streamingProducer and the streaming attribute of registerProducer are boolean, and so should be tested as such.

Please resubmit for review after addressing the above changes.

comment:7 Changed 15 months ago by tom.prince

After looking at #6135, I see that FakeFile is copied from twisted/conch/test/test_knownhosts.py and used both here and there. It would make sense to move it somewhere common (like twisted/test/testutils.py).

Note: See TracTickets for help on using tickets.