refactor(test): migrate remaining 25 test files to createTestDb helper (#3501)

[Yeraze]

Summary

• Migrates 25 test files from hand-rolled CREATE TABLE DDL to the shared createTestDb() helper, which runs the real migration registry on an in-memory SQLite database
• Schema changes now only require a single edit (the migration) rather than hunting down every test file that builds its own schema
• Completes the work started in refactor(test): shared createTestDb helper from migration registry (section B2, part 1) #3498 and refactor(test): migrate messages repo tests to createTestDb (section B2, part 2) #3500, which covered the messages-related tests

Files changed

All 25 files in src/db/repositories/, src/server/services/, and src/services/ that had hand-rolled SQLite CREATE TABLE DDL.

Schema drift fixes found during migration

The migration uncovered several places where test schemas had drifted from production:

• settings table requires createdAt/updatedAt NOT NULL (from baseline 001)
• traceroutes requires fromNodeId/toNodeId TEXT NOT NULL columns
• packet_log.encrypted is integer NOT NULL (boolean mode)
• ignored_nodes has an FK to sources(id) — tests must seed sources rows first (better-sqlite3 defaults foreign_keys=ON)
• Migration 050 seeds a default source on fresh empty DBs — sources.test.ts now clears it in beforeEach
• push_subscriptions composite unique index (user_id, endpoint, source_id) doesn't fire with NULL user_id in SQLite — uniqueness test now uses a real user id
• user_notification_preferences schema defaults: enable_apprise=1, enable_web_push=1 (not 0)

Test plan

• All 25 migrated files pass in isolation
• All 25 migrated files pass together (500 tests, 0 failures)
• Full Vitest suite passes (6,525 tests, 0 failures)
• npm run typecheck passes clean

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: refactor(test): migrate remaining 25 test files to createTestDb helper

Tasks:

• Read PR description and understand the changes
• Review the createTestDb helper implementation
• Sample key changed test files for review
• Check for potential issues or improvements
• Post final review feedback

---

Overall Assessment

This is a well-executed refactor with a clear, well-motivated goal. The createTestDb() helper (src/server/test-helpers/testDb.ts) is clean and correctly runs the actual migration registry on :memory: SQLite, ensuring tests always use the production schema. The net reduction of ~1,300 lines is excellent — less duplicated DDL means fewer future maintenance burdens.

Positive Highlights

• createTestDb() implementation is solid. The defensive try/catch for getSetting/setSetting during the migration bootstrap is the right approach, and explicitly marking the settingsKey after each guarded migration mirrors the production loop faithfully.
• Schema drift bugs surfaced and fixed. The PR description enumerates seven concrete drifts that were silently masking production behavior (e.g. user_notification_preferences defaults of enable_apprise=1, enable_web_push=1). These were real bugs in the test layer, not just cosmetic.
• FK enforcement respected. ignoredNodes.test.ts now correctly seeds sources rows before inserting ignored nodes (lines 303-308), acknowledging that better-sqlite3 defaults foreign_keys=ON.
• sources.test.ts cleanup of migration-050 seed. Clearing the default source in beforeEach (line 24) is the right choice for tests that need to control exactly which sources exist.

Issues / Observations

1. src/services/database.test.ts — inline SQL still uses raw prepare()/exec() against the test DB

The TestDatabaseService embedded in this file (lines 26–255) is a hand-rolled re-implementation of production methods (upsertNode, insertMessage, getAllNodes, etc.) using raw this.db.prepare() calls. While createTestDb() now provides the schema, the logic under test is a local copy, not the real DatabaseService. This means the tests here are validating the test helper's behavior, not the actual production code.

This is pre-existing and outside the stated scope of the PR (which only targets DDL), but worth flagging so it doesn't get lost.

2. src/server/services/pushNotificationService.test.ts — test quality concern

Several tests in the shouldFilterNotification - Security & Validation describe block (lines 36–76) don't actually exercise PushNotificationService — they test JavaScript built-ins (Number.isInteger, null, undefined). They would pass even if the service file didn't exist. This is also pre-existing, but worth tracking.

3. src/server/services/appriseNotificationService.test.ts — Shared Filtering Logic tests are pure logic assertions

The filtering logic tests (lines 190–278) don't import or call anything from the service; they inline the same logic as plain arithmetic/string operations. As with point 2, these pass vacuously. Not introduced by this PR, but visible now that the schema setup is correct.

4. testDb.test.ts references migration 089, but current count is 84

src/server/test-helpers/testDb.test.ts line 19 reads:

it('includes columns added by recent migrations (e.g. telemetry.rxSnr — migration 089)', () => {

The CLAUDE.md states the current migration count is 84 (084_add_auto_favorite_targets). Either the comment references a future migration or there's a numbering mismatch. The test itself is fine (it checks for column presence, not the migration number), but the comment could mislead contributors.

5. src/db/repositories/ignoredNodes.test.ts — residual void schema at EOF

Line 396:

void schema;

This was apparently added to suppress a "dead import" lint warning on the * as schema import, but after the migration the schema import is only used for the TestBackend type (via BetterSQLite3Database<typeof schema>), which TypeScript should handle without the void escape hatch. Minor, but noisy.

6. src/db/repositories/analysis.test.ts — raw INSERT uses table/column names directly

Lines 22-31 insert directly via sqlite.prepare(...) with raw SQL into telemetry. This is fine and expected for test fixtures, but note that if the telemetry schema gains a NOT NULL column without a default, this test will silently start failing. The createTestDb() helper ensures the schema is real, so at least the failure will be loud and immediately debuggable. Not an issue with this PR specifically.

Minor Nits

• The pattern const t = createTestDb(); sqlite = t.sqlite; drizzleDb = t.db; appears verbatim in ~15 files. A tiny destructuring alias like const { sqlite, db: drizzleDb } = createTestDb() would shorten each setup block, but is purely cosmetic.
• Some files still declare import * as schema from '../schema/index.js' but only use it for the Drizzle type parameter. These imports could be narrowed to import type, but this is a separate linting concern.

Summary

The migration is correct, the helper is well-designed, and the schema drift fixes are a genuine improvement in test reliability. The issues noted (vacuous tests, stale comment, void schema) are pre-existing or minor and do not block merging. This is a clean, safe refactor.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: refactor(test): migrate remaining 25 test files to createTestDb helper

Tasks:

• Read PR description and understand the changes
• Review the createTestDb helper implementation
• Sample key changed test files for review
• Check for potential issues or improvements
• Post final review feedback

---

Overall Assessment

This is a well-executed, high-value refactor. Migrating 25 test files from hand-rolled DDL to the shared createTestDb() helper is a significant improvement in test reliability, and the net ~1,300 line reduction is a genuine maintenance win. The helper itself is cleanly implemented, and several real schema drift bugs were surfaced and fixed in the process.

---

createTestDb() Helper (src/server/test-helpers/testDb.ts)

The implementation is correct and thoughtfully designed:

• Running the full migration registry against :memory: means the test schema tracks production automatically — the problem this PR solves.
• The defensive try/catch around getSetting/setSetting handles the bootstrapping window before migration 001 creates the settings table correctly.
• The explicit if (migration.settingsKey) setSetting(migration.settingsKey, 'completed') after each migration faithfully mirrors the production loop.
• The as any cast is appropriate and well-explained in the comment.

One nit: The close() method just calls sqlite.close(). In-memory SQLite closes are essentially no-ops, but being explicit is fine and harmless.

---

Stale Comment in testDb.test.ts (Line 19)

it('includes columns added by recent migrations (e.g. telemetry.rxSnr — migration 089)', () => {

The CLAUDE.md at the time of the earlier review stated the migration count was 84, which flagged this as incorrect. However, the actual registry in src/db/migrations.ts registers migrations up through 90 — migration 089_add_position_snr_hops.ts exists and adds rxSnr, hopStart, and hopLimit. The comment is accurate. The CLAUDE.md is just stale (it lists 84 as the latest). No issue here.

---

Issues Found

1. src/services/database.test.ts and database.extended.test.ts — inline TestDatabaseService with raw SQL

Both files now use createTestDb() for schema setup, which is good. However, the TestDatabaseService class embedded in each (lines 6–302 in database.test.ts, similar in database.extended.test.ts) is a complete hand-rolled reimplementation of production methods using this.db.prepare(...) raw SQL:

• upsertNode, insertMessage, getAllNodes, insertTelemetry, insertTraceroute, etc. are all re-implemented locally.
• Tests in these files validate this local reimplementation, not the actual DatabaseService in production.

This was pre-existing, but it means schema correctness is now guaranteed (via createTestDb()) while logic correctness is not exercised. If DatabaseService.upsertNode has a bug, these tests won't catch it.

Additionally, database.extended.test.ts:20-22 still contains a bare INSERT OR IGNORE INTO channels with only 6 columns (no sourceId):

this.db.prepare(`
  INSERT OR IGNORE INTO channels (id, name, uplinkEnabled, downlinkEnabled, createdAt, updatedAt)
  VALUES (?, ?, ?, ?, ?, ?)
`).run(0, 'Primary', 1, 1, now, now);

If sourceId is NOT NULL in the production channels schema, this will fail at runtime. Worth checking if the production migration has a default for sourceId on channels.

2. src/server/services/pushNotificationService.test.ts — vacuous tests in security validation block

The shouldFilterNotification - Security & Validation describe block (lines 35–77) doesn't test PushNotificationService at all. Each assertion tests JavaScript builtins:

// Line 52 — tests Number.isInteger, not any service code:
expect(Number.isInteger(userId) && userId > 0).toBe(false);

// Lines 60-66 — tests that null is null:
const userId = null;
expect(userId).toBeNull();

These tests pass unconditionally regardless of whether PushNotificationService exists or compiles. This is pre-existing, not introduced by this PR, but the schema migration makes it more visible now that the setup noise is gone.

3. src/db/repositories/ignoredNodes.test.ts:396 — void schema

void schema;

This suppresses a "dead import" lint warning for import * as schema from '../schema/index.js' which is only used for the TypeScript type BetterSQLite3Database<typeof schema>. The void expression approach is unusual — the cleaner fix would be either import type * as schema or removing the namespace import if only the type is needed. Minor, but it's dead runtime code.

4. src/services/database.unread.test.ts — another embedded TestDatabaseService

Same pattern as point 1: a hand-rolled TestDatabaseService with raw SQL methods now backed by createTestDb() schema. The logic under test is the local implementation, not production. Pre-existing, but worth noting for future cleanup.

---

Positive Highlights

• FK enforcement properly handled. ignoredNodes.test.ts seeds sources rows before inserting ignored nodes (lines 303–308). This is exactly right since better-sqlite3 defaults foreign_keys=ON.
• sources.test.ts migration-050 cleanup. Clearing the default source in beforeEach (line 24) is the correct way to test sources in isolation.
• push_subscriptions uniqueness test (lines 485–508 in pushNotificationService.test.ts) correctly uses a real user_id rather than NULL to exercise the unique constraint — since SQLite doesn't enforce uniqueness when any component is NULL. The comment explains this well.
• analysis.test.ts — clean, well-structured test with proper source scoping on all inserts.
• misc.packetlog.test.ts — the seed data correctly sets encrypted as integer (0), matching the NOT NULL integer column from production schema.

---

Minor Nits

• The repeated setup pattern in ~15 files:

  const t = createTestDb();
  sqlite = t.sqlite;
  drizzleDb = t.db;

  Could be const { sqlite, db: drizzleDb } = createTestDb() but this is cosmetic.
• Several files still use import * as schema from '../schema/index.js' only for a type parameter. import type * as schema would be more correct, but this is a separate linting concern.

---

Summary

The migration is correct and the createTestDb() design is solid. The main concerns are pre-existing (vacuous tests, inline TestDatabaseService reimplementations that bypass production logic) and not introduced by this PR. The one actionable new item is the channels insert in database.extended.test.ts missing sourceId — worth verifying whether that column has a schema default to avoid a silent failure. Everything else is clean.