chore: refactor spec test runner #66

wemeetagain · 2025-10-14T21:38:47Z

Consolidate logic for the operations spec runner
Remove external test logger (tests can be more logged via --summary all)
Properly handle bls setting (eg by reading meta.yaml)
Properly handle preset (-Dpreset=minimal) (there are currently bugs, the minimal preset needs work)
Further runners can be added using a similar format:
- Add a new file/struct in test/spec/test_types that handles running the runner type
- Add a new file/function in test/spec/writer that handles writing the tests for that runner type (and add it in test/spec/writer/root
- Add the runner to the supported runners in test/spec/write_spec_tests.zig
- Add the generated file in test/spec/root.zig

gemini-code-assist · 2025-10-14T21:39:10Z

Summary of Changes

Hello @wemeetagain, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the spec test runner infrastructure, particularly for operations tests. The primary goal is to streamline the test generation and execution process, making it more robust and extensible. Key improvements include consolidating test logic, removing a custom logger in favor of native Zig testing features, and enhancing the handling of configuration parameters like BLS settings and presets. This change sets a clearer path for future additions of new test types and improves the overall maintainability of the test suite.

Highlights

Operations Spec Runner Refactor: The core logic for the operations spec test runner has been consolidated and rewritten, moving from a monolithic test/spec/test_case/operations.zig to a more modular structure within test/spec/test_type/operations.zig and test/spec/writer/operations.zig.
External Test Logger Removal: The custom external test logger (test/spec/utils/test_logger.zig) has been removed, with tests now relying on standard Zig test output, which can be configured for detailed logging via --summary all.
BLS Setting Handling: The test runner now properly handles BLS settings by reading the bls_setting from meta.yaml files within test directories, allowing for more accurate test execution based on BLS requirements.
Preset Handling: The system now attempts to properly handle different presets (e.g., minimal) during test generation and execution, though the PR description notes that the minimal preset still requires further work.
New Runner Integration Guidelines: Clear guidelines have been established for adding new test runners, involving creating new files/structs in test/spec/test_type for running, new files/functions in test/spec/writer for writing tests, and updating test/spec/write_spec_tests.zig and test/spec/root.zig.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request is a great refactoring of the spec test runner, consolidating logic and improving the structure for future extensions. The changes make the test generation and execution more modular and easier to understand.

I've found a few issues:

A critical bug in the handling of attestations for different forks in test/spec/test_type/operations.zig.
A couple of correctness issues in src/preset/preset.zig and test/spec/test_case.zig.
Some opportunities to improve code quality by reducing duplication and adhering more closely to the style guide, particularly regarding assertions.

Once these points are addressed, this will be a solid improvement.

test/spec/test_type/operations.zig

src/preset/preset.zig

test/spec/test_case.zig

src/state_transition/test_utils/generate_state.zig

test/spec/test_case.zig

test/spec/test_type/operations.zig

test/spec/writer/operations.zig

ensi321 · 2025-10-15T10:12:50Z

test/spec/writer/operations.zig

+    \\
+;
+
+pub fn writeOperationsTests(comptime forks: []const ForkSeq, writer: std.io.AnyWriter) !void {


I think this file reading logic will be reused so many times for other runners.
Better write a generic version of it.

oh, bring back listAllDirectories? It can be used in the innermost loop here. But the outer loops can't really be reused since we're not iterating thru file contents, generically, rather iterating thru fork and operation slices.

ok take a look now (at write_spec_tests.zig). More of a framework for handling different runners and handlers and a unified directory walker there.

spiral-ladder · 2025-10-15T14:44:36Z

src/state_transition/test_utils/generate_state.zig

 const BLSPubkey = ssz.primitive.BLSPubkey.Type;
 const ValidatorIndex = ssz.primitive.ValidatorIndex.Type;
 const preset = @import("preset").preset;
+const Preset = @import("preset").Preset;


I find this and @import("preset").preset to be pretty confusing

yeah its not great, suggestions?

#66 (comment) possible suggestion?

I'll take a deeper look at the preset module in another PR, it needs a bit of love

test/spec/test_type/operations.zig

Co-authored-by: bing <[email protected]>

spiral-ladder · 2025-10-15T15:43:58Z

src/state_transition/test_utils/generate_state.zig

 const Index2PubkeyCache = state_transition.Index2PubkeyCache;
 const syncPubkeys = state_transition.syncPubkeys;
 const interopPubkeysCached = @import("./interop_pubkeys.zig").interopPubkeysCached;
+const active_chain_config = if (preset.preset == Preset.mainnet) mainnet_chain_config else minimal_chain_config;


we're already depending on build_options, can we just do this maybe?

Suggested change

const active_chain_config = if (preset.preset == Preset.mainnet) mainnet_chain_config else minimal_chain_config;

const preset_str = @import("build_options").preset;

const active_chain_config = if (std.mem.eql(u8, preset_str, "mainnet")) mainnet_chain_config else minimal_chain_config;

if i'm not wrong, the preset field is only used to check this, so we can just get rid of that if we do this possibly

preset is used in a few places in this file

By preset field, I meant preset.preset (the enum stored in the preset struct)

oh, you're talking about preset.preset.

I'd like to do some refactoring of the preset module in another PR to have the following behavior:

move all preset constants for the active preset to be re-exported from the root (eg @import("preset").SHUFFLE_ROUND_COUNT)

its annoying and duplicative to store the values on a decl named preset

re-export active_preset from the root (eg @import("preset").active_preset)

its a nice quality-of-life feature to have the active Preset to compare against (doing string comparison everywhere against the build_options preset is sloppy)

could be named preset_base or something else if active_preset is confusing (this name is taken from lodestar)

re-export the Preset struct from the root

no change from existing behavior

re-export each set of preset constants under a decl on the root (eg @import("preset").minimal.SHUFFLE_ROUND_COUNT)

we're already declaring this stuff, may as well make it available for general usage

chore: refactor spec test runner

256d0cf

gemini-code-assist bot reviewed Oct 14, 2025

View reviewed changes

Merge branch 'nc/spec-test' into cayman/refactor-spec-tests

249aa39

ensi321 reviewed Oct 15, 2025

View reviewed changes

ensi321 mentioned this pull request Oct 15, 2025

feat: add minimal framework for spec test #46

Open

wemeetagain added 2 commits October 15, 2025 10:13

chore: pr review

3cf0bc6

chore: update operations writer

639baff

spiral-ladder reviewed Oct 15, 2025

View reviewed changes

Update test/spec/test_type/operations.zig

a2d07c5

Co-authored-by: bing <[email protected]>

spiral-ladder reviewed Oct 15, 2025

View reviewed changes

wemeetagain added 4 commits October 15, 2025 16:28

chore: pr review

492ffe8

chore: pr review

746c4e5

chore: pr review

26f2feb

chore: more polish and framework

91ab31c

-const active_chain_config = if (preset.preset == Preset.mainnet) mainnet_chain_config else minimal_chain_config;
+const preset_str = @import("build_options").preset;
+const active_chain_config = if (std.mem.eql(u8, preset_str, "mainnet")) mainnet_chain_config else minimal_chain_config;

chore: refactor spec test runner #66

Are you sure you want to change the base?

chore: refactor spec test runner #66

Uh oh!

Conversation

wemeetagain commented Oct 14, 2025

Uh oh!

gemini-code-assist bot commented Oct 14, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wemeetagain Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spiral-ladder Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wemeetagain Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wemeetagain Oct 15, 2025 •

edited

Loading

spiral-ladder Oct 15, 2025 •

edited

Loading

wemeetagain Oct 15, 2025 •

edited

Loading