Opened 4 years ago

Closed 4 years ago

#4817 defect closed fixed (fixed)

IPv4Address and UNIXAddress not-equal comparison is broken

Reported by: ivank Owned by:
Priority: normal Milestone:
Component: core Keywords: easy
Cc: awclinford@… Branch: branches/address-not-equal-comparison-4817-2
(diff, github, buildbot, log)
Author: exarkun, awclin Launchpad Bug:

Description

Both IPv4Address and UNIXAddress implement __eq__, but not __ne__.

>>> from twisted.internet.address import IPv4Address
>>> IPv4Address('TCP', '127.0.0.1', 10000) == IPv4Address('TCP', '127.0.0.1', 10000) 
True
>>> IPv4Address('TCP', '127.0.0.1', 10000) != IPv4Address('TCP', '127.0.0.1', 10000) 
True

Attachments (3)

4817-1.patch (3.6 KB) - added by awclin 4 years ago.
4817-2.patch (9.0 KB) - added by awclin 4 years ago.
4817-3.patch (5.2 KB) - added by awclin 4 years ago.
1st patch against address-not-equal-comparison-4817

Download all attachments as: .zip

Change History (26)

comment:1 Changed 4 years ago by ivank

  • Keywords easy added

comment:2 Changed 4 years ago by exarkun

Using twisted.python.util.FancyEqMixin should make this a snap to fix.

Changed 4 years ago by awclin

comment:3 Changed 4 years ago by awclin

  • Cc awclinford@… added
  • Keywords review added

The test returned a failure ('None' result instead of false) prior to the patch, and passed after. I believe I'm testing the explicit problem detailed in the ticket, though not sure if its advisable to increase the coverage in this area as well?

Post patch, i noticed a test failing in twisted/test/test_udp.py which i've updated to reflect the changes to the equality operator.

comment:4 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/address-comparison-4817

(In [30566]) Branching to 'address-comparison-4817'

comment:5 Changed 4 years ago by exarkun

(In [30567]) Apply 4817-1.patch

refs #4817

comment:6 Changed 4 years ago by exarkun

  • Author exarkun deleted
  • Branch branches/address-comparison-4817 deleted
  • Keywords review removed
  • Owner changed from glyph to awclin

Thanks!

  1. Since testOldAddress no longer emits a warning, the suppression for it should be deleted. Also, can you add a docstring and rename it to test_oldAddress? Any time old code is being changed that doesn't conform to some part of the coding standard is a good time to bring it up to date.
  2. Any modified file should have its copyright date updated as well.
  3. In test_address.py, it would probably be good to use assertTrue for the equality case, to make it more obvious that both == and != are being tested.
  4. It looks like UNIXAddress had some untested behavior - the os.path.samepath check when the two names were not equal. We should try to keep this behavior. Can you add a test exercising it and re-add it? Hopefully FancyEqMixin is still usable, but UNIXAddress.__eq__ probably needs to add the extra samefile check if the inherited __eq__ returns False.
  5. This is probably a good time to delete the deprecated __getitem__ and __getslice__ methods on these classes. The change already removes the deprecated comparison to tuple; removing the whole mess at once would be nice.
  6. Can you also add two news fragments for this change? One should describe the bug being fixed (comparison of address objects). The other should mention the removed features - tuple comparison and indexing. News fragments are described here.

I checked the patch into a branch. Please make future patches against that branch.

Thanks!

Changed 4 years ago by awclin

comment:7 Changed 4 years ago by awclin

  • Keywords review added

Thanks for the detailed review, and apologies for missing a couple of things that were documented in the developer guidelines first time around. The new patch addresses all the points you've raised.

comment:8 Changed 4 years ago by awclin

  • Owner awclin deleted

comment:9 Changed 4 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/address-not-equal-comparison-4817

(In [30574]) Branching to 'address-not-equal-comparison-4817'

comment:10 Changed 4 years ago by exarkun

(In [30575]) Apply 4817-1.patch and 4817-2.patch

refs #4817

comment:11 Changed 4 years ago by exarkun

(In [30576]) Apply the rest of them

refs #4817

comment:12 Changed 4 years ago by exarkun

  • Author changed from exarkun to awclin
  • Keywords review removed
  • Owner set to awclin

Thanks! I noticed a couple more things:

  1. _bwHack looks like it's totally obsolete now. I think we can delete the attribute and the related comments. The __init__ parameter should stay, I guess, and be deprecated separately: /
  2. test_differentNamesComparison looks like something we should be applying to all of the address types. There could be a buildDifferentAddress hook in addition to buildAddress and the base class could define a test that asserts that the two addresses are not equal to each other.

I checked the patches into a branch, so please make the next patch against that branch. Thanks again!

comment:13 Changed 4 years ago by awclin

  • Keywords review added
  • Owner awclin deleted

comments addressed

Changed 4 years ago by awclin

1st patch against address-not-equal-comparison-4817

comment:14 Changed 4 years ago by exarkun

  • Author changed from awclin to exarkun, awclin
  • Branch changed from branches/address-not-equal-comparison-4817 to branches/address-not-equal-comparison-4817-2

(In [30643]) Branching to 'address-not-equal-comparison-4817-2'

comment:15 Changed 4 years ago by exarkun

(In [30645]) Apply 4817-3.patch

refs #4817

comment:16 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

Thanks!

  1. Whenever we introduce deprecations, we should make sure that Twisted itself doesn't trigger them (we've failed to do this in the past, and the results are annoyingly visible in the output of trial twisted). So in this case, all of the places in Twisted that instantiate an address should stop passing the final argument.
  2. Test method docstrings don't need to say "Assert ..." or "Ensure ...". The docstrings should just explain the feature being tested.
  3. The new test methods and helpers should have docstrings as well.
  4. UNIXAddressTestCase.buildDifferentAddress should probably return a fixed address, like the IPv4 cases do.
  5. lines should be less than 80 columns

comment:17 Changed 4 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

I addressed these points in r30646.

comment:19 Changed 4 years ago by exarkun

(In [30647]) Conditionally define UNIXAddress.eq, only if samefile is defined (so, not on Windows).

refs #4817

comment:20 Changed 4 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

The samefile check seems a bit weird to me; won't this change depending on the presence of symlinks and the state of the real filesystem? The TCP addresses don't attempt to determine if their IPs are different interfaces on the same host.

But, I'll trust your judgement if you want to leave it as-is; it otherwise looks fine to merge.

comment:21 Changed 4 years ago by exarkun

The samefile doesn't seem too awesome to me, but since it was there before I want to leave it for now. We may want to get rid of it soon though. I don't think it can ever actually be invoked.

comment:22 Changed 4 years ago by exarkun

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

(In [30743]) Merge address-not-equal-comparison-4817-2

Author: awclin, exarkun
Reviewer: exarkun, glyph
Fixes: #4817

Remove the deprecated support for indexing IPv4Address and UNIXAddress instances.
Also deprecate passing the backwards compatibility hack argument to those classes and
stop doing it everywhere in Twisted.

comment:23 Changed 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.