Ticket #4152 task new

Opened 3 years ago

Last modified 3 months ago

Remove deprecated DomainSMTP and DomainESMTP

Reported by: thijs Owned by: thijs
Priority: normal Milestone:
Component: mail Keywords:
Cc: thijs, richard@… Branch: branches/remove-domainsmtp-4152-2
Author: thijs Launchpad Bug:

Description

DomainESMTP and DomainSMTP in trunk/twisted/mail/protocols.py are deprecated, let's remove them.

Change History

1

Changed 3 years ago by thijs

  • owner changed from exarkun to cyli

Thanks for all the hard work so far cyli, I hope you don't mind if I reassign a couple to you :)

2

Changed 3 years ago by cyli

  • branch set to branches/remove-domainsmtp-4152
  • branch_author set to cyli

(In [29408]) Branching to 'remove-domainsmtp-4152'

3

Changed 3 years ago by cyli

  • owner cyli 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 :)

4

Changed 2 years ago by <automation>

5

Changed 4 months ago by thijs

  • branch changed from branches/remove-domainsmtp-4152 to branches/remove-domainsmtp-4152-2
  • branch_author changed from cyli to cyli, thijs

(In [36827]) Branching to 'remove-domainsmtp-4152-2'

6

Changed 4 months ago by thijs

(In [36828]) remove deprecated protocols.DomainSMTP and DomainESMTP. refs #4152

7

Changed 4 months ago by thijs

  • keywords review added
  • branch_author changed from cyli, thijs to thijs

Besides removing both classes I also:

  1. removed old test_smtp.SMTPTestCase which seemed specifically there for DomainSMTP only and tested elsewhere.
  2. also replaced the stringio import with twisted's compat version and removed unused trial import from test_smtp, fixed 2 pyflakes warnings
  3. removed LoopbackTestCase for same reason as point 1

Related changeset for the original deprecation: r7562.

8

Changed 4 months ago by rwall

  • cc richard@… added
  • status changed from new to assigned
  • owner set to rwall

Reviewing...

9

Changed 4 months ago by rwall

  • owner rwall deleted
  • status changed from assigned to new

Code Review

Thjs. This branch looks fine to me. Here's what I tested...

  1. Branch merged cleanly to trunk
  2. [ http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/remove-domainsmtp-4152-2 Build Results] look fine (some expected failures)
  3. trial twisted.mail.test.test_smtp all pass on my Fedora 18 system
  4. 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
    
    1. 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')
      
    2. 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)
      
    3. 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))
      
    4. So should this branch include some new tests to exercise that code? Or should some new tickets be raised for that?
  5. 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.

10

Changed 3 months ago by tom.prince

  • keywords review removed
  • owner set to thijs

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.

11

Changed 3 months ago by tom.prince

But, file a ticket to improve the test coverage of those cases.

Note: See TracTickets for help on using tickets.