Defake tests by rbtr · Pull Request #4343 · Azure/azure-container-networking

rbtr · 2026-04-10T13:33:17Z

Remove the cns/fakes/ package entirely and replace centralized test doubles with local, consumer-defined stubs. This eliminates a long-standing Go testing antipattern where tests were testing fake instead of prod component behavior. UTs should be easier to write and more reliable in the new pattern.

The old centralized HTTPServiceFake had Locks and caused test ordering issue.
Tests now assert on monitor behavior directly (spec.RequestedIPCount, IPsNotInUse) rather than on fake simulator state we configured ourselves.
The test files no longer depend on a global shared fake. Changing one test can't cascade into another package's tests.

New test coverage

ipampool: GetStateSnapshot, GenerateARMID, clampScaler — coverage 62.7% → 73.0%
ipampool/v2: AsV1, adapter.Update, WithLegacyMetricsObserver — coverage 50.8% → 61.5%
All existing tests pass unchanged
16 new test cases covering previously untested functions

Phase 1: Narrow cns.HTTPService for ipampool.Monitor - Define narrow ipConfigState interface (3 methods) in monitor.go - Replace HTTPServiceFake+RequestControllerFake simulator (~420 LOC) with a coherent ipConfigStore test stub (~80 LOC) local to monitor_test.go - Delete HTTPServiceFake, IPStateManager, StringStack, RequestControllerFake - Delete unused MonitorFake (zero consumers) - Fix cns/service/main_test.go to use local stub instead of HTTPServiceFake Phase 2: Relocate IPTablesMock to consumer test file - Move IPTablesMock and IPTablesLegacyMock from cns/fakes/iptablesfake.go into cns/restserver/internalapi_linux_test.go as unexported types - Delete cns/fakes/iptablesfake.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Move WireserverClientFake, MockIMDSClient, NMAgentClientFake, and WireserverProxyFake from the centralized cns/fakes/ package to the test files that consume them. Types are made unexported since they are now test-only. For cns/restserver/ tests (shared package), types are defined once in restserver_test.go. For cns/restserver/v2/ and cns/client/ (separate packages), local stubs are defined in their test files. Delete the now-empty cns/fakes/ directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Covers previously untested pure functions in ipampool/monitor.go. Coverage: 62.7% → 73.0%. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…erver Coverage: 50.8% → 61.5%. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rbtr · 2026-04-10T13:35:43Z

/azp run Azure Container Networking PR

azure-pipelines · 2026-04-10T13:35:55Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR removes the centralized cns/fakes/ test-double package and updates impacted unit tests to use local, consumer-defined stubs/fakes. It also refactors the IPAM pool monitor to depend on a narrower CNS state interface, and adds/updates tests to validate the new test patterns and improve coverage.

Changes:

Deleted cns/fakes/ and replaced its shared fakes with per-package/per-test local stubs.
Refactored cns/ipampool monitor to depend on a minimal ipConfigState interface rather than cns.HTTPService.
Added/updated unit tests (notably around IPAM pool monitor behavior and the v2 adapter) to validate behavior directly.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
cns/service/main_test.go	Replaces shared HTTPService fake with a minimal local stub for node registration tests.
cns/restserver/v2/server_test.go	Replaces shared fakes with local wireserver/NMAgent/IMDS test doubles.
cns/restserver/restserver_test.go	Introduces shared-in-package test doubles (wireserver, IMDS, NMAgent, proxy) used by multiple restserver tests.
cns/restserver/ipam_test.go	Switches test setup and assertions to local test doubles/constants instead of `cns/fakes`.
cns/restserver/internalapi_test.go	Updates NMAgent/IMDS test doubles usage after `cns/fakes` removal.
cns/restserver/internalapi_linux_test.go	Replaces iptables fakes with locally defined mocks used by Linux-only tests.
cns/restserver/homeazmonitor_test.go	Updates to use the new in-package NMAgent fake.
cns/restserver/helper_for_nodesubnet_test.go	Updates nodesubnet helper to use in-package fakes for NMAgent and wireserver.
cns/restserver/api_test.go	Updates restserver API tests to use in-package fakes/constants; updates IMDS error context key usage.
cns/ipampool/v2/adapter_test.go	Adds tests for v2 adapter behavior (AsV1, Update, legacy observer hook).
cns/ipampool/monitor.go	Narrows dependency from `cns.HTTPService` to an internal `ipConfigState` interface.
cns/ipampool/monitor_test.go	Replaces request-controller-based fakes with a coherent in-memory IP config store stub and adds new unit tests.
cns/client/client_test.go	Replaces shared fakes with local wireserver/NMAgent/IMDS/proxy test doubles.
cns/fakes/wireserverproxyfake.go	Deleted as part of removing centralized fakes package.
cns/fakes/requestcontrollerfake.go	Deleted as part of removing centralized fakes package.
cns/fakes/nmagentclientfake.go	Deleted as part of removing centralized fakes package.
cns/fakes/monitor.go	Deleted as part of removing centralized fakes package.
cns/fakes/iptablesfake.go	Deleted as part of removing centralized fakes package.
cns/fakes/imdsclientfake.go	Deleted as part of removing centralized fakes package.
cns/fakes/cnsfake.go	Deleted as part of removing centralized fakes package.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-10T13:44:57Z

