feat: speedup integration tests #12
Conversation
jedel1043
force-pushed
the
speedup-tests
branch
2 times, most recently
from
98e6a0c to
19165c6
Compare
tests/integration/test_charm.py
Outdated
| filesystem_client, nfs_server_proxy, cephfs_server_proxy = await asyncio.gather( | ||
| filesystem_client_charm, nfs_server_proxy_charm, cephfs_server_proxy_charm | ||
| ) | ||
| async def deploy_cephfs_proxy(): |
There was a problem hiding this comment.
Generally I try to avoid using nested functions since they make their parent functions more "complex," and they inherit their variables from the the outer scope which makes it easier to accidentally stumble upon semantic and/or syntactic issues. They're also harder to mock and unit test... not that we would have tests for our tests (that's infinite recursion).
Given these functions take *_server_proxy_charm and ops_test from the outer scope, and have no return value, I think it would be better from a readability (and debug-ability) standpoint to have them be broken out into their own top-level functions:
async def deploy_cephfs_proxy(
ops_test: OpsTest,
cephfs_server_proxy_charm: Awaitable[str | pathlib.Path]
) -> None:
# Cool async function stuff
async def deploy_nfs_proxy(
ops_test: OpsTest,
cephfs_server_proxy_charm: Awaitable[str | pathlib.Path]
) -> None:
# Cool async function stuff Smaller scope, less possible issues later. Curious to know your thoughts here!
There was a problem hiding this comment.
I think that's just unnecessary because the intention of defining those functions wasn't to separate chunks of code, but a necessity to be able to relate the output of an async function with the input of another async function.
Having them capture the outer variables was intentional, since they should not be reusable pieces of code, but one-shot functions.
There was a problem hiding this comment.
I modified the code to be a bit more clear about this. Now the functions are defined immediately before their call, which makes it really clear that they shouldn't be used outside their single call.
jedel1043
force-pushed
the
speedup-tests
branch
3 times, most recently
from
2ff8e45 to
9bd956e
Compare
tests/integration/helpers.py
Outdated
| @@ -5,10 +5,13 @@ | |||
|
|
|||
| import json | |||
| import logging | |||
| import pathlib | |||
There was a problem hiding this comment.
Another place where we should just import Path directly? Looks like it's just used for the Awaitable annotation in the build_and_deploy_charm function annotation.
| ), | ||
| ) | ||
|
|
||
| nfs_info, _ = await asyncio.gather(bootstrap_nfs_server(ops_test), deploy_charm()) |
There was a problem hiding this comment.
[comment]: I feel like this would be a great time for async lambda to be a thing...
This makes the integration tests start a bit faster by concurrently building all the charms + deploying the servers in a concurrent way. Additionally, we stop depending on LXD for the servers, opting for deploying Juju applications and running commands inside them when required.
jedel1043
force-pushed
the
speedup-tests
branch
from
9bd956e to
8fd0126
Compare
This makes the integration tests start a bit faster by concurrently building all the charms + deploying the servers in a concurrent way.
Additionally, we stop depending on LXD for the servers, opting for deploying Juju applications and running commands inside them when required.