Opened 4 years ago

Last modified 2 years ago

#6638 defect new

AddressAlias.createMessageReceiver() returns wrong value

Reported by: Stacey Sern Owned by: Stacey Sern
Priority: normal Milestone:
Component: mail Keywords:
Cc: Branch: branches/createMessageReceiver-return-6638-2
branch-diff, diff-cov, branch-cov, buildbot
Author: shira

Description

AddressAlias.createMessageReceiver() was changed under ticket #4151 to eliminate a call to the deprecated IDomain.startMessage(). The call was replaced with a call to IDomain.exists(). However, startMessage() returned an IMessage provider. exists() returns a no-argument callable which returns an IMessage provider. createMessageReceiver needs to call the result of the call to exists() to get the proper result.

Change History (17)

comment:1 Changed 4 years ago by Stacey Sern

Owner: set to Stacey Sern

comment:2 Changed 4 years ago by Stacey Sern

Author: shira
Branch: branches/createMessageReceiver-return-6638

(In [39396]) Branching to 'createMessageReceiver-return-6638'

comment:3 Changed 4 years ago by Stacey Sern

(In [39408]) Fixed createMessageReceiver and unit tests.

refs #6638

comment:4 Changed 4 years ago by Stacey Sern

Additionally, I changed the argument to exists from a string to User which it expects.

In test_mail.py, I removed DummyDomain and changed the AliasAddressTests to use TestDomain which had to be modified slightly. I removed AddressAliasTests.test_resolve and AddressAliasTests.test_resolveWithoutAliasMap because the AliasAddress.resolve is being well tested by ProcessAliasTestCase.test_resolution. I modified TestDomain.exists to return TestMessage, which is an IMessage provider, when the address is local instead of AddressAlias.

comment:5 Changed 4 years ago by Stacey Sern

(In [39416]) Changed string formatting.

refs #6638

comment:6 Changed 4 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

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

Keywords: review removed
Owner: set to Stacey Sern

Thanks for your work on this shira. Sorry about the long delay in getting this reviewed.

  1. twisted/mail/alias.py
    1. IAlias and IAlias.createMessageReceiver are badly in need of docstrings. This seems like a pretty good branch in which to add them.
  2. twisted/mail/test/test_mail.py
    1. TestMessage (and all other new code) should use the new style of declaring what interface it implements, the zope.interface.implementer class decorator
    2. It's not necessary to repeat the documentation for the user __init__ parameter and user instance attribute. Just document the __init__ parameter.
    3. New documentation should almost always avoid talking about str - instead talk about bytes or about unicode (most existing Twisted APIs that talk about str should be talking about bytes now, but not all - each case needs to be considered individually).
    4. There's some trailing whitespace that should be cleaned up in TestMessage.__str__ (but see below)
    5. And there should be two blank lines between methods in TestMessage
    6. TestCase.patch is a good replacement for what setUp and tearDown are doing with DNSNAME.
    7. test_createMessageReceiver verifies that it gets the expected object by comparing the string representations of two objects. It would be better to compare the objects themselves. Consider implementing equality on TestMessage instead of __str__ (see twisted.python.util.FancyEqMixin to make that task easier)
    8. When using %s-style string interpolation, always interpolate a tuple. For example, test_str should do '<Address %s>' % (str(self.destination),) or, more simply, '<Address %s>' % (self.destination,) (since "%s" already applies str).
    9. There is a commented-out line in TestDomain that should be deleted (all old code should be deleted, not commented out).
    10. Some test coverage for AddressAlias.resolve has been lost in this branch. in trunk all lines and branches are executed, in the branch this is not the case - http://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r39416/twisted_mail_alias.html

Thanks again.

comment:8 Changed 4 years ago by Stacey Sern

(In [39853]) Address review comments

refs #6638

comment:9 Changed 4 years ago by Stacey Sern

(In [39854]) Fixed twistedchecker errors

refs #6638

comment:10 Changed 4 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

Everything has been fixed and tests were added to cover AddressAlias.resolve.

smtp.DNSNAME didn't need to be patched since creating addresses that have no domain with as the default domain achieves the same effect.

comment:11 Changed 4 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:12 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: changed from Richard Wall to Stacey Sern
Status: assignednew

Notes:

  • Merges cleanly
  • Forced a build:
  • Checked that tests fail after reverting createMessageReceiver. They do.
    [FAIL]
    Traceback (most recent call last):
      File "/home/richard/projects/Twisted/branches/createMessageReceiver-return-6638/twisted/mail/test/test_mail.py", line 1819, in test_createMessageReceiver
        self.assertEqual(messageReceiver, TestMessage(str(self.user2)))
      File "/home/richard/projects/Twisted/branches/createMessageReceiver-return-6638/twisted/trial/_synctest.py", line 356, in assertEqual
        % (msg, pformat(first), pformat(second)))
    twisted.trial.unittest.FailTest: not equal:
    a = <function <lambda> at 0x2865578>
    b = <twisted.mail.test.test_mail.TestMessage instance at 0x2985f80>
    