+func (n *nmaClientFake) SupportedAPIs(ctx context.Context) ([]string, error) {
+	return n.SupportedAPIsF(ctx)
+}
+
+func (n *nmaClientFake) GetNCVersionList(ctx context.Context) (nmagent.NCVersionList, error) {
+	return n.GetNCVersionListF(ctx)
+}
+
+func (n *nmaClientFake) GetHomeAz(ctx context.Context) (nmagent.AzResponse, error) {
+	return n.GetHomeAzF(ctx)
+}
+
+func (n *nmaClientFake) GetInterfaceIPInfo(ctx context.Context) (nmagent.Interfaces, error) {


nmaClientFake methods call the function fields unconditionally, so any test that constructs &nmaClientFake{} (or omits a specific *F field) will panic when that method is invoked. Consider making these methods nil-safe (returning a sensible default like nil, nil) or providing a constructor that initializes default no-op funcs, to avoid brittle tests and hidden coupling between setup and code paths.

Suggested change

func (n *nmaClientFake) SupportedAPIs(ctx context.Context) ([]string, error) {

return n.SupportedAPIsF(ctx)

}

func (n *nmaClientFake) GetNCVersionList(ctx context.Context) (nmagent.NCVersionList, error) {

return n.GetNCVersionListF(ctx)

}

func (n *nmaClientFake) GetHomeAz(ctx context.Context) (nmagent.AzResponse, error) {

return n.GetHomeAzF(ctx)

}

func (n *nmaClientFake) GetInterfaceIPInfo(ctx context.Context) (nmagent.Interfaces, error) {

func (n *nmaClientFake) SupportedAPIs(ctx context.Context) ([]string, error) {

if n.SupportedAPIsF == nil {

return nil, nil

}

return n.SupportedAPIsF(ctx)

}

func (n *nmaClientFake) GetNCVersionList(ctx context.Context) (nmagent.NCVersionList, error) {

if n.GetNCVersionListF == nil {

return nmagent.NCVersionList{}, nil

}

return n.GetNCVersionListF(ctx)

}

func (n *nmaClientFake) GetHomeAz(ctx context.Context) (nmagent.AzResponse, error) {

if n.GetHomeAzF == nil {

return nmagent.AzResponse{}, nil

}

return n.GetHomeAzF(ctx)

}

func (n *nmaClientFake) GetInterfaceIPInfo(ctx context.Context) (nmagent.Interfaces, error) {

if n.GetInterfaceIPInfoF == nil {

return nmagent.Interfaces{}, nil

}

Copilot · 2026-04-10T13:44:57Z

+	store.setAssignedTotal(state.assigned)
+	if state.pendingRelease > 0 {
+		if _, err := store.MarkIPAsPendingRelease(int(state.pendingRelease)); err != nil {
+			logger.Printf("%s", err)


newTestMonitor logs and ignores errors from store.MarkIPAsPendingRelease(...). If the input testState is inconsistent (e.g., pendingRelease > available), the helper will silently produce a monitor/store in an unexpected state and make failures harder to debug. Prefer failing fast (return the error to the caller or panic in the helper) so tests reliably catch invalid setup.

Suggested change

logger.Printf("%s", err)

panic(fmt.Sprintf("newTestMonitor: invalid test state for pending release (%d): %v", state.pendingRelease, err))

…stMonitor - nmaClientFake methods now return zero-values when function fields are nil instead of panicking on unset fields - newTestMonitor panics on invalid test state (pendingRelease > available) instead of silently logging and continuing with bad state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rbtr and others added 5 commits April 9, 2026 21:45

rename nMAgentClientFake to nmaClientFake for acronym casing

f3c5425

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

add tests for GetStateSnapshot, GenerateARMID, clampScaler

68f8881

Covers previously untested pure functions in ipampool/monitor.go. Coverage: 62.7% → 73.0%. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

add tests for ipampool/v2 adapter, AsV1, Update, WithLegacyMetricsObs…

f64f394

…erver Coverage: 50.8% → 61.5%. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

rbtr requested a review from a team as a code owner April 10, 2026 13:33

rbtr requested review from Copilot and nairashu April 10, 2026 13:33

rbtr self-assigned this Apr 10, 2026

rbtr added the cns Related to CNS. label Apr 10, 2026

rbtr requested review from jpayne3506, nddq and timraymond April 10, 2026 13:33

rbtr enabled auto-merge April 10, 2026 13:35

Copilot started reviewing on behalf of rbtr April 10, 2026 13:36 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Defake tests#4343

Defake tests#4343
rbtr wants to merge 6 commits intomasterfrom
feat/defake-tests

rbtr commented Apr 10, 2026 •

edited

Loading

Uh oh!

rbtr commented Apr 10, 2026

Uh oh!

azure-pipelines bot commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 10, 2026

Uh oh!

Copilot AI Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	logger.Printf("%s", err)
	panic(fmt.Sprintf("newTestMonitor: invalid test state for pending release (%d): %v", state.pendingRelease, err))

Conversation

rbtr commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbtr commented Apr 10, 2026

Uh oh!

azure-pipelines bot commented Apr 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rbtr commented Apr 10, 2026 •

edited

Loading