Add test coverage for NetUDPWrapper methods in pkg/services#1524
Add test coverage for NetUDPWrapper methods in pkg/services#1524fincamd wants to merge 1 commit into
Conversation
Added comprehensive tests for the three NetUDPWrapper methods (ResolveUDPAddr, ListenUDP, DialUDP). This improves the overall test coverage for pkg/services from 47.1% to 47.7%. The new TestNetUDPWrapper function includes test cases for: - Valid and invalid UDP address resolution - UDP socket binding to localhost - UDP connection dialing Assisted-By: Claude Sonnet 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughA new test suite Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/services/udp_proxy_test.go`:
- Around line 382-393: The test's nil-address check uses t.Error which doesn't
stop execution, allowing a nil dereference when accessing addr.Port; in the
"ResolveUDPAddr - valid address" t.Run block, replace the addr nil check to use
t.Fatal (or call return after logging) so the test stops immediately when addr
is nil before touching addr.Port; locate the wrapper.ResolveUDPAddr invocation
and the addr variable in that test and change the nil-check to t.Fatal to
prevent the SA5011 panic.
- Around line 402-432: The tests for wrapper.ListenUDP and wrapper.DialUDP
currently only defer conn.Close() when conn != nil, which hides a nil-conn
success case; after each call to wrapper.ListenUDP and wrapper.DialUDP assert
that conn is non-nil (e.g., if conn == nil { t.Fatalf("expected non-nil conn
from ListenUDP/DialUDP") }) and only then defer conn.Close(); reference the conn
variable and the wrapper.ListenUDP / wrapper.DialUDP calls when making this
change so both subtests have the explicit non-nil check and proper defer.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pkg/services/udp_proxy_test.go
| t.Run("ResolveUDPAddr - valid address", func(t *testing.T) { | ||
| addr, err := wrapper.ResolveUDPAddr("udp", "127.0.0.1:8080") | ||
| if err != nil { | ||
| t.Errorf("Expected no error, got %v", err) | ||
| } | ||
| if addr == nil { | ||
| t.Error("Expected non-nil address") | ||
| } | ||
| if addr.Port != 8080 { | ||
| t.Errorf("Expected port 8080, got %d", addr.Port) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Use t.Fatal to stop execution after the nil check, preventing a nil pointer dereference.
t.Error logs a failure but does not stop the test, so execution falls through to addr.Port on line 390 even when addr is nil, causing a panic. The staticcheck tool correctly flags this (SA5011).
🐛 Proposed fix
addr, err := wrapper.ResolveUDPAddr("udp", "127.0.0.1:8080")
if err != nil {
t.Errorf("Expected no error, got %v", err)
}
if addr == nil {
- t.Error("Expected non-nil address")
+ t.Fatal("Expected non-nil address")
}
if addr.Port != 8080 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("ResolveUDPAddr - valid address", func(t *testing.T) { | |
| addr, err := wrapper.ResolveUDPAddr("udp", "127.0.0.1:8080") | |
| if err != nil { | |
| t.Errorf("Expected no error, got %v", err) | |
| } | |
| if addr == nil { | |
| t.Error("Expected non-nil address") | |
| } | |
| if addr.Port != 8080 { | |
| t.Errorf("Expected port 8080, got %d", addr.Port) | |
| } | |
| }) | |
| t.Run("ResolveUDPAddr - valid address", func(t *testing.T) { | |
| addr, err := wrapper.ResolveUDPAddr("udp", "127.0.0.1:8080") | |
| if err != nil { | |
| t.Errorf("Expected no error, got %v", err) | |
| } | |
| if addr == nil { | |
| t.Fatal("Expected non-nil address") | |
| } | |
| if addr.Port != 8080 { | |
| t.Errorf("Expected port 8080, got %d", addr.Port) | |
| } | |
| }) |
🧰 Tools
🪛 GitHub Check: lint-receptor
[failure] 387-387:
SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
[failure] 390-390:
SA5011: possible nil pointer dereference (staticcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/services/udp_proxy_test.go` around lines 382 - 393, The test's
nil-address check uses t.Error which doesn't stop execution, allowing a nil
dereference when accessing addr.Port; in the "ResolveUDPAddr - valid address"
t.Run block, replace the addr nil check to use t.Fatal (or call return after
logging) so the test stops immediately when addr is nil before touching
addr.Port; locate the wrapper.ResolveUDPAddr invocation and the addr variable in
that test and change the nil-check to t.Fatal to prevent the SA5011 panic.
| t.Run("ListenUDP - bind to localhost", func(t *testing.T) { | ||
| addr := &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0} | ||
| conn, err := wrapper.ListenUDP("udp", addr) | ||
| if err != nil { | ||
| t.Errorf("Expected no error, got %v", err) | ||
| } | ||
| if conn != nil { | ||
| defer conn.Close() | ||
| } | ||
| }) | ||
|
|
||
| t.Run("DialUDP - dial localhost", func(t *testing.T) { | ||
| // First, create a listener to dial to | ||
| listenerAddr := &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0} | ||
| listener, err := wrapper.ListenUDP("udp", listenerAddr) | ||
| if err != nil { | ||
| t.Fatalf("Failed to create listener: %v", err) | ||
| } | ||
| defer listener.Close() | ||
|
|
||
| // Get the actual port the listener is using | ||
| localAddr := listener.LocalAddr().(*net.UDPAddr) | ||
|
|
||
| // Now dial to that address | ||
| conn, err := wrapper.DialUDP("udp", nil, localAddr) | ||
| if err != nil { | ||
| t.Errorf("Expected no error, got %v", err) | ||
| } | ||
| if conn != nil { | ||
| defer conn.Close() | ||
| } |
There was a problem hiding this comment.
Missing non-nil assertion for conn after a successful call in both ListenUDP and DialUDP subtests.
if conn != nil { defer conn.Close() } silently passes if conn is nil without an error. A successful call should always produce a non-nil connection; the test should assert that explicitly.
🛡️ Proposed fix (apply the same pattern to both subtests)
conn, err := wrapper.ListenUDP("udp", addr)
if err != nil {
t.Errorf("Expected no error, got %v", err)
}
- if conn != nil {
- defer conn.Close()
+ if conn == nil {
+ t.Fatal("Expected non-nil connection")
}
+ defer conn.Close() conn, err := wrapper.DialUDP("udp", nil, localAddr)
if err != nil {
t.Errorf("Expected no error, got %v", err)
}
- if conn != nil {
- defer conn.Close()
+ if conn == nil {
+ t.Fatal("Expected non-nil connection")
}
+ defer conn.Close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("ListenUDP - bind to localhost", func(t *testing.T) { | |
| addr := &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0} | |
| conn, err := wrapper.ListenUDP("udp", addr) | |
| if err != nil { | |
| t.Errorf("Expected no error, got %v", err) | |
| } | |
| if conn != nil { | |
| defer conn.Close() | |
| } | |
| }) | |
| t.Run("DialUDP - dial localhost", func(t *testing.T) { | |
| // First, create a listener to dial to | |
| listenerAddr := &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0} | |
| listener, err := wrapper.ListenUDP("udp", listenerAddr) | |
| if err != nil { | |
| t.Fatalf("Failed to create listener: %v", err) | |
| } | |
| defer listener.Close() | |
| // Get the actual port the listener is using | |
| localAddr := listener.LocalAddr().(*net.UDPAddr) | |
| // Now dial to that address | |
| conn, err := wrapper.DialUDP("udp", nil, localAddr) | |
| if err != nil { | |
| t.Errorf("Expected no error, got %v", err) | |
| } | |
| if conn != nil { | |
| defer conn.Close() | |
| } | |
| t.Run("ListenUDP - bind to localhost", func(t *testing.T) { | |
| addr := &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0} | |
| conn, err := wrapper.ListenUDP("udp", addr) | |
| if err != nil { | |
| t.Errorf("Expected no error, got %v", err) | |
| } | |
| if conn == nil { | |
| t.Fatal("Expected non-nil connection") | |
| } | |
| defer conn.Close() | |
| }) | |
| t.Run("DialUDP - dial localhost", func(t *testing.T) { | |
| // First, create a listener to dial to | |
| listenerAddr := &net.UDPAddr{IP: net.ParseIP("127.0.0.1"), Port: 0} | |
| listener, err := wrapper.ListenUDP("udp", listenerAddr) | |
| if err != nil { | |
| t.Fatalf("Failed to create listener: %v", err) | |
| } | |
| defer listener.Close() | |
| // Get the actual port the listener is using | |
| localAddr := listener.LocalAddr().(*net.UDPAddr) | |
| // Now dial to that address | |
| conn, err := wrapper.DialUDP("udp", nil, localAddr) | |
| if err != nil { | |
| t.Errorf("Expected no error, got %v", err) | |
| } | |
| if conn == nil { | |
| t.Fatal("Expected non-nil connection") | |
| } | |
| defer conn.Close() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/services/udp_proxy_test.go` around lines 402 - 432, The tests for
wrapper.ListenUDP and wrapper.DialUDP currently only defer conn.Close() when
conn != nil, which hides a nil-conn success case; after each call to
wrapper.ListenUDP and wrapper.DialUDP assert that conn is non-nil (e.g., if conn
== nil { t.Fatalf("expected non-nil conn from ListenUDP/DialUDP") }) and only
then defer conn.Close(); reference the conn variable and the wrapper.ListenUDP /
wrapper.DialUDP calls when making this change so both subtests have the explicit
non-nil check and proper defer.
|
| } | ||
| } | ||
|
|
||
| func TestNetUDPWrapper(t *testing.T) { |
There was a problem hiding this comment.
Decide whether these tests can run in parallel with t.parallel() and then, if we do use t.Fatal() instead of t.Errorf().
Try to convert this in to table-driven testing so that we don't have several t.Run() tasks
Added comprehensive tests for the three NetUDPWrapper methods (ResolveUDPAddr, ListenUDP, DialUDP).
This improves the overall test coverage for pkg/services from 47.1% to 47.7%.
The new TestNetUDPWrapper function includes test cases for:
Assisted-By: Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit