Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-39259: poplib.POP3/POP3_SSL now reject timeout = 0 #17912

Open
wants to merge 2 commits into
base: master
from

Conversation

@corona10
Copy link
Member

corona10 commented Jan 9, 2020

@corona10 corona10 requested a review from python/email-team as a code owner Jan 9, 2020
@corona10 corona10 requested a review from vstinner Jan 9, 2020
@corona10 corona10 force-pushed the corona10:bpo-39259 branch from aa65a71 to 9d99211 Jan 9, 2020
@@ -505,17 +505,17 @@ def server(self, evt, serv):

def testTimeoutDefault(self):
self.assertIsNone(socket.getdefaulttimeout())
socket.setdefaulttimeout(30)
socket.setdefaulttimeout(test_support.SHORT_TIMEOUT)

This comment has been minimized.

Copy link
@vstinner

vstinner Jan 9, 2020

Member

When I added timeout constants to the test.support module, I decided to leave this test unchanged. I understood that the test doesn't connect to real socket. Or does it? I'm not sure. If it does, LOOPBACK_TIMEOUT is preferred.

This comment has been minimized.

Copy link
@corona10

corona10 Jan 9, 2020

Author Member

self.welcome = self._getresp()

As you can see poplib client's constructor actually makes a connection to the server and get a response.

self.thread = threading.Thread(target=self.server, args=(self.evt,self.sock))

So this test intends to check whether poplib client is set by a global socket timeout and also check whether the connection is succeeded.

@corona10

This comment has been minimized.

Copy link
Member Author

corona10 commented Jan 9, 2020

@vstinner I have made the requested changes; please review again

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 9, 2020

Thanks for making the requested changes!

: please review the changes made to this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.