Add missing tests for AbstractMaildirDomain and DomainQueuer

[Thijs Triemstra]

#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).

[Stacey Sern]

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

[Stacey Sern]

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

refs #6383

[Stacey Sern]

(In [39472]) Fixed missing docstring info.

refs #6383

[Stacey Sern]

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

refs #6383

[Stacey Sern]

(In [39611]) Removed trailing whitespace

refs #6383

[Stacey Sern]

(In [39612]) Fix docstring

refs #6383

[Stacey Sern]

(In [39613]) Removed more trailing whitespace

refs #6383

[Stacey Sern]

(In [39790]) Revised API documentation

refs #6383

[Stacey Sern]

(In [39792]) Fixed bad link reference

refs #6383

[Stacey Sern]

(In [39928]) Docstring revisions.

refs #6383

[Richard Wall]

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.

4. 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)"

7. 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
8. 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.
9. 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.

[Stacey Sern]

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

[Stacey Sern]

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

refs #6383