Fix SMTP STARTTLS for Twisted >= 21.2.0 (5386)

[TobiMayr]

Pass hostname to the ESMTPSenderFactory when the twisted version is higher than 21.2.0 to use STARTTLS.

Fixes #5386

[codecov]

Codecov Report

Merging #5406 (e8e5e4f) into master (1356836) will increase coverage by 0.19%.
The diff coverage is 91.03%.

❗ Current head e8e5e4f differs from pull request most recent head 797162f. Consider uploading reports for the commit 797162f to get more accurate results

@@            Coverage Diff             @@
##           master    #5406      +/-   ##
==========================================
+ Coverage   88.61%   88.80%   +0.19%     
==========================================
  Files         163      162       -1     
  Lines       10627    10988     +361     
  Branches     1809     1795      -14     
==========================================
+ Hits         9417     9758     +341     
- Misses        935      949      +14     
- Partials      275      281       +6     

Impacted Files	Coverage Δ
scrapy/__init__.py	84.21% <0.00%> (ø)
scrapy/core/downloader/tls.py	97.05% <ø> (-0.09%)	⬇️
scrapy/core/http2/protocol.py	83.41% <0.00%> (ø)
scrapy/downloadermiddlewares/httpcompression.py	92.64% <ø> (ø)
scrapy/extensions/statsmailer.py	0.00% <ø> (ø)
scrapy/extensions/telnet.py	81.66% <ø> (ø)
scrapy/extensions/throttle.py	46.00% <ø> (ø)
scrapy/linkextractors/__init__.py	100.00% <ø> (+2.89%)	⬆️
scrapy/robotstxt.py	88.88% <ø> (-8.65%)	⬇️
scrapy/utils/boto.py	66.66% <ø> (+22.91%)	⬆️
... and 106 more

[Gallaecio]

Is it OK to set the attribute after the class initialization, instead of passing it to the class init method? Could you include some kind of test for the change (a test that would fail if the change was reverted)?

[wRAR]

Note that usually we check the Twisted version with from twisted import version as twisted_version; twisted_version < Version('twisted', 18, 4, 0)

[wRAR]

As for the assignment question, it should be fine BUT the attribute is _hostname, not hostname so this code is unlikely to work (and I don't like the idea of assigning to an underscored attribute of course). I think we can make a dict for all keyword args we use and pass it.

Not sure what can a test do unless we can mock a SMTP server with STARTTLS, but it may be possible to run some code with just the factory itself (e.g. buildProtocol) and examine the results.

[TobiMayr]

Thank you for your input, I will try to fix that next week

[TobiMayr]

Made the changes but I still need test it with the server and add unit tests

[TobiMayr]

I made some code changes implementing the suggestions in your comments. Thank you @wRAR
I also tried this code with my SFTP server and it works with a newer twisted version (22.1.0). The mail was sent successfully.

What I am struggling with are the unit tests. How can I test the factory itself? I need to make a separate method to create the factory, right? In tests/test_mail.py I tried to execute the _sendmail method. But it returns a deferred and I don't know if it's possible to extract the factory from there.
I tried something like:

        mailsender = MailSender(debug=False, smtphost='localhost', smtpport=587,
                                mailfrom='scrapy@localhost.com',
                                smtpuser='scrapy@localhost.com', smtppass='password', smtptls=True)

        msg = b'Content-Type: text/plain\nMIME-Version: 1.0\nFrom: scrapy@localhost.com\n' \
              b'To: scrapy@localhost.com\nDate: Wed, 23 Feb 2022 22:03:47 +0100\nSubject: test\n\ntest'
        d = mailsender._sendmail(to_addrs='scrapy@localhost.com', msg=msg)

Any ideas?

[wRAR]

Regarding tests, I thought about running some methods that cause ESMTPSender._getContextFactory() to be called (as this is the method that works differently with and without self._hostname)

[TobiMayr]

Regarding tests, I thought about running some methods that cause ESMTPSender._getContextFactory() to be called (as this is the method that works differently with and without self._hostname)

Wouldn't I then test only the twisted library instead of the _sendmail method? Ideally, I should test this method and then analyse the ESMTPSender right?

[wRAR]

Right, I understand now what did you mean.

Maybe we can extract the factory creation? Otherwise we need a mock SMTP server, as the purpose of _sendmail is to actually send an email, and you can't use it for something more simple.

[TobiMayr]

Resolved comment but not all checks ran through. I'm not sure what the issue could be:

FAILED tests/test_engine_stop_download_bytes.py::EngineTest::test_deprecated_has_capacity
= 1 failed, 2697 passed, 119 skipped, 26 xfailed, 529 warnings in 1187.61s (0:19:47) =
ERROR: InvocationError for command /home/runner/work/scrapy/scrapy/.tox/pypy3-pinned/bin/pytest --durations=10 docs scrapy tests (exited with code 1)

Can anyone point me in the right direction?

[Gallaecio]

We have a few tests that may fail randomly. Let me re-run them.

[wRAR]

Have you tried to call some more high-level TLS-related methods that check _hostname instead of checking it directly? See #5406 (comment)

[TobiMayr]

Have you tried to call some more high-level TLS-related methods that check _hostname instead of checking it directly? See #5406 (comment)

I think I got it now. I found a method that triggers ESMTPSender._getContextFactory() and the type of context depends on the version of twisted and whether there is a hostname or not. Let me know if you think it makes sense

[TobiMayr]

The coverage report tells me that the body of this if-block is not covered:

if twisted_version >= Version('twisted', 21, 2, 0):
    factory_keywords['hostname'] = self.smtphost

It seems like the coverage run is executed with a lower version of twisted.
I can't mock the twisted_version inside of the mail.py, right? Because it's an import in a different module with its own local scope. My mock wouldn't take effect. Any idea on how I can make sure this line is covered?

[Gallaecio]

According to the pull request coverage report it gets called at least 6 times. So I think we can ignore the codecov/patch issue.

[TobiMayr]

According to the pull request coverage report it gets called at least 6 times. So I think we can ignore the codecov/patch issue.

Good to know! We'd still have to reach the coverage target though right? I'm not so sure how do to that though because the only other line which is not covered is line 135 which is executing the new function for the factory creation. I would have to evoke the _sendmail function but I'm not sure what I should test there. What do you think @Gallaecio ?
Thanks for your support

[TobiMayr]

@wRAR do you think I should just write a test for _sendmail to cover the line of the function call (see previous comment)? I'm not sure if this would be the right way to fix the coverage. I don't see a better way though. What are your thoughts?

[wRAR]

To test _sendmail, as I mentioned earlier, you would "need a mock SMTP server, as the purpose of _sendmail is to actually send an email, and you can't use it for something more simple." And I suspect this entire function is not currently covered anyway.

[TobiMayr]

Right @wRAR. Not sure if mocking a SMTP would be beneficial here. As the only purpose would be to cover this one function call. Do you think the coverage target miss can be ignored then? Since the only other line is apparently being executed by the tests.
Or is reaching the coverage target crucial? I seriously don't know what do do in this case :)

[Gallaecio]

I wonder if we can exploit mitmproxy for a testing SMTP server, as we do for other tests that need a server.

[wRAR]

The current test implementation matches what I wanted to see initially, and as I wrote earlier, _sendmail was already not covered so it sounds fine if we don't add coverage for it here.