test(spec): walk both presets so mainnet-only cases are emitted#341
test(spec): walk both presets so mainnet-only cases are emitted#341GrapeBaBa wants to merge 9 commits intoChainSafe:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! 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 improves the Ethereum Foundation spec test generation process by addressing an issue where asymmetric vector packs caused certain test cases to be omitted. By iterating over both minimal and mainnet presets and implementing a robust deduplication strategy, the generator now captures the full set of available test cases, increasing test coverage for mainnet operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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
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 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the test generation logic to support asymmetric vector packs by aggregating and sorting test cases from multiple presets, ensuring consistent output across runs. The review feedback identifies several violations of the repository's style guide, including multiple instances where the 100-column line limit is exceeded, a lack of sufficient assertions in new functions, and the use of abbreviated variable names. Additionally, a redundant assignment in the hash map insertion logic was noted for removal to align with the zero technical debt policy.
| @@ -92,55 +105,116 @@ pub fn writeTestRoot(comptime kinds: []const RunnerKind, writer: *std.Io.Writer) | |||
|
|
|||
| pub fn writeTests( | |||
There was a problem hiding this comment.
Adhere to the repository style guide (Rule 51) which requires an average of at least two assertions per function. This function currently has none. Consider adding assertions for preconditions, such as std.debug.assert(forks.len > 0);.
References
- Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. (link)
| @setEvalBranchQuota(8000); | ||
| try TestWriter(kind).writeHeader(writer); | ||
|
|
||
| var root_dir = try std.Io.Dir.openDir(.cwd(), io, spec_test_options.spec_test_out_dir ++ "/" ++ spec_test_options.spec_test_version, .{}); |
There was a problem hiding this comment.
This line exceeds the 100-column limit specified in the repository style guide (Rule 400).
var root_dir = try std.Io.Dir.openDir(
.cwd(),
io,
spec_test_options.spec_test_out_dir ++ "/" ++ spec_test_options.spec_test_version,
.{},
);
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
| } | ||
|
|
||
| for (presets) |preset| { | ||
| try collectCases(io, allocator, root_dir, preset, fork_path, comptime handler.suiteName(), comptime kind.hasSuiteCase(), &cases); |
There was a problem hiding this comment.
This line exceeds the 100-column limit (Rule 400).
try collectCases(
io,
allocator,
root_dir,
preset,
fork_path,
comptime handler.suiteName(),
comptime kind.hasSuiteCase(),
&cases,
);
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
| var it = cases.keyIterator(); | ||
| while (it.next()) |k| ordered.appendAssumeCapacity(k.*); |
There was a problem hiding this comment.
Avoid abbreviations in variable names as per Rule 221 of the repository style guide. Use iterator instead of it and key instead of k.
var iterator = cases.keyIterator();
while (iterator.next()) |key| ordered.appendAssumeCapacity(key.*);
References
- Do not abbreviate variable names, unless the variable is a primitive integer type used as an argument to a sort function or matrix calculation. (link)
| for (ordered.items) |key| { | ||
| if (comptime kind.hasSuiteCase()) { | ||
| const slash = std.mem.indexOfScalar(u8, key, '/').?; | ||
| try TestWriter(kind).writeTest(writer, fork, handler, key[0..slash], key[slash + 1 ..]); |
There was a problem hiding this comment.
This line exceeds the 100-column limit (Rule 400).
try TestWriter(kind).writeTest(
writer,
fork,
handler,
key[0..slash],
key[slash + 1 ..],
);
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
| } | ||
| } | ||
|
|
||
| fn collectCases( |
There was a problem hiding this comment.
This function lacks assertions, violating Rule 51 of the repository style guide. Consider asserting that preset.len > 0 and fork_path.len > 0.
References
- Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. (link)
| var suite_dir = handler_dir.openDir(io, entry.name, .{ .iterate = true }) catch |err| switch (err) { | ||
| error.FileNotFound => continue, | ||
| else => |e| return e, | ||
| }; |
There was a problem hiding this comment.
This block exceeds the 100-column limit (Rule 400).
var suite_dir = handler_dir.openDir(
io,
entry.name,
.{ .iterate = true },
) catch |err| switch (err) {
error.FileNotFound => continue,
else => |e| return e,
};
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
| var case_iter = suite_dir.iterate(); | ||
| while (try case_iter.next(io)) |case_entry| { | ||
| if (case_entry.kind != .directory) continue; | ||
| const key = try std.fmt.allocPrint(allocator, "{s}/{s}", .{ entry.name, case_entry.name }); |
There was a problem hiding this comment.
This line exceeds the 100-column limit (Rule 400).
const key = try std.fmt.allocPrint(
allocator,
"{s}/{s}",
.{ entry.name, case_entry.name },
);
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
| } | ||
| } | ||
|
|
||
| fn insertOrFree(allocator: std.mem.Allocator, set: *std.StringHashMapUnmanaged(void), owned_key: []const u8) !void { |
There was a problem hiding this comment.
The function signature exceeds the 100-column limit (Rule 400).
fn insertOrFree(
allocator: std.mem.Allocator,
set: *std.StringHashMapUnmanaged(void),
owned_key: []const u8,
) !void {
References
- Hard limit all line lengths, without exception, to at most 100 columns. (link)
| if (gop.found_existing) { | ||
| allocator.free(owned_key); | ||
| } else { | ||
| gop.key_ptr.* = owned_key; |
There was a problem hiding this comment.
The EF spec test pack ships asymmetric vector packs — for example ~97 case dirs only exist under `mainnet/tests/mainnet/.../operations/...` while hundreds only exist under minimal. The previous generator walked just `minimal/tests/minimal`, so any mainnet-only case was silently skipped at code-gen time and never reached the spec runner. Walk both presets and dedup the union by case key, so every case the EF pack ships gets a generated test. Adds ~97 mainnet operations cases without touching minimal coverage. Errors from `openDir` are split: `error.FileNotFound` is the legitimate "this preset/fork/handler is not present" gap, anything else is a real filesystem problem and propagates so a corrupt checkout fails loudly instead of silently dropping cases.
82e77c2 to
be97140
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…hack
Replaces StringHashMapUnmanaged<void> + manual `<suite>/<case>` string
concat + indexOfScalar split with:
- Struct key { suite, case } — no string concatenation
- ArrayListUnmanaged(Key) + sortUnstable + adjacent dedup
- Per-handler ArenaAllocator owning all dup'd slices, freed in one
arena.deinit() — no manual key-by-key free, no keyIterator/getOrPut
invariant to maintain
Generated test_case/*.zig files are byte-identical before/after.
Spec tests pass under -Dpreset=minimal.
|
@claude review |
|
@codex review |
|
|
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
The EF spec test pack ships asymmetric vector packs: ~97 case dirs only exist under
mainnet/tests/mainnet/.../operations/...(e.g.random_*_with_duplicatesforsync_aggregate), while hundreds only exist underminimal/tests/minimal/.... The current generator walks just one preset (minimal), so any mainnet-only case is silently dropped at codegen time and never reaches the spec runner.This PR makes
write_spec_tests.zigwalk both presets and dedup the union by case key, so every case the EF pack ships gets a generated test entry.Concrete impact (operations shard, mainnet)
+97newly emitted mainnet-only cases. Skip count is unchanged because those 167 are runner-handled skips, not codegen omissions.