Add unit test for the /pkg/utils/tc.go #1548
Conversation
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @adity1raut, 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 test coverage and reliability of core utility functions, particularly those related to network interface management and traffic control (TC) programs. By introducing comprehensive unit and integration tests for 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. New tests now take flight, Network code, strong and bright, Bugs flee in the night. 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 test coverage for the network interface handling logic in pkg/utils/tc.go, which previously had no tests. The added tests cover various functions and scenarios, including edge cases and permission checks. I've found one issue in the new test file pkg/utils/tc_test.go where a test for invalid mode values is implemented incorrectly. My feedback includes a suggestion to fix this test.
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for network interface handling utilities in pkg/utils/tc.go, which previously had no tests. The PR also improves existing tests by adding permission checks to handle test environments without sufficient privileges and corrects a spelling error.
- Adds 448 lines of tests covering TC (Traffic Control) program management, veth interface handling, and IP address checking functions
- Improves test robustness by adding permission checks in
enroll_test.goandhash_name_test.goto gracefully skip tests when root privileges are unavailable - Fixes spelling error in test comment ("imutate" → "simulate")
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| pkg/utils/tc_test.go | New comprehensive test suite for TC utilities including interface validation, IP checking, qdisc replacement, and TC program management with mocks, edge cases, integration tests, and benchmarks |
| pkg/utils/hash_name_test.go | Adds permission check to gracefully skip test when hash persistence is unavailable; corrects spelling error in comment |
| pkg/utils/enroll_test.go | Adds root privilege check to skip test when CAP_SYS_ADMIN capability is unavailable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/utils/tc_test.go
Outdated
| link := createMockLink("test0", 1) | ||
|
|
||
| // This will fail without root, but shouldn't panic | ||
| err := ManageTCProgramByFd(link, 3, 1) // 1 = TC_ATTACH (assuming constants.TC_ATTACH = 1) |
There was a problem hiding this comment.
The test assumes TC_ATTACH = 1 based on the comment, but according to pkg/constants/constants.go:40-41, TC_ATTACH = 0 and TC_DETACH = 1. This will cause the test to actually test the detach mode instead of the attach mode. Either update the comment to reflect the actual mode being tested (detach), or change the mode value to 0 to test attach mode as intended.
| err := ManageTCProgramByFd(link, 3, 1) // 1 = TC_ATTACH (assuming constants.TC_ATTACH = 1) | |
| err := ManageTCProgramByFd(link, 3, 0) // 0 = TC_ATTACH (according to constants.TC_ATTACH = 0) |
adity1raut
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/utils/tc_test.go
Outdated
| }) | ||
|
|
||
| t.Run("Attach mode", func(t *testing.T) { | ||
| err := ManageTCProgramByFd(link, 3, 0) // TC_ATTACH = 0 |
There was a problem hiding this comment.
Hardcoded magic numbers are used for TC_ATTACH (0) and TC_DETACH (1) modes instead of the defined constants. Import "kmesh.net/kmesh/pkg/constants" and use constants.TC_ATTACH and constants.TC_DETACH throughout the test file for better maintainability and to avoid potential errors if these values change.
pkg/utils/tc_test.go
Outdated
| }) | ||
|
|
||
| t.Run("Detach mode", func(t *testing.T) { | ||
| err := ManageTCProgramByFd(link, 3, 1) // TC_DETACH = 1 |
There was a problem hiding this comment.
Use the constant constants.TC_DETACH instead of the hardcoded value 1 for better code clarity and maintainability.
pkg/utils/tc_test.go
Outdated
| t.Run("Different file descriptors", func(t *testing.T) { | ||
| fds := []int{-1, 0, 1, 3, 100, 999} | ||
| for _, fd := range fds { | ||
| err := ManageTCProgramByFd(link, fd, 0) // Use valid mode TC_ATTACH = 0 |
There was a problem hiding this comment.
Use the constant constants.TC_ATTACH instead of the hardcoded value 0 for better code clarity and maintainability.
pkg/utils/tc_test.go
Outdated
| } | ||
|
|
||
| for _, l := range links { | ||
| err := ManageTCProgramByFd(l, 3, 0) // Use valid mode TC_ATTACH = 0 |
There was a problem hiding this comment.
Use the constant constants.TC_ATTACH instead of the hardcoded value 0 for better code clarity and maintainability.
pkg/utils/tc_test.go
Outdated
| err = ManageTCProgramByFd(link, 3, 0) | ||
| if err != nil { | ||
| t.Logf("Attach error (expected): %v", err) | ||
| } | ||
|
|
||
| // Detach program (TC_DETACH = 1) | ||
| err = ManageTCProgramByFd(link, 3, 1) |
There was a problem hiding this comment.
Use the constants constants.TC_ATTACH and constants.TC_DETACH instead of hardcoded values 0 and 1 for better code clarity and maintainability.
pkg/utils/tc_test.go
Outdated
| t.Run("Invalid mode negative", func(t *testing.T) { | ||
| err := ManageTCProgramByFd(link, 3, -1) | ||
| assert.Error(t, err) | ||
| assert.Contains(t, err.Error(), "invalid mode") | ||
| }) |
There was a problem hiding this comment.
This test case "Invalid mode negative" is duplicated. The same test is already defined at lines 319-323. Remove this duplicate test case.
| t.Run("Invalid mode negative", func(t *testing.T) { | |
| err := ManageTCProgramByFd(link, 3, -1) | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), "invalid mode") | |
| }) |
pkg/utils/tc_test.go
Outdated
| t.Run("Nil link handling", func(t *testing.T) { | ||
| // Test with different link types |
There was a problem hiding this comment.
The comment "Nil link handling" is misleading. The test doesn't actually test nil link handling but instead tests different types of MockLink instances with various properties (empty name, negative index, long name). Consider renaming this test case to "Different link properties" or "Link edge cases" to accurately reflect what's being tested.
| t.Run("Nil link handling", func(t *testing.T) { | |
| // Test with different link types | |
| t.Run("Link edge cases", func(t *testing.T) { | |
| // Test with links having edge-case properties (empty name, negative index, long name) |
Signed-off-by: adity1raut <araut7798@gmail.com>
adity1raut
force-pushed
the
TestCoverage3
branch
from
968f99b to
a652ad4
Compare
Signed-off-by: adity1raut <araut7798@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| type MockLink struct { | ||
| mock.Mock | ||
| attrs *netlink.LinkAttrs | ||
| } | ||
|
|
||
| func (m *MockLink) Attrs() *netlink.LinkAttrs { | ||
| if m.attrs != nil { | ||
| return m.attrs | ||
| } | ||
| args := m.Called() | ||
| return args.Get(0).(*netlink.LinkAttrs) | ||
| } | ||
|
|
||
| func (m *MockLink) Type() string { | ||
| args := m.Called() | ||
| return args.String(0) | ||
| } |
There was a problem hiding this comment.
The MockLink struct embeds mock.Mock but the methods (Attrs() and Type()) are never actually set up with mock expectations using On(). Instead, tests directly set the attrs field. This makes the mock.Mock embedding and the Called() invocations in lines 41 and 46 unnecessary. Either properly use mock expectations with On() calls, or simplify by removing the mock.Mock embedding and just using the attrs field directly.
| func TestIfaceContainIPs_NilIPList(t *testing.T) { | ||
| loopbackIface, err := net.InterfaceByName("lo") | ||
| if err != nil { | ||
| t.Skip("Loopback interface not available") | ||
| } | ||
|
|
||
| // Test with nil IP list | ||
| result, err := IfaceContainIPs(*loopbackIface, nil) | ||
| assert.NoError(t, err) | ||
| assert.False(t, result) | ||
| } | ||
|
|
||
| func TestIfaceContainIPs_MalformedIPs(t *testing.T) { | ||
| loopbackIface, err := net.InterfaceByName("lo") | ||
| if err != nil { | ||
| t.Skip("Loopback interface not available") | ||
| } | ||
|
|
||
| // Test with various malformed IPs | ||
| malformedIPs := []string{ | ||
| "", | ||
| "not-an-ip", | ||
| "999.999.999.999", | ||
| "127.0.0", | ||
| "127.0.0.1.1", | ||
| ":::", | ||
| "fg::1", | ||
| } | ||
|
|
||
| result, err := IfaceContainIPs(*loopbackIface, malformedIPs) | ||
| assert.NoError(t, err) | ||
| assert.False(t, result, "Malformed IPs should not match any interface addresses") |
There was a problem hiding this comment.
[nitpick] Test cases like TestIfaceContainIPs_EmptyIPList (line 178), TestIfaceContainIPs_NilIPList (line 287), and the empty string case in TestIfaceContainIPs_MalformedIPs (line 307) test very similar behavior (empty/nil IP lists). Consider consolidating these into a single table-driven test with multiple scenarios to reduce redundancy and improve maintainability.
| func TestIfaceContainIPs_NilIPList(t *testing.T) { | |
| loopbackIface, err := net.InterfaceByName("lo") | |
| if err != nil { | |
| t.Skip("Loopback interface not available") | |
| } | |
| // Test with nil IP list | |
| result, err := IfaceContainIPs(*loopbackIface, nil) | |
| assert.NoError(t, err) | |
| assert.False(t, result) | |
| } | |
| func TestIfaceContainIPs_MalformedIPs(t *testing.T) { | |
| loopbackIface, err := net.InterfaceByName("lo") | |
| if err != nil { | |
| t.Skip("Loopback interface not available") | |
| } | |
| // Test with various malformed IPs | |
| malformedIPs := []string{ | |
| "", | |
| "not-an-ip", | |
| "999.999.999.999", | |
| "127.0.0", | |
| "127.0.0.1.1", | |
| ":::", | |
| "fg::1", | |
| } | |
| result, err := IfaceContainIPs(*loopbackIface, malformedIPs) | |
| assert.NoError(t, err) | |
| assert.False(t, result, "Malformed IPs should not match any interface addresses") | |
| func TestIfaceContainIPs_EmptyNilMalformedIPLists(t *testing.T) { | |
| loopbackIface, err := net.InterfaceByName("lo") | |
| if err != nil { | |
| t.Skip("Loopback interface not available") | |
| } | |
| tests := []struct { | |
| name string | |
| ipList []string | |
| expected bool | |
| }{ | |
| { | |
| name: "Empty IP list", | |
| ipList: []string{}, | |
| expected: false, | |
| }, | |
| { | |
| name: "Nil IP list", | |
| ipList: nil, | |
| expected: false, | |
| }, | |
| { | |
| name: "IP list with empty string", | |
| ipList: []string{""}, | |
| expected: false, | |
| }, | |
| { | |
| name: "Malformed IPs", | |
| ipList: []string{"not-an-ip", "999.999.999.999", "127.0.0", "127.0.0.1.1", ":::", "fg::1"}, | |
| expected: false, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| result, err := IfaceContainIPs(*loopbackIface, tt.ipList) | |
| assert.NoError(t, err) | |
| assert.False(t, result, "Test case: %s", tt.name) | |
| }) | |
| } |
| // Test with different case variations of localhost | ||
| // Note: IP addresses are case-insensitive for IPv6, but we're testing the parsing | ||
| result, err := IfaceContainIPs(*loopbackIface, []string{"127.0.0.1"}) | ||
| assert.NoError(t, err) | ||
| // This should match the loopback interface | ||
| assert.True(t, result) |
There was a problem hiding this comment.
This test is testing "case insensitivity" but actually just tests a regular IP "127.0.0.1" which doesn't demonstrate case sensitivity/insensitivity. IPv4 addresses don't have case variations. The comment mentions IPv6 for case insensitivity, but the test doesn't use an IPv6 address. Either use an IPv6 address with uppercase hex letters (e.g., "::1" vs "::1") or rename the test to better reflect what it's actually testing (e.g., TestIfaceContainIPs_BasicMatch).
| // Test with different case variations of localhost | |
| // Note: IP addresses are case-insensitive for IPv6, but we're testing the parsing | |
| result, err := IfaceContainIPs(*loopbackIface, []string{"127.0.0.1"}) | |
| assert.NoError(t, err) | |
| // This should match the loopback interface | |
| assert.True(t, result) | |
| // Test with different case variations of IPv6 loopback address | |
| // IPv6 addresses are case-insensitive, so "ABCD::1234" and "abcd::1234" should be treated the same. | |
| // We'll use "::1" (loopback) in both lower and upper case forms. | |
| resultLower, err := IfaceContainIPs(*loopbackIface, []string{"::1"}) | |
| assert.NoError(t, err) | |
| assert.True(t, resultLower, "Lowercase IPv6 loopback should match") | |
| resultUpper, err := IfaceContainIPs(*loopbackIface, []string{"::1"}) | |
| assert.NoError(t, err) | |
| assert.True(t, resultUpper, "Uppercase IPv6 loopback should match") |
| func TestReplaceQdisc_DirectCall(t *testing.T) { | ||
| // Test the replaceQdisc function directly with a mock | ||
| mockLink := &MockLink{ | ||
| attrs: &netlink.LinkAttrs{ | ||
| Index: 1, | ||
| Name: "test", | ||
| }, | ||
| } | ||
|
|
||
| // This will likely fail due to netlink operations, but tests the function structure | ||
| err := replaceQdisc(mockLink) | ||
| assert.Error(t, err) // Expected to fail in test environment | ||
| } |
There was a problem hiding this comment.
The function replaceQdisc is not exported (it's lowercase), so it cannot be called from the test file. This test will fail to compile. If you want to test this function directly, it should either be exported in tc.go (renamed to ReplaceQdisc) or this test should be removed and the function should be tested indirectly through ManageTCProgramByFd with TC_ATTACH mode.
| func TestReplaceQdisc_DirectCall(t *testing.T) { | |
| // Test the replaceQdisc function directly with a mock | |
| mockLink := &MockLink{ | |
| attrs: &netlink.LinkAttrs{ | |
| Index: 1, | |
| Name: "test", | |
| }, | |
| } | |
| // This will likely fail due to netlink operations, but tests the function structure | |
| err := replaceQdisc(mockLink) | |
| assert.Error(t, err) // Expected to fail in test environment | |
| } |
| func TestIfaceContainIPs_LogWarning(t *testing.T) { | ||
| // This test is mainly for coverage of the log.Warnf line | ||
| // In practice, this case is hard to trigger since net.Interface.Addrs() | ||
| // typically returns *net.IPNet addresses | ||
| loopbackIface, err := net.InterfaceByName("lo") | ||
| if err != nil { | ||
| t.Skip("Loopback interface not available") | ||
| } | ||
|
|
||
| // Test with a valid IP that should be found | ||
| result, err := IfaceContainIPs(*loopbackIface, []string{"127.0.0.1"}) | ||
| assert.NoError(t, err) | ||
| // The result depends on the actual interface configuration | ||
| t.Logf("IfaceContainIPs result for 127.0.0.1: %v", result) | ||
| } |
There was a problem hiding this comment.
[nitpick] According to the implementation in tc.go line 129, when an address cannot be converted to *net.IPNet, the function logs a warning and continues. However, the log message includes %v for err which will always be nil since the conversion failure doesn't set an error. This test won't actually trigger the warning path since the conversion failure is a type assertion failure, not an error.
2 participants
What type of PR is this?
This PR is for improving and testing the file of code, and enhancing overall test coverage.
/kind enhancement
/kind feature
What this PR does / why we need it
This PR improves the test coverage for the file
./pkg/utils/tc.go.This file contains important core logic, but currently has no test cases. Adding proper tests ensures reliability, prevents future regressions, and increases overall code quality.
Which issue(s) this PR fixes
Fixes: #
Special notes for your reviewer
N/A
Does this PR introduce a user-facing change?