feat(consume): add --extract-to parameter for direct fixture extraction
#1861
Conversation
There was a problem hiding this comment.
Awesome! Thanks a lot for this.
I tried it directly from your branch and it just worked:
uvx --from git+https://github.com/richardgreg/execution-spec-tests@feat/extract-to-flag-for-consume \
consume cache [email protected] --extract-to=./zkevm-fixtures
Updated https://github.com/richardgreg/execution-spec-tests (bee9a798163e6f97aa2350c81a2214247790c1af)
Updated https://github.com/spencer-tb/ethereum-spec-evm-resolver (ee273e7344e24a739ebfbf0ea1f758530c4d032b)
Built ethereum-execution-spec-tests @ git+https://github.com/richardgreg/execution-spec-tests@bee9a798163e6f97aa2350c81a2214247790c1af
Installed 70 packages in 7ms
Exit: Fixtures downloaded and extracted to specified directory.
Path: zkevm-fixtures/fixtures
Input: https://github.com/ethereum/execution-spec-tests/releases/download/zkevm%40v0.1.0/fixtures_zkevm.tar.gz
Release page: https://github.com/ethereum/execution-spec-tests/releases/tag/zkevm%40v0.1.0
However, I think we can simplify the logic here and achieve the same goal:
- See the "major quibble" 😆 in the
FixtureDownloaderbelow, and, - The over-complicated logic in
pytest_configure()-FixturesSourceexists to not have so much of this boilerplate present inpytest_configure().
Perhaps you can give add this feedback to Claude's context and he can refactor it for you. Make a backup of your branch before you start just in case. You can also make a separate PR if that's easier (if so move this PR to "Draft") and then we can merge the other one if it's cleaner.
Thanks for work @richardgreg.
| if ( | ||
| hasattr(config.option, "extract_to_folder") | ||
| and config.option.extract_to_folder is not None | ||
| ): |
There was a problem hiding this comment.
This condition should be something more inline with the checks below. Something like:
| if ( | |
| hasattr(config.option, "extract_to_folder") | |
| and config.option.extract_to_folder is not None | |
| ): | |
| if config.fixtures_source.extract_to_local_path: |
This attribute will of course have to be added to the FixturesSource class and set appropriately.
| # Validate --extract-to usage | ||
| if hasattr(config.option, "extract_to_folder") and config.option.extract_to_folder is not None: | ||
| if "cache" not in sys.argv: | ||
| pytest.exit("The --extract-to flag is only valid with the 'cache' command.") |
There was a problem hiding this comment.
I think checking whether the extract_to_folder is available is overly cautious and unnecessary as it's defined above in this plugin. If that is removed, you could try simplifying the condition here and keeping it to a single if statement. Similar to the other pytest.exit() condition below in L401.
| extract_to_path = None | ||
| if ( | ||
| hasattr(config.option, "extract_to_folder") | ||
| and config.option.extract_to_folder is not None | ||
| ): | ||
| extract_to_path = Path(config.option.extract_to_folder) | ||
|
|
There was a problem hiding this comment.
FixturesSource exists to abstract/avoid this kind of code in pytest_configure(). I would suggest removing this check and passing config.option.extract_to_folder directly to FixturesSource.from_input(). Then FixturesSource should handle the option.
| help=( | ||
| "Extract downloaded fixtures to the specified directory. Only valid with 'cache' " | ||
| "command. When used, fixtures are extracted directly to this path instead of the " | ||
| "cache structure." |
There was a problem hiding this comment.
| "cache structure." | |
| "user's execution-spec-tests cache directory." |
| def extract_to_directory(self, target_dir: Path) -> Path: | ||
| """ | ||
| Download and extract fixtures directly to the specified directory. | ||
| This bypasses the normal cache structure and extracts to the target directory. | ||
| """ | ||
| # Download the archive to a temporary location first | ||
| temp_archive = target_dir.parent / f"temp_{self.archive_name}.tar.gz" | ||
| response = requests.get(self.url) | ||
| response.raise_for_status() | ||
|
|
||
| # Write the archive to temp location | ||
| temp_archive.write_bytes(response.content) | ||
|
|
||
| target_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Extract directly to target directory | ||
| with tarfile.open(temp_archive, mode="r:gz") as tar: | ||
| tar.extractall(path=target_dir, filter="data") | ||
|
|
||
| temp_archive.unlink() | ||
|
|
||
| # Detect if there's a single top-level directory and return its path | ||
| extracted_dirs = [d for d in target_dir.iterdir() if d.is_dir() and d.name != ".meta"] | ||
| return extracted_dirs[0] if len(extracted_dirs) == 1 else target_dir |
There was a problem hiding this comment.
This is my major quibble with this PR as-is: It duplicates quite a bit of code from fetch_and_extract().
The CLI UX and the hooks pytest_addoption() and pytest_configure() could stay the same... and I'm not entirely sure, but I've got a hunch that FixturesSource could be refactored to combine the cache_folder and extract_to values to one variable which holds the target destination for the folder. Then FixtureDownloader could be agnostic as to whether we're extracting to a cache folder or a user-specified local folder.
danceratopz
changed the title
--extract-to parameter for direct fixture extraction
Add new --extract-to flag to consume cache command that extracts fixtures directly to a specified directory, bypassing the normal cache structure. This replaces the need for separate download scripts like 'download-and-extract-fixtures.sh' by integrating the functionality into the existing consume tooling. Usage: uvx --from git+https://github.com/ethereum/execution-spec-tests \ consume cache [email protected] --extract-to=./zkevm-fixtures
richardgreg
force-pushed
the
feat/extract-to-flag-for-consume
branch
from
bee9a79 to
b16f567
Compare
🗒️ Description
Add new --extract-to flag to consume cache command that extracts fixtures directly to a specified directory, bypassing the normal cache structure. This replaces the need for separate download scripts like 'download-and-extract-fixtures.sh' by integrating the functionality into the existing consume tooling.
Usage:
🔗 Related Issues or PRs
Fixes #1835
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlinttype(scope):.