Skip to content

gh-120048: Make test_imaplib faster#120050

Merged
colesbury merged 4 commits into
python:mainfrom
colesbury:gh-120048-imaplib
Jun 4, 2024
Merged

gh-120048: Make test_imaplib faster#120050
colesbury merged 4 commits into
python:mainfrom
colesbury:gh-120048-imaplib

Conversation

@colesbury
Copy link
Copy Markdown
Contributor

@colesbury colesbury commented Jun 4, 2024

The test_imaplib was taking 40+ minutes in the refleak build bots because the tests waiting on a client self._setup() was creating a client that prevented progress until its connection timed out, which scaled with the global timeout.

We should set connect=False for the tests that don't want _setup() to create a client.

The `test_imaplib` was taking 40+ minutes in the refleak build bots
because we were using LOOPBACK_TIMEOUT for tests cases that expect to
timeout. LOOPBACK_TIMEOUT scales with the global timeout.

We should use a fixed, very small timeout for cases that we expect to
timeout.
@colesbury colesbury added tests Tests in the Lib/test dir skip news needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Jun 4, 2024
@colesbury colesbury requested a review from a team as a code owner June 4, 2024 16:00
Comment thread Lib/test/test_imaplib.py Outdated
Comment on lines +477 to +478
client = self.imap_class("localhost", addr, timeout=FAST_TIMEOUT)
self.assertEqual(client.sock.timeout, FAST_TIMEOUT)
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.

Should not an explicit timeout be different from SimpleIMAPHandler.timeout? Otherwise we cannot know how the value of client.sock.timeout was set.

You perhaps can use expect_timeout() as a context manager instead of a decorator.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I misunderstood the problem with this test -- these connections should not time out. The problem was that the call self._setup(SimpleIMAPHandler) created a client and the remainder of the test did not run until that client timedout.

We should use self._setup(SimpleIMAPHandler, connect=False)

Comment thread Lib/test/test_imaplib.py Outdated
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@colesbury
Copy link
Copy Markdown
Contributor Author

!buildbot AMD64 Ubuntu NoGIL Refleaks

@bedevere-bot
Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @colesbury for commit fcce846 🤖

The command will test the builders whose names match following regular expression: AMD64 Ubuntu NoGIL Refleaks

The builders matched are:

  • AMD64 Ubuntu NoGIL Refleaks PR

Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@colesbury colesbury merged commit 710cbea into python:main Jun 4, 2024
@miss-islington-app
Copy link
Copy Markdown

Thanks @colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

@colesbury colesbury deleted the gh-120048-imaplib branch June 4, 2024 18:59
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 4, 2024
The `test_imaplib` was taking 40+ minutes in the refleak build bots because
the tests waiting on a client `self._setup()` was creating a client that
prevented progress until its connection timed out, which scaled with the
global timeout.

We should set `connect=False` for the tests that don't want `_setup()` to
create a client.

(cherry picked from commit 710cbea)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented Jun 4, 2024

GH-120069 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Jun 4, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 4, 2024
The `test_imaplib` was taking 40+ minutes in the refleak build bots because
the tests waiting on a client `self._setup()` was creating a client that
prevented progress until its connection timed out, which scaled with the
global timeout.

We should set `connect=False` for the tests that don't want `_setup()` to
create a client.

(cherry picked from commit 710cbea)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link
Copy Markdown

bedevere-app Bot commented Jun 4, 2024

GH-120070 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jun 4, 2024
colesbury added a commit that referenced this pull request Jun 4, 2024
The `test_imaplib` was taking 40+ minutes in the refleak build bots because
the tests waiting on a client `self._setup()` was creating a client that
prevented progress until its connection timed out, which scaled with the
global timeout.

We should set `connect=False` for the tests that don't want `_setup()` to
create a client.

(cherry picked from commit 710cbea)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
colesbury added a commit that referenced this pull request Jun 4, 2024
The `test_imaplib` was taking 40+ minutes in the refleak build bots because
the tests waiting on a client `self._setup()` was creating a client that
prevented progress until its connection timed out, which scaled with the
global timeout.

We should set `connect=False` for the tests that don't want `_setup()` to
create a client.

(cherry picked from commit 710cbea)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
barneygale pushed a commit to barneygale/cpython that referenced this pull request Jun 5, 2024
The `test_imaplib` was taking 40+ minutes in the refleak build bots because
the tests waiting on a client `self._setup()` was creating a client that
prevented progress until its connection timed out, which scaled with the
global timeout.

We should set `connect=False` for the tests that don't want `_setup()` to
create a client.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
The `test_imaplib` was taking 40+ minutes in the refleak build bots because
the tests waiting on a client `self._setup()` was creating a client that
prevented progress until its connection timed out, which scaled with the
global timeout.

We should set `connect=False` for the tests that don't want `_setup()` to
create a client.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
The `test_imaplib` was taking 40+ minutes in the refleak build bots because
the tests waiting on a client `self._setup()` was creating a client that
prevented progress until its connection timed out, which scaled with the
global timeout.

We should set `connect=False` for the tests that don't want `_setup()` to
create a client.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
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.

3 participants