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

Convert sydent/http/srvresolver.py and associated files to async/await #365

Merged
merged 6 commits into from
Jun 22, 2021

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jun 18, 2021

This PR is the result of converting the file sydent/http/srvresolver.py to async/await, and updating the return value in other files that use or call the converted method (there was only one method in srvresolver.py which was converted.

@H-Shay
Copy link
Contributor Author

H-Shay commented Jun 18, 2021

Ahh the 3.6 unit tests are failing (I suspect) because the older versions of mock don't automatically convert an asynchronous function to an asyncMock when patched in. I'll figure it out monday morning!

@clokep
Copy link
Member

clokep commented Jun 21, 2021

because the older versions of mock don't automatically convert an asynchronous function to an asyncMock when patched in. I'll figure it out monday morning!

We define an async mock somewhere in the Synapse test suite. (There's a simple_async_mock function.)

@H-Shay
Copy link
Contributor Author

H-Shay commented Jun 21, 2021

So-after much researching and reading, (I haven't used mocks before, it's new to me!) I found a solution, which is to create an AsyncMock class and use it in the patch. I looked in synapse and found the function you were talking about, although I couldn't figure out how to port that definition to this situation, as I think I needed a class (to create an object) rather than a function. So I went with some code I found on Stack Overflow, and rather than defining the AsyncMock class in the test_blacklisting.py file, I stuck it in test.utils. I am sure this is not the most graceful way to handle all this, but it seems to be working.

@clokep clokep self-requested a review June 22, 2021 11:22
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple of minor comments!

tests/utils.py Outdated Show resolved Hide resolved
changelog.d/365.misc Outdated Show resolved Hide resolved
@H-Shay H-Shay requested a review from clokep June 22, 2021 15:23
@clokep clokep merged commit 712b3c6 into matrix-org:main Jun 22, 2021
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.

2 participants