test: support server-side unit testing#2485
test: support server-side unit testing#2485michaelhthomas wants to merge 6 commits intoseerr-team:developfrom
Conversation
📝 WalkthroughWalkthroughAdds a test suite and CI job, Jest TypeScript config, test DB seeding/reset utilities, extensive authentication integration tests, awaits persisted saves in auth routes, and migrates user.recoveryLinkExpirationDate from date to datetime with corresponding Postgres and SQLite migrations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/utils/seedTestDb.ts (1)
65-76: Guard against re-initializing the DataSource.
dataSource.initialize()throws if called twice in the same process (e.g., watch mode or repeated setup). Consider usingisInitializedto make this helper idempotent.♻️ Suggested change
- const dbConnection = await dataSource.initialize(); + const dbConnection = dataSource.isInitialized + ? dataSource + : await dataSource.initialize();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/seedTestDb.ts` around lines 65 - 76, The helper calls dataSource.initialize() unconditionally which throws if the DataSource is already initialized; make seedTestDb idempotent by checking dataSource.isInitialized and only calling dataSource.initialize() when false, otherwise reuse the existing dataSource instance for dbConnection (e.g., let dbConnection = dataSource.isInitialized ? dataSource : await dataSource.initialize()); keep the rest of the logic (dropDatabase, runMigrations/synchronize) the same so operations run against the reused connection.server/routes/auth.test.ts (1)
28-32: Avoid hard‑coded the session secret in tests.The test hardcodes
'test-secret', while the production code inserver/index.tsproperly usessettings.clientId. Consider using a dynamic secret per test run to prevent accidental reuse:🔧 Suggested update
+import { randomBytes } from 'crypto'; import { getRepository } from '@server/datasource'; import { User } from '@server/entity/User'; import { getSettings } from '@server/lib/settings'; import { checkUser } from '@server/middleware/auth'; import { setupTestDb } from '@server/test/db'; import type { Express } from 'express'; import express from 'express'; import session from 'express-session'; import request from 'supertest'; import authRoutes from './auth'; function createApp() { const app = express(); app.use(express.json()); app.use( session({ - secret: 'test-secret', + secret: process.env.TEST_SESSION_SECRET ?? randomBytes(32).toString('hex'), resave: false, saveUninitialized: false, }) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.test.ts` around lines 28 - 32, The test currently hardcodes the session secret in the session({ secret: 'test-secret', ... }) call; replace this static secret with a dynamic/test-safe value: derive the secret from the existing application test settings or environment (e.g., reuse settings.clientId or generate a random string per test run) and inject it into the session configuration used by the tests (locate the session(...) call in auth.test.ts and update the secret parameter to read from the test settings or a helper that produces a per-run secret).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/datasource.ts`:
- Around line 41-46: The test DataSourceOptions object testConfig currently sets
synchronize: true and dropSchema: true which causes initialize() to always
wipe/recreate the schema and prevents seedTestDb({ preserveDb: true }) and
withMigrations flows from working correctly; change testConfig to set
synchronize: false and dropSchema: false and let seedTestDb (and its preserveDb
/ withMigrations logic) control whether the schema is dropped/seeded/migrated,
keeping other options (e.g., logging) unchanged and ensuring initialize() no
longer forcibly resets the DB.
---
Nitpick comments:
In `@server/routes/auth.test.ts`:
- Around line 28-32: The test currently hardcodes the session secret in the
session({ secret: 'test-secret', ... }) call; replace this static secret with a
dynamic/test-safe value: derive the secret from the existing application test
settings or environment (e.g., reuse settings.clientId or generate a random
string per test run) and inject it into the session configuration used by the
tests (locate the session(...) call in auth.test.ts and update the secret
parameter to read from the test settings or a helper that produces a per-run
secret).
In `@server/utils/seedTestDb.ts`:
- Around line 65-76: The helper calls dataSource.initialize() unconditionally
which throws if the DataSource is already initialized; make seedTestDb
idempotent by checking dataSource.isInitialized and only calling
dataSource.initialize() when false, otherwise reuse the existing dataSource
instance for dbConnection (e.g., let dbConnection = dataSource.isInitialized ?
dataSource : await dataSource.initialize()); keep the rest of the logic
(dropDatabase, runMigrations/synchronize) the same so operations run against the
reused connection.
1f4589b to
e27b29c
Compare
TypeORM represents the 'date' column type (date only) as a string, not a JS date object. This was causing the reset password expiration check to never activate, since it compares the date (which is a string) with new Date(), which is always truthy.
e27b29c to
89cc9f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/routes/auth.test.ts (1)
60-70: Consider isolating settings mutations.The
authenticatedAgenthelper mutatessettings.main.localLogindirectly. SincegetSettings()returns a singleton, these mutations persist across tests. While this works when tests run sequentially, it could cause flaky behavior if Jest runs tests in parallel or in unexpected order.Consider wrapping settings mutations in a try/finally or using Jest's
beforeEach/afterEachto ensure cleanup:Example pattern for settings cleanup
// In tests that modify settings: let originalLocalLogin: boolean; beforeEach(() => { const settings = getSettings(); originalLocalLogin = settings.main.localLogin; }); afterEach(() => { const settings = getSettings(); settings.main.localLogin = originalLocalLogin; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/auth.test.ts` around lines 60 - 70, The helper authenticatedAgent currently mutates the singleton returned by getSettings() by setting settings.main.localLogin = true and never restoring it; change this so the original value is preserved and restored (either wrap the mutation in a try/finally inside authenticatedAgent or capture/restore in the test suite using beforeEach/afterEach). Specifically, read and save the original boolean from settings.main.localLogin, set it to true for the authentication attempt, then restore the saved value in a finally block (or restore it in afterEach) to avoid leaking state across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/scripts/prepareTestDb.ts`:
- Line 18: The top-level call to prepareDb() can reject and cause an unhandled
promise rejection; wrap or replace the bare call so rejections are handled: call
prepareDb() and attach a .catch(err => { console.error('prepareDb failed', err);
process.exit(1); }) (or use an async IIFE with try/catch and process.exit(1) on
error) to ensure errors are logged and the process exits non-zero; reference the
prepareDb() invocation in server/scripts/prepareTestDb.ts.
---
Nitpick comments:
In `@server/routes/auth.test.ts`:
- Around line 60-70: The helper authenticatedAgent currently mutates the
singleton returned by getSettings() by setting settings.main.localLogin = true
and never restoring it; change this so the original value is preserved and
restored (either wrap the mutation in a try/finally inside authenticatedAgent or
capture/restore in the test suite using beforeEach/afterEach). Specifically,
read and save the original boolean from settings.main.localLogin, set it to true
for the authentication attempt, then restore the saved value in a finally block
(or restore it in afterEach) to avoid leaking state across tests.
Description
This PR adds support for server-side unit testing using Jest and Supertest (Express testing). It also adds an example test for the auth endpoints, which ended up catching a few issues. Those issues are also resolved here.
Tests for the auth endpoints were generated by Claude Code. All other code is my own.
How Has This Been Tested?
Tests pass!
Screenshots / Logs (if applicable)
Checklist:
pnpm buildpnpm i18n:extractSummary by CodeRabbit
Tests
Bug Fixes