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.

Sign up for GitHub

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

Jump to bottom

tasks: added --pytest-args to tests #100

Closed
wants to merge 1 commit into from
Closed

tasks: added --pytest-args to tests #100

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ def build_docs(c):
def linters(c, color=False):
"""Run code linters"""
run(f"flake8 --color {'always' if color else 'never'} tests valkey")
run(f"black {'--color' if color else ''} --target-version py37 --check --diff tests valkey")
run(
Copy link
Member

Choose a reason for hiding this comment

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

Am I missing something or is this just a formatting change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah autoformatting changed this line for me :/ Do you want me to move it to a separate commit?

f"black {'--color' if color else ''} --target-version py37 --check --diff tests valkey"
)
run(f"isort {'--color' if color else ''} --check-only --diff tests valkey")
run("vulture valkey whitelist.py --min-confidence 80")
run("flynt --fail-on-change --dry-run tests valkey")
Expand All @@ -42,40 +44,43 @@ def all_tests(c, color=False):


@task
def tests(c, uvloop=False, protocol=2, color=False):
def tests(c, uvloop=False, protocol=2, color=False, pytest_args=None):
"""Run the valkey-py test suite against the current python,
with and without libvalkey.
"""
print("Starting Valkey tests")
print("pytest_args:", pytest_args)
Copy link
Member

Choose a reason for hiding this comment

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

By default this will print "pytest_args: None" which could be misleading as we pass a lot of arguments to pytest.
Maybe it would be better to explicitly say something like "user provided args"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is actually a leftover that I forgot to remove. Would you rather see it removed or having an additional proper message about pytest args?

standalone_tests(c, uvloop=uvloop, protocol=protocol, color=color)
cluster_tests(c, uvloop=uvloop, protocol=protocol, color=color)


@task
def standalone_tests(c, uvloop=False, protocol=2, color=False):
def standalone_tests(c, uvloop=False, protocol=2, color=False, pytest_args=None):
"""Run tests against a standalone valkey instance"""
color_arg = f"--color={'yes' if color else 'no'}"
shared_args = f"{color_arg} --protocol={protocol} --cov=./ --cov-report=xml:coverage_valkey.xml -W always -m 'not onlycluster'"
pytest_args = pytest_args or ""
if uvloop:
run(
f"pytest --color={'yes' if color else 'no'} --protocol={protocol} --cov=./ --cov-report=xml:coverage_valkey.xml -W always -m 'not onlycluster' --uvloop --junit-xml=standalone-uvloop-results.xml"
f"pytest {shared_args} --uvloop --junit-xml=standalone-uvloop-results.xml {pytest_args}"
)
else:
run(
f"pytest --color={'yes' if color else 'no'} --protocol={protocol} --cov=./ --cov-report=xml:coverage_valkey.xml -W always -m 'not onlycluster' --junit-xml=standalone-results.xml"
)
run(f"pytest {shared_args} --junit-xml=standalone-results.xml {pytest_args}")


@task
def cluster_tests(c, uvloop=False, protocol=2, color=False):
def cluster_tests(c, uvloop=False, protocol=2, color=False, pytest_args=None):
"""Run tests against a valkey cluster"""
cluster_url = "valkey://localhost:16379/0"
color_arg = f"--color={'yes' if color else 'no'}"
shared_args = f"{color_arg} --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not valkeymod' --valkey-url={cluster_url}"
pytest_args = pytest_args or ""
Copy link
Member

Choose a reason for hiding this comment

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

Should we somehow try to validate pytest_args ?
As it is, it can be used to inject any kind of command (I don't mean just any arguments, you can use it to run any executable).
We, probably, shouldn't be too concerned about a malicious actor slipping some bad command in this argument, as it seems an unlikely scenario. But it may be possible for someone to inadvertently do something bad/wrong. Especially since complex argument passing will require a lot of escaping to be done correctly in this way.
I'm not sure if it's possible to sanitize this command in any reasonable way returning intelligible errors (most likely not). Maybe it would be better to handle the main pytest arguments as separate args here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can do this, yes. This approach leaves it to user's discretion. After all, any arguments they would pass they would run on their machine :)

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that this argument seem to be not easy to use, but it's easy to misuse.
Not easy to use: because you need to put your desired list of arguments in quotes but the most typical argument you may want to pass (i.e. -k) often needs quotes itself, so you need to use the right type of quotes or use escapes.
Easy to misuse: because I wouldn't expect a command like invoke tests --pytest-args="-k test ; rm -rf /" to wipe my HD. This is an extreme example, but still you may forgot some quotes here and there and run a completely different set of tests and get a green build when it should be red or the opposite.

if uvloop:
run(
f"pytest --color={'yes' if color else 'no'} --protocol={protocol} --cov=./ --cov-report=xml:coverage_cluster.xml -W always -m 'not onlynoncluster and not valkeymod' --valkey-url={cluster_url} --junit-xml=cluster-uvloop-results.xml --uvloop"
f"pytest {shared_args} --junit-xml=cluster-uvloop-results.xml --uvloop {pytest_args}"
)
else:
run(
f"pytest --color={'yes' if color else 'no'} --protocol={protocol} --cov=./ --cov-report=xml:coverage_clusteclient.xml -W always -m 'not onlynoncluster and not valkeymod' --valkey-url={cluster_url} --junit-xml=cluster-results.xml"
)
run(f"pytest {shared_args} --junit-xml=cluster-results.xml {pytest_args}")


@task
Expand Down
Loading