Expand test suite with adapter and DB coverage #42
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive testing and benchmarking support for both the in-memory and legacy database adapters, and includes the SQLite driver dependency.
- Adds
github.com/mattn/go-sqlite3ingo.modfor DB tests. - Expands unit tests for
LegacyDBAdapterandLoggingAdapter, including error paths and metrics. - Introduces a full integration test for the OAuth2 flow against the legacy DB and performance benchmarks for token creation.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| logging_adapter_test.go | Unit tests for LoggingAdapter metrics and error increment logic |
| integration_legacy_db_test.go | Full OAuth2 authorization code flow against LegacyDBAdapter |
| go.mod | Added SQLite driver dependency |
| db_adapter_test.go | Unit tests for LegacyDBAdapter client, token, session, and JWT |
| benchmark_test.go | Benchmarks for token creation in in-memory and legacy adapters |
| } | ||
|
|
||
| metrics := adapter.Metrics() | ||
| expected := []string{"CreateClient", "GetClient", "UpdateClient", "DeleteClient", "CreateToken", "GetToken", "RevokeToken", "DeleteToken", "CreateSession", "GetSession", "DeleteSession", "ValidateJWT", "MarkJWTAsUsed", "CreatePKCESession", "GetPKCESession", "DeletePKCESession"} |
There was a problem hiding this comment.
The expected metric names for PKCE sessions (CreatePKCESession, GetPKCESession, DeletePKCESession) do not match the adapter's actual method names (CreatePKCERequestSession, GetPKCERequestSession, DeletePKCERequestSession). Update the expected keys to match the correct method names.
| expected := []string{"CreateClient", "GetClient", "UpdateClient", "DeleteClient", "CreateToken", "GetToken", "RevokeToken", "DeleteToken", "CreateSession", "GetSession", "DeleteSession", "ValidateJWT", "MarkJWTAsUsed", "CreatePKCESession", "GetPKCESession", "DeletePKCESession"} | |
| expected := []string{"CreateClient", "GetClient", "UpdateClient", "DeleteClient", "CreateToken", "GetToken", "RevokeToken", "DeleteToken", "CreateSession", "GetSession", "DeleteSession", "ValidateJWT", "MarkJWTAsUsed", "CreatePKCERequestSession", "GetPKCERequestSession", "DeletePKCERequestSession"} |
| if _, err := db.Exec(s); err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The benchmark setup (opening DB and creating tables) is included in the measurement; to focus the benchmark on token creation only, call b.ResetTimer() after setup and before the measurement loop.
| } | |
| } | |
| b.ResetTimer() |
| "github.com/ory/fosite" | ||
| ) | ||
|
|
||
| func setupTestDB(t *testing.T) *sql.DB { |
There was a problem hiding this comment.
The in-memory database opened here is never closed, which can lead to resource leaks. Consider using t.Cleanup to close the database before tests complete (e.g., t.Cleanup(func() { db.Close() })).
| "github.com/ory/fosite/token/jwt" | ||
| ) | ||
|
|
||
| // setupLegacyStore initializes the global store with a LoggingAdapter over LegacyDBAdapter. |
There was a problem hiding this comment.
The comment mentions wrapping the store with a LoggingAdapter, but the code uses NewLegacyDBAdapter directly. Either wrap with NewLoggingAdapter(NewLegacyDBAdapter(db)) or update the comment to reflect the actual behavior.
Summary
Testing
go vet ./...go test ./... -vgo test -bench=. -run=NONEhttps://chatgpt.com/codex/tasks/task_e_687443f066ac832881de289ac38519fa