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

Fix test utils on Python 3.11. #6867

Closed
wants to merge 2 commits into from
Closed

Conversation

felixxm
Copy link

@felixxm felixxm commented Aug 4, 2022

What do these changes do?

Fix test utils on Python 3.11

Are there changes in behavior for the user?

No.

Related issue number

#6757.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • (no need 🤔) Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the CHANGES folder

@felixxm felixxm requested a review from asvetlov as a code owner August 4, 2022 06:53
@felixxm felixxm requested a review from webknjaz as a code owner August 4, 2022 06:56
@webknjaz
Copy link
Member

webknjaz commented Aug 4, 2022

  • (no need 🤔) Add a new news fragment into the CHANGES folder

I think having a misc change note wouldn't hurt...

@webknjaz
Copy link
Member

webknjaz commented Aug 4, 2022

Fix test utils on Python 3.11

Could you add more detail on what's the effect of this? Meanwhile, I've let the CI run on this PR to see what's different.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 4, 2022
@felixxm
Copy link
Author

felixxm commented Aug 4, 2022

Fix test utils on Python 3.11

Could you add more detail on what's the effect of this? Meanwhile, I've let the CI run on this PR to see what's different.

As far as I'm aware get_event_loop() should not be used to create a new event (it was deprecated in Python 3.10), see https://bugs.python.org/issue39529.

CHANGES/6867.misc Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

webknjaz commented Aug 4, 2022

As far as I'm aware get_event_loop() should not be used to create a new event (it was deprecated in Python 3.10), see bugs.python.org/issue39529.

It seems like this is just used as a fallback under older CPython versions, but the recent ones shouldn't hit it.

According to the docs at https://docs.python.org/3.11/library/asyncio-eventloop.html#asyncio.get_event_loop, it's deprecated only when there's no running loop:

Because this function has rather complex behavior (especially when custom event loop policies are in use), using the get_running_loop() function is preferred to get_event_loop() in coroutines and callbacks.

And when the loop is running, get_running_loop() will be used:

Deprecated since version 3.10: Deprecation warning is emitted if there is no running event loop. In future Python releases, this function will be an alias of get_running_loop().

And looking at the source, it's evident that get_running_loop() is already there. It seems like by design, the test helper is not supposed to create new loops.
Although, the pytest plugin does this, I think, via the loop_context() CM. So I'm not sure whether this change is actually desired.
It'd be great to get opinions from @WisdomPill, @Dreamsorcerer, @Hanaasagi, and @H--o-l first.

@webknjaz
Copy link
Member

webknjaz commented Aug 4, 2022

Also, this PR makes tests fail under Python 3.10: https://github.com/aio-libs/aiohttp/runs/7673650666?check_suite_focus=true. It causes DeprecationWarnings. The more I see, the more I'm convinced that this patch isn't really what we want...

@Dreamsorcerer
Copy link
Member

I started questioning why it is even useful to have that in the test case, then saw that it is already deprecated in our docs:
https://docs.aiohttp.org/en/stable/testing.html#aiohttp.test_utils.AioHTTPTestCase.loop

So, actually, I think this should be completely removed, atleast from master. Then maybe a minimum effort fix/hack for 3.x.

How about just moving the call to a @property method, so it is only run when the user accesses .loop? Could add an explicit deprecation warning in then too. Anybody not using the deprecated attribute will then not get bothered by any warnings from cpython.

@webknjaz
Copy link
Member

webknjaz commented Aug 4, 2022

@Dreamsorcerer I guess we could drop it in 3.9 but still need in 3.8. Regarding the property — I'm not sure. Might work.

@felixxm
Copy link
Author

felixxm commented Aug 5, 2022

Closing for now 😞. Hopefully someone more async-experience will pick it up 🤞

@felixxm felixxm closed this Aug 5, 2022
@felixxm felixxm deleted the new_event_loop branch August 5, 2022 11:31
@@ -433,7 +433,11 @@ def setUp(self) -> None:
try:
self.loop = asyncio.get_running_loop()
except RuntimeError:
self.loop = asyncio.get_event_loop_policy().get_event_loop()
try:
self.loop = asyncio.get_event_loop()
Copy link
Contributor

@graingert graingert Aug 6, 2022

Choose a reason for hiding this comment

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

yeah this is not correct - you'll be getting a new and different loop see python/cpython#95736

@graingert
Copy link
Contributor

How about just moving the call to a @Property method, so it is only run when the user accesses .loop?

@Dreamsorcerer I think this will fix it

@cached_property
def loop(self):
    return asyncio.get_running_loop()

@webknjaz
Copy link
Member

webknjaz commented Aug 8, 2022

@graingert

@Dreamsorcerer I think this will fix it

@cached_property
def loop(self):
    return asyncio.get_running_loop()

Tried it locally, didn't work:

    @property
    def loop(self):
>       return asyncio.get_running_loop()
E       RuntimeError: no running event loop

@Dreamsorcerer
Copy link
Member

Removed loop from master completely in #7107.
Not sure if there's still an issue we need to resolve in 3.9 or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants