Opened 21 months ago

Last modified 3 months ago

#6383 task new

Add missing tests for AbstractMaildirDomain and DomainQueuer

Reported by: thijs Owned by: shira
Priority: normal Milestone:
Component: mail Keywords:
Cc: thijs Branch: branches/unit-tests-6383-2
(diff, github, buildbot, log)
Author: shira Launchpad Bug:

Description

#4151 removes startMessage from the twisted.mail.mail.IDomain interface.

startMessage was kept around in twisted.mail.maildir.AbstractMaildirDomain and twisted.mail.relay.DomainQueuer and it:

  • appears that neither remaining startMessage implementation is unit tested (LiveFireExercise does do some integration tests on it).
  • would be good to test that AbstractMaildirDomain.exists and DomainQueuer.exists both return a callable that can be called multiple times to create distinct messages (both in terms of objects, and the underlying storage backing those messages).

Change History (16)

comment:1 Changed 17 months ago by shira

  • Author set to shira
  • Branch set to branches/unit-tests-6383

(In [39428]) Branching to 'unit-tests-6383'

comment:2 Changed 17 months ago by shira

  • Owner set to shira

comment:3 Changed 17 months ago by shira

(In [39470]) Add unit tests and refactor relay rules.

refs #6383

comment:4 Changed 17 months ago by shira

(In [39472]) Fixed missing docstring info.

refs #6383

comment:5 Changed 17 months ago by shira

  • Keywords review added
  • Owner shira deleted

comment:6 Changed 16 months ago by shira

(In [39610]) Add test for non-existent user on AbstractMailDomain

refs #6383

comment:7 Changed 16 months ago by shira

(In [39611]) Removed trailing whitespace

refs #6383

comment:8 Changed 16 months ago by shira

(In [39612]) Fix docstring

refs #6383

comment:9 Changed 16 months ago by shira

(In [39613]) Removed more trailing whitespace

refs #6383

comment:10 Changed 16 months ago by shira

(In [39790]) Revised API documentation

refs #6383

comment:11 Changed 16 months ago by shira

(In [39792]) Fixed bad link reference

refs #6383

comment:12 Changed 16 months ago by shira

(In [39928]) Docstring revisions.

refs #6383

comment:13 Changed 15 months ago by rwall

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

comment:14 Changed 15 months ago by rwall

  • Keywords review removed
  • Owner changed from rwall to shira
  • Status changed from assigned to new

Thanks Shira,

The new docstrings and tests look good. Here are a few things I noticed. I'm not
that familiar with twisted.mail so don't hesitate to disagree with any of things
I've suggested. :)

Notes:

  1. Merges cleanly
  2. Clean build results
    1. https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/unit-tests-6383

Points:

  1. On the face of it, this branch seems to go further than the ticket required. I'm not saying its a bad thing, I just wasn't sure about the motivation for adding the new RelayRules class etc.
    1. Can you write a few notes explaining what you've done and why?
    2. And maybe the new extensible RelayRules API warrants a paragraph in the howto documentation.
    3. And probably the new API means that this should have a .feature news file.
  2. Coverage
    1. coverage run --source=twisted.mail ./bin/trial --reporter=text twisted.mail.test.test_mail
    2. AbstractRelayRules has missing coverage.
  3. source:branches/unit-tests-6383/twisted/mail/relay.py
    1. AbstractRelayRules
      1. The documentation says that this is "a base class for relay rules"
      2. But the concrete class, DomainQueuerRelayRules, doesn't actually inherit from it.
      3. I can see that you do subclass it in the tests, but.
      4. Isn't this an indication that an AbstractRelayRules class is unnecessary?
      5. Or perhaps it's an indication that AbstractRelayRules should be IRelayRules?
      6. Additionally, coverage shows that AbstractRelayRules.willRelay isn't tested. 7.
    2. DomainQueuerRelayRules
      1. This default rules class doesn't have a constructor
      2. So it might just as well be a free function.
    3. DomainQueuer
      1. 84 @ivar authed:
        1. I think you're allowed to write this as "See C{authenticated} parameter of L{init}"
        2. https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto9
      2. 100 @type relayRules: L{NoneType <types.NoneType>} or L{AbstractRelayRules}
        1. I know that L{None} results in a broken link when rendered using the current version of pydoctor. But I think this has been discussed on #twisted-dev and the conclusion was that pydoctor should be fixed. It would drive me nuts if I had to document every None argument like this. Consider changing this to L{None}.
        2. See https://bugs.launchpad.net/pydoctor/+bug/1138975
      3. 106 self.relayRules = relayRules
        1. Consider writing this "if relayRules is None: relayRules = DomainQueuerRelayRules; self.relayRules = relayRules
        2. eg https://twistedmatrix.com/trac/browser/branches/static-path-3765/twisted/web/static.py?rev=39867#L1143
      4. 142 return self.relayRules.willRelay
        1. So instead of supplying a "relayRules" instance, you could just supply the "willRelay" function. It might be a free function, or it might be a bound method, but at least then the user has a choice. 2.
  1. source: branches/unit-tests-6383/twisted/mail/test/test_mail.py
    1. TestMaildirDomain
      1. C{NoneType} > L{None}
    2. AbstractMaildirDomainTestCase.tearDown
      1. I don't *think* you need to remove the root dir. self.mktemp creates it beneath _trial_temp and gets cleaned up after the test run.
      2. And if not, it's probably better to use addCleanup
        1. https://twistedmatrix.com/documents/current/api/twisted.trial.unittest.SynchronousTestCase.html#addCleanup
    3. Use assertIsInstance instead of self.assertTrue(isinstance...
    4. Use assertNotEqual instead of self.assertTrue(msg1 != msg2) etc
    5. Drop the "Verify that" from each of the test docstrings
    6. TestRelayRules
      1. So this does look like a valid case for a RelayRules class, but rather than supply the class, you could write it as...
      2. "DomainQueuer(self.service, False, willRelay=TestRelayRules(True).willRelay)"
  1. DummyTransport
    1. Use @implementer instead of implements(...
    2. Consider using twisted.test.proto_helpers.StringTransport instead
    3. https://twistedmatrix.com/documents/current/core/howto/trial.html#auto6
  2. DummyProtocol
    1. There might be an existing alternative to this too.
    2. Ask someone in #twisted-dev about which are the latest greatest test fakes.
  3. nit: Consider wrapping function arguments on the next line or lining them up.
    995              self.assertFalse(self.rules.willRelay("user@example.com", protocol,
            996                  False))
    

Ok that's all for now. Please re-submit for review after addressing or answering
the numbered points above and posting a link to clean buildbot results.

-RichardW.

comment:15 Changed 3 months ago by shira

  • Branch changed from branches/unit-tests-6383 to branches/unit-tests-6383-2

(In [43199]) Branching to 'unit-tests-6383-2'

comment:16 Changed 3 months ago by shira

(In [43200]) Merge forward and resolve conflict in relay.py

refs #6383

Note: See TracTickets for help on using tickets.