Add test coverage for pkg/services configuration Run() methods#1532
Add test coverage for pkg/services configuration Run() methods#1532matoval wants to merge 2 commits intoansible:develfrom
Conversation
This commit adds comprehensive tests for previously untested configuration Run() methods in the services package. Changes: - Add tests for NetUDPWrapper methods (ResolveUDPAddr, ListenUDP, DialUDP) - Add tests for UDPProxyInboundCfg.Run() and UDPProxyOutboundCfg.Run() - Add tests for CommandSvcCfg.Run() with TLS validation - Add tests for UnixProxyInboundCfg.Run() and UnixProxyOutboundCfg.Run() Tests added: - 8 new test functions with 20 sub-test cases - All tests follow existing patterns and use table-driven approach - Error cases validated with expected error messages Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds unit tests in pkg/services for CommandSvcCfg, UDP utilities and proxy configs, and Unix proxy configs; tests exercise Run() behaviors for valid setups, IPv4/IPv6 scenarios, and invalid TLS strings, and wire a cancellable context via netceptor.MainInstance where required. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/command_test.go`:
- Around line 140-155: Save the original netceptor.MainInstance, then create a
cancellable context (context.WithCancel) and assign netceptor.MainInstance =
netceptor.New(ctx, "test_command_svc_cfg_run"); immediately defer a function
that cancels the context and restores the saved original MainInstance so
background goroutines are stopped and global state is restored after the test;
keep the existing loop over testCases and calls to tc.configObj.Run() unchanged.
In `@pkg/services/udp_proxy_test.go`:
- Around line 613-626: netceptor.MainInstance is being set with
context.Background() without cancellation or restore which can leak goroutines;
update the setup in the test that calls netceptor.New to use ctx, cancel :=
context.WithCancel(context.Background()) and assign netceptor.MainInstance =
netceptor.New(ctx, "test_udp_proxy_inbound_cfg_run"), store the previous
instance in a variable (prev := netceptor.MainInstance) before replacing it and
use defer to restore it and call defer cancel() so the instance is cleaned up;
apply the identical pattern to the other test (TestUDPProxyOutboundCfgRun) where
netceptor.MainInstance is set.
- Around line 592-611: The tests use fixed UDP ports in testCases
(UDPProxyInboundCfg.Port = 8080/8081) which can collide on CI; change the tests
to use ephemeral ports by either setting Port: 0 in the UDPProxyInboundCfg and
letting the code bind to an OS-assigned port or obtain a free UDP port before
creating the config via a small helper (e.g., getFreeUDPPort using
net.ListenUDP, read the assigned port, close the listener) and set
UDPProxyInboundCfg.Port to that value; update any assertions or Run() calls that
depend on the port to use the dynamically assigned port and ensure the helper
listener is closed to avoid resource leaks.
- Around line 538-548: The "Listen on IPv6" test currently hard-fails when the
environment lacks IPv6; modify the test to first probe whether IPv6 is available
(e.g., attempt a quick ephemeral IPv6 listen/dial or inspect addresses) and if
the probe indicates IPv6 is unsupported, call t.Skipf to skip the test instead
of failing; keep the existing test logic that uses wrapper.ListenUDP and
net.IPv6loopback but replace the immediate t.Fatalf on bind failure with a
conditional skip when the error indicates lack of IPv6 support, while still
failing for other unexpected errors.
In `@pkg/services/unix_proxy_test.go`:
- Around line 93-117: Tests in unix_proxy_test.go use fixed socket paths like
"/tmp/test-receptor-unix-inbound.sock" for multiple subtests (config objects of
type UnixProxyInboundCfg) which can cause flakes when stale socket files remain;
update each subtest to generate a unique socket path (use t.TempDir() or include
the subtest name/iteration in the filename) and ensure any existing socket file
is removed before calling t.Run (or before invoking the code that
binds/connects), and rely on t.Cleanup or defer to remove the socket after the
subtest completes so each Run() uses a fresh, unique socket path.
- Around line 122-140: The tests set netceptor.MainInstance =
netceptor.New(context.Background(), "test_unix_proxy_inbound_cfg_run") which
leaves a live global instance that can leak into other tests; modify the test
setup to create a cancellable context (ctx, cancel :=
context.WithCancel(context.Background())), assign netceptor.MainInstance =
netceptor.New(ctx, "..."), and defer both cancel() and restoration of the prior
netceptor.MainInstance (save old := netceptor.MainInstance before replacing,
then defer func(){ cancel(); netceptor.MainInstance = old }()). Apply this same
pattern to the other test (TestUnixProxyOutboundCfgRun) so both tests restore
and cancel the global MainInstance when finished.
ℹ️ 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 (3)
pkg/services/command_test.gopkg/services/udp_proxy_test.gopkg/services/unix_proxy_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/services/udp_proxy_test.go (1)
541-546:⚠️ Potential issue | 🟠 MajorNarrow the IPv6 skip condition to actual “IPv6 unavailable” errors.
This currently skips for any
*net.OpError, which can hide real failures unrelated to IPv6 availability. Skip only on known unsupported-family errors and fail otherwise.Proposed fix
import ( "context" "errors" "fmt" "net" + "syscall" "testing" @@ t.Run("Listen on IPv6", func(t *testing.T) { addr := &net.UDPAddr{IP: net.IPv6loopback, Port: 0} conn, err := wrapper.ListenUDP("udp", addr) if err != nil { - // Skip if IPv6 is not available - if opErr, ok := err.(*net.OpError); ok { - t.Skipf("IPv6 not available: %v", opErr) - } + if errors.Is(err, syscall.EAFNOSUPPORT) || + errors.Is(err, syscall.EPROTONOSUPPORT) || + errors.Is(err, syscall.EADDRNOTAVAIL) { + t.Skipf("IPv6 not available in this environment: %v", err) + } t.Fatalf("expected no error, got %v", err) }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 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 541 - 546, The test currently skips on any *net.OpError which is too broad; change the check in the error branch that inspects err to only skip when the underlying error is a syscall.Errno indicating IPv6 is unsupported (for example syscall.EAFNOSUPPORT or syscall.EADDRNOTAVAIL). Concretely, replace the broad branch that does "if opErr, ok := err.(*net.OpError); ok { t.Skipf(...)" with nested type assertions that extract opErr.Err as *os.SyscallError and then its Err as syscall.Errno, and call t.Skipf only when errno == syscall.EAFNOSUPPORT || errno == syscall.EADDRNOTAVAIL; otherwise call t.Fatalf("expected no error, got %v", err). This uses the existing err variable and net.OpError to precisely detect IPv6-unavailable errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/services/udp_proxy_test.go`:
- Around line 541-546: The test currently skips on any *net.OpError which is too
broad; change the check in the error branch that inspects err to only skip when
the underlying error is a syscall.Errno indicating IPv6 is unsupported (for
example syscall.EAFNOSUPPORT or syscall.EADDRNOTAVAIL). Concretely, replace the
broad branch that does "if opErr, ok := err.(*net.OpError); ok { t.Skipf(...)"
with nested type assertions that extract opErr.Err as *os.SyscallError and then
its Err as syscall.Errno, and call t.Skipf only when errno ==
syscall.EAFNOSUPPORT || errno == syscall.EADDRNOTAVAIL; otherwise call
t.Fatalf("expected no error, got %v", err). This uses the existing err variable
and net.OpError to precisely detect IPv6-unavailable errors.
ℹ️ 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 (3)
pkg/services/command_test.gopkg/services/udp_proxy_test.gopkg/services/unix_proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/services/command_test.go
- pkg/services/unix_proxy_test.go
Address code review feedback to improve test reliability and prevent resource leaks. Changes: - Fix global MainInstance leaks by using cancellable contexts - Restore original MainInstance in defer to prevent test pollution - Use ephemeral ports (Port: 0) for UDP tests to avoid CI collisions - Use t.TempDir() for unique Unix socket paths per subtest - Add conditional skip for IPv6 tests when IPv6 is unavailable - Fix octal literal formatting (0600 -> 0o600) per gofumpt All tests pass with proper cleanup and isolation. Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
matoval
force-pushed
the
increase-service-pkg-test-coverage
branch
from
c3b2cea to
ee8ecb7
Compare
|
arrestle
left a comment
arrestle
left a comment
There was a problem hiding this comment.
Question: Was this part of a Jira or epic or guardian duty? If so please add a reference to the PR description.
Minor: this setup is repeated in 5 places; consider extracting a withTestMainInstance helper if more tests need it.
// withTestMainInstance creates a temporary MainInstance for tests, runs fn,
// then restores the original instance and cancels the context to prevent leaks.
func withTestMainInstance(t *testing.T, testName string, fn func()) {
t.Helper()
originalInstance := netceptor.MainInstance
ctx, cancel := context.WithCancel(context.Background())
netceptor.MainInstance = netceptor.New(ctx, testName)
defer func() {
cancel()
netceptor.MainInstance = originalInstance
}()
fn()
}
and suggesting docstrings. Thanks for helping us meet our testing standards!
| } | ||
| } | ||
|
|
||
| func TestCommandSvcCfgRun(t *testing.T) { |
There was a problem hiding this comment.
| func TestCommandSvcCfgRun(t *testing.T) { | |
| // TestCommandSvcCfgRun verifies CommandSvcCfg.Run() for valid configs, empty TLS, and invalid TLS configurations. | |
| func TestCommandSvcCfgRun(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestNetUDPWrapper_ResolveUDPAddr(t *testing.T) { |
There was a problem hiding this comment.
| func TestNetUDPWrapper_ResolveUDPAddr(t *testing.T) { | |
| // TestNetUDPWrapper_ResolveUDPAddr tests address resolution for valid, invalid, and IPv4 addresses. | |
| func TestNetUDPWrapper_ResolveUDPAddr(t *testing.T) { |
| }) | ||
| } | ||
|
|
||
| func TestNetUDPWrapper_ListenUDP(t *testing.T) { |
There was a problem hiding this comment.
| func TestNetUDPWrapper_ListenUDP(t *testing.T) { | |
| // TestNetUDPWrapper_ListenUDP tests UDP listen on IPv4 and IPv6 (skips when IPv6 is unavailable). | |
| func TestNetUDPWrapper_ListenUDP(t *testing.T) { |
| }) | ||
| } | ||
|
|
||
| func TestNetUDPWrapper_DialUDP(t *testing.T) { |
There was a problem hiding this comment.
| func TestNetUDPWrapper_DialUDP(t *testing.T) { | |
| // TestNetUDPWrapper_DialUDP tests UDP dial with and without a local address. | |
| func TestNetUDPWrapper_DialUDP(t *testing.T) { |
| }) | ||
| } | ||
|
|
||
| func TestUDPProxyInboundCfgRun(t *testing.T) { |
There was a problem hiding this comment.
| func TestUDPProxyInboundCfgRun(t *testing.T) { | |
| // TestUDPProxyInboundCfgRun verifies UDPProxyInboundCfg.Run() with valid inbound proxy configurations. | |
| func TestUDPProxyInboundCfgRun(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestUDPProxyOutboundCfgRun(t *testing.T) { |
There was a problem hiding this comment.
| func TestUDPProxyOutboundCfgRun(t *testing.T) { | |
| // TestUDPProxyOutboundCfgRun verifies UDPProxyOutboundCfg.Run() with valid outbound proxy configurations. | |
| func TestUDPProxyOutboundCfgRun(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestUnixProxyInboundCfgRun(t *testing.T) { |
There was a problem hiding this comment.
| func TestUnixProxyInboundCfgRun(t *testing.T) { | |
| // TestUnixProxyInboundCfgRun verifies UnixProxyInboundCfg.Run() for valid configs and invalid TLS. | |
| func TestUnixProxyInboundCfgRun(t *testing.T) { |
| } | ||
| } | ||
|
|
||
| func TestUnixProxyOutboundCfgRun(t *testing.T) { |
There was a problem hiding this comment.
| func TestUnixProxyOutboundCfgRun(t *testing.T) { | |
| // TestUnixProxyOutboundCfgRun verifies UnixProxyOutboundCfg.Run() for valid configs and invalid TLS. | |
| func TestUnixProxyOutboundCfgRun(t *testing.T) { |
There is no jira for this PR, just a coverage gap I identified and wrote some tests to add some coverage. |
3 participants
This commit adds comprehensive tests for previously untested configuration Run() methods in the services package.
Changes:
Tests added:
Assisted-by: Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit