Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdd configurable lockout duration to AuthService, remove several legacy unit test files, expand and reorganize auth integration/unit tests (including real lockout tests), and make test DB cleanup dynamic and resilient. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR migrates the AuthService tests from mock-based unit tests to integration tests using real database dependencies via testcontainers. The migration improves test realism while maintaining comprehensive coverage of the authentication workflows.
Changes:
- Deleted
auth_internal_test.gowhich contained mock-based tests for the AuthService - Enhanced
auth_test.gowith structured subtests for Register, Login, and new error scenarios using real database connections - Added mixed testing approach for error scenarios, combining real UserRepo with mocked services to test rollback and error handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| internal/core/services/auth_test.go | Expanded with real DB tests for Register (Success, DuplicateEmail, WeakPassword), Login (Success, InvalidPassword, UserNotFound), Lockout, ValidateUser, and added error scenarios for tenant/identity failures; updated existing tests with unique emails using uuid |
| internal/core/services/auth_internal_test.go | Removed mock-based internal tests; functionality migrated to auth_test.go with real database |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestAuthServiceValidateToken(t *testing.T) { | ||
| _, svc, _, identitySvc := setupAuthServiceTest(t) | ||
| ctx := context.Background() | ||
| user, _ := svc.Register(ctx, "val_" + uuid.NewString() + "@test.com", testPassword, "Val User") |
There was a problem hiding this comment.
The error from svc.Register is being ignored with _. If registration fails, the ValidateUser test will not function correctly since it assumes a user exists. Consider using require.NoError to ensure the test setup is valid.
| mockIdentity := new(MockIdentityService) | ||
| mockAudit := new(MockAuditService) | ||
| mockTenant := new(MockTenantService) | ||
| svc := services.NewAuthService(userRepo, mockIdentity, mockAudit, mockTenant) |
There was a problem hiding this comment.
The PR description states this is migrating from mock-based unit tests to tests using real database dependencies. However, the TestAuthService_ErrorScenarios test still uses mocks for IdentityService, AuditService, and TenantService. While this mixed approach is valid for testing specific error scenarios, consider documenting why mocks are still needed for these specific cases or whether these scenarios could also use real services for consistency.
| t.Run("Login_IdentityFailure", func(t *testing.T) { | ||
| email := "idfail_" + uuid.NewString() + "@test.com" | ||
| user := &domain.User{ID: uuid.New(), Email: email} | ||
| hash, _ := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) |
There was a problem hiding this comment.
The error from bcrypt.GenerateFromPassword is being ignored with _. If password hashing fails, the test will insert a user with an invalid password hash, leading to confusing test failures. Consider using require.NoError to ensure password hashing succeeds in the test setup.
| hash, _ := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | |
| hash, err := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | |
| require.NoError(t, err) |
| assert.Contains(t, err.Error(), "failed to create personal tenant") | ||
|
|
||
| // Verify user was rolled back (deleted) | ||
| user, _ := userRepo.GetByEmail(ctx, email) |
There was a problem hiding this comment.
The error from userRepo.GetByEmail is being ignored with _. If this call fails for reasons other than "user not found" (e.g., database connection error), the test could incorrectly pass when it should fail. Consider checking the error to distinguish between "user not found" (expected) and actual errors (unexpected), or use require.NoError if nil is expected after successful rollback.
| user, _ := userRepo.GetByEmail(ctx, email) | |
| user, err := userRepo.GetByEmail(ctx, email) | |
| require.NoError(t, err) |
| apiKey, err := identitySvc.CreateKey(ctx, user.ID, "session") | ||
| require.NoError(t, err) | ||
| func TestAuthService_ErrorScenarios(t *testing.T) { | ||
| db := setupDB(t) |
There was a problem hiding this comment.
The TestAuthService_ErrorScenarios test doesn't call cleanDB(t, db) before running subtests. This could lead to test pollution if the database has leftover data from previous tests, potentially causing flaky tests. Consider adding cleanDB(t, db) after line 125 to ensure a clean slate for the error scenarios tests.
| db := setupDB(t) | |
| db := setupDB(t) | |
| cleanDB(t, db) |
| assert.NotEmpty(t, token) | ||
| assert.Equal(t, email, user.Email) | ||
| email := "login_" + uuid.NewString() + "@example.com" | ||
| _, _ = svc.Register(ctx, email, testPassword, "Login User") |
There was a problem hiding this comment.
The error from svc.Register is being ignored with _. If registration fails, this test should fail since the rest of the test assumes a user exists for login testing. Consider using require.NoError to ensure the test setup is valid.
| _, _ = svc.Register(ctx, email, testPassword, "Login User") | |
| _, err := svc.Register(ctx, email, testPassword, "Login User") | |
| require.NoError(t, err) |
| _, svc, _, _ := setupAuthServiceTest(t) | ||
| ctx := context.Background() | ||
| email := "locked_" + uuid.NewString() + "@test.com" | ||
| _, _ = svc.Register(ctx, email, testPassword, "Locked User") |
There was a problem hiding this comment.
The error from svc.Register is being ignored with _. If registration fails, the lockout test will not function correctly since it assumes a user exists. Consider using require.NoError to ensure the test setup is valid.
| _ = userRepo.Create(ctx, user) | ||
|
|
||
| mockIdentity.On("CreateKey", mock.Anything, user.ID, "Default Key"). | ||
| Return(nil, errors.New("key failed")).Once() | ||
|
|
||
| _, _, err := svc.Login(ctx, email, testPassword) |
There was a problem hiding this comment.
The error from userRepo.Create is being ignored with _. If user creation fails, the Login_IdentityFailure test will not function correctly since it assumes a user exists in the database for login testing. Consider using require.NoError to ensure the test setup is valid.
| _ = userRepo.Create(ctx, user) | |
| mockIdentity.On("CreateKey", mock.Anything, user.ID, "Default Key"). | |
| Return(nil, errors.New("key failed")).Once() | |
| _, _, err := svc.Login(ctx, email, testPassword) | |
| err := userRepo.Create(ctx, user) | |
| require.NoError(t, err) | |
| mockIdentity.On("CreateKey", mock.Anything, user.ID, "Default Key"). | |
| Return(nil, errors.New("key failed")).Once() | |
| _, _, err = svc.Login(ctx, email, testPassword) |
|
|
||
| t.Run("DuplicateEmail", func(t *testing.T) { | ||
| email := "dup_" + uuid.NewString() + "@test.com" | ||
| _, _ = svc.Register(ctx, email, testPassword, "User") |
There was a problem hiding this comment.
The first svc.Register call in the DuplicateEmail test is ignoring both the returned user and error with _. If the first registration fails, the test will not behave as expected. Consider using require.NoError to ensure the first registration succeeds before testing the duplicate scenario.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/core/services/auth_test.go (2)
40-69:⚠️ Potential issue | 🟠 MajorTests are not table-driven.
TestAuthServiceRegister(and the other test functions below) uset.Runfor subtests but don't follow the table-driven pattern ([]struct{ name, input, expected … }driven by afor _, tc := range testsloop). This is required by the project's test standards.♻️ Example table-driven shape for RegisterTests
-func TestAuthServiceRegister(t *testing.T) { - _, svc, userRepo, _ := setupAuthServiceTest(t) - ctx := context.Background() - - t.Run("Success", func(t *testing.T) { … }) - t.Run("DuplicateEmail", func(t *testing.T) { … }) - t.Run("WeakPassword", func(t *testing.T) { … }) -} +func TestAuthServiceRegister(t *testing.T) { + tests := []struct { + name string + email func() string + password string + wantErr bool + errContains string + }{ + {name: "Success", email: func() string { return "reg_" + uuid.NewString() + "@example.com" }, password: testPassword}, + {name: "DuplicateEmail", email: func() string { return "dup_" + uuid.NewString() + "@test.com" }, password: testPassword, wantErr: true, errContains: "already exists"}, + {name: "WeakPassword", email: func() string { return "weak_" + uuid.NewString() + "@test.com" }, password: "123", wantErr: true, errContains: "too weak"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { … }) + } +}As per coding guidelines: "Use table-driven tests in test files" (
**/*_test.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 40 - 69, Convert TestAuthServiceRegister to a table-driven test: create a []struct slice with fields like name, email, password, displayName, expectError, expectErrContains, and any pre-test setup flags, then loop over it with for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { ... }) }; inside each case call svc.Register(ctx, tc.email, tc.password, tc.displayName) and assert based on tc.expectError (for success also verify persistence via userRepo.GetByEmail(ctx, tc.email) and compare IDs); ensure existing "DuplicateEmail" case is implemented by seeding the user before running that test case and reuse symbols TestAuthServiceRegister, svc.Register, userRepo.GetByEmail and testPassword to locate the code to change.
22-38:⚠️ Potential issue | 🟠 MajorEntire test suite violates the "no real external dependencies in unit tests" guideline.
Every test wired through
setupAuthServiceTestcreates real pgx pools, real repositories, and real service implementations. The PR title frames this as intentional, but it directly contradicts the project's stated standards.All service-layer tests must use
testify/mockobjects for the repository layer rather than live Postgres connections. TheTestAuthService_ErrorScenariosblock (lines 124–161) is the only test that partially does this, but it still uses a realpostgres.UserRepo.♻️ Sketch of the correct pattern for the setup helper
-func setupAuthServiceTest(t *testing.T) (*pgxpool.Pool, *services.AuthService, *postgres.UserRepo, *services.IdentityService) { - t.Helper() - db := setupDB(t) - cleanDB(t, db) - - userRepo := postgres.NewUserRepo(db) - auditRepo := postgres.NewAuditRepository(db) - identityRepo := postgres.NewIdentityRepository(db) - tenantRepo := postgres.NewTenantRepo(db) - - auditSvc := services.NewAuditService(auditRepo) - identitySvc := services.NewIdentityService(identityRepo, auditSvc, slog.Default()) - tenantSvc := services.NewTenantService(tenantRepo, userRepo, slog.Default()) - svc := services.NewAuthService(userRepo, identitySvc, auditSvc, tenantSvc) - - return db, svc, userRepo, identitySvc -} +func setupAuthServiceTest(t *testing.T) (*services.AuthService, *MockUserRepo, *MockIdentityService, *MockAuditService, *MockTenantService) { + t.Helper() + mockUserRepo := new(MockUserRepo) + mockIdentity := new(MockIdentityService) + mockAudit := new(MockAuditService) + mockTenant := new(MockTenantService) + svc := services.NewAuthService(mockUserRepo, mockIdentity, mockAudit, mockTenant) + return svc, mockUserRepo, mockIdentity, mockAudit, mockTenant +}As per coding guidelines: "Do not test with real external dependencies in unit tests - use mocks instead" (
**/*_test.go) and "Mock repositories in service tests" (internal/core/services/**/*_test.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 22 - 38, The test helper setupAuthServiceTest currently creates real DB connections and concrete repo implementations (postgres.NewUserRepo, postgres.NewAuditRepository, postgres.NewIdentityRepository, postgres.NewTenantRepo) and wires real services into services.NewAuthService; change it to use testify/mock-based repository mocks instead: replace the postgres.* repo constructions with mock implementations (or testify/mock-generated mocks) and inject those mock repo interfaces into AuditService/IdentityService/TenantService/AuthService in the setup helper, and update TestAuthService_ErrorScenarios to use the mock UserRepo rather than postgres.UserRepo so all service tests avoid real pgx pools and real DB dependencies.
♻️ Duplicate comments (1)
internal/core/services/auth_test.go (1)
163-254: Real-dependency violations inherited fromsetupAuthServiceTest.
TestAuthServiceRevokeToken,TestAuthServiceRotateToken,TestAuthServiceLogout, andTestAuthServiceTokenRotationIntegrationall inherit the real-DB setup. The root cause is already flagged above atsetupAuthServiceTest. No additional distinct issues in this block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 163 - 254, These tests (TestAuthServiceRevokeToken, TestAuthServiceRotateToken, TestAuthServiceLogout, TestAuthServiceTokenRotationIntegration) are unintentionally using the real DB because they call setupAuthServiceTest; update the tests to use a test-friendly setup that uses an in-memory or mocked datastore (e.g., add a new helper setupAuthServiceTestWithMock or add an option to setupAuthServiceTest to return a mock DB), then update each test to call the mock setup (replace setupAuthServiceTest(...) with the mock variant) so the IdentityService and AuthService operate against the fake DB; ensure the new helper returns the same tuple types (db, svc, ..., identitySvc) or adjust the tests to use the returned mock types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/services/auth_test.go`:
- Around line 56-62: The DuplicateEmail subtest currently discards the error
from the first call to svc.Register which can mask setup failures; change the
setup to assert the first registration succeeded (use require.NoError on the
first svc.Register call and capture its result) so any DB or setup error
surfaces immediately before calling svc.Register a second time and asserting the
duplicate error.
- Around line 114-121: In TestAuthServiceValidateUser, don't ignore the error
returned by svc.Register: capture the error (e.g., user, err :=
svc.Register(...)) and assert/require no error before using user.ID so the test
fails fast instead of panicking; update the test to call require.NoError(t, err)
immediately after svc.Register and only then call svc.ValidateUser and access
user.ID.
- Around line 124-132: TestAuthService_ErrorScenarios opens its own DB with
setupDB(t) but never calls cleanDB, so test data from subtests like
Register_TenantFailure_RollbackSuccess and Login_IdentityFailure can leak; fix
by ensuring DB isolation—either replace the manual setup with
setupAuthServiceTest (which calls cleanDB) or call cleanDB(db) (or defer
cleanDB(db)) around setupDB(t) so the DB is cleaned before/after the test and
rows do not persist across tests.
- Around line 75-76: The tests TestAuthServiceLogin and TestAuthServiceLockout
silently ignore svc.Register results; instead capture the returned user and
error from svc.Register (e.g., user, err := svc.Register(...)) and assert/handle
the error immediately (use require.NoError(t, err) or t.Fatalf on err) before
proceeding; do this for both occurrences (the Register call in
TestAuthServiceLogin and the similar call in TestAuthServiceLockout) so setup
failures fail the test rather than continuing against a non-existent user.
- Around line 142-144: The test currently discards the error from
userRepo.GetByEmail which can hide DB errors; change the call in the rollback
verification to capture the error (user, err := userRepo.GetByEmail(ctx,
email)), then assert the error is the expected "not found" condition (or assert
err == nil if your repo returns nil, nil for missing users) and assert user is
nil; update the assertions around userRepo.GetByEmail and assert.Nil to check
err explicitly so the test fails on DB/query errors rather than silently
passing.
- Around line 150-152: The test silently ignores errors from userRepo.Create
which can mask failures (e.g., zero UUID causing DB constraint violations) and
lead svc.Login to fail for the wrong reason; update the test to check and
require no error from userRepo.Create (fail the test immediately if err != nil)
and ensure the created user has a non-zero TenantID (assign a valid UUID to
user.TenantID before calling userRepo.Create) so the Login_IdentityFailure
subtest exercises the intended identity failure path.
---
Outside diff comments:
In `@internal/core/services/auth_test.go`:
- Around line 40-69: Convert TestAuthServiceRegister to a table-driven test:
create a []struct slice with fields like name, email, password, displayName,
expectError, expectErrContains, and any pre-test setup flags, then loop over it
with for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { ... }) };
inside each case call svc.Register(ctx, tc.email, tc.password, tc.displayName)
and assert based on tc.expectError (for success also verify persistence via
userRepo.GetByEmail(ctx, tc.email) and compare IDs); ensure existing
"DuplicateEmail" case is implemented by seeding the user before running that
test case and reuse symbols TestAuthServiceRegister, svc.Register,
userRepo.GetByEmail and testPassword to locate the code to change.
- Around line 22-38: The test helper setupAuthServiceTest currently creates real
DB connections and concrete repo implementations (postgres.NewUserRepo,
postgres.NewAuditRepository, postgres.NewIdentityRepository,
postgres.NewTenantRepo) and wires real services into services.NewAuthService;
change it to use testify/mock-based repository mocks instead: replace the
postgres.* repo constructions with mock implementations (or
testify/mock-generated mocks) and inject those mock repo interfaces into
AuditService/IdentityService/TenantService/AuthService in the setup helper, and
update TestAuthService_ErrorScenarios to use the mock UserRepo rather than
postgres.UserRepo so all service tests avoid real pgx pools and real DB
dependencies.
---
Duplicate comments:
In `@internal/core/services/auth_test.go`:
- Around line 163-254: These tests (TestAuthServiceRevokeToken,
TestAuthServiceRotateToken, TestAuthServiceLogout,
TestAuthServiceTokenRotationIntegration) are unintentionally using the real DB
because they call setupAuthServiceTest; update the tests to use a test-friendly
setup that uses an in-memory or mocked datastore (e.g., add a new helper
setupAuthServiceTestWithMock or add an option to setupAuthServiceTest to return
a mock DB), then update each test to call the mock setup (replace
setupAuthServiceTest(...) with the mock variant) so the IdentityService and
AuthService operate against the fake DB; ensure the new helper returns the same
tuple types (db, svc, ..., identitySvc) or adjust the tests to use the returned
mock types.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/core/services/auth_internal_test.gointernal/core/services/auth_test.go
💤 Files with no reviewable changes (1)
- internal/core/services/auth_internal_test.go
| t.Run("DuplicateEmail", func(t *testing.T) { | ||
| email := "dup_" + uuid.NewString() + "@test.com" | ||
| _, _ = svc.Register(ctx, email, testPassword, "User") | ||
| _, err := svc.Register(ctx, email, testPassword, "User") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "already exists") | ||
| }) |
There was a problem hiding this comment.
Silent failure masks a false-positive in the DuplicateEmail subtest.
If the first Register call silently fails (e.g., DB error), the second call won't be a duplicate and the test will wrongly pass. Use require.NoError so the setup failure is surfaced immediately.
- _, _ = svc.Register(ctx, email, testPassword, "User")
+ _, err := svc.Register(ctx, email, testPassword, "User")
+ require.NoError(t, err)As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()" (**/*.go).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.Run("DuplicateEmail", func(t *testing.T) { | |
| email := "dup_" + uuid.NewString() + "@test.com" | |
| _, _ = svc.Register(ctx, email, testPassword, "User") | |
| _, err := svc.Register(ctx, email, testPassword, "User") | |
| require.Error(t, err) | |
| assert.Contains(t, err.Error(), "already exists") | |
| }) | |
| t.Run("DuplicateEmail", func(t *testing.T) { | |
| email := "dup_" + uuid.NewString() + "@test.com" | |
| _, err := svc.Register(ctx, email, testPassword, "User") | |
| require.NoError(t, err) | |
| _, err := svc.Register(ctx, email, testPassword, "User") | |
| require.Error(t, err) | |
| assert.Contains(t, err.Error(), "already exists") | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/auth_test.go` around lines 56 - 62, The DuplicateEmail
subtest currently discards the error from the first call to svc.Register which
can mask setup failures; change the setup to assert the first registration
succeeded (use require.NoError on the first svc.Register call and capture its
result) so any DB or setup error surfaces immediately before calling
svc.Register a second time and asserting the duplicate error.
| // Verify user was rolled back (deleted) | ||
| user, _ := userRepo.GetByEmail(ctx, email) | ||
| assert.Nil(t, user) |
There was a problem hiding this comment.
Partial silent failure on userRepo.GetByEmail in rollback verification.
user, _ discards the error. If GetByEmail returns a DB error rather than nil, nil, assert.Nil(t, user) will still pass (the returned pointer is nil on error) but for the wrong reason — you haven't confirmed the user was deleted vs. the query itself failing.
- user, _ := userRepo.GetByEmail(ctx, email)
+ user, err := userRepo.GetByEmail(ctx, email)
+ require.NoError(t, err)
assert.Nil(t, user)As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()" (**/*.go).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Verify user was rolled back (deleted) | |
| user, _ := userRepo.GetByEmail(ctx, email) | |
| assert.Nil(t, user) | |
| // Verify user was rolled back (deleted) | |
| user, err := userRepo.GetByEmail(ctx, email) | |
| require.NoError(t, err) | |
| assert.Nil(t, user) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/auth_test.go` around lines 142 - 144, The test
currently discards the error from userRepo.GetByEmail which can hide DB errors;
change the call in the rollback verification to capture the error (user, err :=
userRepo.GetByEmail(ctx, email)), then assert the error is the expected "not
found" condition (or assert err == nil if your repo returns nil, nil for missing
users) and assert user is nil; update the assertions around userRepo.GetByEmail
and assert.Nil to check err explicitly so the test fails on DB/query errors
rather than silently passing.
| hash, _ := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | ||
| user.PasswordHash = string(hash) | ||
| _ = userRepo.Create(ctx, user) |
There was a problem hiding this comment.
Silent failure on userRepo.Create hides the root cause for the Login_IdentityFailure subtest.
If Create fails (e.g., because user.TenantID is a zero UUID and violates a NOT NULL constraint), the subsequent svc.Login call will fail because the user doesn't exist, not because of the identity service error — giving a false-positive on err but testing the wrong failure path.
- _ = userRepo.Create(ctx, user)
+ require.NoError(t, userRepo.Create(ctx, user))Additionally, user.TenantID is a zero UUID. Verify the users table allows a zero UUID for that column, or assign a real tenant ID.
As per coding guidelines: "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()" (**/*.go).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hash, _ := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | |
| user.PasswordHash = string(hash) | |
| _ = userRepo.Create(ctx, user) | |
| hash, _ := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | |
| user.PasswordHash = string(hash) | |
| require.NoError(t, userRepo.Create(ctx, user)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/auth_test.go` around lines 150 - 152, The test
silently ignores errors from userRepo.Create which can mask failures (e.g., zero
UUID causing DB constraint violations) and lead svc.Login to fail for the wrong
reason; update the test to check and require no error from userRepo.Create (fail
the test immediately if err != nil) and ensure the created user has a non-zero
TenantID (assign a valid UUID to user.TenantID before calling userRepo.Create)
so the Login_IdentityFailure subtest exercises the intended identity failure
path.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| email := "login_" + uuid.NewString() + "@example.com" | ||
| _, _ = svc.Register(ctx, email, testPassword, "Login User") | ||
|
|
There was a problem hiding this comment.
The setup Register call's error is ignored, which can make the subtests fail or report misleading errors. Use require.NoError for the setup registration so the test fails fast if prerequisites aren't met.
| email := "locked_" + uuid.NewString() + "@test.com" | ||
| _, _ = svc.Register(ctx, email, testPassword, "Locked User") | ||
|
|
There was a problem hiding this comment.
Registration error is ignored here; if it fails, the lockout assertions become unreliable. Use require.NoError on Register since it's a prerequisite for the test.
| apiKey, err := identitySvc.CreateKey(ctx, user.ID, "session") | ||
| require.NoError(t, err) | ||
| func TestAuthService_ErrorScenarios(t *testing.T) { | ||
| db := setupDB(t) |
There was a problem hiding this comment.
This test sets up a DB but doesn't call cleanDB(t, db). Other service tests use cleanDB after setupDB for isolation; without it, data can accumulate across tests when DATABASE_URL points to a shared DB. Consider calling cleanDB(t, db) here too.
| db := setupDB(t) | |
| db := setupDB(t) | |
| defer cleanDB(t, db) |
| hash, _ := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | ||
| user.PasswordHash = string(hash) | ||
| _ = userRepo.Create(ctx, user) |
There was a problem hiding this comment.
bcrypt.GenerateFromPassword and userRepo.Create errors are ignored. If hashing or insert fails, this test may pass/fail for the wrong reason. Use require.NoError for both operations to ensure the failure is specifically due to CreateKey.
| hash, _ := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | |
| user.PasswordHash = string(hash) | |
| _ = userRepo.Create(ctx, user) | |
| hash, err := bcrypt.GenerateFromPassword([]byte(testPassword), bcrypt.DefaultCost) | |
| require.NoError(t, err) | |
| user.PasswordHash = string(hash) | |
| err = userRepo.Create(ctx, user) | |
| require.NoError(t, err) |
| // Wait for expiration | ||
| time.Sleep(150 * time.Millisecond) | ||
|
|
There was a problem hiding this comment.
Using a fixed time.Sleep for lockout expiration is prone to flakes under load/slow CI. Prefer require.Eventually (with a timeout and polling interval) to wait until Login succeeds after the lockout window.
| email := "dup_" + uuid.NewString() + "@test.com" | ||
| _, _ = svc.Register(ctx, email, testPassword, "User") | ||
| _, err := svc.Register(ctx, email, testPassword, "User") |
There was a problem hiding this comment.
In this test, the first Register call's error is ignored. If registration fails (e.g. DB/migrations issue), the second call may fail for the wrong reason and the test becomes flaky. Use require.NoError on the initial Register and then assert the duplicate error on the second call.
| ctx := context.Background() | ||
| user, _ := svc.Register(ctx, "val_"+uuid.NewString()+"@test.com", testPassword, "Val User") | ||
|
|
There was a problem hiding this comment.
Register error is ignored and the returned user is used immediately; if Register returns (nil, err) this will panic on user.ID. Capture the error with require.NoError before using user.
| assert.Contains(t, err.Error(), "failed to create personal tenant") | ||
|
|
||
| // Verify user was rolled back (deleted) | ||
| user, _ := userRepo.GetByEmail(ctx, email) |
There was a problem hiding this comment.
The GetByEmail error is ignored; if it fails, asserting user == nil can hide real DB issues. Use require.NoError for the lookup before asserting the rollback behavior.
| user, _ := userRepo.GetByEmail(ctx, email) | |
| user, err := userRepo.GetByEmail(ctx, email) | |
| require.NoError(t, err) |
| // SetLockoutDuration overrides the default lockout duration. Useful for testing. | ||
| func (s *AuthService) SetLockoutDuration(d time.Duration) { | ||
| s.mu.Lock() | ||
| defer s.mu.Unlock() | ||
| s.lockoutDuration = d | ||
| } |
There was a problem hiding this comment.
SetLockoutDuration accepts non-positive durations, which effectively disables lockout (duration <= 0 makes lockout time <= now). Consider validating d > 0 (or documenting/handling 0 explicitly) to avoid accidental misconfiguration.
poyrazK
force-pushed
the
feature/auth-service-real-unit-tests
branch
from
fa24baa to
5bb5478
Compare
poyrazK
force-pushed
the
feature/auth-service-real-unit-tests
branch
from
5bb5478 to
dcc85b4
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Trigger lockout | ||
| for i := 0; i < 5; i++ { | ||
| _, _, _ = svc.Login(ctx, email3, "wrong") |
There was a problem hiding this comment.
Failed login attempts are silently ignored with blank identifiers in the lockout trigger loop. If any of these Login calls don't fail as expected, the test will continue without detecting the issue. Following the established pattern in this codebase (as seen throughout this file), errors should be checked with require.Error to ensure the test logic is sound.
| _, _, _ = svc.Login(ctx, email3, "wrong") | |
| _, _, err := svc.Login(ctx, email3, "wrong") | |
| require.Error(t, err) |
| func TestAuthService_LockoutLogicReal(t *testing.T) { | ||
| _, svc, _, _ := setupAuthServiceTest(t) | ||
| ctx := context.Background() | ||
| email := "locked_" + uuid.NewString() + "@example.com" | ||
| pass := testPassword | ||
| _, err := svc.Register(ctx, email, pass, "Locked User") | ||
| require.NoError(t, err) | ||
|
|
||
| t.Run("Lockout after 5 attempts", func(t *testing.T) { | ||
| // 5 failed attempts | ||
| for i := 0; i < 5; i++ { | ||
| _, _, err := svc.Login(ctx, email, "wrong-password") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "invalid email or password") | ||
| } | ||
|
|
||
| // 6th attempt should be locked out | ||
| _, _, err = svc.Login(ctx, email, pass) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "account is locked") | ||
| }) | ||
|
|
||
| t.Run("Success clears failures", func(t *testing.T) { | ||
| email2 := "clean_" + uuid.NewString() + "@example.com" | ||
| _, err := svc.Register(ctx, email2, pass, "Clean User") | ||
| require.NoError(t, err) | ||
|
|
||
| // 2 failed attempts | ||
| for i := 0; i < 2; i++ { | ||
| _, _, err := svc.Login(ctx, email2, "wrong") | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| // Success | ||
| _, _, err = svc.Login(ctx, email2, pass) | ||
| require.NoError(t, err) | ||
| }) | ||
|
|
||
| t.Run("Lockout Expiration", func(t *testing.T) { | ||
| email3 := "expire_" + uuid.NewString() + "@example.com" | ||
| _, _ = svc.Register(ctx, email3, pass, "Expire User") | ||
|
|
||
| // Trigger lockout | ||
| for i := 0; i < 5; i++ { | ||
| _, _, _ = svc.Login(ctx, email3, "wrong") | ||
| } | ||
|
|
||
| // Verify locked | ||
| _, _, err := svc.Login(ctx, email3, pass) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "locked") | ||
|
|
||
| // Manually expire lockout via reflection | ||
| v := reflect.ValueOf(svc).Elem() | ||
| lockoutsField := v.FieldByName("lockouts") | ||
| lockoutsField.SetMapIndex(reflect.ValueOf(email3), reflect.ValueOf(time.Now().Add(-1*time.Minute))) | ||
|
|
||
| // Login should now work | ||
| _, _, err = svc.Login(ctx, email3, pass) | ||
| require.NoError(t, err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The AuthService instance is shared across all three sub-tests, creating a test isolation issue. The lockout state (failedAttempts and lockouts maps) is stored in memory in the AuthService, so state from one sub-test can leak into another. Each sub-test should call setupAuthServiceTest to get a fresh service instance, ensuring proper test isolation. This is especially critical for lockout testing where the state directly affects test outcomes.
|
|
||
| func TestAuthService_LockoutLogicReal(t *testing.T) { | ||
| _, svc, _, _ := setupAuthServiceTest(t) | ||
| ctx := context.Background() | ||
| email := "locked_" + uuid.NewString() + "@example.com" | ||
| pass := testPassword | ||
| _, err := svc.Register(ctx, email, pass, "Locked User") | ||
| require.NoError(t, err) | ||
|
|
||
| t.Run("Lockout after 5 attempts", func(t *testing.T) { | ||
| // 5 failed attempts | ||
| for i := 0; i < 5; i++ { | ||
| _, _, err := svc.Login(ctx, email, "wrong-password") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "invalid email or password") | ||
| } | ||
|
|
||
| // 6th attempt should be locked out | ||
| _, _, err = svc.Login(ctx, email, pass) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "account is locked") | ||
| }) | ||
|
|
||
| t.Run("Success clears failures", func(t *testing.T) { | ||
| email2 := "clean_" + uuid.NewString() + "@example.com" | ||
| _, err := svc.Register(ctx, email2, pass, "Clean User") | ||
| require.NoError(t, err) | ||
|
|
||
| // 2 failed attempts | ||
| for i := 0; i < 2; i++ { | ||
| _, _, err := svc.Login(ctx, email2, "wrong") | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| // Success | ||
| _, _, err = svc.Login(ctx, email2, pass) | ||
| require.NoError(t, err) | ||
| }) | ||
|
|
||
| t.Run("Lockout Expiration", func(t *testing.T) { | ||
| email3 := "expire_" + uuid.NewString() + "@example.com" | ||
| _, _ = svc.Register(ctx, email3, pass, "Expire User") | ||
|
|
||
| // Trigger lockout | ||
| for i := 0; i < 5; i++ { | ||
| _, _, _ = svc.Login(ctx, email3, "wrong") | ||
| } | ||
|
|
||
| // Verify locked | ||
| _, _, err := svc.Login(ctx, email3, pass) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "locked") | ||
|
|
||
| // Manually expire lockout via reflection | ||
| v := reflect.ValueOf(svc).Elem() | ||
| lockoutsField := v.FieldByName("lockouts") | ||
| lockoutsField.SetMapIndex(reflect.ValueOf(email3), reflect.ValueOf(time.Now().Add(-1*time.Minute))) | ||
|
|
||
| // Login should now work | ||
| _, _, err = svc.Login(ctx, email3, pass) | ||
| require.NoError(t, err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The new lockout test duplicates functionality already covered by auth_lockout_unit_test.go. The existing unit test file (auth_lockout_unit_test.go) already contains mock-based tests for lockout logic that test the same scenarios: lockout after 5 attempts and success clearing failures. Adding this integration test creates redundancy without adding meaningful coverage beyond what the existing unit test provides, unless the purpose is to replace the mock-based test entirely (but the mock-based test is still present).
| func TestAuthService_LockoutLogicReal(t *testing.T) { | |
| _, svc, _, _ := setupAuthServiceTest(t) | |
| ctx := context.Background() | |
| email := "locked_" + uuid.NewString() + "@example.com" | |
| pass := testPassword | |
| _, err := svc.Register(ctx, email, pass, "Locked User") | |
| require.NoError(t, err) | |
| t.Run("Lockout after 5 attempts", func(t *testing.T) { | |
| // 5 failed attempts | |
| for i := 0; i < 5; i++ { | |
| _, _, err := svc.Login(ctx, email, "wrong-password") | |
| require.Error(t, err) | |
| assert.Contains(t, err.Error(), "invalid email or password") | |
| } | |
| // 6th attempt should be locked out | |
| _, _, err = svc.Login(ctx, email, pass) | |
| require.Error(t, err) | |
| assert.Contains(t, err.Error(), "account is locked") | |
| }) | |
| t.Run("Success clears failures", func(t *testing.T) { | |
| email2 := "clean_" + uuid.NewString() + "@example.com" | |
| _, err := svc.Register(ctx, email2, pass, "Clean User") | |
| require.NoError(t, err) | |
| // 2 failed attempts | |
| for i := 0; i < 2; i++ { | |
| _, _, err := svc.Login(ctx, email2, "wrong") | |
| require.Error(t, err) | |
| } | |
| // Success | |
| _, _, err = svc.Login(ctx, email2, pass) | |
| require.NoError(t, err) | |
| }) | |
| t.Run("Lockout Expiration", func(t *testing.T) { | |
| email3 := "expire_" + uuid.NewString() + "@example.com" | |
| _, _ = svc.Register(ctx, email3, pass, "Expire User") | |
| // Trigger lockout | |
| for i := 0; i < 5; i++ { | |
| _, _, _ = svc.Login(ctx, email3, "wrong") | |
| } | |
| // Verify locked | |
| _, _, err := svc.Login(ctx, email3, pass) | |
| require.Error(t, err) | |
| assert.Contains(t, err.Error(), "locked") | |
| // Manually expire lockout via reflection | |
| v := reflect.ValueOf(svc).Elem() | |
| lockoutsField := v.FieldByName("lockouts") | |
| lockoutsField.SetMapIndex(reflect.ValueOf(email3), reflect.ValueOf(time.Now().Add(-1*time.Minute))) | |
| // Login should now work | |
| _, _, err = svc.Login(ctx, email3, pass) | |
| require.NoError(t, err) | |
| }) | |
| } |
| // Manually expire lockout via reflection | ||
| v := reflect.ValueOf(svc).Elem() | ||
| lockoutsField := v.FieldByName("lockouts") | ||
| lockoutsField.SetMapIndex(reflect.ValueOf(email3), reflect.ValueOf(time.Now().Add(-1*time.Minute))) |
There was a problem hiding this comment.
Using reflection to manipulate private fields (lockouts map) violates encapsulation and makes the test brittle. If the internal implementation of AuthService changes (e.g., field renaming or using a different data structure), this test will break. Additionally, this approach bypasses the actual expiration logic that would be used in production. A better approach would be to either: (1) mock the time function if one exists, (2) use a shorter lockout duration in a test-specific service, or (3) accept that testing lockout expiration in integration tests may require waiting or is better suited to unit tests with mocks.
| _, _ = svc.Register(ctx, email3, pass, "Expire User") | ||
|
|
||
| // Trigger lockout | ||
| for i := 0; i < 5; i++ { | ||
| _, _, _ = svc.Login(ctx, email3, "wrong") | ||
| } |
There was a problem hiding this comment.
Test setup errors are silently ignored with blank identifiers. If Register fails in line 242, the subsequent test logic will not behave as expected. Following the established pattern in this codebase (as seen in lines 79, 98, 114, 132, 158, 182), setup operations should use require.NoError to ensure test validity.
| _, _ = svc.Register(ctx, email3, pass, "Expire User") | |
| // Trigger lockout | |
| for i := 0; i < 5; i++ { | |
| _, _, _ = svc.Login(ctx, email3, "wrong") | |
| } | |
| _, err := svc.Register(ctx, email3, pass, "Expire User") | |
| require.NoError(t, err) | |
| // Trigger lockout | |
| for i := 0; i < 5; i++ { | |
| _, _, _ = svc.Login(ctx, email3, "wrong") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Truncate all tables in one command with CASCADE to handle foreign keys | ||
| truncateQuery := "TRUNCATE " + strings.Join(tables, ", ") + " RESTART IDENTITY CASCADE" |
There was a problem hiding this comment.
cleanDB now uses strings.Join(...) but setup_test.go doesn’t import the strings package, so the test suite won’t compile. Add the missing import (and ensure it’s used only once to satisfy gofmt/goimports).
| ` | ||
| rows, err := db.Query(ctx, query) | ||
| if err != nil { | ||
| t.Logf("Warning: failed to query tables for cleanup: %v", err) | ||
| return | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var tables []string | ||
| for rows.Next() { | ||
| var table string | ||
| if err := rows.Scan(&table); err == nil { | ||
| tables = append(tables, table) | ||
| } |
There was a problem hiding this comment.
cleanDB silently ignores table discovery/cleanup failures (it logs a warning and returns). Since cleanDB is invoked at the start of many DB-backed tests, skipping cleanup can leave stale data and cause flaky, order-dependent failures. Prefer failing the test on query/truncate errors (e.g., require.NoError), and also handle rows.Scan/rows.Err() instead of ignoring them so cleanup is deterministic.
| // Manually expire lockout via reflection | ||
| v := reflect.ValueOf(svc).Elem() | ||
| lockoutsField := v.FieldByName("lockouts") | ||
| lockoutsField.SetMapIndex(reflect.ValueOf(email3), reflect.ValueOf(time.Now().Add(-1*time.Minute))) | ||
|
|
There was a problem hiding this comment.
The lockout-expiration subtest uses reflection to mutate AuthService.lockouts from package services_test. Because lockouts is an unexported field, reflect.Value.FieldByName(...).SetMapIndex will panic (the field isn’t settable from another package). To test expiration, introduce a testable time source (e.g., inject a clock/now func into AuthService) or expose a narrow test helper API so the test can advance time without reflection.
| _, _ = svc.Register(ctx, email3, pass, "Expire User") | ||
|
|
||
| // Trigger lockout | ||
| for i := 0; i < 5; i++ { | ||
| _, _, _ = svc.Login(ctx, email3, "wrong") | ||
| } | ||
|
|
||
| // Verify locked | ||
| _, _, err := svc.Login(ctx, email3, pass) |
There was a problem hiding this comment.
In the lockout-expiration subtest, errors from Register/Login are discarded (_ = / _, _, _ =). If registration fails or the failed-attempt loop doesn’t behave as expected, the test can pass/fail for the wrong reason and mask regressions. Use require.NoError/require.Error for these setup steps so the test only proceeds when the preconditions are met.
| _, _ = svc.Register(ctx, email3, pass, "Expire User") | |
| // Trigger lockout | |
| for i := 0; i < 5; i++ { | |
| _, _, _ = svc.Login(ctx, email3, "wrong") | |
| } | |
| // Verify locked | |
| _, _, err := svc.Login(ctx, email3, pass) | |
| _, err := svc.Register(ctx, email3, pass, "Expire User") | |
| require.NoError(t, err) | |
| // Trigger lockout | |
| for i := 0; i < 5; i++ { | |
| _, _, err = svc.Login(ctx, email3, "wrong") | |
| require.Error(t, err) | |
| } | |
| // Verify locked | |
| _, _, err = svc.Login(ctx, email3, pass) |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/services/auth_test.go (1)
20-36:⚠️ Potential issue | 🟠 MajorAll tests use real Postgres repositories — coding guidelines require mocks for service tests.
The
setupAuthServiceTestfunction wires realpostgres.*repositories instead of mock implementations. The coding guidelines are explicit:
**/*_test.go: "Do not test with real external dependencies in unit tests - use mocks instead"**/*_test.go: "Usetestify/mockfor creating mock objects in tests"internal/core/services/**/*_test.go: "Mock repositories in service tests"The PR description states the intent is to replace mock-based tests with real-DB tests. If the team wants integration tests alongside unit tests, these should live in a separate file (e.g.,
auth_integration_test.go) with a build tag, while mock-based unit tests are retained. Deleting the mock-based tests removes the fast, isolated feedback loop that unit tests provide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 20 - 36, The test helper setupAuthServiceTest currently constructs real Postgres repositories (postgres.NewUserRepo, postgres.NewAuditRepository, postgres.NewIdentityRepository, postgres.NewTenantRepo) so replace those with testify/mock-based mock implementations for unit tests: create mocks for UserRepo, AuditRepository, IdentityRepository, TenantRepo (or use existing mocks) and pass them into services.NewAuthService, services.NewIdentityService, and services.NewTenantService; if you intend to keep real-DB tests, move this real-repo wiring into a separate integration test file (e.g., auth_integration_test.go) with an appropriate build tag so unit tests continue to use the mock-based setup and remain fast and isolated.
🧹 Nitpick comments (4)
internal/core/services/setup_test.go (1)
131-136: Quote table identifiers to avoid breakage with special characters or reserved words.Table names are concatenated directly into the SQL string. While they come from
information_schema, quoting them with double-quotes is standard Postgres hygiene and prevents breakage if a table name ever contains a hyphen, space, or is a reserved keyword.Suggested fix
+ // Quote each table name for safety + quoted := make([]string, len(tables)) + for i, tbl := range tables { + quoted[i] = `"` + strings.ReplaceAll(tbl, `"`, `""`) + `"` + } + // Truncate all tables in one command with CASCADE to handle foreign keys - truncateQuery := "TRUNCATE " + strings.Join(tables, ", ") + " RESTART IDENTITY CASCADE" + truncateQuery := "TRUNCATE " + strings.Join(quoted, ", ") + " RESTART IDENTITY CASCADE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/setup_test.go` around lines 131 - 136, The truncate SQL concatenates table names directly (tables -> truncateQuery) which can break for hyphens/reserved words; update the code that builds truncateQuery so each identifier in tables is wrapped in double quotes and any internal double quotes are escaped (replace " with ""), then join the quoted names with ", " and use that in db.Exec(ctx, truncateQuery); ensure the quoting logic is applied where truncateQuery is constructed so TRUNCATE "table1", "table-2" RESTART IDENTITY CASCADE is produced.internal/core/services/auth.go (1)
40-50: Constructor has 4 dependencies — coding guidelines require a params struct.
NewAuthServiceaccepts 4 positional dependencies. As per coding guidelines, "Use a params struct for service constructors with 3 or more dependencies" (internal/core/services/*.go).Suggested params struct
+// AuthServiceParams holds the dependencies for AuthService. +type AuthServiceParams struct { + UserRepo ports.UserRepository + APIKeySvc ports.IdentityService + AuditSvc ports.AuditService + TenantSvc ports.TenantService +} + -func NewAuthService(userRepo ports.UserRepository, apiKeySvc ports.IdentityService, auditSvc ports.AuditService, tenantSvc ports.TenantService) *AuthService { +func NewAuthService(p AuthServiceParams) *AuthService { return &AuthService{ - userRepo: userRepo, - apiKeySvc: apiKeySvc, - auditSvc: auditSvc, - tenantSvc: tenantSvc, + userRepo: p.UserRepo, + apiKeySvc: p.APIKeySvc, + auditSvc: p.AuditSvc, + tenantSvc: p.TenantSvc, failedAttempts: make(map[string]int), lockouts: make(map[string]time.Time), lockoutDuration: defaultLockout, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth.go` around lines 40 - 50, The constructor NewAuthService currently takes four positional dependencies; create a params struct (e.g., AuthServiceParams) containing fields for UserRepository, IdentityService, AuditService, and TenantService (typed from ports), change NewAuthService to accept a single AuthServiceParams parameter and initialize the AuthService fields from params (preserving failedAttempts, lockouts, lockoutDuration defaults), and update all call sites to pass the new params struct; ensure the struct and constructor names (AuthServiceParams, NewAuthService, AuthService) are used so callers and DI code can be updated consistently.internal/core/services/auth_test.go (2)
38-91: Tests are not table-driven.
TestAuthServiceRegister,TestAuthServiceLogin,TestAuthServiceLoginInvalidCredentials, andTestAuthServiceLoginUserNotFoundeach test a single scenario. The guidelines require table-driven tests. For example, the login variants (valid credentials, invalid password, user not found) are natural candidates for a single table-driven test. As per coding guidelines, "Use table-driven tests in test files" (**/*_test.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 38 - 91, Refactor the standalone tests into table-driven tests: convert TestAuthServiceRegister into a table-driven TestAuthServiceRegister with cases for successful register and any edge cases, and merge TestAuthServiceLogin, TestAuthServiceLoginInvalidCredentials, and TestAuthServiceLoginUserNotFound into a single table-driven TestAuthServiceLogin that iterates cases (valid credentials -> expect user+token, invalid password -> expect error, user not found -> expect error). For each case use setupAuthServiceTest to get svc, call svc.Register where needed, then call svc.Login or svc.Register with inputs from the test case, and assert expectations (presence/absence of token, error vs nil, user email/name) inside t.Run subtests; keep referencing svc.Register, svc.Login and setupAuthServiceTest to locate the logic.
210-216: Magic number5duplicates the lockout threshold.The literal
5on lines 212 and 249 mirrorsmaxFailedAttemptsfromauth.gobut isn't referenced by name. If the constant changes, these tests silently test the wrong boundary. SincemaxFailedAttemptsis unexported and in a different test package (services_test), consider either exporting it or defining a test-local constant.As per coding guidelines, "Do not use magic numbers - use named constants instead" (
**/*.go).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_test.go` around lines 210 - 216, Replace the hard-coded literal "5" used to mirror the lockout threshold in the auth tests with a named test-local constant (e.g., const testMaxFailedAttempts = 5) and use that constant in the t.Run("Lockout after 5 attempts") loop and any other assertions that depend on the lockout threshold; this avoids coupling to the unexported maxFailedAttempts in auth.go while removing the magic number from calls to svc.Login and related assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/services/auth_test.go`:
- Around line 240-264: The "Lockout Expiration" test uses a fragile real-time
sleep (svc.SetLockoutDuration(100 * time.Millisecond) then time.Sleep(150 *
time.Millisecond)) which can flake under CI; change the test to be deterministic
by either (preferred) using the service's clock abstraction if available (make
the test advance the fake clock and call svc.Login/register with that clock) or,
if adding a clock is out of scope, adjust the timings to a safer margin (e.g.,
SetLockoutDuration(1 * time.Millisecond) and Sleep(10 * time.Millisecond) or
increase the sleep to several multiples of the duration) and assert accordingly;
reference svc.SetLockoutDuration, svc.Register, svc.Login and the "Lockout
Expiration" test when making the change.
- Line 6: Remove the unused reflect import in the test file: delete the
"reflect" entry from the import block (or, if reflect was intended to be used,
add the missing usage where tests reference reflection). Ensure no remaining
references to reflect exist so the test package builds cleanly.
- Around line 248-251: The test currently discards svc.Login returns in the
lockout loop; capture the returned error (e.g., _, _, err := svc.Login(ctx,
email3, "wrong")) and assert it is the expected authentication failure (not a
DB/transport error) so the loop actually exercises the auth-failure path and
increments the lockout counter; if you have a specific sentinel like
ErrInvalidCredentials or auth.ErrInvalidCredentials, assert equality to that,
otherwise fail the test on any unexpected error to avoid silent setup failures.
In `@internal/core/services/setup_test.go`:
- Around line 119-125: The rows.Scan error is being silently ignored in the
loop; update the test to handle scan errors instead of discarding them. Inside
the loop that uses rows.Next() and calls rows.Scan(&table), check if err != nil
and fail the test (e.g., use t.Fatalf or t.Fatalf-like helper) with the error
message, otherwise append to tables; this ensures the scan failure in the test
is surfaced. Target the loop using rows.Next(), rows.Scan and the tables slice
in the test function in setup_test.go.
---
Outside diff comments:
In `@internal/core/services/auth_test.go`:
- Around line 20-36: The test helper setupAuthServiceTest currently constructs
real Postgres repositories (postgres.NewUserRepo, postgres.NewAuditRepository,
postgres.NewIdentityRepository, postgres.NewTenantRepo) so replace those with
testify/mock-based mock implementations for unit tests: create mocks for
UserRepo, AuditRepository, IdentityRepository, TenantRepo (or use existing
mocks) and pass them into services.NewAuthService, services.NewIdentityService,
and services.NewTenantService; if you intend to keep real-DB tests, move this
real-repo wiring into a separate integration test file (e.g.,
auth_integration_test.go) with an appropriate build tag so unit tests continue
to use the mock-based setup and remain fast and isolated.
---
Nitpick comments:
In `@internal/core/services/auth_test.go`:
- Around line 38-91: Refactor the standalone tests into table-driven tests:
convert TestAuthServiceRegister into a table-driven TestAuthServiceRegister with
cases for successful register and any edge cases, and merge
TestAuthServiceLogin, TestAuthServiceLoginInvalidCredentials, and
TestAuthServiceLoginUserNotFound into a single table-driven TestAuthServiceLogin
that iterates cases (valid credentials -> expect user+token, invalid password ->
expect error, user not found -> expect error). For each case use
setupAuthServiceTest to get svc, call svc.Register where needed, then call
svc.Login or svc.Register with inputs from the test case, and assert
expectations (presence/absence of token, error vs nil, user email/name) inside
t.Run subtests; keep referencing svc.Register, svc.Login and
setupAuthServiceTest to locate the logic.
- Around line 210-216: Replace the hard-coded literal "5" used to mirror the
lockout threshold in the auth tests with a named test-local constant (e.g.,
const testMaxFailedAttempts = 5) and use that constant in the t.Run("Lockout
after 5 attempts") loop and any other assertions that depend on the lockout
threshold; this avoids coupling to the unexported maxFailedAttempts in auth.go
while removing the magic number from calls to svc.Login and related assertions.
In `@internal/core/services/auth.go`:
- Around line 40-50: The constructor NewAuthService currently takes four
positional dependencies; create a params struct (e.g., AuthServiceParams)
containing fields for UserRepository, IdentityService, AuditService, and
TenantService (typed from ports), change NewAuthService to accept a single
AuthServiceParams parameter and initialize the AuthService fields from params
(preserving failedAttempts, lockouts, lockoutDuration defaults), and update all
call sites to pass the new params struct; ensure the struct and constructor
names (AuthServiceParams, NewAuthService, AuthService) are used so callers and
DI code can be updated consistently.
In `@internal/core/services/setup_test.go`:
- Around line 131-136: The truncate SQL concatenates table names directly
(tables -> truncateQuery) which can break for hyphens/reserved words; update the
code that builds truncateQuery so each identifier in tables is wrapped in double
quotes and any internal double quotes are escaped (replace " with ""), then join
the quoted names with ", " and use that in db.Exec(ctx, truncateQuery); ensure
the quoting logic is applied where truncateQuery is constructed so TRUNCATE
"table1", "table-2" RESTART IDENTITY CASCADE is produced.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/core/services/auth.gointernal/core/services/auth_internal_test.gointernal/core/services/auth_lockout_unit_test.gointernal/core/services/auth_test.gointernal/core/services/auth_unit_test.gointernal/core/services/setup_test.go
💤 Files with no reviewable changes (3)
- internal/core/services/auth_internal_test.go
- internal/core/services/auth_unit_test.go
- internal/core/services/auth_lockout_unit_test.go
| t.Run("Lockout Expiration", func(t *testing.T) { | ||
| email3 := "expire_" + uuid.NewString() + "@example.com" | ||
| _, err := svc.Register(ctx, email3, pass, "Expire User") | ||
| require.NoError(t, err) | ||
|
|
||
| // Set a very short lockout | ||
| svc.SetLockoutDuration(100 * time.Millisecond) | ||
|
|
||
| // Trigger lockout | ||
| for i := 0; i < 5; i++ { | ||
| _, _, _ = svc.Login(ctx, email3, "wrong") | ||
| } | ||
|
|
||
| // Verify locked | ||
| _, _, err = svc.Login(ctx, email3, pass) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "locked") | ||
|
|
||
| // Wait for expiration | ||
| time.Sleep(150 * time.Millisecond) | ||
|
|
||
| // Login should now work | ||
| _, _, err = svc.Login(ctx, email3, pass) | ||
| require.NoError(t, err) | ||
| }) |
There was a problem hiding this comment.
Sleep-based lockout expiration test is inherently flaky.
time.Sleep(150 * time.Millisecond) with a 100ms lockout leaves only a 50ms margin. Under CI load (especially with race detection enabled), this can flake. Consider using a shorter lockout (e.g., 1 * time.Millisecond) with a longer sleep, or better yet, use a clock abstraction that can be advanced deterministically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/auth_test.go` around lines 240 - 264, The "Lockout
Expiration" test uses a fragile real-time sleep (svc.SetLockoutDuration(100 *
time.Millisecond) then time.Sleep(150 * time.Millisecond)) which can flake under
CI; change the test to be deterministic by either (preferred) using the
service's clock abstraction if available (make the test advance the fake clock
and call svc.Login/register with that clock) or, if adding a clock is out of
scope, adjust the timings to a safer margin (e.g., SetLockoutDuration(1 *
time.Millisecond) and Sleep(10 * time.Millisecond) or increase the sleep to
several multiples of the duration) and assert accordingly; reference
svc.SetLockoutDuration, svc.Register, svc.Login and the "Lockout Expiration"
test when making the change.
| // Trigger lockout | ||
| for i := 0; i < 5; i++ { | ||
| _, _, _ = svc.Login(ctx, email3, "wrong") | ||
| } |
There was a problem hiding this comment.
Silent triple-discard masks setup failures in the lockout loop.
Line 250 discards all three return values from svc.Login. If Login fails for an unexpected reason (e.g., DB connectivity), the loop won't actually trigger the lockout counter, and the subsequent assertions will pass or fail for the wrong reason.
for i := 0; i < 5; i++ {
- _, _, _ = svc.Login(ctx, email3, "wrong")
+ _, _, err := svc.Login(ctx, email3, "wrong")
+ require.Error(t, err)
}As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()" (**/*.go).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Trigger lockout | |
| for i := 0; i < 5; i++ { | |
| _, _, _ = svc.Login(ctx, email3, "wrong") | |
| } | |
| // Trigger lockout | |
| for i := 0; i < 5; i++ { | |
| _, _, err := svc.Login(ctx, email3, "wrong") | |
| require.Error(t, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/auth_test.go` around lines 248 - 251, The test
currently discards svc.Login returns in the lockout loop; capture the returned
error (e.g., _, _, err := svc.Login(ctx, email3, "wrong")) and assert it is the
expected authentication failure (not a DB/transport error) so the loop actually
exercises the auth-failure path and increments the lockout counter; if you have
a specific sentinel like ErrInvalidCredentials or auth.ErrInvalidCredentials,
assert equality to that, otherwise fail the test on any unexpected error to
avoid silent setup failures.
| var tables []string | ||
| for rows.Next() { | ||
| var table string | ||
| if err := rows.Scan(&table); err == nil { | ||
| tables = append(tables, table) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Silent scan error violates coding guidelines.
Line 122 silently discards the rows.Scan error. While unlikely to fail here, the guideline is clear.
Suggested fix
for rows.Next() {
var table string
- if err := rows.Scan(&table); err == nil {
- tables = append(tables, table)
- }
+ if err := rows.Scan(&table); err != nil {
+ t.Logf("Warning: failed to scan table name: %v", err)
+ continue
+ }
+ tables = append(tables, table)
}As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()" (**/*.go).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var tables []string | |
| for rows.Next() { | |
| var table string | |
| if err := rows.Scan(&table); err == nil { | |
| tables = append(tables, table) | |
| } | |
| } | |
| var tables []string | |
| for rows.Next() { | |
| var table string | |
| if err := rows.Scan(&table); err != nil { | |
| t.Logf("Warning: failed to scan table name: %v", err) | |
| continue | |
| } | |
| tables = append(tables, table) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/setup_test.go` around lines 119 - 125, The rows.Scan
error is being silently ignored in the loop; update the test to handle scan
errors instead of discarding them. Inside the loop that uses rows.Next() and
calls rows.Scan(&table), check if err != nil and fail the test (e.g., use
t.Fatalf or t.Fatalf-like helper) with the error message, otherwise append to
tables; this ensures the scan failure in the test is surfaced. Target the loop
using rows.Next(), rows.Scan and the tables slice in the test function in
setup_test.go.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Truncate all tables in one command with CASCADE to handle foreign keys | ||
| truncateQuery := "TRUNCATE " + strings.Join(tables, ", ") + " RESTART IDENTITY CASCADE" |
There was a problem hiding this comment.
The table names from information_schema are concatenated directly into a SQL query without proper identifier quoting. While PostgreSQL's information_schema.tables returns safe table names, it's a best practice to quote identifiers to prevent potential SQL injection if table naming conventions change or special characters are introduced. Consider using proper identifier quoting (e.g., wrapping each table name in double quotes) or using pgx's identifier handling.
| // Truncate all tables in one command with CASCADE to handle foreign keys | |
| truncateQuery := "TRUNCATE " + strings.Join(tables, ", ") + " RESTART IDENTITY CASCADE" | |
| // Truncate all tables in one command with CASCADE to handle foreign keys. | |
| // Quote identifiers to avoid issues with special characters or reserved words. | |
| quotedTables := make([]string, 0, len(tables)) | |
| for _, tbl := range tables { | |
| safeName := `"` + strings.ReplaceAll(tbl, `"`, `""`) + `"` | |
| quotedTables = append(quotedTables, safeName) | |
| } | |
| truncateQuery := "TRUNCATE " + strings.Join(quotedTables, ", ") + " RESTART IDENTITY CASCADE" |
| require.NoError(t, err) | ||
|
|
||
| // Set a very short lockout | ||
| svc.SetLockoutDuration(100 * time.Millisecond) |
There was a problem hiding this comment.
Modifying the shared service instance's lockout duration in a subtest could cause issues if subtests are run in parallel in the future. Consider creating a fresh service instance for this subtest or resetting the lockout duration at the end of the subtest to avoid potential test pollution.
This PR replaces mock-based unit tests for AuthService with tests using real database dependencies via testcontainers.
Summary by CodeRabbit
New Features
Tests