test(stf): add unit tests for initiateValidatorExit#281
test(stf): add unit tests for initiateValidatorExit#281lodekeeper-z wants to merge 1 commit intoChainSafe:mainfrom
Conversation
Tests cover: - No-op when validator already has exit epoch set - Sets exit and withdrawable epochs correctly (electra) - Multiple sequential exits produce valid exit epochs - Second call on same validator is a no-op (idempotent) 🤖 Generated with AI assistance
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 reliability of the codebase by introducing unit tests for the validator exit initiation logic. By validating the state transitions under various conditions, these tests ensure that the exit and withdrawable epoch calculations are performed correctly and consistently within the Electra fork context. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds unit tests for the initiateValidatorExit function, covering no-op cases, successful epoch assignments, and idempotency. The review feedback suggests improving the test suite by extracting common setup logic to reduce duplication and using descriptive variable names to align with the style guide. Additionally, the reviewer recommends increasing assertion density by verifying withdrawable_epoch in no-op cases and validating the relationship between sequential exit epochs.
| const allocator = std_testing.allocator; | ||
| const num_validators = 256; | ||
| const pool_size = num_validators * 5; | ||
| var pool = try Node.Pool.init(allocator, pool_size); | ||
| defer pool.deinit(); | ||
|
|
||
| var test_state = try TestCachedBeaconState.init(allocator, &pool, num_validators); | ||
| defer test_state.deinit(); | ||
|
|
||
| const state = test_state.cached_state.state.castToFork(.electra); |
There was a problem hiding this comment.
There is significant code duplication in the setup phase of all four new tests. Consider extracting this common setup logic into a helper function to improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle. A helper function could initialize TestCachedBeaconState and return the necessary context (test_state, state, epoch_cache, etc.), reducing boilerplate in each test.
References
- The style guide emphasizes simplicity and avoiding duplication (line 311: "Don't duplicate variables or take aliases to them"). While this rule is about runtime variables, the principle of avoiding duplication to improve maintainability is a core concept. (link)
| try initiateValidatorExit(.electra, test_state.config, epoch_cache, state, validator); | ||
|
|
||
| // exit_epoch should remain unchanged | ||
| try std_testing.expectEqual(@as(u64, 10), try validator.get("exit_epoch")); |
There was a problem hiding this comment.
This test correctly checks that exit_epoch is unchanged for an already-exited validator. However, the setup on line 96 also sets withdrawable_epoch. To make the test more robust and ensure there are no unintended side effects, please also assert that withdrawable_epoch remains unchanged. This also improves assertion density, as recommended by the style guide.
try std_testing.expectEqual(@as(u64, 10), try validator.get("exit_epoch"));
try std_testing.expectEqual(10 + test_state.config.chain.MIN_VALIDATOR_WITHDRAWABILITY_DELAY, try validator.get("withdrawable_epoch"));
References
- The style guide recommends a high density of assertions to ensure correctness (lines 51-55: "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."). Adding this check improves test robustness and assertion density. (link)
| var v0 = try validators.get(0); | ||
| var v1 = try validators.get(1); | ||
| var v2 = try validators.get(2); |
There was a problem hiding this comment.
The variable names v0, v1, and v2 are abbreviations. The repository style guide (line 221) advises: "Do not abbreviate variable names". Using more descriptive names like validator_0, validator_1, and validator_2 would improve readability and align with the style guide. Please update their usages throughout the test as well.
var validator_0 = try validators.get(0);
var validator_1 = try validators.get(1);
var validator_2 = try validators.get(2);
References
- The style guide (line 221) states: "Do not abbreviate variable names". This improves code clarity and maintainability. (link)
| try std_testing.expect(exit0 != FAR_FUTURE_EPOCH); | ||
| try std_testing.expect(exit1 != FAR_FUTURE_EPOCH); | ||
| try std_testing.expect(exit2 != FAR_FUTURE_EPOCH); |
There was a problem hiding this comment.
The test verifies that the validators have been assigned a valid exit epoch. To make the test more robust, it should also assert the relationship between the exit epochs. Since the validators are exited sequentially, their exit epochs should be non-decreasing (i.e., exit0 <= exit1 <= exit2).
try std_testing.expect(exit0 != FAR_FUTURE_EPOCH);
try std_testing.expect(exit1 != FAR_FUTURE_EPOCH);
try std_testing.expect(exit2 != FAR_FUTURE_EPOCH);
try std_testing.expect(exit0 <= exit1);
try std_testing.expect(exit1 <= exit2);
| try initiateValidatorExit(.electra, test_state.config, epoch_cache, state, validator); | ||
| const exit_epoch = try validator.get("exit_epoch"); | ||
| try std_testing.expect(exit_epoch != FAR_FUTURE_EPOCH); | ||
|
|
||
| // Second call — should be a no-op | ||
| try initiateValidatorExit(.electra, test_state.config, epoch_cache, state, validator); | ||
| try std_testing.expectEqual(exit_epoch, try validator.get("exit_epoch")); |
There was a problem hiding this comment.
This test correctly asserts that exit_epoch is not changed on the second call, confirming idempotency. For completeness, it should also assert that withdrawable_epoch is not changed, as this is also set by initiateValidatorExit. This ensures the no-op behavior applies to all fields modified by the function.
try initiateValidatorExit(.electra, test_state.config, epoch_cache, state, validator);
const exit_epoch = try validator.get("exit_epoch");
const withdrawable_epoch = try validator.get("withdrawable_epoch");
try std_testing.expect(exit_epoch != FAR_FUTURE_EPOCH);
// Second call — should be a no-op
try initiateValidatorExit(.electra, test_state.config, epoch_cache, state, validator);
try std_testing.expectEqual(exit_epoch, try validator.get("exit_epoch"));
try std_testing.expectEqual(withdrawable_epoch, try validator.get("withdrawable_epoch"));
References
- The style guide recommends a high density of assertions to ensure correctness (lines 51-55). Adding this check for
withdrawable_epochensures all aspects of the function's idempotency are tested. (link)
1 participant
Summary
Adds 4 unit tests for
initiateValidatorExit, which previously had zero test coverage.Tests
computeExitEpochAndUpdateChurn, withdrawable = exit + MIN_VALIDATOR_WITHDRAWABILITY_DELAYAll tests use
TestCachedBeaconStatewith electra fork (current fork).🤖 Generated with AI assistance