Opened 10 years ago
Last modified 6 years ago
#4152 task new
Remove deprecated DomainSMTP and DomainESMTP
Reported by: | Thijs Triemstra | Owned by: | Tom Prince |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | Keywords: | ||
Cc: | Thijs Triemstra, Richard Wall | Branch: |
branches/remove-domainsmtp-4152-2
branch-diff, diff-cov, branch-cov, buildbot |
Author: | thijs |
Description
DomainESMTP
and DomainSMTP
in [source:trunk/twisted/mail/protocols.py] are deprecated, let's remove them.
Change History (13)
comment:1 Changed 9 years ago by
Owner: | changed from Jean-Paul Calderone to Ying Li |
---|
comment:2 Changed 9 years ago by
Author: | → cyli |
---|---|
Branch: | → branches/remove-domainsmtp-4152 |
(In [29408]) Branching to 'remove-domainsmtp-4152'
comment:3 Changed 9 years ago by
Owner: | Ying Li deleted |
---|
I don't know enough about this to be able to change the tests to reflect their removal. I tried replacing DomainSMTP in SMTPTestCase with smtp.SMTP, and I just got errors.
Someone else should probably take over this ticke.t :)
comment:4 Changed 9 years ago by
comment:5 Changed 7 years ago by
Author: | cyli → cyli, thijs |
---|---|
Branch: | branches/remove-domainsmtp-4152 → branches/remove-domainsmtp-4152-2 |
(In [36827]) Branching to 'remove-domainsmtp-4152-2'
comment:6 Changed 7 years ago by
comment:7 Changed 7 years ago by
Author: | cyli, thijs → thijs |
---|---|
Keywords: | review added |
Besides removing both classes I also:
- removed old
test_smtp.SMTPTestCase
which seemed specifically there forDomainSMTP
only and tested elsewhere. - also replaced the stringio import with twisted's
compat
version and removed unused trial import fromtest_smtp
, fixed 2 pyflakes warnings - removed
LoopbackTestCase
for same reason as point 1
Related changeset for the original deprecation: r7562.
comment:8 Changed 7 years ago by
Cc: | Richard Wall added |
---|---|
Owner: | set to Richard Wall |
Status: | new → assigned |
Reviewing...
comment:9 Changed 7 years ago by
Owner: | Richard Wall deleted |
---|---|
Status: | assigned → new |
Code Review
Thjs. This branch looks fine to me. Here's what I tested...
- Branch merged cleanly to trunk
- [http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-domainsmtp-4152-2 Build Results] look fine (some expected failures)
- trial twisted.mail.test.test_smtp all pass on my Fedora 18 system
- Coverage: I was trying to think "how would exarkun review this
branch?" ;) so I thought it would be interesting to try trial
--coverarage, since some tests have been removed. So I compared
the output of trial --coverage between your branch and trunk. I
noticed these blocks no longer seem to have coverage...assuming
I'm interpreting the output correctly.
diff -u \ trunk/_trial_temp/coverage/twisted.mail.smtp.cover \ branches/remove-domainsmtp-4152-2/_trial_temp/coverage/twisted.mail.smtp.cover
- t.m.smtp.SMTP.do_UNKNOWN
1: def do_UNKNOWN(self, rest): - 1: self.sendCode(500, 'Command not implemented') +>>>>>> self.sendCode(500, 'Command not implemented')
- t.m.smtp.SMTP._ebToValidate
1: def _ebToValidate(self, failure): - 1: if failure.check(SMTPBadRcpt, SMTPServerError): - 1: self.sendCode(failure.value.code, failure.value.resp) +>>>>>> if failure.check(SMTPBadRcpt, SMTPServerError): +>>>>>> self.sendCode(failure.value.code, failure.value.resp)
- t.m.smtp.SMTPClient.lineReceived
else: - 1: why = self._failresponse(self.code,'\n'.join(self.resp)) +>>>>>> why = self._failresponse(self.code,'\n'.join(self.resp))
- So should this branch include some new tests to exercise that code? Or should some new tickets be raised for that?
- t.m.smtp.SMTP.do_UNKNOWN
- The deprecation is 10 years old - so there were no tests for the deprecation its self.
So its a +1 from me.
But I won't remove the review keyword. I think someone more qualified than me (probably exarkun) should double check before this is removed.
-RichardW.
PS Thanks thijs for reviewing my other branches.
comment:10 Changed 7 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Thijs Triemstra |
Some more investigation of those coverage changes indicate that those tests are also exercising twisted.mail.smtp.SMTP
class in a few cases, but not making much of any assertions about them. (SMTPTestCase.testMessage
does make an assertion that the message is delivered to foo
but not bar
).
In any case, that behavior should be tested in some unit tests, rather than (or in addition to) these weak functional tests.
So, go ahead and merge.
comment:11 follow-up: 12 Changed 7 years ago by
But, file a ticket to improve the test coverage of those cases.
comment:12 Changed 6 years ago by
Owner: | changed from Thijs Triemstra to Tom Prince |
---|
Replying to tom.prince:
But, file a ticket to improve the test coverage of those cases.
This ticket hasn't been merged yet because I'm not sure what new ticket I should file here. I'll assign it back to you Tom so you can merge this (or assign it back to me) once that particular ticket is filed if that thats ok.
comment:13 Changed 6 years ago by
After spending some time looking at this, it looks like there are a few tests for smtp.SMTP
already, although certainly not enough to provide full coverage. And it looks (although I haven't specifically checked) that the code exercised by the tests removed here are still being exercised.
Many of the existing test seem to functional tests (AnotherTestCase
, LiveFireExercise
) and it looks like some recent changes had unit tests added to them, but the base functionality mostly isn't tested with unit tests.
I guess the only reasonable ticket would be "Add unit tests for smtp.SMTP
and smtp.ESMTP
". But, perhaps that doesn't deserve a ticket, any more than all the rest of twisted that doesn't have unittests.
Thanks for all the hard work so far cyli, I hope you don't mind if I reassign a couple to you :)