test: support server-side unit testing#2485
Merged
fallenbagel merged 5 commits intoseerr-team:developfrom Mar 12, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Node/TypeScript ESM test runner, CI job for unit tests, test DB seeding/reset utilities and hooks, extensive auth integration tests, migrations converting recoveryLinkExpirationDate to datetime, a User filtering tweak, and small auth save-await fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as "GitHub Actions (unit-test)"
participant Container as "Node container"
participant Runner as "server/test/index.mts"
participant App as "Express app under test"
participant DB as "Test DB (in-memory SQLite)"
participant Reporter as "JUnit reporter / artifact"
CI->>Container: checkout, install deps, run `pnpm test`
Container->>Runner: execute test runner
Runner->>DB: initialize data source (in-memory)
DB-->>Runner: initialized
Runner->>App: boot app (routes, sessions)
App->>DB: run queries/mutations for tests
DB-->>App: responses
Runner->>Reporter: emit JUnit report
Reporter-->>CI: upload report artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
michaelhthomas
commented
Feb 17, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
michaelhthomas
force-pushed
the
feat/server-tests
branch
2 times, most recently
from
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.
89cc9f7 to
624bfa4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/routes/auth.test.ts (1)
88-114: Consider using a cleanup pattern for settings mutation.The test correctly restores
userEmailRequiredtofalseat line 113, but if an assertion fails before reaching that line, the setting remains mutated. WhilebeforeEachresets the database, it doesn't reset the in-memory settings object.Consider using Node's test context cleanup or a try/finally pattern for robustness:
♻️ Suggested improvement using test cleanup
it('includes userEmailRequired warning when email is required but invalid', async (t) => { const settings = getSettings(); settings.notifications.agents.email.options.userEmailRequired = true; + t.after(() => { + settings.notifications.agents.email.options.userEmailRequired = false; + }); // Change the user's email to something invalid const userRepo = getRepository(User); ... assert.ok(res.body.warnings.includes('userEmailRequired')); - - settings.notifications.agents.email.options.userEmailRequired = 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 88 - 114, The test "includes userEmailRequired warning when email is required but invalid" mutates the in-memory flag settings.notifications.agents.email.options.userEmailRequired and only restores it at the end, which can leak if assertions fail; wrap the mutation in a try/finally (or register a test cleanup) so that settings.notifications.agents.email.options.userEmailRequired is always reset to its original value even on failure—capture the original value before changing it, set the flag, run the test logic (including login and /auth/me checks), and restore the original value in finally (or use the test framework's after/cleanup hook) to ensure no persistent side effects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/auth.test.ts`:
- Around line 88-114: The test "includes userEmailRequired warning when email is
required but invalid" mutates the in-memory flag
settings.notifications.agents.email.options.userEmailRequired and only restores
it at the end, which can leak if assertions fail; wrap the mutation in a
try/finally (or register a test cleanup) so that
settings.notifications.agents.email.options.userEmailRequired is always reset to
its original value even on failure—capture the original value before changing
it, set the flag, run the test logic (including login and /auth/me checks), and
restore the original value in finally (or use the test framework's after/cleanup
hook) to ensure no persistent side effects.
0f02aaf to
f9cbd97
Compare
michaelhthomas
commented
Feb 18, 2026
michaelhthomas
force-pushed
the
feat/server-tests
branch
from
f9cbd97 to
7ecb018
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/test/index.mts (1)
88-92: Consider awaiting the JUnit report stream completion.The JUnit report stream is piped but not awaited. If the test process exits before the file is fully written, the report could be truncated. This is typically fine since Node waits for streams, but for robustness in CI you might want to track completion.
Optional: Track stream completion
// In CI, write a JUnit report to a file for use by GitHub if (process.env.CI) { const reportStream = createWriteStream(join(BASE_DIR, 'report.xml')); - stream.compose(reporters.junit).pipe(reportStream); + const junitPipeline = stream.compose(reporters.junit).pipe(reportStream); + junitPipeline.on('error', (err) => console.error('JUnit report error:', err)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/test/index.mts` around lines 88 - 92, The JUnit report write is piped but not awaited, which can truncate the file in CI; after creating the reportStream via createWriteStream and piping stream.compose(reporters.junit).pipe(reportStream), await the stream completion by hooking the 'finish' and 'error' events (or use stream.finished/promisify/pipeline) on reportStream so the process waits for successful write before exiting and log or rethrow errors if the stream fails..github/workflows/ci.yml (1)
170-170: Pin this action to a specific SHA for consistency and security.All other actions in the workflow use full SHA pins (e.g.,
actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd). Using@v6allows the tag to be moved, which is a supply chain risk. Updatemikepenz/action-junit-report@v6to use a full SHA pin like the rest of the workflow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 170, The workflow uses the floating tag "mikepenz/action-junit-report@v6" which is a supply-chain risk; update that reference to a full commit SHA so the action is pinned immutably. Locate the job step that references "mikepenz/action-junit-report@v6" and replace the tag with the exact commit SHA from the mikepenz/action-junit-report repository (e.g., "mikepenz/action-junit-report@<FULL_SHA>")—fetch the latest stable commit SHA from that repo/releases and use it in place of "@v6" in the CI workflow.
🤖 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/routes/auth.test.ts`:
- Around line 24-33: The createApp function currently passes a hard-coded
session secret ('test-secret') to session(); replace this by reading a secret
from an environment variable (e.g., process.env.SESSION_SECRET) with a secure
fallback that generates a random secret at runtime (for tests) using a
cryptographically secure generator (e.g., crypto.randomBytes or similar) so
tests don't bake a static secret into source; update the session(...) call in
createApp to use that secret and add the necessary import or test setup that
sets SESSION_SECRET when you want a reproducible value.
---
Duplicate comments:
In `@server/scripts/prepareTestDb.ts`:
- Line 18: The top-level call prepareDb() lacks error handling; wrap the
invocation so any rejection is caught and causes a non-zero exit (e.g., use
prepareDb().catch(err => { console.error(..., err); process.exit(1); }) or an
async IIFE with try/catch), ensuring you log the error and call process.exit(1)
on failure to surface CI failures; target the prepareDb invocation in
prepareTestDb.ts.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 170: The workflow uses the floating tag "mikepenz/action-junit-report@v6"
which is a supply-chain risk; update that reference to a full commit SHA so the
action is pinned immutably. Locate the job step that references
"mikepenz/action-junit-report@v6" and replace the tag with the exact commit SHA
from the mikepenz/action-junit-report repository (e.g.,
"mikepenz/action-junit-report@<FULL_SHA>")—fetch the latest stable commit SHA
from that repo/releases and use it in place of "@v6" in the CI workflow.
In `@server/test/index.mts`:
- Around line 88-92: The JUnit report write is piped but not awaited, which can
truncate the file in CI; after creating the reportStream via createWriteStream
and piping stream.compose(reporters.junit).pipe(reportStream), await the stream
completion by hooking the 'finish' and 'error' events (or use
stream.finished/promisify/pipeline) on reportStream so the process waits for
successful write before exiting and log or rethrow errors if the stream fails.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
michaelhthomas
force-pushed
the
feat/server-tests
branch
from
7ecb018 to
eed3103
Compare
gauthier-th
reviewed
Feb 19, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a node:test-based server-side testing setup (with Supertest) for the Express auth routes, along with test DB utilities and CI wiring. It also includes a couple of auth-related fixes and a small security hardening around user serialization.
Changes:
- Add a custom
node:testrunner (server/test/index.mts) + initial auth endpoint tests using Supertest. - Add reusable test DB seeding/reset utilities and reuse them in the Cypress DB prep script.
- Fix missing
awaiton password reset saves, exclude password from filtered user responses, and change recovery link expiration to datetime/timestamptz via migrations.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
server/utils/seedTestDb.ts |
Adds reusable DB seed/reset helpers for tests. |
server/test/setup.ts |
Silences server logging during tests. |
server/test/index.mts |
Implements the node:test runner CLI and reporting. |
server/test/db.ts |
Provides a helper to seed/reset DB around tests. |
server/scripts/prepareTestDb.ts |
Refactors Cypress DB prep to reuse seedTestDb. |
server/routes/auth.ts |
Fixes missing await when saving reset-password changes. |
server/routes/auth.test.ts |
Adds Supertest coverage for auth endpoints. |
server/migration/sqlite/1771337037917-... |
Migrates recoveryLinkExpirationDate from date to datetime in SQLite. |
server/migration/postgres/1771337333450-... |
Migrates recoveryLinkExpirationDate from date to timestamptz in Postgres. |
server/entity/User.ts |
Filters out password and updates expiration column to DbAwareColumn(datetime). |
server/datasource.ts |
Adds an in-memory sqlite test datasource config behind NODE_ENV=test. |
package.json |
Adds pnpm test entry + dev deps for test runner/editor integration. |
pnpm-lock.yaml |
Locks new dependencies. |
.vscode/settings.json |
Adds VS Code test runner integration settings. |
.vscode/extensions.json |
Recommends an additional VS Code extension for test running. |
.gitignore |
Ignores lcov.info. |
.github/workflows/ci.yml |
Adds a PR-only unit test job publishing a JUnit report. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gauthier-th
reviewed
Feb 23, 2026
server/entity/User.ts
Outdated
| } | ||
|
|
||
| static readonly filteredFields: string[] = ['email', 'plexId']; | ||
| static readonly filteredFields: string[] = ['email', 'plexId', 'password']; |
Member
There was a problem hiding this comment.
@michaelhthomas can you please remove this change? This will be fixed in another PR.
michaelhthomas
force-pushed
the
feat/server-tests
branch
from
eed3103 to
d42ae30
Compare
Collaborator
|
@michaelhthomas could you rebase this PR now from develop? The fix has now been released for the parts that was requested to be removed |
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.
michaelhthomas
force-pushed
the
feat/server-tests
branch
from
d42ae30 to
282ad92
Compare
gauthier-th
approved these changes
Mar 4, 2026
|
Does someone else need to approve this? Sorry, just trying to make sure it does not fall through the cracks |
Collaborator
Why would it fall through the cracks? |
fallenbagel
approved these changes
Mar 11, 2026
seerr
|
||||||||||||||||||||||||||||
| Project |
seerr
|
| Branch Review |
develop
|
| Run status |
|
| Run duration | 02m 13s |
| Commit |
|
| Committer | Michael Thomas |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
31
|
| View all changes introduced in this branch ↗︎ | |
This was referenced Mar 13, 2026
gwlsn
pushed a commit
to gwlsn/seerr
that referenced
this pull request
Mar 21, 2026
7 tasks
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds support for server-side unit testing using
node:testand 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.A GitHub Actions check was added to run tests and report failures on pull requests.
After far too much toil, there is also editor integration and debugging support within VS Code. I personally use Zed and have a separate (much simpler!) configuration for that but not sure what the policy is on adding new editor configuration, so I've omitted that for now.
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