Skip to content

fix: restore the internal unit tests and run them in CI - #148

Open
husniadil wants to merge 1 commit into
securestart:mainfrom
husniadil:fix/restore-internal-unit-tests
Open

fix: restore the internal unit tests and run them in CI#148
husniadil wants to merge 1 commit into
securestart:mainfrom
husniadil:fix/restore-internal-unit-tests

Conversation

@husniadil

Copy link
Copy Markdown

The state on main

$ go test ./internal/...
FAIL  internal/cache                 [build failed]
FAIL  internal/provider/vault        [build failed]
FAIL  internal/provider/gcsm         0.221s

Two of these fail to compile, which is the part worth pausing on: internal/cache and internal/provider/vault have not been running at all for a while. Their test files reference struct fields that no longer exist, so there is no coverage there — only the appearance of it.

Why nothing noticed

No workflow runs them. The test job runs exactly one command:

gotestsum ... -- -short ./tests/end2end/...

The Makefile has test: go test ./..., which would have caught all three, but CI never calls it. So the packages under ./internal/... were refactored, their tests were left behind, and every check stayed green throughout.

This is the same shape as the azure_keyvault gap in #146: a check that proves something adjacent to what you actually care about.

What was wrong

internal/cachekeyringTested was replaced by a sync.Once guard, but TestCache_KeyringNotAvailable still assigned to the removed field. Consuming keyringOnce before setting keyringDisabled preserves the test's intent (force the unavailable path) without the deleted field.

Once it compiled, a second failure surfaced underneath: TestCache_Stats used a 150ms TTL and then performed two Set calls. Each Set shells out to the OS keyring and can cost hundreds of milliseconds, so entry one had already expired before entry two finished writing — the test was timing-dependent and would fail on any slow keyring. It now asserts the valid case with a generous TTL and the expired case with a negative TTL, so neither branch depends on wall-clock timing and no sleep is needed.

internal/provider/vault — the auth config moved from flat top-level keys (auth: "jwt", authMount, role) into a nested Auth *VaultAuthConfig{Method, Mount, Role, Token}. The old shape does not even unmarshal now: json.Unmarshal cannot put a string into a struct field. The tests are updated to the nested form, which is what CONFIGURATION.md and SSO.md already document.

Two expectations also drifted with the code and are corrected against what authenticateWithJWT actually returns: the JWT/OIDC role error is now requires 'auth.role' field, not requires 'role' field. The top-level token: shorthand still works — parseConfig folds it into Auth.Token for backward compatibility — so those cases keep their flat config and only the assertion moved.

internal/provider/gcsm — the config with empty project_id table case omitted wantSecretID, so it asserted "" against a config that sets secret_id: my-secret. Filled in, matching the sibling case directly below it.

Change of scope

Everything above is test-only; no production code is touched. The single non-test change adds one step to the test job:

- name: Run unit tests
  run: go test ./internal/...

Without it these tests can rot again the same way, and the fixes here would be worth very little. I put it before the e2e step so a compile failure fails fast.

Verification

ok  github.com/dirathea/sstart/internal/cache
ok  github.com/dirathea/sstart/internal/mcp
ok  github.com/dirathea/sstart/internal/provider
ok  github.com/dirathea/sstart/internal/provider/aws
ok  github.com/dirathea/sstart/internal/provider/dotenv
ok  github.com/dirathea/sstart/internal/provider/gcsm
ok  github.com/dirathea/sstart/internal/provider/vault

go build ./... and go vet ./internal/... are clean. Stashing the changes reproduces the original compile failures, so the fixes are load-bearing rather than incidental.

Note that ten packages under ./internal/ still report [no test files], including config, secrets, and oidc. That is a real gap but out of scope here — this PR only restores what already existed.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Hmh2p2Bg6kmxxvzpFDW2WL

`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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hmh2p2Bg6kmxxvzpFDW2WL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant