Skip to content

feat(fill): add --generate-all-formats to enable generation of BlockhainEngineXFixture with other formats #1855

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

Merged
merged 10 commits into from
Jul 14, 2025

Conversation

danceratopz
Copy link
Member

@danceratopz danceratopz commented Jul 3, 2025

🗒️ Description

TLDR: This change will generate all fixture fixture formats for both benchmark and normal releases.

The plugin gets 4 modes of operation:

class ExecutionPhase(Enum):
"""Execution phase for fixture generation."""
NORMAL = "normal"
PHASE_1_PREALLOC = "phase_1_prealloc"
PHASE_2_ENGINE_X_ONLY = "phase_2_engine_x_only"
PHASE_2_ALL_FORMATS = "phase_2_all_formats"

  • About NORMAL - this maintains previous behavior w/o --generate-all-formats flag.

Otherwise, this PR:

  • Adds --generate-all-formats flag to enable BlockchainEngineXFixture generation alongside other fixture formats.
  • Auto-enables --generate-all-formats when --output specifies a .tar.gz file.
  • Enables generating all fixture formats in a single command with two-phase execution.
  • All formats are generated in phase 2 (phase 1 is the pre-sweep for engine-x fixtures).

I don't think, we need to change anything under .github/, as all formats are generated by default if output is a tarball.

Usage

# Generate all fixture formats explicitly
uv run fill --output=fixtures-tmp tests/istanbul/eip1344_chainid/ --clean --generate-all-formats -v

# Auto-enabled for tarball output
uv run fill --output=fixtures-tmp.tar.gz tests/istanbul/eip1344_chainid/ --clean -v

Both approaches perform two-phase execution to generate all supported fixture formats.

🔗 Related Issues or PRs

Follow-up to:

Fixes #1778

✅ Checklist

  • All: Ran fast 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
  • All: PR title adheres to the repo standard - it will be used as the squash commit message and should start type(scope):.
  • All: Considered adding an entry to CHANGELOG.md.
  • All: Considered updating the online docs in the ./docs/ directory.
  • All: Set appropriate labels for the changes (only maintainers can apply labels).

… generation

Add new --generate-all-formats command line option that enables generation of all
fixture formats including BlockchainEngineXFixture in a single command execution.

Key changes:
- Add --generate-all-formats CLI option with two-phase execution support.
- Phase 1: Generate pre-allocation groups (same as --generate-pre-alloc-groups).
- Phase 2: Generate ALL supported fixture formats (vs only BlockchainEngineXFixture).
- Update FixtureOutput class to handle new generate_all_formats parameter.
- Preserve --generate-all-formats flag in phase 2 execution for proper format selection.
- Maintain backward compatibility with existing --generate-pre-alloc-groups workflow.

This makes BlockchainEngineXFixture a first-class citizen alongside other fixture
formats, enabling comprehensive test coverage generation in a single command while
leveraging the performance optimizations of pre-allocation groups.
@danceratopz danceratopz added type:feat type: Feature scope:fill Scope: fill command labels Jul 3, 2025
@danceratopz danceratopz requested a review from marioevz July 3, 2025 21:06
- Extract _should_use_two_phase_execution() method to simplify complex conditional logic.
- Extract _ensure_generate_all_formats_for_tarball() method for cleaner argument processing.
- Update create_executions() to use the new helper methods.
- Add should_auto_enable_all_formats property to clarify auto-enable logic.
- Improve variable naming: should_generate_all_formats instead of generate_all_formats.
- Fix duplication: use existing is_tarball property logic instead of creating duplicate method.
- Update tests to use new field name.
…hase enum

- Add ExecutionPhase enum with 4 distinct phases: NORMAL, PHASE_1_PREALLOC, PHASE_2_ENGINE_X_ONLY, PHASE_2_ALL_FORMATS.
- Extract _determine_execution_phase() method to encapsulate phase logic.
- Extract _is_blockchain_engine_x_fixture() helper for format checking.
- Extract _determine_fixture_formats() method to handle all 4 execution phases.
- Simplify pytest_generate_tests() from 40+ lines of nested conditionals to 2 clean lines.
Copy link
Member Author

@danceratopz danceratopz left a comment

Choose a reason for hiding this comment

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

@marioevz had another thought below on this change. Let me know what you think if you agree, I can make the changes in this PR or close this PR and make a new one. Perhaps it's good enough as an intermediate step.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes, I reverted mine and they can be moved to a separate PR, sorry about this.

@marioevz marioevz merged commit dfdd433 into main Jul 14, 2025
25 checks passed
@marioevz marioevz deleted the feat/fill-enable-all-formats branch July 14, 2025 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope:fill Scope: fill command type:feat type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include all test formats in benchmark releases
2 participants