Skip to content

bpo-34246: Make sure test_smtplib always cleans resources when finished#9108

Merged
pablogsal merged 2 commits into
python:masterfrom
pablogsal:bpo34246
Sep 7, 2018
Merged

bpo-34246: Make sure test_smtplib always cleans resources when finished#9108
pablogsal merged 2 commits into
python:masterfrom
pablogsal:bpo34246

Conversation

@pablogsal

@pablogsal pablogsal commented Sep 7, 2018

Copy link
Copy Markdown
Member

@pablogsal pablogsal requested a review from a team as a code owner September 7, 2018 21:50
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting merge labels Sep 7, 2018

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall change LGTM, but I have a few comments/requests.

Comment thread Lib/test/test_smtplib.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use support.join_thread(self.thread) to raise an error if the thread cannot be joined in 30 seconds, rather than being stuck.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will change then the rest of thread.joins of this module as there are plenty.

Comment thread Lib/test/test_smtplib.py Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to rename the attribute to "self.thread_key".

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@pablogsal pablogsal merged commit 5b7a2cb into python:master Sep 7, 2018
@pablogsal pablogsal deleted the bpo34246 branch September 7, 2018 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants