Ticket #6383 task new

Opened 13 months ago

Last modified 7 months ago

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
(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

1

Changed 9 months ago by shira

  • branch set to branches/unit-tests-6383
  • branch_author set to shira

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

2

Changed 9 months ago by shira

  • owner set to shira

3

Changed 9 months ago by shira

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

refs #6383

4

Changed 9 months ago by shira

(In [39472]) Fixed missing docstring info.

refs #6383

5

Changed 9 months ago by shira

  • keywords review added
  • owner shira deleted

6

Changed 8 months ago by shira

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

refs #6383

7

Changed 8 months ago by shira

(In [39611]) Removed trailing whitespace

refs #6383

8

Changed 8 months ago by shira

(In [39612]) Fix docstring

refs #6383

9

Changed 8 months ago by shira

(In [39613]) Removed more trailing whitespace

refs #6383

10

Changed 8 months ago by shira

(In [39790]) Revised API documentation

refs #6383

11

Changed 8 months ago by shira

(In [39792]) Fixed bad link reference

refs #6383

12

Changed 7 months ago by shira

(In [39928]) Docstring revisions.

refs #6383

13

Changed 7 months ago by rwall

  • owner set to rwall
  • status changed from new to assigned

14

Changed 7 months ago by rwall

  • owner changed from rwall to shira
  • keywords review removed
  • 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.

Note: See TracTickets for help on using tickets.