-
Notifications
You must be signed in to change notification settings - Fork 182
Description
Thanks a lot for tackling this @LouisTsai-Csie! The PR description is really helpful and documents the expectation very well 🙏
The benchmark paths aren't correct in the following, but it should explain my suggestion.
With the additional requirement to "filter" stateful
tests, I'm wondering if we're adding too much complexity to our pytest setup via markers in order to run the correct subset of tests. The logic alone for tests/benchmark/conftest.py
is quite complex and then we overlay the logic from tests/benchmark/stateful/conftest.py
on top of this.
I won't block this, but I think the requirement is actually quite simple and perhaps we can quickly consider an alternative approach before merging. My proposal would be to remove all of this conftest logic and use an approach that modifies pytest's testpaths
at the CLI logic stage.
I.e., adding a mode
flag, None
by default (=just fill regular tests), but that can be set to "benchmark" or "stateful".
uv run fill --mode=benchmark
Then, we add (simpler) logic in src/cli/pytest_commands
to modify fill and execute's ini config from (and others...):
- https://github.com/LouisTsai-Csie/execution-spec-tests/blob/7e4d3e214252fa878a7ac5c381fcab0d48d1c40a/src/cli/pytest_commands/pytest_ini_files/pytest-fill.ini#L5
https://github.com/LouisTsai-Csie/execution-spec-tests/blob/7e4d3e214252fa878a7ac5c381fcab0d48d1c40a/src/cli/pytest_commands/pytest_ini_files/pytest-execute.ini#L5
to overwite testpaths
with the relevant path (needs a well-organized structure underneath ./tests/benchmarks/
). For this, we don't modify or add new ini files, but rather modify it using this functionality:
-o testpaths=tests/benchmark/stateful
From pytest --help
:
-o OVERRIDE_INI, --override-ini=OVERRIDE_INI
Override ini option with "option=value" style, e.g. `-o
xfail_strict=True -o cache_dir=cache`.
For normal filling mode (no benchmarking), we can append:
uv run pytest --help | grep ignore
--ignore=path Ignore path during collection (multi-allowed)
to ignore ./tests/benchmarks
in regular filling.
Originally posted by @danceratopz in #2169 (review)