From d6152a5dec5fdd846a38aa472a65e323cacc3be2 Mon Sep 17 00:00:00 2001 From: Husni Adil Makmur Date: Sun, 2 Aug 2026 12:30:00 +0700 Subject: [PATCH] fix: restore the internal unit tests and run them in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `go test ./internal/...` fails on main. Two of the three packages fail to compile, which means internal/cache and internal/provider/vault have not actually been tested for a while — their test files reference struct fields that no longer exist. Nothing caught this because no workflow runs them. CI runs only `-short ./tests/end2end/...`; the Makefile's `test` target does run `./...`, but CI does not call it. So the tests rotted in place while every check stayed green. cache: keyringTested was replaced by a sync.Once guard, so the test set a field that no longer exists. Consuming keyringOnce before flipping keyringDisabled reproduces the intent without reaching for the removed field. TestCache_Stats was also timing-dependent — a 150ms TTL against two keyring writes that each shell out to the OS keyring, so the first entry expired while the second was still being written. It now asserts validity and expiry with separate TTLs and no sleeps. vault: the flat auth config (auth/authMount/role as top-level keys) moved into a nested Auth struct, and the JWT role error message became 'auth.role'. The tests still used the old shape, which no longer even unmarshals. Updated to the nested form that CONFIGURATION.md and SSO.md already document. gcsm: one table case omitted wantSecretID, so it asserted "" against a config that sets secret_id. All changes are to test files; no production behaviour is touched. The one non-test change adds `go test ./internal/...` to CI so these cannot rot again. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Hmh2p2Bg6kmxxvzpFDW2WL --- .github/workflows/ci.yml | 8 ++++ internal/cache/cache_test.go | 35 ++++++++------ internal/provider/gcsm/gcsm_test.go | 4 +- internal/provider/vault/vault_test.go | 66 +++++++++++++++------------ 4 files changed, 70 insertions(+), 43 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 67aca6d..9905db8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,6 +24,14 @@ jobs: with: go-version-file: go.mod + # The unit tests under ./internal/... were not covered by any workflow, so + # they silently stopped compiling as the packages they test were + # refactored. Run them first and let a failure fail the job. + - name: Run unit tests + env: + CGO_LDFLAGS: -lm + run: go test ./internal/... + - name: Run tests in short mode env: CGO_LDFLAGS: -lm diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index a651331..5abbf08 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -294,28 +294,34 @@ func TestCache_Stats(t *testing.T) { t.Errorf("expected empty stats after clear, got total=%d", total) } - // Use short-lived cache for this test - shortCache := New(WithTTL(150 * time.Millisecond)) - - // Use unique keys + // Use unique keys so a leftover store from another run cannot affect counts key1 := fmt.Sprintf("stats-key1-%d", time.Now().UnixNano()) key2 := fmt.Sprintf("stats-key2-%d", time.Now().UnixNano()) - // Add entries quickly - _ = shortCache.Set(key1, map[string]string{"K": "V"}) - _ = shortCache.Set(key2, map[string]string{"K": "V"}) + // Freshly written entries are valid. The TTL has to be comfortably longer + // than two keyring writes take: each Set shells out to the OS keyring, which + // can cost hundreds of milliseconds, and a tight TTL expires key1 while key2 + // is still being written. + validCache := New(WithTTL(5 * time.Minute)) + _ = validCache.Set(key1, map[string]string{"K": "V"}) + _ = validCache.Set(key2, map[string]string{"K": "V"}) - total, valid, expired = shortCache.Stats() + total, valid, expired = validCache.Stats() if valid != 2 { t.Errorf("expected 2 valid entries immediately after set, got valid=%d, expired=%d", valid, expired) } - // Wait for expiration - time.Sleep(200 * time.Millisecond) + _ = c.Clear() + + // A negative TTL writes entries whose ExpiresAt is already in the past, so + // expiry is asserted without sleeping on a real clock. + expiredCache := New(WithTTL(-1 * time.Minute)) + _ = expiredCache.Set(key1, map[string]string{"K": "V"}) + _ = expiredCache.Set(key2, map[string]string{"K": "V"}) - total, valid, expired = shortCache.Stats() + total, valid, expired = expiredCache.Stats() if expired != 2 { - t.Errorf("expected 2 expired entries after TTL, got valid=%d, expired=%d", valid, expired) + t.Errorf("expected 2 expired entries, got valid=%d, expired=%d", valid, expired) } // Clean up @@ -342,8 +348,9 @@ func TestCache_IsAvailable(t *testing.T) { func TestCache_KeyringNotAvailable(t *testing.T) { cache := New() - // Force keyring to be disabled - cache.keyringTested = true + // Force keyring to be disabled. Consuming keyringOnce first stops + // isKeyringAvailable from probing the real keyring and overwriting this. + cache.keyringOnce.Do(func() {}) cache.keyringDisabled = true // All operations should gracefully handle unavailable keyring diff --git a/internal/provider/gcsm/gcsm_test.go b/internal/provider/gcsm/gcsm_test.go index 62947e1..a591d9e 100644 --- a/internal/provider/gcsm/gcsm_test.go +++ b/internal/provider/gcsm/gcsm_test.go @@ -75,7 +75,9 @@ func TestParseConfig(t *testing.T) { "project_id": "", "secret_id": "my-secret", }, - wantErr: false, // parseConfig doesn't validate, Fetch does + wantProjectID: "", + wantSecretID: "my-secret", + wantErr: false, // parseConfig doesn't validate, Fetch does }, { name: "config with missing project_id field", diff --git a/internal/provider/vault/vault_test.go b/internal/provider/vault/vault_test.go index 23b5281..871ac12 100644 --- a/internal/provider/vault/vault_test.go +++ b/internal/provider/vault/vault_test.go @@ -30,7 +30,7 @@ func TestParseConfigWithAuthOptions(t *testing.T) { name: "config with explicit token auth", config: map[string]interface{}{ "path": "myapp/secret", - "auth": "token", + "auth": map[string]interface{}{"method": "token"}, }, wantAuth: "token", wantAuthMount: "", @@ -41,8 +41,7 @@ func TestParseConfigWithAuthOptions(t *testing.T) { name: "config with oidc auth", config: map[string]interface{}{ "path": "myapp/secret", - "auth": "oidc", - "role": "my-role", + "auth": map[string]interface{}{"method": "oidc", "role": "my-role"}, }, wantAuth: "oidc", wantAuthMount: "", @@ -52,10 +51,12 @@ func TestParseConfigWithAuthOptions(t *testing.T) { { name: "config with jwt auth and custom mount", config: map[string]interface{}{ - "path": "myapp/secret", - "auth": "jwt", - "authMount": "custom-jwt", - "role": "app-role", + "path": "myapp/secret", + "auth": map[string]interface{}{ + "method": "jwt", + "mount": "custom-jwt", + "role": "app-role", + }, }, wantAuth: "jwt", wantAuthMount: "custom-jwt", @@ -75,14 +76,15 @@ func TestParseConfigWithAuthOptions(t *testing.T) { return } - if cfg.Auth != tt.wantAuth { - t.Errorf("parseConfig() Auth = %v, want %v", cfg.Auth, tt.wantAuth) + method, mount, role, _ := authFields(cfg) + if method != tt.wantAuth { + t.Errorf("parseConfig() Auth.Method = %v, want %v", method, tt.wantAuth) } - if cfg.AuthMount != tt.wantAuthMount { - t.Errorf("parseConfig() AuthMount = %v, want %v", cfg.AuthMount, tt.wantAuthMount) + if mount != tt.wantAuthMount { + t.Errorf("parseConfig() Auth.Mount = %v, want %v", mount, tt.wantAuthMount) } - if cfg.Role != tt.wantRole { - t.Errorf("parseConfig() Role = %v, want %v", cfg.Role, tt.wantRole) + if role != tt.wantRole { + t.Errorf("parseConfig() Auth.Role = %v, want %v", role, tt.wantRole) } }) } @@ -91,8 +93,7 @@ func TestParseConfigWithAuthOptions(t *testing.T) { func TestParseConfigWithSSOTokens(t *testing.T) { config := map[string]interface{}{ "path": "myapp/secret", - "auth": "oidc", - "role": "my-role", + "auth": map[string]interface{}{"method": "oidc", "role": "my-role"}, "_sso_access_token": "test-access-token-123", "_sso_id_token": "test-id-token-456", } @@ -123,18 +124,17 @@ func TestVaultProvider_Fetch_OIDCAuthValidation(t *testing.T) { name: "oidc auth without role", config: map[string]interface{}{ "path": "myapp/secret", - "auth": "oidc", + "auth": map[string]interface{}{"method": "oidc"}, "_sso_access_token": "test-token", }, wantErr: true, - errMsg: "requires 'role' field", + errMsg: "requires 'auth.role' field", }, { name: "oidc auth without SSO token", config: map[string]interface{}{ "path": "myapp/secret", - "auth": "oidc", - "role": "my-role", + "auth": map[string]interface{}{"method": "oidc", "role": "my-role"}, }, wantErr: true, errMsg: "no SSO token available", @@ -143,17 +143,17 @@ func TestVaultProvider_Fetch_OIDCAuthValidation(t *testing.T) { name: "jwt auth without role", config: map[string]interface{}{ "path": "myapp/secret", - "auth": "jwt", + "auth": map[string]interface{}{"method": "jwt"}, "_sso_id_token": "test-token", }, wantErr: true, - errMsg: "requires 'role' field", + errMsg: "requires 'auth.role' field", }, { name: "unsupported auth method", config: map[string]interface{}{ "path": "myapp/secret", - "auth": "invalid-method", + "auth": map[string]interface{}{"method": "invalid-method"}, }, wantErr: true, errMsg: "unsupported auth method", @@ -286,8 +286,9 @@ func TestParseConfig(t *testing.T) { if cfg.Address != tt.wantAddress { t.Errorf("parseConfig() Address = %v, want %v", cfg.Address, tt.wantAddress) } - if cfg.Token != tt.wantToken { - t.Errorf("parseConfig() Token = %v, want %v", cfg.Token, tt.wantToken) + // A top-level `token:` is folded into Auth.Token for backward compatibility + if _, _, _, token := authFields(cfg); token != tt.wantToken { + t.Errorf("parseConfig() Auth.Token = %v, want %v", token, tt.wantToken) } if cfg.Mount != tt.wantMount { t.Errorf("parseConfig() Mount = %v, want %v", cfg.Mount, tt.wantMount) @@ -381,8 +382,8 @@ func TestVaultProvider_ConfigFields(t *testing.T) { if cfg.Address != "https://custom-vault.example.com:8200" { t.Errorf("Config.Address = %v, want %v", cfg.Address, "https://custom-vault.example.com:8200") } - if cfg.Token != "custom-token-123" { - t.Errorf("Config.Token = %v, want %v", cfg.Token, "custom-token-123") + if _, _, _, token := authFields(cfg); token != "custom-token-123" { + t.Errorf("Config.Auth.Token = %v, want %v", token, "custom-token-123") } if cfg.Mount != "custom-secret-engine" { t.Errorf("Config.Mount = %v, want %v", cfg.Mount, "custom-secret-engine") @@ -406,8 +407,8 @@ func TestVaultProvider_ConfigWithOptionalFields(t *testing.T) { if cfg.Address != "" { t.Errorf("Config.Address = %v, want empty string", cfg.Address) } - if cfg.Token != "" { - t.Errorf("Config.Token = %v, want empty string", cfg.Token) + if _, _, _, token := authFields(cfg); token != "" { + t.Errorf("Config.Auth.Token = %v, want empty string", token) } if cfg.Mount != "" { t.Errorf("Config.Mount = %v, want empty string", cfg.Mount) @@ -453,3 +454,12 @@ func containsSubstring(s, substr string) bool { return false } + +// authFields flattens the nested auth config so tests can assert on it without +// nil-checking VaultConfig.Auth at every call site. +func authFields(cfg *VaultConfig) (method, mount, role, token string) { + if cfg == nil || cfg.Auth == nil { + return "", "", "", "" + } + return cfg.Auth.Method, cfg.Auth.Mount, cfg.Auth.Role, cfg.Auth.Token +}