Refactoring tests in kratos package - #846
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors test functions in the kratos package by consolidating multiple individual test functions into table-driven tests, improving test maintainability and reducing code duplication.
Changes:
- Consolidated multiple test functions into table-driven tests with subtests using
t.Run - Reduced test code duplication by parameterizing test cases
- Improved test organization and readability
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/kratos/handlers_test.go | Refactored handler tests into table-driven format, consolidating tests for flow creation, session handling, login/recovery flows, and update operations |
| pkg/kratos/cookies_test.go | Refactored cookie manager tests into table-driven format, consolidating state cookie operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tests := []struct { | ||
| name string | ||
| }{ | ||
| { | ||
| name: "ClearState", | ||
| }, | ||
| } |
There was a problem hiding this comment.
The table-driven test structure for TestAuthCookieManager_ClearStateCookie contains only a single test case with no variations. This adds unnecessary complexity without benefit. Either add multiple test cases to justify the table-driven approach, or revert to a simple test function.
| if tt.requestCookie == nil { | ||
| mockEncrypt = nil | ||
| } | ||
| if mockEncrypt != nil { | ||
| tt.setupMocks(mockEncrypt, mockLogger) | ||
| } else { | ||
| tt.setupMocks(nil, mockLogger) | ||
| } |
There was a problem hiding this comment.
The conditional logic reassigning mockEncrypt to nil and then passing it to setupMocks is convoluted. Consider having setupMocks handle nil cases directly, or restructure to avoid reassigning the mock variable after initialization.
fc0f7a6 to
387139c
Compare
No description provided.