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

gh-128201: Fix DeprecationWarning in Lib/test/test_pdb.py #128202

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Eclips4
Copy link
Member

@Eclips4 Eclips4 commented Dec 23, 2024

@bedevere-app bedevere-app bot added tests Tests in the Lib/test dir awaiting core review labels Dec 23, 2024
@Eclips4 Eclips4 changed the title Fix DeprecationWarning in Lib/test/test_pdb.py gh-128201: Fix DeprecationWarning in Lib/test/test_pdb.py Dec 23, 2024
@gaogaotiantian
Copy link
Member

Is this the correct way to fix it? I actually don't know what set policy does. Someone with asyncio expertise should check if this is needed or what's the modern way to write it. Replacing it with a "private" call might elimiate the warning but may not be the best way to fix this.

@hugovk
Copy link
Member

hugovk commented Dec 23, 2024

If asyncio.set_event_loop_policy is deprecated, what should end users call instead? Presumably not asyncio._set_event_loop_policy?

Can we also call the replacement?

@Eclips4
Copy link
Member Author

Eclips4 commented Dec 23, 2024

Initially I was thinking about a "default way" to fix that DeprecationWarning by rewriting code to avoid using deprecated things.
But after looking at #128024 I changed my mind.

This one was accidentally omitted in #128024:

  • Line 2135 has been fixed
  • Line 2256 has been fixed
  • Line 2356 has been fixed

UPD: asyncio._set_event_loop_policy was added in #128024 for internal use in tests, I believe.
I think Kumar's plan is to eliminate use of asyncio._set_event_loop_policy in follow-up PR's.

@gaogaotiantian
Copy link
Member

Okay I think the fix is okay at this point. However, is there an asyncio expert that can take a look at the testing code and see if this line is even needed? As there's no explicit code to change the policy. The testing code is pretty trivial, will a new_event_loop change the policy? Why do we need to set it back to default in the first place?

@kumaraditya303
Copy link
Contributor

Okay I think the fix is okay at this point. However, is there an asyncio expert that can take a look at the testing code and see if this line is even needed? As there's no explicit code to change the policy. The testing code is pretty trivial, will a new_event_loop change the policy? Why do we need to set it back to default in the first place?

For pdb tests, asyncio.run could be used instead of asyncio.new_event_loop as that would take care of policy stuff internally.
AFAICS pdb doesn't mess around the policy stuff so migrating of all tests to use newer asyncio.run or asyncio.Runner seems worthwhile.

@Eclips4 Eclips4 marked this pull request as draft December 24, 2024 10:33
@Eclips4 Eclips4 marked this pull request as ready for review December 24, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants