test(utils): add unit tests for epoch utilities and intSqrt#259
test(utils): add unit tests for epoch utilities and intSqrt#259lodekeeper-z wants to merge 1 commit intoChainSafe:mainfrom
Conversation
Add 10 unit tests covering pure utility functions: - computeEpochAtSlot - computeStartSlotAtEpoch - computeEndSlotAtEpoch - computeCheckpointEpochAtStateSlot - computeActivationExitEpoch - computePreviousEpoch - computeSyncPeriodAtEpoch - computeSyncPeriodAtSlot - isStartSlotOfEpoch - intSqrt 🤖 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 enhances the robustness and reliability of core utility functions by introducing a suite of unit tests. These tests ensure the correctness of epoch-related calculations and the integer square root function, which are fundamental to the state transition logic. 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 a good set of unit tests for utility functions in epoch.zig and math.zig. The tests for epoch utilities improve coverage for these pure functions. My review uncovered a critical issue with the intSqrt function's implementation, which can produce incorrect results for large numbers due to floating-point precision loss. I've recommended a more robust implementation and an additional test case to cover this. I also found minor inconsistencies in type casting within the epoch utility tests and have suggested improvements for better clarity and consistency, in line with the project's style guide.
| test "intSqrt" { | ||
| try std.testing.expectEqual(@as(u64, 0), intSqrt(0)); | ||
| try std.testing.expectEqual(@as(u64, 1), intSqrt(1)); | ||
| try std.testing.expectEqual(@as(u64, 1), intSqrt(2)); | ||
| try std.testing.expectEqual(@as(u64, 1), intSqrt(3)); | ||
| try std.testing.expectEqual(@as(u64, 2), intSqrt(4)); | ||
| try std.testing.expectEqual(@as(u64, 3), intSqrt(9)); | ||
| try std.testing.expectEqual(@as(u64, 10), intSqrt(100)); | ||
| try std.testing.expectEqual(@as(u64, 31), intSqrt(999)); | ||
| try std.testing.expectEqual(@as(u64, 1000), intSqrt(1_000_000)); | ||
| // Large value. | ||
| try std.testing.expectEqual(@as(u64, 4_294_967_295), intSqrt(18_446_744_065_119_617_025)); | ||
| } |
There was a problem hiding this comment.
The intSqrt implementation is incorrect for large u64 values due to precision loss when converting to f64. f64 only has 53 bits of mantissa precision, while u64 has 64 bits. For numbers larger than 2^53, the conversion to f64 is lossy, leading to incorrect square root calculations.
A failing test case that demonstrates this is intSqrt(std.math.max_int(u64)). The expected result is 4294967295 (2^32 - 1), but the current implementation returns 4294967296 (2^32).
While the existing tests are good, they don't cover this critical edge case. Please add a test for u64.max to ensure the function's correctness. This will require fixing intSqrt.
Suggested test case to add:
// u64.max
try std.testing.expectEqual(@as(u64, 4_294_967_295), intSqrt(std.math.max_int(u64)));A correct implementation should use integer arithmetic. Here is a suggestion using binary search:
pub inline fn intSqrt(n: u64) u64 {
if (n < 2) {
return n;
}
var low: u64 = 1;
// The integer square root of a u64 is at most 2^32.
var high: u64 = 1 << 32;
var ans: u64 = 1;
while (low <= high) {
const mid = low + (high - low) / 2;
// Using division to avoid overflow with mid*mid
if (mid > n / mid) {
high = mid - 1;
} else {
ans = mid;
low = mid + 1;
}
}
return ans;
}| test "computeStartSlotAtEpoch" { | ||
| try std.testing.expectEqual(@as(Slot, 0), computeStartSlotAtEpoch(0)); | ||
| try std.testing.expectEqual(preset.SLOTS_PER_EPOCH, computeStartSlotAtEpoch(1)); | ||
| try std.testing.expectEqual(5 * preset.SLOTS_PER_EPOCH, computeStartSlotAtEpoch(5)); | ||
| } |
There was a problem hiding this comment.
For consistency with other tests and to be more explicit, please cast the expected values to Slot using @as(Slot, ...). The first assertion in this test does this, but the following two do not. This aligns with the style guide's preference for explicitness.
test "computeStartSlotAtEpoch" {
try std.testing.expectEqual(@as(Slot, 0), computeStartSlotAtEpoch(0));
try std.testing.expectEqual(@as(Slot, preset.SLOTS_PER_EPOCH), computeStartSlotAtEpoch(1));
try std.testing.expectEqual(@as(Slot, 5 * preset.SLOTS_PER_EPOCH), computeStartSlotAtEpoch(5));
}
References
- The style guide recommends being explicit to minimize dependence on the compiler (Line 200) and using explicitly-sized types (Line 44). (link)
| test "computeEndSlotAtEpoch" { | ||
| try std.testing.expectEqual(preset.SLOTS_PER_EPOCH - 1, computeEndSlotAtEpoch(0)); | ||
| try std.testing.expectEqual(2 * preset.SLOTS_PER_EPOCH - 1, computeEndSlotAtEpoch(1)); | ||
| } |
There was a problem hiding this comment.
For consistency and explicitness, please cast the expected values to Slot using @as(Slot, ...). Other tests in this file follow this pattern, and it aligns with the style guide's preference for explicitness.
test "computeEndSlotAtEpoch" {
try std.testing.expectEqual(@as(Slot, preset.SLOTS_PER_EPOCH - 1), computeEndSlotAtEpoch(0));
try std.testing.expectEqual(@as(Slot, 2 * preset.SLOTS_PER_EPOCH - 1), computeEndSlotAtEpoch(1));
}
References
- The style guide recommends being explicit to minimize dependence on the compiler (Line 200) and using explicitly-sized types (Line 44). (link)
1 participant
Add 10 unit tests covering pure utility functions in
epoch.zigandmath.zig:epoch.zig (9 tests):
computeEpochAtSlot— slot-to-epoch conversioncomputeStartSlotAtEpoch— epoch start slotcomputeEndSlotAtEpoch— epoch end slotcomputeCheckpointEpochAtStateSlot— checkpoint boundary logiccomputeActivationExitEpoch— activation/exit delaycomputePreviousEpoch— genesis clampingcomputeSyncPeriodAtEpoch— sync committee periodcomputeSyncPeriodAtSlot— slot-to-sync-periodisStartSlotOfEpoch— epoch boundary checkmath.zig (1 test):
intSqrt— integer square root via f64, edge cases and large valuesAll tests verified locally with
zig build test:state_transition.🤖 Generated with AI assistance