From 38819660ef0a0eb52dc4d5a8e1cbaac1707fdb1b Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 17 Mar 2026 08:44:07 +0000 Subject: [PATCH] =?UTF-8?q?fix(unifi):=20DX=20audit=20=E2=80=94=20fix=20te?= =?UTF-8?q?sts,=20add=20missing=20Commit(),=20improve=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix SaveConfig: add cfg.Commit() so credentials actually persist to disk - Fix TestResolveConfig and TestNewFromConfig: isolate from real config file by setting HOME to temp dir, preventing env/config bleed - Add RouteTypeName, GetRoutes, and GetNetworks unit tests with httptest mocks (coverage 39% → 55%) - Update CLAUDE.md: add error handling and test isolation conventions - Update docs: fix Go version (1.25 → 1.26), remove stale replace directive references, add cmd/unifi/ to architecture diagram Co-Authored-By: Virgil --- CLAUDE.md | 2 + docs/architecture.md | 12 +++++- docs/development.md | 24 ++++------- unifi/config.go | 4 ++ unifi/config_test.go | 13 ++++-- unifi/networks_test.go | 95 ++++++++++++++++++++++++++++++++++++++++++ unifi/routes_test.go | 92 ++++++++++++++++++++++++++++++++++++++++ 7 files changed, 221 insertions(+), 21 deletions(-) create mode 100644 unifi/networks_test.go create mode 100644 unifi/routes_test.go diff --git a/CLAUDE.md b/CLAUDE.md index af82700..0ebdd5f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -19,6 +19,8 @@ go vet ./... # vet check - `go test -race ./...` and `go vet ./...` must pass before commit - Conventional commits: `type(scope): description` - Co-Author: `Co-Authored-By: Virgil ` +- Errors must use `log.E()` from `go-log`, never `fmt.Errorf` +- Config tests must isolate from real `~/.core/config.yaml` via `t.Setenv("HOME", t.TempDir())` ## Docs diff --git a/docs/architecture.md b/docs/architecture.md index a0daea1..815fd94 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -13,6 +13,14 @@ config resolution from file, environment, and CLI flags. ``` go-netops/ +├── cmd/unifi/ # CLI commands (cobra) +│ ├── cmd_unifi.go # Root 'unifi' command registration +│ ├── cmd_clients.go # 'unifi clients' command +│ ├── cmd_config.go # 'unifi config' command +│ ├── cmd_devices.go # 'unifi devices' command +│ ├── cmd_networks.go # 'unifi networks' command +│ ├── cmd_routes.go # 'unifi routes' command +│ └── cmd_sites.go # 'unifi sites' command ├── unifi/ │ ├── client.go # Client struct and constructor (New) │ ├── clients.go # Connected client queries with filtering @@ -22,7 +30,9 @@ go-netops/ │ ├── routes.go # Gateway routing table queries │ ├── sites.go # Site listing │ ├── client_test.go # Client creation tests -│ └── config_test.go # Config resolution and persistence tests +│ ├── config_test.go # Config resolution and persistence tests +│ ├── networks_test.go # Network query tests +│ └── routes_test.go # Route query and type name tests ├── go.mod └── go.sum ``` diff --git a/docs/development.md b/docs/development.md index f91d768..9c16329 100644 --- a/docs/development.md +++ b/docs/development.md @@ -6,12 +6,9 @@ Module: `forge.lthn.ai/core/go-netops` | Tool | Version | Notes | |------------|-----------|-----------------------------------| -| Go | 1.25+ | Module uses `go 1.25.5` | -| core/go | local | `replace` directive in go.mod | +| Go | 1.26+ | Module uses `go 1.26.0` | -The `go.mod` contains a `replace` directive pointing at `../go` for the -`forge.lthn.ai/core/go` dependency. Ensure the `core/go` repository is -checked out alongside this one (the standard workspace layout handles this). +Dependencies are resolved from the Forge registry (`forge.lthn.ai`). ## Build and Test @@ -60,19 +57,12 @@ Config tests isolate the environment by: ### What Is Not Tested -The query methods (`GetClients`, `GetDevices`, `GetDeviceList`, `GetNetworks`, -`GetRoutes`, `GetSites`) require a live or fully-mocked UniFi controller and -are not covered by unit tests. Current coverage is **39%**. +The client-discovery methods (`GetClients`, `GetDevices`, `GetDeviceList`, +`GetSites`) delegate to the unpoller SDK and would require integration tests +against a real or emulated controller to cover meaningfully. -## Coverage - -``` -ok forge.lthn.ai/core/go-netops/unifi coverage: 39.0% of statements -``` - -Coverage is concentrated on the config resolution and client construction paths. -The query methods delegate to the unpoller SDK and would require integration -tests against a real or emulated controller to cover meaningfully. +`GetNetworks`, `GetRoutes`, and `RouteTypeName` are covered by unit tests +using httptest mocks and direct assertions. ## Coding Standards diff --git a/unifi/config.go b/unifi/config.go index d5c98be..18c51cc 100644 --- a/unifi/config.go +++ b/unifi/config.go @@ -141,5 +141,9 @@ func SaveConfig(url, user, pass, apikey string, insecure *bool) error { } } + if err := cfg.Commit(); err != nil { + return log.E("unifi.SaveConfig", "failed to write config file", err) + } + return nil } diff --git a/unifi/config_test.go b/unifi/config_test.go index 1827a8b..c1e4a63 100644 --- a/unifi/config_test.go +++ b/unifi/config_test.go @@ -11,7 +11,8 @@ import ( ) func TestResolveConfig(t *testing.T) { - // Clear environment variables to start clean + // Isolate from real config file and environment + t.Setenv("HOME", t.TempDir()) os.Unsetenv("UNIFI_URL") os.Unsetenv("UNIFI_USER") os.Unsetenv("UNIFI_PASS") @@ -74,6 +75,14 @@ func TestResolveConfig(t *testing.T) { } func TestNewFromConfig(t *testing.T) { + // Isolate from real config file and environment + t.Setenv("HOME", t.TempDir()) + os.Unsetenv("UNIFI_URL") + os.Unsetenv("UNIFI_USER") + os.Unsetenv("UNIFI_PASS") + os.Unsetenv("UNIFI_APIKEY") + os.Unsetenv("UNIFI_INSECURE") + // Mock UniFi controller ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -88,8 +97,6 @@ func TestNewFromConfig(t *testing.T) { assert.Equal(t, ts.URL, client.URL()) // 2. Error case: No credentials - os.Unsetenv("UNIFI_USER") - os.Unsetenv("UNIFI_APIKEY") client, err = NewFromConfig("", "", "", "", nil) assert.Error(t, err) assert.Nil(t, client) diff --git a/unifi/networks_test.go b/unifi/networks_test.go new file mode 100644 index 0000000..8a81b5c --- /dev/null +++ b/unifi/networks_test.go @@ -0,0 +1,95 @@ +package unifi + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestGetNetworks(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + // New-style controller: paths are prefixed with /proxy/network + if r.URL.Path == "/proxy/network/api/s/default/rest/networkconf" { + fmt.Fprintln(w, `{"data":[{"_id":"abc123","name":"LAN","purpose":"corporate","ip_subnet":"10.69.1.0/24","vlan":0,"vlan_enabled":false,"enabled":true,"networkgroup":"LAN","dhcpd_enabled":true,"dhcpd_start":"10.69.1.100","dhcpd_stop":"10.69.1.254"}]}`) + return + } + fmt.Fprintln(w, `{"meta":{"rc":"ok"}, "data": []}`) + })) + defer ts.Close() + + client, err := New(ts.URL, "user", "pass", "", true) + assert.NoError(t, err) + + networks, err := client.GetNetworks("") + assert.NoError(t, err) + if assert.Len(t, networks, 1) { + assert.Equal(t, "abc123", networks[0].ID) + assert.Equal(t, "LAN", networks[0].Name) + assert.Equal(t, "corporate", networks[0].Purpose) + assert.Equal(t, "10.69.1.0/24", networks[0].IPSubnet) + assert.True(t, networks[0].Enabled) + assert.True(t, networks[0].DHCPEnabled) + assert.Equal(t, "10.69.1.100", networks[0].DHCPStart) + assert.Equal(t, "10.69.1.254", networks[0].DHCPStop) + } +} + +func TestGetNetworks_CustomSite(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/proxy/network/api/s/office/rest/networkconf" { + fmt.Fprintln(w, `{"data":[{"_id":"def456","name":"Office LAN","purpose":"corporate","ip_subnet":"172.16.0.0/24"}]}`) + return + } + fmt.Fprintln(w, `{"meta":{"rc":"ok"}, "data": []}`) + })) + defer ts.Close() + + client, err := New(ts.URL, "user", "pass", "", true) + assert.NoError(t, err) + + networks, err := client.GetNetworks("office") + assert.NoError(t, err) + if assert.Len(t, networks, 1) { + assert.Equal(t, "Office LAN", networks[0].Name) + } +} + +func TestGetNetworks_Empty(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintln(w, `{"meta":{"rc":"ok"}, "data": []}`) + })) + defer ts.Close() + + client, err := New(ts.URL, "user", "pass", "", true) + assert.NoError(t, err) + + networks, err := client.GetNetworks("") + assert.NoError(t, err) + assert.Empty(t, networks) +} + +func TestGetNetworks_InvalidJSON(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/proxy/network/api/s/default/rest/networkconf" { + fmt.Fprintln(w, `not valid json`) + return + } + fmt.Fprintln(w, `{"meta":{"rc":"ok"}, "data": []}`) + })) + defer ts.Close() + + client, err := New(ts.URL, "user", "pass", "", true) + assert.NoError(t, err) + + networks, err := client.GetNetworks("") + assert.Error(t, err) + assert.Nil(t, networks) + assert.Contains(t, err.Error(), "failed to parse networks") +} diff --git a/unifi/routes_test.go b/unifi/routes_test.go new file mode 100644 index 0000000..da6974b --- /dev/null +++ b/unifi/routes_test.go @@ -0,0 +1,92 @@ +package unifi + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRouteTypeName(t *testing.T) { + tests := []struct { + code string + expected string + }{ + {"S", "static"}, + {"C", "connected"}, + {"K", "kernel"}, + {"B", "bgp"}, + {"O", "ospf"}, + {"X", "X"}, + {"", ""}, + } + + for _, tt := range tests { + assert.Equal(t, tt.expected, RouteTypeName(tt.code)) + } +} + +func TestGetRoutes(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + // New-style controller: paths are prefixed with /proxy/network + if r.URL.Path == "/proxy/network/api/s/default/stat/routing" { + fmt.Fprintln(w, `{"data":[{"pfx":"10.0.0.0/24","nh":"10.0.0.1","intf":"br0","type":"C","distance":0,"metric":0,"uptime":3600,"fib":true}]}`) + return + } + fmt.Fprintln(w, `{"meta":{"rc":"ok"}, "data": []}`) + })) + defer ts.Close() + + client, err := New(ts.URL, "user", "pass", "", true) + assert.NoError(t, err) + + routes, err := client.GetRoutes("") + assert.NoError(t, err) + if assert.Len(t, routes, 1) { + assert.Equal(t, "10.0.0.0/24", routes[0].Network) + assert.Equal(t, "10.0.0.1", routes[0].NextHop) + assert.Equal(t, "br0", routes[0].Interface) + assert.Equal(t, "C", routes[0].Type) + assert.True(t, routes[0].Selected) + } +} + +func TestGetRoutes_CustomSite(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/proxy/network/api/s/mysite/stat/routing" { + fmt.Fprintln(w, `{"data":[{"pfx":"192.168.1.0/24","nh":"192.168.1.1","intf":"eth0","type":"S","distance":1,"metric":0,"uptime":0,"fib":true}]}`) + return + } + fmt.Fprintln(w, `{"meta":{"rc":"ok"}, "data": []}`) + })) + defer ts.Close() + + client, err := New(ts.URL, "user", "pass", "", true) + assert.NoError(t, err) + + routes, err := client.GetRoutes("mysite") + assert.NoError(t, err) + if assert.Len(t, routes, 1) { + assert.Equal(t, "192.168.1.0/24", routes[0].Network) + assert.Equal(t, "S", routes[0].Type) + } +} + +func TestGetRoutes_Empty(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + fmt.Fprintln(w, `{"meta":{"rc":"ok"}, "data": []}`) + })) + defer ts.Close() + + client, err := New(ts.URL, "user", "pass", "", true) + assert.NoError(t, err) + + routes, err := client.GetRoutes("") + assert.NoError(t, err) + assert.Empty(t, routes) +}