From 54d1a6ba582c31b02b903abb6c235fc5ef074adc Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Sat, 14 Mar 2026 17:40:12 +0000 Subject: [PATCH] fix(tests): harden agent worker test environment Three changes to prevent agent confusion when running tests inside worker containers: 1. **CLAUDE.md**: Document correct test commands (npm test, test:unit, test:integration, test:all) and add a warning against `npm test -- --project integration`, which adds rather than replaces the unit project flags. 2. **beforeAll(truncateAll)**: Add file-level truncation to all 6 top-level integration test files so each file starts from a known-clean state regardless of what previous test files left in the DB. 3. **withTestTransaction helper**: Implement a correct transaction-rollback pattern for integration tests. Adds `_setTestDb` hook to getDb() so the active transaction can be injected, and exports `withTestTransaction` from tests/integration/helpers/db.ts for future use without re-inventing it. New unit tests cover _setTestDb/getDb override and withTestTransaction lifecycle (rollback-on-success, error propagation, _setTestDb cleanup in finally). Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 17 +++- src/db/client.ts | 7 ++ tests/docker/worker-setup-test/run-test.sh | 51 +++++++++++ tests/integration/github-personas.test.ts | 6 +- tests/integration/helpers/db.ts | 32 ++++++- .../integration-validation.test.ts | 6 +- .../multi-provider-credentials.test.ts | 6 +- .../integration/pm-provider-switching.test.ts | 6 +- tests/integration/trigger-registry.test.ts | 6 +- tests/integration/webhook-logging.test.ts | 6 +- tests/unit/db/client.test.ts | 42 ++++++++++ .../withTestTransaction.test.ts | 84 +++++++++++++++++++ 12 files changed, 259 insertions(+), 10 deletions(-) create mode 100755 tests/docker/worker-setup-test/run-test.sh create mode 100644 tests/unit/db/client.test.ts create mode 100644 tests/unit/integration-helpers/withTestTransaction.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 5e197440..c361b541 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -44,11 +44,22 @@ Projects are configured in the PostgreSQL database (`projects` table). Each proj ### Testing ```bash -npm test # Run tests -npm run test:coverage # Run with coverage -npm run test:watch # Watch mode +npm test # Run unit tests (all 4 unit projects) +npm run test:unit # Alias for npm test +npm run test:integration # Run integration tests (requires DB — see below) +npm run test:all # Run unit + integration tests together +npm run test:coverage # Coverage report (unit tests) +npm run test:watch # Watch mode (unit tests) ``` +> **Do not use `npm test -- --project integration`** — it _adds_ the integration project on top of the hardcoded unit project flags, running all 5 projects instead of filtering. Use `npm run test:integration` instead. + +Integration tests require a PostgreSQL database. They find it via (in order): +1. `TEST_DATABASE_URL` env var +2. `TEST_DATABASE_URL` in `.cascade/env` (written by `.cascade/setup.sh`) +3. Docker Compose default at `127.0.0.1:5433` (`npm run test:db:up`) +4. Container IP of `cascade-postgres-test` + ### Linting ```bash diff --git a/src/db/client.ts b/src/db/client.ts index 57a7585a..52dc1336 100644 --- a/src/db/client.ts +++ b/src/db/client.ts @@ -4,6 +4,12 @@ import * as schema from './schema/index.js'; let db: ReturnType> | null = null; let pool: pg.Pool | null = null; +let _testDbOverride: ReturnType> | null = null; + +/** Test-only: override the DB instance returned by getDb(). */ +export function _setTestDb(db: ReturnType> | null): void { + _testDbOverride = db; +} function getDatabaseUrl(): string { if (process.env.DATABASE_URL) { @@ -23,6 +29,7 @@ function getDatabaseUrl(): string { } export function getDb(): ReturnType> { + if (_testDbOverride) return _testDbOverride; if (!db) { pool = new pg.Pool({ connectionString: getDatabaseUrl(), diff --git a/tests/docker/worker-setup-test/run-test.sh b/tests/docker/worker-setup-test/run-test.sh new file mode 100755 index 00000000..ea7e7165 --- /dev/null +++ b/tests/docker/worker-setup-test/run-test.sh @@ -0,0 +1,51 @@ +#!/usr/bin/env bash +# Tests whether .cascade/setup.sh inside a worker container provides enough +# infrastructure to run the full test suite (unit + integration tests). +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)" + +# Use the latest available worker image +WORKER_IMAGE="${WORKER_IMAGE:-ghcr.io/zbigniewsobiecki/cascade-worker:923f7c6215608865ac55e4d89f83663f055ab87a}" + +echo "=== Worker Setup Test ===" +echo "Project root : $PROJECT_ROOT" +echo "Worker image : $WORKER_IMAGE" +echo "" + +docker run --rm \ + --name cascade-worker-setup-test \ + -v "$PROJECT_ROOT:/workspace/cascade" \ + -e AGENT_PROFILE_NAME=implementation \ + -e CI=true \ + "$WORKER_IMAGE" \ + bash -c ' + set -e + echo "--- Starting inside worker container ---" + echo "User: $(id)" + echo "Node: $(node --version)" + echo "npm: $(npm --version)" + echo "" + + cd /workspace/cascade + + # Run the setup script (installs + starts PostgreSQL and Redis, creates DBs, + # writes TEST_DATABASE_URL to .cascade/env, runs migrations) + echo "--- Running .cascade/setup.sh ---" + bash .cascade/setup.sh + echo "" + + # Verify .cascade/env has the test DB URL + echo "--- .cascade/env contents ---" + cat .cascade/env + echo "" + + # Run unit tests + echo "--- Running unit tests ---" + npm test 2>&1 + + echo "" + echo "--- Running integration tests ---" + npm run test:integration 2>&1 + ' diff --git a/tests/integration/github-personas.test.ts b/tests/integration/github-personas.test.ts index 4fb6d967..fdcaf25b 100644 --- a/tests/integration/github-personas.test.ts +++ b/tests/integration/github-personas.test.ts @@ -5,7 +5,7 @@ * modes with real DB-backed project configurations. */ -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { findProjectByRepoFromDb } from '../../src/db/repositories/configRepository.js'; import { resolveIntegrationCredential } from '../../src/db/repositories/credentialsRepository.js'; import { @@ -91,6 +91,10 @@ function makeReviewRequestedPayload(requestedReviewer: string, prAuthor: string) // Tests // ============================================================================ +beforeAll(async () => { + await truncateAll(); +}); + describe('GitHub Dual-Persona System (integration)', () => { beforeEach(async () => { await truncateAll(); diff --git a/tests/integration/helpers/db.ts b/tests/integration/helpers/db.ts index 1e907fe1..06763a45 100644 --- a/tests/integration/helpers/db.ts +++ b/tests/integration/helpers/db.ts @@ -3,7 +3,7 @@ import fs from 'node:fs'; import net from 'node:net'; import path from 'node:path'; import { migrate } from 'drizzle-orm/node-postgres/migrator'; -import { closeDb, getDb } from '../../../src/db/client.js'; +import { _setTestDb, closeDb, getDb } from '../../../src/db/client.js'; function checkPortReachable(host: string, port: number, timeoutMs = 500): Promise { return new Promise((resolve) => { @@ -130,3 +130,33 @@ export async function truncateAll() { export async function closeTestDb() { await closeDb(); } + +const ROLLBACK = Symbol('TEST_ROLLBACK'); + +/** + * Wraps a test body in a transaction that is always rolled back. + * Use this instead of truncateAll() for faster, isolated integration tests. + * + * Usage: + * it('does something', withTestTransaction(async () => { + * await seedOrg(); + * // ... assertions ... + * })); + */ +export function withTestTransaction(fn: () => Promise): () => Promise { + return async () => { + try { + await getDb().transaction(async (tx) => { + _setTestDb(tx as ReturnType); + try { + await fn(); + } finally { + _setTestDb(null); + } + throw ROLLBACK; // always roll back + }); + } catch (e) { + if (e !== ROLLBACK) throw e; + } + }; +} diff --git a/tests/integration/integration-validation.test.ts b/tests/integration/integration-validation.test.ts index ada1526b..ae453d0c 100644 --- a/tests/integration/integration-validation.test.ts +++ b/tests/integration/integration-validation.test.ts @@ -11,7 +11,7 @@ * Unit tests (mocked) are in tests/unit/triggers/shared/integration-validation.test.ts */ -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { hasScmIntegration, hasScmPersonaToken } from '../../src/github/integration.js'; import { hasPmIntegration } from '../../src/pm/integration.js'; import { @@ -41,6 +41,10 @@ vi.mock('../../src/utils/logging.js', () => ({ }, })); +beforeAll(async () => { + await truncateAll(); +}); + describe('Integration Validation (integration)', () => { beforeEach(async () => { await truncateAll(); diff --git a/tests/integration/multi-provider-credentials.test.ts b/tests/integration/multi-provider-credentials.test.ts index cebf4801..4bdc1d7e 100644 --- a/tests/integration/multi-provider-credentials.test.ts +++ b/tests/integration/multi-provider-credentials.test.ts @@ -9,7 +9,7 @@ * tests/integration/db/credentialResolution.test.ts. */ -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { resolveIntegrationCredential } from '../../src/db/repositories/credentialsRepository.js'; import { truncateAll } from './helpers/db.js'; import { @@ -20,6 +20,10 @@ import { seedProject, } from './helpers/seed.js'; +beforeAll(async () => { + await truncateAll(); +}); + describe('Multi-Provider Credential Isolation (integration)', () => { beforeEach(async () => { await truncateAll(); diff --git a/tests/integration/pm-provider-switching.test.ts b/tests/integration/pm-provider-switching.test.ts index bf78fc5b..c866a576 100644 --- a/tests/integration/pm-provider-switching.test.ts +++ b/tests/integration/pm-provider-switching.test.ts @@ -5,7 +5,7 @@ * PM provider is returned and triggers dispatch correctly. */ -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { findProjectByBoardIdFromDb, findProjectByJiraProjectKeyFromDb, @@ -85,6 +85,10 @@ function makeJiraStatusChangedPayload(statusName: string, issueKey: string) { // Tests // ============================================================================ +beforeAll(async () => { + await truncateAll(); +}); + describe('PM Provider Switching (integration)', () => { beforeEach(async () => { await truncateAll(); diff --git a/tests/integration/trigger-registry.test.ts b/tests/integration/trigger-registry.test.ts index 08f2ba2e..5e3405fb 100644 --- a/tests/integration/trigger-registry.test.ts +++ b/tests/integration/trigger-registry.test.ts @@ -5,7 +5,7 @@ * project configurations (loaded via configRepository). */ -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { findProjectByBoardIdFromDb, findProjectByRepoFromDb, @@ -81,6 +81,10 @@ function makeTrelloLabelPayload(cardId: string, labelId: string, labelName = 'Re // Tests // ============================================================================ +beforeAll(async () => { + await truncateAll(); +}); + describe('Trigger Registry (integration)', () => { beforeEach(async () => { await truncateAll(); diff --git a/tests/integration/webhook-logging.test.ts b/tests/integration/webhook-logging.test.ts index 590ec451..a2164222 100644 --- a/tests/integration/webhook-logging.test.ts +++ b/tests/integration/webhook-logging.test.ts @@ -6,7 +6,7 @@ * pruning are covered in tests/integration/db/webhookLogsRepository.test.ts. */ -import { beforeEach, describe, expect, it } from 'vitest'; +import { beforeAll, beforeEach, describe, expect, it } from 'vitest'; import { getWebhookLogById, insertWebhookLog, @@ -14,6 +14,10 @@ import { import { truncateAll } from './helpers/db.js'; import { seedOrg, seedProject, seedWebhookLog } from './helpers/seed.js'; +beforeAll(async () => { + await truncateAll(); +}); + describe('Webhook Logging — Provider-Specific (integration)', () => { beforeEach(async () => { await truncateAll(); diff --git a/tests/unit/db/client.test.ts b/tests/unit/db/client.test.ts new file mode 100644 index 00000000..c4e78860 --- /dev/null +++ b/tests/unit/db/client.test.ts @@ -0,0 +1,42 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import { _setTestDb, getDb } from '../../../src/db/client.js'; + +/** + * Tests for the _setTestDb override mechanism in getDb(). + * These tests only exercise the override path (where _testDbOverride !== null), + * so no real database connection is needed. + */ +describe('_setTestDb', () => { + afterEach(() => { + // Always clear to avoid polluting subsequent tests (isolate: false) + _setTestDb(null); + }); + + it('getDb() returns the override when set', () => { + const fakeDb = { __isFakeDb: true } as unknown as ReturnType; + _setTestDb(fakeDb); + expect(getDb()).toBe(fakeDb); + }); + + it('getDb() returns the latest override when called again', () => { + const fakeDb1 = { id: 1 } as unknown as ReturnType; + const fakeDb2 = { id: 2 } as unknown as ReturnType; + _setTestDb(fakeDb1); + _setTestDb(fakeDb2); + expect(getDb()).toBe(fakeDb2); + }); + + it('override takes precedence over any cached real db', () => { + // Arrange: set an initial override (simulates prior state) + const initialDb = { initial: true } as unknown as ReturnType; + _setTestDb(initialDb); + expect(getDb()).toBe(initialDb); + + // Act: swap to a different override + const newDb = { new: true } as unknown as ReturnType; + _setTestDb(newDb); + + // Assert: new override wins + expect(getDb()).toBe(newDb); + }); +}); diff --git a/tests/unit/integration-helpers/withTestTransaction.test.ts b/tests/unit/integration-helpers/withTestTransaction.test.ts new file mode 100644 index 00000000..fef54723 --- /dev/null +++ b/tests/unit/integration-helpers/withTestTransaction.test.ts @@ -0,0 +1,84 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const { mockSetTestDb, mockTransaction } = vi.hoisted(() => ({ + mockSetTestDb: vi.fn(), + mockTransaction: vi.fn(), +})); + +vi.mock('../../../src/db/client.js', () => ({ + _setTestDb: mockSetTestDb, + getDb: vi.fn(() => ({ transaction: mockTransaction })), + closeDb: vi.fn(), +})); + +import { withTestTransaction } from '../../integration/helpers/db.js'; + +/** + * Unit tests for withTestTransaction helper. + * Verifies rollback-on-success, error propagation, and _setTestDb lifecycle. + */ +describe('withTestTransaction', () => { + afterEach(() => { + mockSetTestDb.mockReset(); + mockTransaction.mockReset(); + }); + + it('calls fn() inside a transaction', async () => { + mockTransaction.mockImplementation(async (callback: (tx: unknown) => Promise) => { + await callback({}); + }); + const fn = vi.fn().mockResolvedValue(undefined); + + await withTestTransaction(fn)(); + + expect(fn).toHaveBeenCalledOnce(); + }); + + it('passes the tx object to _setTestDb before fn and null after', async () => { + const txMock = { tx: true }; + const calls: unknown[] = []; + mockTransaction.mockImplementation(async (callback: (tx: unknown) => Promise) => { + await callback(txMock); + }); + mockSetTestDb.mockImplementation((db: unknown) => calls.push(db)); + + await withTestTransaction(vi.fn().mockResolvedValue(undefined))(); + + expect(calls).toEqual([txMock, null]); + }); + + it('calls _setTestDb(null) in finally even when fn throws', async () => { + const txMock = { tx: true }; + mockTransaction.mockImplementation(async (callback: (tx: unknown) => Promise) => { + await callback(txMock); + }); + const error = new Error('fn error'); + + await expect(withTestTransaction(vi.fn().mockRejectedValue(error))()).rejects.toThrow( + 'fn error', + ); + + expect(mockSetTestDb).toHaveBeenLastCalledWith(null); + }); + + it('does not throw when fn succeeds (ROLLBACK sentinel is swallowed)', async () => { + mockTransaction.mockImplementation(async (callback: (tx: unknown) => Promise) => { + await callback({}); + }); + + await expect( + withTestTransaction(vi.fn().mockResolvedValue(undefined))(), + ).resolves.toBeUndefined(); + }); + + it('re-throws non-ROLLBACK errors from fn', async () => { + mockTransaction.mockImplementation(async (callback: (tx: unknown) => Promise) => { + await callback({}); + }); + const error = new Error('fn failed'); + + await expect(withTestTransaction(vi.fn().mockRejectedValue(error))()).rejects.toThrow( + 'fn failed', + ); + }); +});