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

[glyph]

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.

[moijes12]

This is a demp patch.

[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 ?

[moijes12]

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

[exarkun]

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.

[moijes12]

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 ?

[moijes12]

Here's another one.

[exarkun]

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!

[moijes12]

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.

[habnabit]

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

[habnabit]

(In [33692]) Applying patch from ticket.

Refs #4386.

[habnabit]

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

Thanks!

[moijes12]

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

[moijes12]

Made the changes to twisted.internet.test_iocp.

[wsanchez]

4386.3 plus the incremental patch looks good.

[wsanchez]

(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