Opened 4 years ago

Closed 3 years ago

#4386 defect closed fixed (fixed)

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

Reported by: glyph Owned by: wsanchez
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/filedescriptor-interfaces-4386
(diff, github, buildbot, log)
Author: habnabit Launchpad Bug:

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 3 years ago.
This is a demp patch.
4386.patch (1.9 KB) - added by moijes12 3 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 3 years ago.
Here's another one.
4386.3.patch (3.9 KB) - added by moijes12 3 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 3 years ago.
All calls to verifyClass only changed assertTrue(verifyClass(...))

Download all attachments as: .zip

Change History (20)

comment:1 Changed 4 years ago by glyph

  • Type changed from enhancement to defect

comment:2 Changed 4 years ago by <automation>

  • Owner glyph deleted

Changed 3 years ago by moijes12

This is a demp patch.

comment:3 Changed 3 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 3 years ago by moijes12

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

comment:5 Changed 3 years ago by exarkun

  • 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 3 years ago by 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 ?

comment:6 Changed 3 years ago by moijes12

  • Keywords review added

Changed 3 years ago by moijes12

Here's another one.

comment:7 Changed 3 years ago by exarkun

  • 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 3 years ago by 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.

comment:8 Changed 3 years ago by moijes12

  • Keywords review added

comment:9 Changed 3 years ago by thijs

  • Owner moijes12 deleted

comment:10 Changed 3 years ago by habnabit

  • Author set to habnabit
  • Branch set to branches/filedescriptor-interfaces-4386

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

comment:11 Changed 3 years ago by habnabit

(In [33692]) Applying patch from ticket.

Refs #4386.

comment:12 Changed 3 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 3 years ago by moijes12

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

comment:13 Changed 3 years ago by moijes12

  • Keywords review added
  • Owner moijes12 deleted

Made the changes to twisted.internet.test_iocp.

comment:14 Changed 3 years ago by wsanchez

  • Keywords review removed
  • Owner set to wsanchez
  • Status changed from new to assigned

4386.3 plus the incremental patch looks good.

comment:15 Changed 3 years ago by wsanchez

  • Resolution set to fixed
  • Status changed from assigned to closed

(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.