Points:

  1. source:branches/createMessageReceiver-return-6638/twisted/mail/test/test_mail.py
    1. TestMessage
      1. Maybe worth sticking a zope.interface.verify.verifyClass to ensure that this fake properly implements the interface. (It looks like it does).
      2. The __init__ docstring says the user is supplied as bytes, but you then convert it using str. Maybe bytes(smtp.Address(user)) would be more explicit? Not sure.
      3. __str__ You could mixin FancyStrMixin to create this for you automatically in a standardised form.
      4. lineReceived: The pass statement is unnecessary, and maybe the docstring should say this is a noop and that the argument is discarded.
      5. eomReceived: The pass statement isn't needed and the @rtype is wrong. In this case, nothing is returned. Is that ok?
    2. AddressAliasTests.setUp
      1. I find this wrapping difficult to read. I'd rather all the arguments on the line below or wrapped beneath self. Not sure if it's in the coding standard though.
         1804         self.alias1to2 = mail.alias.AddressAlias(self.user2, domains,
         1805             user1)
        
      2. AddressAlias.test_createMessageReceiver
        1. Use assertEqual and exarkun keeps telling me to put the expected result as the first argument, which I'm getting used to now.
        2. L{createMessageReceiver} isn't a valid reference...but I guess it doesn't matter since pydoctor won't be rendering these docstrings.
        3. "should get" -> "results in" (see #6301 (you probably already have))
        4. Full stop
      3. test_str
        1. "should include" -> "includes"
        2. And actually aren't we really testing the repr here. I'd have thought the str (if different) would be just an email address.
      4. test_resolve
        1. "should" again.
        2. I think the original docstring was better in that it explicitly mentions L{None}
      5. test_resolveWithAliasmap
        1. Use assertEqual or assertIdentical (since it's testing for None)
    3. ProcessAliasTestCase.test_aliasResolution
      1. We're still using string comparison here (exarkun mentioned it in previous review) but it always did and I guess that could be because MessageWrapper and FileWrapper don't support rich comparison. It would be nice if those objects could be compared directly though.
    4. TestDomain.exists
      1. Needs a docstring (unless this fake should actually be implementing a documented interface?)
      2. Also fix the whitespace before the class
  2. source:branches/createMessageReceiver-return-6638/twisted/mail/alias.py
    1. IAlias: Add standard 3 lines above and below "class IAlias"
    2. IAlias.createMessageReceiver
      1. I'd like to see a little more description in the docstring. What is this message receiver and how does it relate to this AddressAlias?
      2. No need for the "pass" statement since there's a docstring. https://buildbot.twistedmatrix.com/builds/twisted-coverage.py/twisted-coverage.py-r39854/twisted_mail_alias.html#n104
    3. AddressAlias.createMessageReceiver
      1. Add 2 blank lines above and below

Most of this is trivial nit picking stuff. Exarkun seemed happy with the change to createMessageReceiver in his previous review and it makes sense to me.

So I think you should go ahead and merge this after addressing or answering the numbered points above and linking to some clean build results.

Thanks again shira.

comment:13 Changed 3 years ago by Stacey Sern

Branch: branches/createMessageReceiver-return-6638branches/createMessageReceiver-return-6638-2

(In [43004]) Branching to 'createMessageReceiver-return-6638-2'

comment:14 Changed 3 years ago by Stacey Sern

Keywords: review added
Owner: changed from Stacey Sern to Glyph

I addressed issues identified in comment:12. The changeset is http://twistedmatrix.com/trac/changeset/43027.

comment:15 Changed 3 years ago by Stacey Sern

(In [43032]) Fix Twisted checker problems

refs #6638

comment:16 Changed 3 years ago by Glyph

Owner: Glyph deleted

Un-assigning all review tickets assigned to me. Unless there's a specific reason I need to review something, let's leave them unassigned :-).

comment:17 Changed 2 years ago by Glyph

Keywords: review removed
Owner: set to Stacey Sern

Hi Shira,

I apologize, but it has been long enough now that this doesn't merge cleanly so I can't run it on the buildbot. Can you merge forward and resolve the conflicts?

Thanks, and sorry that long review latency causes these problems; we'll try to get to it sooner next time :).

-glyph

Note: See TracTickets for help on using tickets.