Opened 4 years ago

Last modified 2 years ago

#6383 task new

Add missing tests for AbstractMaildirDomain and DomainQueuer

Reported by: Thijs Triemstra Owned by: Stacey Sern
Priority: normal Milestone:
Component: mail Keywords:
Cc: Thijs Triemstra Branch: branches/unit-tests-6383-2
branch-diff, diff-cov, branch-cov, buildbot
Author: shira

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 3 years ago by Stacey Sern

Author: shira
Branch: branches/unit-tests-6383

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

comment:2 Changed 3 years ago by Stacey Sern

Owner: set to Stacey Sern

comment:3 Changed 3 years ago by Stacey Sern

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

refs #6383

comment:4 Changed 3 years ago by Stacey Sern

(In [39472]) Fixed missing docstring info.

refs #6383

comment:5 Changed 3 years ago by Stacey Sern

Keywords: review added
Owner: Stacey Sern deleted

comment:6 Changed 3 years ago by Stacey Sern

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

refs #6383

comment:7 Changed 3 years ago by Stacey Sern

(In [39611]) Removed trailing whitespace

refs #6383

comment:8 Changed 3 years ago by Stacey Sern

(In [39612]) Fix docstring

refs #6383

comment:9 Changed 3 years ago by Stacey Sern

(In [39613]) Removed more trailing whitespace

refs #6383

comment:10 Changed 3 years ago by Stacey Sern

(In [39790]) Revised API documentation

refs #6383

comment:11 Changed 3 years ago by Stacey Sern

(In [39792]) Fixed bad link reference

refs #6383

comment:12 Changed 3 years ago by Stacey Sern

(In [39928]) Docstring revisions.

refs #6383

comment:13 Changed 3 years ago by Richard Wall

Owner: set to Richard Wall
Status: newassigned

comment:14 Changed 3 years ago by Richard Wall

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

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 2 years ago by Stacey Sern

Branch: branches/unit-tests-6383branches/unit-tests-6383-2

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

comment:16 Changed 2 years ago by Stacey Sern

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

refs #6383

Note: See TracTickets for help on using tickets.