Opened 2 years ago

Last modified 2 days ago

#6176 enhancement new

Port twisted.cred.credentials to Python 3

Reported by: itamar Owned by:
Priority: lowest Milestone: Python-3.x
Component: core Keywords: review
Cc: Branch: branches/tccredentials-py3-6176-2
(diff, github, buildbot, log)
Author: hawkowl Launchpad Bug:

Description

twisted.cred.credentials should run on Python 3.

Change History (11)

comment:1 Changed 21 months ago by ihrachyshka

  • Owner set to ihrachyshka
  • Status changed from new to assigned

comment:2 Changed 21 months ago by ihrachyshka

  • Owner ihrachyshka deleted
  • Status changed from assigned to new

This also depends on #5203 being fixed (unicode vs. str in py3k is not considered in filepath code).

comment:3 Changed 7 days ago by hawkowl

  • Owner set to hawkowl

comment:4 Changed 7 days ago by hawkowl

  • Author set to hawkowl
  • Branch set to branches/tccredentials-py3-6176

(In [44283]) Branching to tccredentials-py3-6176.

comment:5 Changed 7 days ago by hawkowl

  • Owner hawkowl deleted

Buildbots are green, coverage looks good, please review.

comment:6 Changed 7 days ago by hawkowl

  • Keywords review added

comment:7 Changed 5 days ago by adiroiban

  • Keywords review removed
  • Owner set to hawkowl

Thanks for working on this. Changes looks good.

Thistedchecker is red... at least networkFormat needs a docstring


I know that this is not specific to current changes.

I am a bit nervous while reviewing this as I see that some tests just reuse the logic from the implementation so for example let's say that binascii.hexlify is broken, than the test will still pass as it uses the same broken implementation. Most of the time this is ok, but not for code related to authentication.

So instead of computing expected in test code using the same methods and logic from the implementation I was expecting to see hardcoded expected values.


just minor comments

  1. There is no news file.
  1. Hope that md5-sess fix will land soon and we can remove it from the diff
  1. I see the logic for __all3__ duplicated in many test module, maybe we need to refactor it into a helper method... not what all tests suites ends with 'Tests' maybe we can have something like `partialPy3Ported('SomeTests', 'AnotherTests') and the code can automatically remove all other tests suites.
  1. For class Testable(components.Adapter): maybe instead of pass we can add a docstring.
  1. Maybe you can implement networkFormat as networkFormat(format, *args) so that we don't need to put the tuple each time.

Thanks!

Please merge md5-sess fix and fix twistedchecker + newsfile and then resubmit for review.

comment:8 Changed 3 days ago by hawkowl

  • Branch changed from branches/tccredentials-py3-6176 to branches/tccredentials-py3-6176-2

(In [44322]) Branching to tccredentials-py3-6176-2.

comment:9 Changed 2 days ago by hawkowl

  • Keywords review added

Thanks Adi!

  • Fixed the TwistedChecker errors.
  • Added a newsfile.
  • Removed the md5-sess from the diff when I merged forward.
  • Reimplemented networkFormat as a bytestring join operation, since it's much faster, and doesn't do encoding dances as much.
  • I think args vs *args is clearer... not sure.

Builders spun, please review.

comment:10 Changed 2 days ago by hawkowl

  • Owner hawkowl deleted

comment:11 Changed 2 days ago by adiroiban

Changes looks good and I am +1 for merging them.

The newsfile is a bit vague to me. Maybe it should talk about the credential checkers available on Py3 as not all credential checkers are available yet.


I am still not happy with code duplication from __all3__ part and the manual work need to update the __all__ list. I don't think that this should stop this merge.

What do you say if we have something like this in compat... implemented in a separate ticket?

# Defined in compat
def testSuitesPortedToPython3(py3Names):
    """
    Should be call at the end of a module.
    """
    allNames = []
    for key, value in globals().items():
        if value is unittest.TestCase:
            continue
        if isinstance(value, unittest.TestCase):
            allNames.append(key)

    if not allNames:
        raise AssertionError('No test suite. Did you call this at the end of the module?')

    for name in allNames:
        if name not in py3Names:
            del globals()[name]


# called at the end of test module
testSuitesPortedToPython3(["CramMD5CredentialsTestCase"])

since this is security related code I will leave this for another dev to review it.

Thanks!

Note: See TracTickets for help on using tickets.