test: add unit tests and refactor kernal_version utility#1590
test: add unit tests and refactor kernal_version utility#1590yashisrani wants to merge 5 commits intokmesh-net:mainfrom
Conversation
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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 code health of 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. Changelog
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 refactored, Tests now run on every OS, Bugs flee in fear. 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 effectively refactors the kernel version utility for better testability and cross-platform support by separating platform-specific logic using build tags. The addition of unit tests significantly improves the code's reliability. I've identified one area for improvement concerning code duplication of a helper function, which should be addressed to enhance maintainability.
| func int8ToStr(arr []int8) string { | ||
| b := make([]byte, 0, len(arr)) | ||
| for _, v := range arr { | ||
| if v == 0x00 { | ||
| break | ||
| } | ||
| b = append(b, byte(v)) | ||
| } | ||
| return string(b) | ||
| } |
There was a problem hiding this comment.
The int8ToStr function is unused in this file and is a duplicate of the one in kernel_version_linux.go. This introduces dead code and code duplication, which can lead to maintenance issues. This function should be removed from this file. To avoid duplication altogether, the function could be moved from kernel_version_linux.go to the shared kernel_version.go file.
There was a problem hiding this comment.
Pull request overview
This PR improves code health in the pkg/utils package by adding comprehensive unit tests for kernel version utilities and refactoring the code to be platform-independent. The refactoring extracts version parsing logic into a testable function and splits platform-specific syscall operations into separate files using build tags.
Changes:
- Refactored kernel version logic to extract
isVersionLowerThanas a platform-independent, testable function - Split implementation into platform-specific files using build tags (kernel_version_linux.go for Linux, kernel_version_other.go for non-Linux)
- Added comprehensive unit tests covering version comparisons, edge cases, and helper functions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/utils/kernel_version.go | Removed build constraints and syscall dependency; refactored to use platform-agnostic isVersionLowerThan helper |
| pkg/utils/kernel_version_linux.go | Linux-specific implementation using syscall.Uname to get kernel version |
| pkg/utils/kernel_version_other.go | Non-Linux stub implementation returning dummy version "0.0.0" for testing |
| pkg/utils/kernel_version_test.go | Comprehensive tests for version comparison logic and int8 to string conversion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| func int8ToStr(arr []int8) string { | ||
| b := make([]byte, 0, len(arr)) | ||
| for _, v := range arr { | ||
| if v == 0x00 { | ||
| break | ||
| } | ||
| b = append(b, byte(v)) | ||
| } | ||
| return string(b) | ||
| } |
There was a problem hiding this comment.
The int8ToStr function is duplicated in both kernel_version_linux.go and kernel_version_other.go with identical implementations. Since this is a utility function that doesn't depend on platform-specific features, consider extracting it to the main kernel_version.go file to eliminate the duplication and improve maintainability.
| func int8ToStr(arr []int8) string { | |
| b := make([]byte, 0, len(arr)) | |
| for _, v := range arr { | |
| if v == 0x00 { | |
| break | |
| } | |
| b = append(b, byte(v)) | |
| } | |
| return string(b) | |
| } |
| func TestIsVersionLowerThan(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| kernelVersion string | ||
| major int | ||
| minor int | ||
| want bool | ||
| }{ | ||
| { | ||
| name: "lower major version", | ||
| kernelVersion: "4.15.0", | ||
| major: 5, | ||
| minor: 13, | ||
| want: true, | ||
| }, | ||
| { | ||
| name: "higher major version", | ||
| kernelVersion: "6.0.0", | ||
| major: 5, | ||
| minor: 13, | ||
| want: false, | ||
| }, | ||
| { | ||
| name: "same major, lower minor version", | ||
| kernelVersion: "5.10.0", | ||
| major: 5, | ||
| minor: 13, | ||
| want: true, | ||
| }, | ||
| { | ||
| name: "same major, same minor version", | ||
| kernelVersion: "5.13.0", | ||
| major: 5, | ||
| minor: 13, | ||
| want: false, | ||
| }, | ||
| { | ||
| name: "same major, higher minor version", | ||
| kernelVersion: "5.15.0", | ||
| major: 5, | ||
| minor: 13, | ||
| want: false, | ||
| }, | ||
| { | ||
| name: "invalid version format - single number", | ||
| kernelVersion: "5", | ||
| major: 5, | ||
| minor: 13, | ||
| want: true, | ||
| }, | ||
| { | ||
| name: "invalid version format - non-numeric", | ||
| kernelVersion: "abc.def", | ||
| major: 5, | ||
| minor: 13, | ||
| want: true, | ||
| }, | ||
| { | ||
| name: "empty version string", | ||
| kernelVersion: "", | ||
| major: 5, | ||
| minor: 13, | ||
| want: true, | ||
| }, | ||
| { | ||
| name: "complex version string (Ubuntu style)", | ||
| kernelVersion: "5.15.0-101-generic", | ||
| major: 5, | ||
| minor: 13, | ||
| want: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if got := isVersionLowerThan(tt.kernelVersion, tt.major, tt.minor); got != tt.want { | ||
| t.Errorf("isVersionLowerThan(%v, %v, %v) = %v, want %v", tt.kernelVersion, tt.major, tt.minor, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The public API function KernelVersionLowerThan5_13() is not tested. While the internal isVersionLowerThan function has comprehensive tests, the public function should also have tests to ensure the integration works correctly, especially since GetKernelVersion() is now platform-specific and could return different values on different platforms. Consider adding a test that calls KernelVersionLowerThan5_13() to verify the complete flow.
Signed-off-by: Yash Israni <118755067+yashisrani@users.noreply.github.com>
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Adding label DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
What this PR does / why we need it:
Key Changes
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: