Speed up test_starttls.py::TestTLSEnding::test_eof_received

[cuu508]

What do these changes do?

It fixes a 30 second delay in aiosmtpd/tests/test_starttls.py::TestTLSEnding::test_eof_received.

The contents of the test case was wrapped in try ... finally, and the finally block runs tls_controller.stop(). The finally block is where the 30 seconds were spent.

The tls_controller is prepared by a pytest fixture which also takes care of closing it so I suspect the finally block was not doing anything useful anyway.

After removing try .. finally tests on all platforms still pass, and the 30 second delay is gone.

Are there changes in behavior for the user?

No, it speeds up a single slow test.

Related issue number

Checklist

• I think the code is well written
• Unit tests for the changes exist
• tox testenvs have been executed in the following environments:
  • Linux (Ubuntu 18.04, Ubuntu 20.04, Arch): {py36,py37,py38,py39}-{nocov,cov,diffcov}, qa, docs
  • Windows (7, 10): {py36,py37,py38,py39}-{nocov,cov,diffcov}
  • WSL 1.0 (Ubuntu 18.04): {py36,py37,py38,py39}-{nocov,cov,diffcov}, pypy3-{nocov,cov}, qa, docs
  • FreeBSD (12.2, 12.1, 11.4): {py36,pypy3}-{nocov,cov,diffcov}, qa
  • Cygwin: py36-{nocov,cov,diffcov}, qa, docs
• Documentation reflects the changes
• Add a news fragment into the NEWS.rst file

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.79%. Comparing base (003c265) to head (d165014).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
- Coverage   97.81%   97.79%   -0.02%     
==========================================
  Files          23       23              
  Lines        5714     5712       -2     
  Branches      352      352              
==========================================
- Hits         5589     5586       -3     
- Misses         75       77       +2     
+ Partials       50       49       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[webknjaz]

This seems like a part of the puzzle?

[webknjaz]

Doesn't this refer to the thing you're removing? Are there any other in-test stops?

[webknjaz]

Have you checked git blame for clues?

[webknjaz]

After removing try .. finally tests on all platforms still pass, and the 30 second delay is gone.

It's possible that certain patch versions would reproduce it. Have you built like every Python 3.9.x with pyenv to confirm? This is likely hitting one of the cases that we don't test in CI or maybe something flaky that only happens under mysterious circumstances.

Overall, the cleanup idea seems good, but I'd also like to hear a second opinion from @loqs at least..

[cuu508]

The "I don't like this ... mystery" comment was added by @pepoluan in 2020, before the try .. finally block existed.

The try .. finally block was added by @pepoluan in 2021 as a part of converting test cases to pytest.

The fact that controller.stop() was in a finally block at the end of the test case suggest to me it was a cleanup thing (not testing some behavior). Perhaps a test could fail in such a way the tls_controller was still hogging a port and causing failures in other test cases?

It's possible that certain patch versions would reproduce it. Have you built like every Python 3.9.x with pyenv to confirm?

Perhaps – I have not tested every Python 3.9.x version. It would be nice to find the exact thing that caused the addition of try .. finally originally, but that's a lot of effort – I would prefer to remove try .. finally and see how we do. If we afterwards see flaky tests or code coverage, that's then something concrete we can work with.