tasks: added --pytest-args to tests #100
Conversation
Fixes #62 Signed-off-by: Mikhail Koviazin <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
- Coverage 75.18% 75.17% -0.01%
==========================================
Files 132 132
Lines 34480 34480
==========================================
- Hits 25925 25922 -3
- Misses 8555 8558 +3 ☔ View full report in Codecov by Sentry. |
| @@ -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( | |||
There was a problem hiding this comment.
Am I missing something or is this just a formatting change?
There was a problem hiding this comment.
Yeah autoformatting changed this line for me :/ Do you want me to move it to a separate commit?
| """Run the valkey-py test suite against the current python, | ||
| with and without libvalkey. | ||
| """ | ||
| print("Starting Valkey tests") | ||
| print("pytest_args:", pytest_args) |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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?
| """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 "" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
|
Closed in favor of #106 |
Pull Request check-list
Description of change
Adds support for arbitrary pytest args for
invoke tests/standalone_tests/cluster_testsFixes #62