angel
4e77052a37
- dev auth flow (side-step OAuth) - db event processing integration tests - dev scripts (eg Makefile) - db / test db migration setup scripts.
35 lines
6.7 KiB
Markdown
35 lines
6.7 KiB
Markdown
# Work Summary — 2026-08-04 19:14
|
|
|
|
## Task
|
|
Broaden test coverage beyond `server/ui/charts/*_test.go` (which was the only tested package). Final step in the dev-iteration plan.
|
|
|
|
## Context
|
|
Surveyed `domains/accounts`, `domains/reports`, `domains/raw_events`, `domains/amazon` and found the codebase is almost entirely thin `Store` methods wrapping SQL - there's very little pure logic to unit test in isolation. The real risk lives in the queries themselves (already proved this by finding a live bug: `RefreshAccessToken` in `domains/authentication/auth.go:270` passes the empty named return `accessToken` instead of the `oldAccessToken` parameter to `getRefreshTokenForAccessToken` - token refresh is silently broken. Not fixed here, flagging for a decision - see Follow-ups.).
|
|
|
|
Asked the user how to test DB-heavy code; they chose a dedicated test database with the explicit ability to point the same suite at the real dev DB when wanted.
|
|
|
|
## Infra changes
|
|
- Created `inventory_2_test` Postgres database (owned by `app_client`, matching `inventory_2`'s setup). The `angel` OS-peer-auth Postgres role has `CREATEDB`; `app_client` does not, so this had to be created out-of-band, not from app code.
|
|
- **Found and fixed a real migration bug while doing this**: `database_migrations/000010_oauth_login_states.up.sql` had invalid SQL (`NOW() + 10 'minute'`) that fails on any fresh database - a hard blocker for setting up the test DB, and for anyone else spinning up this project from scratch. Fixed to `NOW() + '10 minutes'::interval`, matching what the live dev DB actually runs (confirmed via `psql \d`).
|
|
- **Found and fixed schema drift**: migrating fresh revealed `oauth_tokens` in the migration files still has a `claims JSONB NOT NULL` column and a `TEXT`-typed `id_token_custom_claims_updated_at` - neither matches the live dev DB (no `claims` column at all; that column is `timestamptz`), and no application code reads/writes `claims`. Someone patched the dev DB by hand at some point without ever committing the migration. Added `database_migrations/000030_fix_oauth_tokens_schema_drift.{up,down}.sql` to close the gap, then `migrate force 30` on the *dev* DB (schema already matched, just needed the migration bookkeeping to catch up) and a normal `migrate up` on the new test DB. Verified both DBs now have an identical `oauth_tokens` shape (column ordering differs cosmetically, doesn't matter - the app scans by column name).
|
|
- New `internal/testdb` package: `Pool(t)` connects via `TEST_DATABASE_URL` (skips the test if unset, so `go test ./...` doesn't hard-require Postgres), `Logger()` for a discard-output logger, `NewUserID(t)` for collision-safe fixture IDs, `SeedOAuthUser`/`SeedOAuthSession` for tests that need a valid `oauth_users`/`oauth_tokens` row. All seed helpers register `t.Cleanup` in FK-safe order.
|
|
- `Makefile`: added `test` (migrates the test DB, then `go test ./...`), `test-against-dev-db` (same suite, `TEST_DATABASE_URL` overridden to `DATABASE_URL` for one run - the "option to run against the real database" the user asked for), and `migrate-test-up`/`down`/`version`.
|
|
- `.env` / `.env.example`: added `TEST_DATABASE_URL`.
|
|
|
|
## Test coverage added
|
|
- `domains/authentication/dev_test.go`: `DevLogin` round-trips through `GetAccessTokenClaimsAndExpiration` correctly, is safe to call twice for the same `user_id` (mints a new token each time), and `GetAccessTokenClaimsAndExpiration` returns `ErrNotFound` for an unknown token.
|
|
- `domains/accounts/accounts_test.go`: `CreateAccount` happy path + round-trip through `GetAccount`; duplicate `user_id` returns `ErrConflict`; `GetAccount` on a missing ID returns `ErrNotFound`; `GetUserAndAccountByAccessToken` correctly resolves the user-but-no-account state and the user-with-account state (the same join logic the auth middleware depends on for every request).
|
|
- `domains/raw_events/events_test.go`: `Save` + `LoadEventsForStore` round-trip, ordering (newest first), and the empty-store case.
|
|
|
|
## Verification
|
|
- `make test`: all new tests pass against `inventory_2_test`. `make test-against-dev-db`: same suite, same results, against the real dev DB - confirmed zero leftover rows afterward (`SELECT count(*) FROM oauth_users WHERE user_id LIKE 'test-%'` → 0, same for `raw_store_events`).
|
|
- `go build ./...` and `go vet ./...` clean except two pre-existing unreachable-code warnings unrelated to this work.
|
|
- The only test failures are `ExampleBar`/`ExampleLineChart` in `server/ui/charts` - confirmed pre-existing and unrelated (verified via `git stash` that they fail identically on the pre-session code; see Incident below). Their hardcoded `// Output:` expectations are stale relative to the current chart-rendering code (missing padding/border/label styling that's since been added).
|
|
|
|
## Incident during verification (self-caused, recovered cleanly)
|
|
Used `git stash` / `git stash pop` to check whether the chart test failures were pre-existing, and the pop conflicted on `server/ui/charts/test.svg` (a file the chart tests overwrite as a side effect of running - it was already dirty before this session even started). Resolved by discarding the working-tree's post-test-run `test.svg` (disposable scratch content) and re-running `git stash pop`, which then applied cleanly. Verified afterward with `go build ./...` and a full `git status` review that every change from this session (and the prior two) was intact. No work was lost, but noting it: don't reach for `git stash` casually in a working tree with substantial uncommitted work when a narrower check (e.g. `git worktree` or just re-reading the file) would do.
|
|
|
|
## Follow-ups / not done here
|
|
- **`RefreshAccessToken` bug** (`domains/authentication/auth.go:270`): passes the empty named-return `accessToken` instead of the `oldAccessToken` parameter to `getRefreshTokenForAccessToken`, so token refresh looks up an empty string instead of the real token and will always fail to find a refresh token. This means the "refresh when access token is old enough" path in `server/auth/auth.go` (`AuthenticateHandler`) is currently broken for real (non-dev) sessions - users would get bounced to `/` with a "failed to refresh access token" body once their 48-hour ID token crosses the refresh floor, instead of transparently refreshing. Did not fix it in this pass since it's a production auth-flow behavior change, not a test-coverage change - flagging for an explicit decision. It's a one-line fix (`accessToken` → `oldAccessToken` on that call).
|
|
- No coverage added for `domains/reports` or `domains/amazon` - `reports.GetRawShopEvents` needs a mock-shop fixture (depends on `accts.GetMockShop`, which lives in the much larger `mocks.go`/`sync_groups.go` surface) and `domains/amazon`'s mock event pipeline is stateful/background-process-shaped, both meaningfully bigger lifts than what fit in this pass.
|