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

Prefer AsyncMock to as_future #1366

Merged
merged 4 commits into from
Oct 25, 2021
Merged

Conversation

pquentin
Copy link
Member

@pquentin pquentin commented Oct 25, 2021

In Python, calling an async function f() can be done in two steps:

  1. call the function using regular function call f()
  2. await the result, possibly later

The driver tests took advantage of this detail to make MagicMock work with async functions by using as_future. With that trick, mocked functions return an awaitable result, which are then awaited in the tested code.

However, coroutines are best thought as an implementation detail, and it's better to always call an async function f using await f(), never separating the two steps mentioned above. Thankfully, Python 3.8 introduced AsyncMock that allows removing as_future by just specifying a return value, which avoids thinking about coroutines, which is what we want.

There's just one wrinkle: while mock.patch() replaces the target with an AsyncMock, it does not work recursively. So while we would like to rewrite

es.cluster.health.return_value = as_future({"status": "green", "relocating_shards": 0})

to

es.cluster.health.return_value = {"status": "green", "relocating_shards": 0}

we need to use

es.cluster.health = mock.AsyncMock(return_value={"status": "green", "relocating_shards": 0})

which is still an improvement as it avoids the as_future code smell.

@pquentin pquentin added cleanup Linter changes, reformatting, removal of unused code etc. :internal Changes for internal, undocumented features: e.g. experimental, release scripts labels Oct 25, 2021
@pquentin pquentin added this to the 2.3.1 milestone Oct 25, 2021
@pquentin pquentin self-assigned this Oct 25, 2021
Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Good catch! I noticed that there are still places where we use as_future and I assume it's because the mock.patch annotation uses regular mocks? In any case I think we should add pydocs with guidance when to use as_future and state explicitly that mock.AsyncMock is preferred. Also, can you please add a description in the PR body documenting the motivation for this PR?

@pquentin
Copy link
Member Author

@danielmitterdorfer I ended up completely removing as_future, and explained in the description why the second part was harder. Do we still need guidance now?

@danielmitterdorfer
Copy link
Member

I ended up completely removing as_future [...] Do we still need guidance now?

Thanks! As there is now only one approach to test async code there's no need for any guidance anymore.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

Nice cleanup! LGTM

@pquentin pquentin merged commit 24dee9e into elastic:master Oct 25, 2021
@pquentin pquentin deleted the reduce-as-future-usage branch October 25, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Linter changes, reformatting, removal of unused code etc. :internal Changes for internal, undocumented features: e.g. experimental, release scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants