Add unit test file for hash package #1532
Conversation
Summary of ChangesHello @manmathbh, 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 reliability and robustness of the Highlights
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. 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. Code is written, then it's run, But bugs may hide, spoiling fun. Tests are added, clear and bright, To make sure all is truly right. 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 introduces a comprehensive and well-structured test suite for the hash package, significantly improving its test coverage. The tests cover a wide range of scenarios, including various input sizes, determinism, helper functions, and benchmarks. The addition of an avalanche test for fmix64 and a test for unaligned memory access is particularly valuable. However, the unaligned access test reveals a critical portability issue in the existing murmur3.go implementation, which will cause panics on certain hardware architectures. While the fix for this is outside the scope of this PR, the test is crucial for identifying this problem.
pkg/utils/hash/hash_test.go
Outdated
| func TestHash128_UnalignedInput(t *testing.T) { | ||
| // Create a buffer and then a sub-slice that is not 8-byte aligned. | ||
| buf := make([]byte, 33) | ||
| for i := range buf { | ||
| buf[i] = byte(i) | ||
| } | ||
| unalignedData := buf[1:] // len=32 | ||
|
|
||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| t.Errorf("Hash128 panicked on unaligned data: %v", r) | ||
| } | ||
| }() | ||
|
|
||
| Hash128(unalignedData, 0) | ||
| } No newline at end of file |
There was a problem hiding this comment.
This test correctly identifies a critical portability issue in the Hash128 implementation. The use of unsafe.Pointer in murmur3.go to perform unaligned 64-bit reads will cause the program to panic on architectures that enforce memory alignment (like ARM, MIPS, etc.).
While this test may pass on x86 platforms, it will fail on others, making the function unreliable in a cross-platform environment. This is a critical issue.
To fix this, the implementation in murmur3.go should be updated to use encoding/binary for safe unaligned reads. For example:
// In murmur3.go, inside the loop:
k1 := binary.LittleEndian.Uint64(data[i*16 : i*16+8])
k2 := binary.LittleEndian.Uint64(data[i*16+8 : i*16+16])Although fixing murmur3.go is outside the scope of this PR, this test is essential for exposing the bug. A follow-up PR to fix the implementation is highly recommended.
There was a problem hiding this comment.
Fixed in the latest commit!
I've updated murmur3.go to use encoding/binary. LittleEndian.Uint64 for safe unaligned reads as recommended. The implementation is now portable across all architectures including ARM and MIPS.
Test results:
All 8 test suites pass (36 total test cases)
TestHash128_UnalignedInput now passes without panics
No unsafe.Pointer usage for unaligned memory access
Commit: 737f9f6
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive unit tests for the MurmurHash3 implementation in the hash package. The tests validate the Hash128 function and its internal helper functions (rotl64 and fmix64), ensuring correct hash computation, deterministic behavior, and proper handling of edge cases including unaligned memory access.
Key Changes
- Added 318 lines of unit tests covering Hash128 function with various input sizes and seeds
- Added tests for internal helper functions (rotl64, fmix64) with expected values
- Added benchmark tests for small, medium, and large data inputs
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/utils/hash/hash_test.go
Outdated
| h1_1, h2_1 := Hash128(testData, seed) | ||
| h1_2, h2_2 := Hash128(testData, seed) |
There was a problem hiding this comment.
[nitpick] Naming: Variable names like h1_1, h2_1, h1_2, h2_2 don't follow Go naming conventions which typically avoid underscores. Consider using camelCase names like h1First, h2First, h1Second, h2Second or h1Call1, h2Call1, h1Call2, h2Call2 for better readability.
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Signed-off-by: manmathbh <[email protected]>
- Rename test functions to follow Go conventions for unexported functions - Add TestHash128_UnalignedInput to verify handling of unaligned data - Address feedback from gemini-code-assist and Copilot Signed-off-by: manmathbh <[email protected]>
Signed-off-by: manmathbh <[email protected]>
manmathbh
force-pushed
the
add-hash-tests
branch
from
c607a39 to
cbb9b47
Compare
|
@manmathbh I would suggest first testing the core package and functions |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/utils/hash/hash_test.go
Outdated
| h1_1, h2_1 := Hash128(testData, seed) | ||
| h1_2, h2_2 := Hash128(testData, seed) |
There was a problem hiding this comment.
[nitpick] Variable names h1_1, h2_1, h1_2, h2_2 use underscores which is not idiomatic Go naming. Consider using camelCase like h1First, h2First, h1Second, h2Second to follow Go naming conventions.
pkg/utils/hash/hash_test.go
Outdated
| h1_1, h2_1 := Hash128(data1, seed) | ||
| h1_2, h2_2 := Hash128(data2, seed) |
There was a problem hiding this comment.
[nitpick] Variable names h1_1, h2_1, h1_2, h2_2 use underscores which is not idiomatic Go naming. Consider using camelCase like h1Data1, h2Data1, h1Data2, h2Data2 to follow Go naming conventions.
pkg/utils/hash/hash_test.go
Outdated
| data := append(baseData, []byte("extra tail bytes")[:tailLen]...) | ||
| h1, h2 := Hash128(data, seed) | ||
|
|
||
| // Verify it doesn't panic and produces some output |
There was a problem hiding this comment.
[nitpick] The assertion if h1 == 0 && h2 == 0 && len(data) > 0 assumes that a hash of non-empty data can never be (0, 0). While extremely unlikely, a hash function could legitimately produce (0, 0) for some inputs. Consider removing this check or adding a note that it's testing for the hash function not being broken (returning zero for all inputs), rather than a strict correctness check.
| // Verify it doesn't panic and produces some output | |
| // This check is a sanity check to catch a hash function that is broken | |
| // (e.g., always returning zero). It is not a strict correctness check, | |
| // as a valid hash function could theoretically return (0, 0) for some input. |
Fixes the critical portability bug identified by gemini-code-assist. The previous unsafe.Pointer cast to [2]uint64 could panic on architectures with strict memory alignment requirements (ARM, MIPS). Using encoding/binary. LittleEndian.Uint64 provides safe, portable unaligned reads while preserving the algorithm's behavior. Removed duplicate murmur3_test.go test file to resolve test name conflicts. All tests now consolidated in hash_test.go. All 8 test suites pass including the critical TestHash128_UnalignedInput. Signed-off-by: manmathbh <[email protected]>
manmathbh
force-pushed
the
add-hash-tests
branch
from
737f9f6 to
4e37da6
Compare
|
Fixed the critical portability bug and addressed all code review feedback: Bug Fix: Code Review Feedback Addressed: Test Results: Ready for review! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Description
This PR adds comprehensive unit tests for the hash package, including tests for Hash128 function and helper functions.
Changes
Hash128functionrotl64helper functionfmix64helper functionfmix64Related Issues
Fixes #1529
Notes
This PR replaces #1530. The changes have been moved from the main branch to a feature branch following best practices.
Testing
All tests pass locally. The unaligned data test ensures Hash128 handles unaligned input without panicking on architectures that enforce memory alignment.