test(epoch): add unit tests for inactivity leak functions#262
test(epoch): add unit tests for inactivity leak functions#262lodekeeper-z wants to merge 4 commits intoChainSafe:mainfrom
Conversation
Add unit tests for getFinalityDelay and isInInactivityLeak covering: - Normal delay calculation - Boundary conditions (delay exactly at threshold) - Edge cases (epoch=1, finalized=0) - Leak detection above/below MIN_EPOCHS_TO_INACTIVITY_PENALTY 🤖 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 significantly improves the reliability of the epoch state transition logic by introducing dedicated unit tests for the 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 valuable unit tests for getFinalityDelay and isInInactivityLeak, which previously had no test coverage. The tests cover normal operation, edge cases, and boundary conditions correctly. My review includes a couple of suggestions to improve the test comments to better align with the project's style guide, specifically regarding test descriptions and comment formatting. Overall, this is a good addition to improve code quality and robustness.
| test "getFinalityDelay" { | ||
| // current_epoch=10, finalized_epoch=5: previous=9, delay=9-5=4 | ||
| try std.testing.expectEqual(@as(u64, 4), getFinalityDelay(10, 5)); | ||
| // current_epoch=10, finalized_epoch=9: previous=9, delay=0 | ||
| try std.testing.expectEqual(@as(u64, 0), getFinalityDelay(10, 9)); | ||
| // current_epoch=10, finalized_epoch=0: previous=9, delay=9 | ||
| try std.testing.expectEqual(@as(u64, 9), getFinalityDelay(10, 0)); | ||
| // edge: current_epoch=1, finalized_epoch=0: previous=0, delay=0 | ||
| try std.testing.expectEqual(@as(u64, 0), getFinalityDelay(1, 0)); | ||
| } |
There was a problem hiding this comment.
The test comments could be improved to better align with the repository's style guide. Rule #301 suggests adding a description at the top of tests to explain their goal, and Rule #304 specifies that comments on their own line should be sentences. Restructuring the comments will improve readability and adherence to the style guide.
test "getFinalityDelay" {
// Tests the calculation of finality delay for various epoch differences.
// Case: current_epoch=10, finalized_epoch=5 -> previous=9, delay=4.
try std.testing.expectEqual(@as(u64, 4), getFinalityDelay(10, 5));
// Case: current_epoch=10, finalized_epoch=9 -> previous=9, delay=0.
try std.testing.expectEqual(@as(u64, 0), getFinalityDelay(10, 9));
// Case: current_epoch=10, finalized_epoch=0 -> previous=9, delay=9.
try std.testing.expectEqual(@as(u64, 9), getFinalityDelay(10, 0));
// Edge case: current_epoch=1, finalized_epoch=0 -> previous=0, delay=0.
try std.testing.expectEqual(@as(u64, 0), getFinalityDelay(1, 0));
}
References
- Rule feat: add clock module for slot/epoch timing (Zig 0.16) #301: Tests should include a top-level description of their goal and methodology to help readers understand the test's purpose without needing to read the implementation. (link)
- Rule test: add negative BLS verify and FAV cases #304: Comments should be formatted as complete sentences with proper capitalization and punctuation. Comments on their own line should be sentences, while comments after a line of code can be phrases. (link)
| test "isInInactivityLeak" { | ||
| // MIN_EPOCHS_TO_INACTIVITY_PENALTY = 4 | ||
| // Not in leak: delay <= 4 | ||
| try std.testing.expect(!isInInactivityLeak(10, 9)); // delay=0 | ||
| try std.testing.expect(!isInInactivityLeak(10, 5)); // delay=4 (not strictly greater) | ||
|
|
||
| // In leak: delay > 4 | ||
| try std.testing.expect(isInInactivityLeak(10, 4)); // delay=5 | ||
| try std.testing.expect(isInInactivityLeak(10, 0)); // delay=9 | ||
| try std.testing.expect(isInInactivityLeak(100, 0)); // delay=99 | ||
| } |
There was a problem hiding this comment.
Similar to the getFinalityDelay test, the comments in this test block can be improved to follow the style guide. Adding a descriptive comment at the top (Rule #301) and rephrasing comments to be full sentences (Rule #304) will enhance clarity and consistency.
test "isInInactivityLeak" {
// Tests the inactivity leak condition by checking boundaries around MIN_EPOCHS_TO_INACTIVITY_PENALTY.
// An inactivity leak occurs if the finality delay is strictly greater than 4.
// Test cases that are not in an inactivity leak (delay <= 4).
try std.testing.expect(!isInInactivityLeak(10, 9)); // delay=0
try std.testing.expect(!isInInactivityLeak(10, 5)); // delay=4 (not strictly greater)
// Test cases that are in an inactivity leak (delay > 4).
try std.testing.expect(isInInactivityLeak(10, 4)); // delay=5
try std.testing.expect(isInInactivityLeak(10, 0)); // delay=9
try std.testing.expect(isInInactivityLeak(100, 0)); // delay=99
}
References
- Rule feat: add clock module for slot/epoch timing (Zig 0.16) #301: Tests should include a top-level description of their goal and methodology to help readers understand the test's purpose without needing to read the implementation. (link)
- Rule test: add negative BLS verify and FAV cases #304: Comments should be formatted as complete sentences with proper capitalization and punctuation. Comments on their own line should be sentences, while comments after a line of code can be phrases. (link)
|
made these changes:
|
3 participants
Add unit tests for
getFinalityDelayandisInInactivityLeakininactivity_leak.zig.Coverage added:
getFinalityDelay: normal delay calculation, edge cases (epoch=1)isInInactivityLeak: boundary atMIN_EPOCHS_TO_INACTIVITY_PENALTY(delay=4 → no leak, delay=5 → leak), extreme casesThese are pure functions with zero prior test coverage.
🤖 Generated with AI assistance