Opened 5 years ago
Last modified 4 years ago
#6383 task new
Add missing tests for AbstractMaildirDomain and DomainQueuer
Reported by: | Thijs Triemstra | Owned by: | Stacey Sern |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | 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
andDomainQueuer.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 5 years ago by
Author: | → shira |
---|---|
Branch: | → branches/unit-tests-6383 |
comment:2 Changed 5 years ago by
Owner: | set to Stacey Sern |
---|
comment:5 Changed 5 years ago by
Keywords: | review added |
---|---|
Owner: | Stacey Sern deleted |
comment:6 Changed 5 years ago by
comment:13 Changed 5 years ago by
Owner: | set to Richard Wall |
---|---|
Status: | new → assigned |
comment:14 Changed 5 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Richard Wall to Stacey Sern |
Status: | assigned → 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:
- Merges cleanly
- Clean build results
Points:
- 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.
- Can you write a few notes explaining what you've done and why?
- And maybe the new extensible RelayRules API warrants a paragraph in the howto documentation.
- And probably the new API means that this should have a .feature news file.
- Coverage
- coverage run --source=twisted.mail ./bin/trial --reporter=text twisted.mail.test.test_mail
- AbstractRelayRules has missing coverage.
- source:branches/unit-tests-6383/twisted/mail/relay.py
- AbstractRelayRules
- The documentation says that this is "a base class for relay rules"
- But the concrete class, DomainQueuerRelayRules, doesn't actually inherit from it.
- I can see that you do subclass it in the tests, but.
- Isn't this an indication that an AbstractRelayRules class is unnecessary?
- Or perhaps it's an indication that AbstractRelayRules should be IRelayRules?
- Additionally, coverage shows that AbstractRelayRules.willRelay isn't tested. 7.
- DomainQueuerRelayRules
- This default rules class doesn't have a constructor
- So it might just as well be a free function.
- DomainQueuer
84 @ivar authed:
- I think you're allowed to write this as "See C{authenticated} parameter of L{init}"
- https://twistedmatrix.com/documents/current/core/development/policy/coding-standard.html#auto9
100 @type relayRules: L{NoneType <types.NoneType>} or L{AbstractRelayRules}
- 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}.
- See https://bugs.launchpad.net/pydoctor/+bug/1138975
106 self.relayRules = relayRules
- Consider writing this "if relayRules is None: relayRules = DomainQueuerRelayRules; self.relayRules = relayRules
- eg https://twistedmatrix.com/trac/browser/branches/static-path-3765/twisted/web/static.py?rev=39867#L1143
142 return self.relayRules.willRelay
- 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.
- AbstractRelayRules
- source: branches/unit-tests-6383/twisted/mail/test/test_mail.py
- TestMaildirDomain
- C{NoneType} > L{None}
- AbstractMaildirDomainTestCase.tearDown
- 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.
- And if not, it's probably better to use addCleanup
- Use assertIsInstance instead of self.assertTrue(isinstance...
- Use assertNotEqual instead of self.assertTrue(msg1 != msg2) etc
- Drop the "Verify that" from each of the test docstrings
- TestRelayRules
- So this does look like a valid case for a RelayRules class, but rather than supply the class, you could write it as...
- "DomainQueuer(self.service, False, willRelay=TestRelayRules(True).willRelay)"
- TestMaildirDomain
- DummyTransport
- Use @implementer instead of implements(...
- Consider using twisted.test.proto_helpers.StringTransport instead
- https://twistedmatrix.com/documents/current/core/howto/trial.html#auto6
- DummyProtocol
- There might be an existing alternative to this too.
- Ask someone in #twisted-dev about which are the latest greatest test fakes.
- 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 4 years ago by
Branch: | branches/unit-tests-6383 → branches/unit-tests-6383-2 |
---|
(In [43199]) Branching to 'unit-tests-6383-2'
comment:16 Changed 4 years ago by
Note: See
TracTickets for help on using
tickets.
(In [39428]) Branching to 'unit-tests-6383'