Add extensive tests using interfaces and dependency injection#28
Add extensive tests using interfaces and dependency injection#28
Conversation
Co-authored-by: mnaser <435815+mnaser@users.noreply.github.com>
|
Copilot can you go over this pr and improve it and make sure you maybe find a better way of stubbing things rather than a const that changes, there is a better way of doing this with golang, so lets go to that path. |
…injection Co-authored-by: mnaser <435815+mnaser@users.noreply.github.com>
Refactored in accb75b. Replaced all mutable package-level variables with idiomatic Go patterns:
Coverage: |
There was a problem hiding this comment.
Pull request overview
This PR substantially increases test coverage across previously untested packages by introducing interface-based dependency injection (notably hostname/FQDN resolution and Kubernetes/cert-manager clients) and adding comprehensive unit tests for pkg/net, pkg/template, and pkg/tls.
Changes:
- Introduces
net.Resolver+SystemResolver, and threads resolver injection through template value loading and TLS manager construction. - Adds DI fields to
tls.Config(resolver, retry delay, injected clients) and adjustsManager.Watch/watchto be context-cancellable with configurable retry delay. - Adds extensive new tests for
pkg/net,pkg/template, andpkg/tls, plus updates module sums from dependency updates/tidying.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/net/hostname.go | Adds Resolver interface and SystemResolver implementation with configurable hostname command path. |
| pkg/net/hostname_test.go | Adds tests for SystemResolver behavior (hostname, fqdn, default/custom command path). |
| pkg/template/template_values.go | Changes LoadValues to accept an injected net.Resolver and use it for hostname/FQDN lookup. |
| pkg/template/template_values_test.go | Adds tests for LoadValues branches (success and error paths). |
| pkg/template/template_test.go | Adds negative-path tests for template parsing, file loading, execution, and YAML unmarshal failures. |
| pkg/tls/config.go | Adds DI/configuration fields for resolver, retry delay, and injected Kubernetes/cert-manager clients. |
| pkg/tls/manager.go | Uses injected dependencies with defaults; adds configurable retry delay and ensures watch exits on context cancellation. |
| pkg/tls/manager_test.go | Adds a large test suite covering NewManager, Create, writeFile, informer watch behavior, and retry/cancellation paths. |
| go.mod | Updates indirect dependencies (tidy/upgrade effects). |
| go.sum | Updates checksums consistent with dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| default: | ||
| } | ||
|
|
||
| m.logger.Info("watch closed or disconnected, retrying in 5 seconds") | ||
|
|
||
| time.Sleep(5 * time.Second) | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-time.After(m.watchRetryDelay()): | ||
| } |
There was a problem hiding this comment.
The retry log message is hard-coded to "5 seconds", but the actual delay is now configurable via WatchRetryDelay (watchRetryDelay()). This can mislead operators when a custom delay is set; consider logging the computed delay value instead of a fixed string.
| log.StandardLogger().ExitFunc = func(int) { panic("log.Fatal") } | ||
| return func() { log.StandardLogger().ExitFunc = nil } |
There was a problem hiding this comment.
interceptFatal restores logrus' ExitFunc by setting it to nil, which may not restore the original behavior if it was already customized (and can be surprising even if the default is os.Exit). Capture the previous ExitFunc and restore it in the returned cleanup function instead.
| log.StandardLogger().ExitFunc = func(int) { panic("log.Fatal") } | |
| return func() { log.StandardLogger().ExitFunc = nil } | |
| prevExitFunc := log.StandardLogger().ExitFunc | |
| log.StandardLogger().ExitFunc = func(int) { panic("log.Fatal") } | |
| return func() { log.StandardLogger().ExitFunc = prevExitFunc } |
| fakeK8s := k8sfake.NewSimpleClientset(secret) | ||
|
|
||
| var fw *kwatchpkg.FakeWatcher | ||
| watcherReady := make(chan struct{}) | ||
|
|
||
| fakeK8s.PrependWatchReactor("secrets", | ||
| func(action k8stesting.Action) (bool, kwatchpkg.Interface, error) { | ||
| fw = kwatchpkg.NewFake() | ||
| close(watcherReady) | ||
| return true, fw, nil | ||
| }) |
There was a problem hiding this comment.
buildWatchMgr creates a watcherReady channel (closed when the watch reactor assigns fw), but it is never returned/awaited. Tests that call (*fwPtr).Modify/Delete after only waiting for AddFunc can race with fw still being nil (AddFunc can be triggered from the initial List before Watch is set up), causing a nil-pointer panic. Consider returning watcherReady (or otherwise synchronizing) and waiting for it before using fwPtr.
| // Allow the retry loop to fire (the informer returns, triggering the | ||
| // retry delay), then cancel on the second iteration. | ||
| // The WatchRetryDelay is 10ms (set by buildWatchMgr), so this resolves | ||
| // quickly. We wait a bit to let the retry log message and delay execute. | ||
| time.Sleep(50 * time.Millisecond) | ||
| cancel() | ||
|
|
There was a problem hiding this comment.
This test uses a fixed sleep to try to let the retry loop advance before cancelling. Time-based synchronization is prone to flakiness on slow/loaded CI runners. Prefer a deterministic signal (e.g., instrument Manager/watch to notify on each iteration, or block until a second watch reactor is observed) before calling cancel().
| t.Run("success with SystemResolver", func(t *testing.T) { | ||
| t.Setenv("POD_UID", "test-uid") | ||
| t.Setenv("POD_NAME", "test-name") | ||
| t.Setenv("POD_NAMESPACE", "test-namespace") | ||
| t.Setenv("POD_IP", "10.0.0.1") | ||
|
|
||
| values, err := LoadValues(net.SystemResolver{}) | ||
| require.NoError(t, err) | ||
|
|
||
| assert.NotEmpty(t, values.Hostname) | ||
| assert.NotEmpty(t, values.FQDN) | ||
| }) |
There was a problem hiding this comment.
This subtest exercises net.SystemResolver{} end-to-end, which depends on the runtime environment (presence/behavior of /bin/hostname --fqdn) and can be flaky. Since SystemResolver is already unit-tested in pkg/net, consider replacing this with a mock resolver (or a deterministic HostnameCmd test helper) to keep template.LoadValues tests hermetic.
| func TestSystemResolver_FQDN(t *testing.T) { | ||
| t.Run("success", func(t *testing.T) { | ||
| r := SystemResolver{} | ||
| fqdn, err := r.FQDN() | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, fqdn) | ||
| }) |
There was a problem hiding this comment.
The FQDN success test depends on the host environment having a working /bin/hostname that supports --fqdn and returns a non-empty value. This can be flaky across CI images/distros and makes the unit test non-deterministic. Consider using HostnameCmd to point at a small test helper (script/binary) that returns a known value so the test is hermetic.
| log.StandardLogger().ExitFunc = func(int) { | ||
| select { | ||
| case fatalCalled <- struct{}{}: | ||
| default: | ||
| } | ||
| } | ||
| defer func() { log.StandardLogger().ExitFunc = nil }() |
There was a problem hiding this comment.
This test also overrides logrus' global ExitFunc and restores it by setting it to nil, which may not restore the prior value if it was customized. Save the previous ExitFunc and restore it in the deferred cleanup to avoid leaking global state between tests.
| log.StandardLogger().ExitFunc = func(int) { | |
| select { | |
| case fatalCalled <- struct{}{}: | |
| default: | |
| } | |
| } | |
| defer func() { log.StandardLogger().ExitFunc = nil }() | |
| previousExitFunc := log.StandardLogger().ExitFunc | |
| log.StandardLogger().ExitFunc = func(int) { | |
| select { | |
| case fatalCalled <- struct{}{}: | |
| default: | |
| } | |
| } | |
| defer func() { log.StandardLogger().ExitFunc = previousExitFunc }() |
| func LoadValues(resolver net.Resolver) (*Values, error) { | ||
| podInfo, err := podinfo.Load() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| hostname, err := net.Hostname() | ||
| hostname, err := resolver.Hostname() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| fqdn, err := net.FQDN() | ||
| fqdn, err := resolver.FQDN() | ||
| if err != nil { |
There was a problem hiding this comment.
LoadValues now dereferences the provided resolver without checking for nil. Since this is an exported function, a nil resolver will panic at runtime. Consider returning a clear error (or defaulting to net.SystemResolver{}) when resolver is nil to make the API safer for callers.
The codebase had ~22% test coverage with entire packages (
pkg/net,pkg/tls/manager) completely untested. This brings coverage up to 97.4% using idiomatic Go patterns — interfaces and dependency injection via Config struct fields — instead of mutable package-level variables.Test-enabling production changes
pkg/net/hostname.goResolverinterface withHostname()andFQDN()methodsSystemResolverstruct implementingResolverwith a configurableHostnameCmdfield (defaults to/bin/hostname)pkg/template/template_values.goLoadValuesnow accepts anet.Resolverparameter instead of calling package-level functions directlypkg/tls/config.goConfig:Resolver net.Resolver,WatchRetryDelay time.Duration,SecretClient kubernetes.SecretInterface,CertificateClient cmclient.CertificateInterfacepkg/tls/manager.goNewManageruses Config-injected dependencies with defaults: creates clients fromRestConfigonly whenSecretClient/CertificateClientare nil, usesSystemResolver{}whenResolveris nilWatchnow returns onctx.Done()with a select-based configurable retry delaywatchcloses the informer stop channel via a goroutine on context cancellation, making the function actually return rather than block foreverdefaultWatchRetryDelayconstant andm.config.WatchRetryDelayfor overrideNew test coverage
pkg/netSystemResolver.Hostname/FQDNsuccess + error paths;hostnameCmddefault and custompkg/templateNewinvalid syntax,NewFromFilemissing file,Executeruntime error,ExecuteYAML unmarshal error; all fourLoadValuesbranches using mockResolver(success, podinfo, hostname, fqdn errors)pkg/tlsNewManager(5 error paths + 2 success paths including injected clients);Create(5 paths including cert-not-ready logging);write;writeFile(7 paths including fourlog.Fatalbranches via logrusExitFuncinterception);Watchretry loop and cancellation;watchAddFunc/UpdateFunc/DeleteFunc via fake k8s/cert-manager clients;watchRetryDelaydefault and customThe remaining 3.6% uncovered in
pkg/tlsconsists of unreachable defensive code: thejson.Marshalerror path (cannot fail on well-typed[]map[string]interface{}withOwnerReferences) and the Watch retry loop (controller.Run only returns when the stop channel closes, which is tied to context cancellation).pkg/tls/manager_test.goincludes aTestMainthat disables theWatchListClientfeature gate — required because client-go v0.35 enables it by default (Beta), and the fake watch does not produce the bookmark events the reflector then expects.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.