-
Notifications
You must be signed in to change notification settings - Fork 169
feat(cli): add filter_fixtures
cli command
#2045
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
base: main
Are you sure you want to change the base?
Conversation
This funtion was added to handle transition forks.
with before_index_path.open() as f: | ||
before_index = json.load(f) | ||
|
||
with after_index_path.open() as f: | ||
after_index = json.load(f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test function is critical to verify releases in CI: It should be ran (see docstring/PR description) to compare the original (fixtures_develop) and filtered output (fixtures_stable) to automatically sanity check them and provide console output for an easy manual visual check.
Note, this is only validating via index files. filter_fixtures
will generate a new index.json for the new fixtures output dir via genindex
. But, in a way, the verification here is a "light" verification. Perhaps we should do a full verification by loading all fixture files ourselves here, but perhaps not, I mean this is what genindex
does anyway... Will leave as-is for now. Happy to hear your feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, we should first merge #2051, apply the comments, and then merge this one, because otherwise the logic to select the forks is going to differ from the logic that we use in fill
.
@click.option("--fork", "single_fork", help="Only include fixtures for the specified fork.") | ||
@click.option( | ||
"--from", "forks_from", help="Include fixtures from and including the specified fork." | ||
) | ||
@click.option( | ||
"--until", "forks_until", help="Include fixtures until and including the specified fork." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use ForkSetAdapter
from #2051 to make this a bit easier.
Then:
@click.option("--fork", "single_fork", help="Only include fixtures for the specified fork.") | |
@click.option( | |
"--from", "forks_from", help="Include fixtures from and including the specified fork." | |
) | |
@click.option( | |
"--until", "forks_until", help="Include fixtures until and including the specified fork." | |
) | |
@click.option( | |
"--fork", | |
"single_fork", | |
type=ForkSetAdapter.validate_python, | |
default=set(), | |
help="Only include fixtures for the specified fork.", | |
) | |
@click.option( | |
"--from", | |
"from_fork", | |
type=ForkSetAdapter.validate_python, | |
default=set(), | |
help="Include fixtures from and including the specified fork.", | |
) | |
@click.option( | |
"--until", | |
"until_fork", | |
type=ForkSetAdapter.validate_python, | |
default=set(), | |
help="Include fixtures until and including the specified fork.", | |
) |
single_fork_obj, from_fork, until_fork = get_valid_fork_range( | ||
single_fork, forks_from, forks_until | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed with the previous comment.
def should_include_fixture_fork( | ||
fixture_fork: Type[BaseFork], | ||
single_fork: Type[BaseFork] | None, | ||
from_fork: Type[BaseFork] | None, | ||
until_fork: Type[BaseFork] | None, | ||
) -> bool: | ||
""" | ||
Determine if a fixture's fork should be included based on the filtering criteria. | ||
|
||
This uses fork comparison operators to handle both regular and transition forks. | ||
""" | ||
if single_fork: | ||
return fixture_fork == single_fork | ||
|
||
# Default range is all deployed forks if no range specified | ||
if not from_fork and not until_fork: | ||
deployed_forks = get_deployed_forks() | ||
return deployed_forks[0] <= fixture_fork <= deployed_forks[-1] | ||
|
||
# Handle range filtering | ||
from_ok = True | ||
until_ok = True | ||
|
||
if from_fork: | ||
from_ok = fixture_fork >= from_fork | ||
|
||
if until_fork: | ||
until_ok = fixture_fork <= until_fork | ||
|
||
return from_ok and until_ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use the get_selected_fork_set
function introduced in #2051 to match the exact logic that fill
uses.
The result of that function is a set, so you can do fork in get_selected_fork_set(...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole folder can be generated on the fly. IMO we should not be adding this much static files for an unit test for a subcommand.
all_forks_chronological = get_all_forks_chronologically() | ||
fork_names_ordered = [fork.name() for fork in all_forks_chronological] | ||
|
||
# Filter to only forks present in either directory | ||
relevant_forks = [ | ||
fork_name | ||
for fork_name in fork_names_ordered | ||
if fork_name in before_forks or fork_name in after_forks | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this seems unnecessary (including the new get_all_forks_chronologically
).
Let's remove most of the fork logic in here now and just reuse the ForkSetAdapter.validate_python(config_str)
and get_selected_fork_set
in here:
execution-spec-tests/src/pytest_plugins/forks/forks.py
Lines 454 to 479 in 961cdc8
def get_fork_option(config, option_name: str, parameter_name: str) -> Set[Fork]: | |
"""Post-process get option to allow for external fork conditions.""" | |
config_str = config.getoption(option_name) | |
try: | |
return ForkSetAdapter.validate_python(config_str) | |
except InvalidForkError: | |
print( | |
f"Error: Unsupported fork provided to {parameter_name}:", | |
config_str, | |
"\n", | |
file=sys.stderr, | |
) | |
print(available_forks_help, file=sys.stderr) | |
pytest.exit("Invalid command-line options.", returncode=pytest.ExitCode.USAGE_ERROR) | |
single_fork = get_fork_option(config, "single_fork", "--fork") | |
forks_from = get_fork_option(config, "forks_from", "--from") | |
forks_until = get_fork_option(config, "forks_until", "--until") | |
show_fork_help = config.getoption("show_fork_help") | |
dev_forks_help = textwrap.dedent( | |
"To run tests for a fork under active development, it must be " | |
"specified explicitly via --until=FORK.\n" | |
"Tests are only ran for deployed mainnet forks by default, i.e., " | |
f"until {get_deployed_forks()[-1].name()}.\n" | |
) |
🗒️ Description
Implements a new CLI command to filter fixture files by fork, enabling creation of stable release sets from development fixtures.
Example Usage
Filter out Osaka, ToOsaka transition forks (and newer forks):
Filter specific fork only
CI Release Workflow
This cli tool will be a critical element of the fixture release flow. The unit test
pytest src/cli/tests/test_filter_fixtures.py::test_ci_release_validation
aims to allow sanity checking of the filtered fixture output directoryfixtures_stable
and provides verbose output for easy manual verification.Integration in CI is to be done in a follow-up PR.
The idea is:
🔗 Related Issues or PRs
Partially resolves:
✅ Checklist
tox
checks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlint
type(scope):
.