Opened 7 years ago

Closed 5 years ago

#4386 defect closed fixed (fixed)

twisted.internet.abstract.FileDescriptor says implements(IProducer) but means implements(IPushProducer)

Reported by: Glyph Owned by: Wilfredo Sánchez Vega
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/filedescriptor-interfaces-4386
branch-diff, diff-cov, branch-cov, buildbot
Author: habnabit

Description

This leads to some very unfortunate gaffes, like this one:

>>> from twisted.internet.tcp import Server
>>> from twisted.internet.interfaces import IPushProducer
>>> IPushProducer.implementedBy(Server)
False

tcp.Server is very definitely supposed to implement IPushProducer; if it doesn't, it would be nearly impossible to implement any other push producers.

Attachments (5)

demo.patch (1.1 KB) - added by moijes12 6 years ago.
This is a demp patch.
4386.patch (1.9 KB) - added by moijes12 6 years ago.
Please find attached the patch. I felt I should have created class FiledescriptorTests which contains tests for writeSequence and checks for implementing IPushProducer. Would something like that be ok ?
4386.2.patch (2.0 KB) - added by moijes12 6 years ago.
Here's another one.
4386.3.patch (3.9 KB) - added by moijes12 6 years ago.
Please find attached the patch. However, the tests for the changes made to iocpreactor were skipped so I'm not sure how they will behave. If anyone can, please tell me what I require to run the tests.
4386.patch.patch (892 bytes) - added by moijes12 5 years ago.
All calls to verifyClass only changed assertTrue(verifyClass(...))

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by Glyph

Type: enhancementdefect

comment:2 Changed 7 years ago by <automation>

Owner: Glyph deleted

Changed 6 years ago by moijes12

Attachment: demo.patch added

This is a demp patch.

comment:3 Changed 6 years ago by moijes12

How about something like the patch.

One query : I've only added a test to check if Server implements IPushProducer. Should tests be added for every class that directly or indirectly inherites abstract.FileDescriptor ?

comment:4 Changed 6 years ago by moijes12

I meant "demo" and not "demp" patch.

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

Owner: set to moijes12

Thanks for working on this. Please add the review keyword when you're ready for feedback, even if you're not certain the patch is perfect.

  1. I think we'll be okay with just a unit test for FileDescriptor implementing this interface. It wouldn't be terrible to make a similar assertion for every other class which is also supposed to implement IPushProducer, but we can rely on inheritance to work properly, so if FileDescriptor implements the proper interface so will all of its subclasses.
  2. The test should use zope.interface.verify.verifyClass or zope.interface.verify.verifyObject instead.
  3. Since it will be a test for FileDescriptor, it can go in twisted/internet/test/test_filedescriptor.py.
  4. This merits a bug fix news fragment.

Thanks again.

Changed 6 years ago by moijes12

Attachment: 4386.patch added

Please find attached the patch. I felt I should have created class FiledescriptorTests which contains tests for writeSequence and checks for implementing IPushProducer. Would something like that be ok ?

comment:6 Changed 6 years ago by moijes12

Keywords: review added

Changed 6 years ago by moijes12

Attachment: 4386.2.patch added

Here's another one.

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

Keywords: review removed

Thanks.

I felt I should have created class FiledescriptorTests which contains tests for writeSequence and checks for implementing IPushProducer.

I'm satisfied with this change, yes. If FileDescriptorWriteSequenceTests had contained more tests, or had contained only tests for FileDescript.writeSequence, then I would have recommended leaving it alone and adding a brand new TestCase subclass for the new test method. However, since it only had one writeSequence test, and it also had a write test, I think it was misnamed, and it works better as a general container for FileDescriptor tests.

  1. test_fileDescriptorImplementsIPushProducer is a little verbose, though. It's already clear we're testing FileDescriptor, since the test method belongs to FileDescriptorTests.
  2. The news fragment should be a .bugfix fragment instead of a .misc fragment. This is a nice fix and we'll want it to show up in the news file. .misc fragments just have ticket numbers listed, they don't get any descriptive text.
  3. twisted.internet.iocpreactor.abstract.FileHandle has the same bug. It implements IPushProducer but only claims to implement IProducer. Can you add a similar fix for it?

Thanks again!

Changed 6 years ago by moijes12

Attachment: 4386.3.patch added

Please find attached the patch. However, the tests for the changes made to iocpreactor were skipped so I'm not sure how they will behave. If anyone can, please tell me what I require to run the tests.

comment:8 Changed 6 years ago by moijes12

Keywords: review added

comment:9 Changed 5 years ago by Thijs Triemstra

Owner: moijes12 deleted

comment:10 Changed 5 years ago by habnabit

Author: habnabit
Branch: branches/filedescriptor-interfaces-4386

(In [33691]) Branching to 'filedescriptor-interfaces-4386'

comment:11 Changed 5 years ago by habnabit

(In [33692]) Applying patch from ticket.

Refs #4386.

comment:12 Changed 5 years ago by habnabit

Keywords: review removed
Owner: set to moijes12

Tests pass on iocpreactor, but the tests in t.i.test.test_iocp should be doing self.assertTrue(verifyClass(...)) instead of just verifyClass(...).

Thanks!

Changed 5 years ago by moijes12

Attachment: 4386.patch.patch added

All calls to verifyClass only changed assertTrue(verifyClass(...))

comment:13 Changed 5 years ago by moijes12

Keywords: review added
Owner: moijes12 deleted

Made the changes to twisted.internet.test_iocp.

comment:14 Changed 5 years ago by Wilfredo Sánchez Vega

Keywords: review removed
Owner: set to Wilfredo Sánchez Vega
Status: newassigned

4386.3 plus the incremental patch looks good.

comment:15 Changed 5 years ago by Wilfredo Sánchez Vega

Resolution: fixed
Status: assignedclosed

(In [33856]) Apple 4386.3.patch and 4386.patch.patch from #4386: FileDescriptor says implements(IProducer) but means implements(IPushProducer)

Author: moijes12 Reviewer: wsanchez Fixes: #4386

Note: See TracTickets for help on using tickets.