feat: Add integration test infrastructure with PostgreSQL CI#100
feat: Add integration test infrastructure with PostgreSQL CI#100colleenpridemore wants to merge 6 commits into
Conversation
…anches, 45% functions/lines/statements) Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
…5%, 45%, 45%, 45%) Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds PostgreSQL-backed integration test scaffolding and CI workflow support, alongside Jest configuration adjustments and database client cleanup to prevent hanging test runs.
Changes:
- Added integration test infrastructure (Jest integration config + setup/teardown + seeding utilities + example integration tests).
- Updated PostgreSQL
DatabaseClienttyping/cleanup and addedpgdependencies. - Aligned Jest coverage thresholds (and related docs) to current coverage levels.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/integration-tests.yml |
Adds a dedicated GitHub Actions workflow that provisions PostgreSQL and runs integration tests. |
jest.integration.config.js |
Configures Jest integration test discovery and setup/teardown hooks. |
jest.integration.setup.ts |
Adds integration test global setup (currently connection verification). |
jest.integration.teardown.ts |
Adds integration test global teardown (currently logging placeholder). |
src/database/__tests__/integration/database-client.integration.test.ts |
Adds a DB integration test suite for connection, CRUD, and transaction behavior. |
src/database/client.ts |
Fixes pg pool initialization/typing and ensures pool listeners are removed on close. |
src/database/module.test.ts |
Updates unit tests to attempt closing DatabaseClient instances after the suite. |
src/database/test-seeder.ts |
Adds helper utilities for seeding/cleanup of DB test data (TRUNCATE CASCADE). |
src/knowledge-base/module.test.ts |
Updates unit tests to attempt closing DatabaseClient instances after the suite. |
package.json |
Adds pg and @types/pg dependencies and sets Node engine to >=18.18.0. |
package-lock.json |
Locks newly added pg-related dependencies and updates Node engine metadata. |
jest.config.js |
Lowers global Jest coverage thresholds to match current baseline. |
jest.unit.config.js |
Lowers unit-test coverage thresholds and removes forceExit. |
jest.globalTeardown.ts |
Increases Jest global teardown delay to help async cleanup finish. |
CONTRIBUTING.md |
Updates documented coverage expectations to align with enforced thresholds. |
TESTING.md |
Updates documented coverage goals to align with enforced thresholds. |
docs/CI_WORKFLOW.md |
Updates CI documentation to reflect new coverage thresholds and guidance. |
docs/WALLET_SETUP_GUIDE.md |
Updates documented coverage requirements to match current thresholds. |
| let testClient: DatabaseClient | null = null; | ||
|
|
||
| afterAll(async () => { | ||
| // Close any test client that was created | ||
| if (testClient) { | ||
| await testClient.close(); | ||
| testClient = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
testClient only tracks a single DatabaseClient instance, but this file creates multiple clients (e.g., in should have required methods and other tests). Earlier clients can be overwritten and never closed, leaving pools/listeners open and reintroducing Jest open-handle issues. Consider creating one shared client in beforeAll and closing in afterAll, or track all created clients (e.g., array) and close them in afterEach/afterAll.
| describe('Knowledge Base Module', () => { | ||
| let testClient: DatabaseClient | null = null; | ||
|
|
||
| afterAll(async () => { | ||
| // Close any test client that was created | ||
| if (testClient) { | ||
| await testClient.close(); | ||
| testClient = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Like src/database/module.test.ts, this suite creates multiple DatabaseClient instances but only closes the last one assigned to testClient. Earlier instances can be overwritten and never closed, which can leak pg pools and cause Jest open-handle/force-exit issues. Prefer a single shared client (beforeAll/afterAll) or track and close all created clients.
| * Clean all agent-related test data from all relevant tables | ||
| * Deletes from agent_status_history, capability_vectors, ipfs_metadata, and agents | ||
| * Uses TRUNCATE with CASCADE for performance and to honor FK constraints | ||
| */ | ||
| async cleanAll(): Promise<void> { | ||
| await this.db.query('TRUNCATE TABLE agents CASCADE'); |
There was a problem hiding this comment.
cleanAll() uses TRUNCATE ... CASCADE, which will wipe all agents and dependent tables for the whole database. This is fine for a single-threaded integration suite, but will cause cross-test interference if multiple integration test files run concurrently against the same DB. Pair this helper with serialized integration test execution (e.g., Jest maxWorkers: 1) or per-suite DB/schema isolation.
| * Clean all agent-related test data from all relevant tables | |
| * Deletes from agent_status_history, capability_vectors, ipfs_metadata, and agents | |
| * Uses TRUNCATE with CASCADE for performance and to honor FK constraints | |
| */ | |
| async cleanAll(): Promise<void> { | |
| await this.db.query('TRUNCATE TABLE agents CASCADE'); | |
| * Clean all agent-related test data created by this seeder | |
| * Deletes from capability_vectors and agents for test DIDs only | |
| * Uses targeted DELETEs rather than TRUNCATE to avoid cross-test interference | |
| */ | |
| async cleanAll(): Promise<void> { | |
| await this.db.query('BEGIN'); | |
| try { | |
| // Delete dependent capability vectors for agents created by this seeder | |
| await this.db.query( | |
| `DELETE FROM capability_vectors | |
| WHERE agent_id IN ( | |
| SELECT id FROM agents WHERE did LIKE $1 | |
| )`, | |
| ['did:vexel:test:%'] | |
| ); | |
| // Delete agents created by this seeder (identified by DID prefix) | |
| await this.db.query( | |
| 'DELETE FROM agents WHERE did LIKE $1', | |
| ['did:vexel:test:%'] | |
| ); | |
| await this.db.query('COMMIT'); | |
| } catch (err) { | |
| await this.db.query('ROLLBACK'); | |
| throw err; | |
| } |
| - name: Initialize database schema | ||
| run: | | ||
| # Install PostgreSQL client | ||
| sudo apt-get update | ||
| sudo apt-get install -y postgresql-client | ||
|
|
There was a problem hiding this comment.
Schema initialization here duplicates the existing MigrationRunner (src/database/migrate.ts) which already applies database/schema.sql via the app's DB client. Using npm run migrate (or invoking MigrationRunner from jest.integration.setup.ts) would remove the need for apt-get install postgresql-client and keep local/CI schema setup consistent.
| try { | ||
| // Test database connection | ||
| const db = new DatabaseClient(); | ||
| const isConnected = await db.testConnection(); | ||
|
|
||
| if (!isConnected) { | ||
| console.error('❌ Failed to connect to the database'); | ||
| console.error('Required environment variables:'); | ||
| console.error(' - DATABASE_HOST (default: localhost)'); | ||
| console.error(' - DATABASE_PORT (default: 5432)'); | ||
| console.error(' - DATABASE_NAME (default: vexel)'); | ||
| console.error(' - DATABASE_USER (default: vexel_user)'); | ||
| console.error(' - DATABASE_PASSWORD (required)'); | ||
| throw new Error('Database connection failed'); | ||
| } | ||
|
|
||
| console.log('✅ Database connection verified'); | ||
| await db.close(); | ||
|
|
||
| console.log('\n✅ Integration test environment ready\n'); | ||
| } catch (error) { | ||
| console.error('❌ Failed to setup integration test environment:', error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
If testConnection() returns false (or throws before db.close()), the DatabaseClient pool is never closed. In failing CI runs this can leave open handles during Jest globalSetup failure. Consider using a finally block to always await db.close() (guarding for partial init) before rethrowing.
| const client = new DatabaseClient(); | ||
| testClient = client; | ||
| const repo = new AgentRepository(client); | ||
| expect(repo).toBeInstanceOf(AgentRepository); | ||
| }); |
There was a problem hiding this comment.
This assignment overwrites testClient, so any DatabaseClient created in earlier tests may never be closed. If multiple tests instantiate clients, consider creating a single shared DatabaseClient for the whole suite or tracking all created clients and closing them all in afterAll.
| it('should have required methods', () => { | ||
| const db = new DatabaseClient(); | ||
| testClient = db; | ||
| const repo = new AgentRepository(db); | ||
| const extractor = new MemoryExtractor(db, repo); |
There was a problem hiding this comment.
Each test assigns testClient = db, overwriting any previous DatabaseClient without closing it. If multiple tests instantiate clients, earlier pools may leak. Consider creating one shared DatabaseClient for the suite or closing/replacing testClient before reassigning.
| beforeEach(async () => { | ||
| await seeder.cleanAll(); | ||
| }); |
There was a problem hiding this comment.
seeder.cleanAll() truncates shared tables. Since the integration suite already contains other DB-backed integration tests (e.g., src/__tests__/integration/repository.test.ts), running test files in parallel can cause this beforeEach to delete data used by other suites and create flakes. Ensure integration tests run single-threaded (or isolate each suite into its own DB/schema).
| jobs: | ||
| integration-tests: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
There was a problem hiding this comment.
This workflow sets up PostgreSQL, but the existing .github/workflows/ci.yml job still runs npm run test:integration without any Postgres service or schema init. With the new DB-backed integration tests and jest.integration.setup.ts connection check, CI will fail (and/or run integration tests twice) unless the main CI workflow is updated to either provision Postgres too or stop running integration tests there.
Consolidates PRs #91, #93, #94, #95-#99 into a single clean PR.
Changes
Fixes
Closes #91
Closes #93
Closes #94
Closes #95
Closes #96
Closes #97
Closes #98
Closes #99