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 passing coroutine objects to wait() #24

Merged
merged 9 commits into from
Nov 21, 2022

Conversation

lqhuang
Copy link
Collaborator

@lqhuang lqhuang commented Nov 19, 2022

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

Due to break change introduced in Python 3.11, wait() only accept scheduled tasks.

Changed in version 3.11: Passing coroutine objects to wait() directly is forbidden.

In services.py line 794 of current master, two coroutines are passed into wait() function. So the program exits.

stopped = self._stopped.wait()
crashed = self._crashed.wait()
done, pending = await asyncio.wait(
    [stopped, crashed],
    return_when=asyncio.FIRST_COMPLETED,
    timeout=timeout,
)

This PR fixes #19.

This patch is enough to let mode successfully run under Python 3.11 except passing all test cases. Current tests of mode rely on its self defined AsyncMock which is already not working under py3.11,
so there are still a lot of hard work to rewrite test cases to make everything looks good.

@lqhuang lqhuang requested a review from wbarnha November 19, 2022 04:25
@wbarnha
Copy link
Member

wbarnha commented Nov 19, 2022

This patch is enough to let mode successfully run under Python 3.11 except passing all test cases. Current tests of mode relies on its self defined AsyncMock already not working under py3.11,

Very interesting, I'll test this branch with faust-streaming before merging, assuming all test cases above pass for all other versions of Python.

@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 19, 2022

@wbarnha Maybe we should stop actions? Something blocked, it already runs 3hrs more. I'm afraid to consume all free time.

@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 19, 2022

I see. I removed from asyncio import coroutine which should not show in Python 3.11, so tests for all past Python failed, too. What sad news.

@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 20, 2022

@wbarnha Hey, you could review now, I have fixed all tests with new standard library modules.

@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 20, 2022

@wbarnha Currently, it fits all versions after Python 3.8. Python 3.7 is lack of test suites AsyncMock, but it should work well too.

@wbarnha
Copy link
Member

wbarnha commented Nov 20, 2022

That's incredible! Fantastic work @lqhuang! I'll see if I can add compatibility for 3.7+3.8, or we could just drop support for those versions.

@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 21, 2022

Actually, the incompatibility for both Py3.7 and Py3.8 happens in test codebase. Py3.8 is caused by the nested with syntax.

Copy link
Member

@wbarnha wbarnha left a comment

Choose a reason for hiding this comment

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

Thank you so much! LGTM!

@wbarnha wbarnha merged commit 3624a98 into faust-streaming:master Nov 21, 2022
@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 21, 2022

Also, thank you for your efforts in review work!

@wbarnha
Copy link
Member

wbarnha commented Nov 22, 2022

Regarding commit 14ca981, I think we need to bring the mocks back because they had specific logic that was hardcoded for faust-streaming unittests...

@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 22, 2022

I definitely understand. Sure, you could bring them back, but the key problem is this module also will make faust-streaming unable to run under Python 3.11. I said it would be a hard work to upgrade test cases for both mode and faust.

@wbarnha
Copy link
Member

wbarnha commented Nov 22, 2022

Yeah, I'm starting to remember why you removed the mocks. Upgrading the test cases for Faust will take a while.

@lqhuang
Copy link
Collaborator Author

lqhuang commented Nov 22, 2022

I also read codes from faust, through I does't use it yet, otherwise I could help to maintain it either. One good thing is you could start from simply replace codes in special pattern. Basically, updating .coro.assert_called_once[_with]() with .assert_awaited_once[_with]() works in most cases. Hope my commits in this PR will give you a hand.

@wbarnha
Copy link
Member

wbarnha commented Nov 22, 2022

Basically, updating .coro.assert_called_once[_with]() with .assert_awaited_once[_with]()

I've observed the same thing! I'll keep trying to go through the unit tests. I've made some progress but there are some quirks I still need to clear up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mode not ready for 3.11
2 participants