Add PostgreSQL service integration test workflow with database lifecycle management#91
Add PostgreSQL service integration test workflow with database lifecycle management#91Copilot wants to merge 8 commits into
Conversation
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>
Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
|
@copilot ok! |
There was a problem hiding this comment.
Pull request overview
Adds a dedicated integration testing pipeline backed by a PostgreSQL service, including automated schema lifecycle management and test data utilities, to make CI and local integration test runs reproducible.
Changes:
- Added a GitHub Actions workflow that provisions PostgreSQL and runs the integration test suite.
- Added Jest integration
globalSetup/globalTeardownto initialize and clean up database schema for integration runs. - Introduced a
TestDataSeederutility and updated integration tests + docs to standardize test DB usage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/integration-tests.yml |
New CI workflow to run integration tests with a PostgreSQL service container. |
jest.integration.setup.ts |
Global Jest setup for DB validation, connection test, and schema initialization via migrations. |
jest.integration.teardown.ts |
Global Jest teardown for DB cleanup and connection closure. |
jest.integration.config.js |
Wires integration setup/teardown into Jest and enforces serial execution. |
src/database/test-seeder.ts |
Adds utilities for cleaning and seeding test data for integration tests. |
src/__tests__/integration/repository.test.ts |
Ensures isolation by cleaning DB before/after integration tests using the new seeder. |
docs/INTEGRATION_TEST_IMPLEMENTATION_SUMMARY.md |
Summarizes the integration test infrastructure additions and usage. |
docs/INTEGRATION_TESTING.md |
Full guide for local + CI integration testing setup and troubleshooting. |
docs/INTEGRATION_TESTING_QUICKREF.md |
Quick reference for common commands and fixes. |
database/README.md |
Adds an integration testing section pointing to the new guides/utilities. |
README.md |
Updates top-level testing instructions to include integration tests. |
.env.test |
Adds a template environment file for local integration test configuration. |
| // Create database client | ||
| globalDb = new DatabaseClient(); | ||
|
|
||
| // Test database connection | ||
| console.log('Testing database connection...'); | ||
| const isConnected = await globalDb.testConnection(); | ||
|
|
||
| if (!isConnected) { | ||
| throw new Error('Failed to connect to test database. Please ensure PostgreSQL is running.'); | ||
| } | ||
|
|
||
| console.log('✓ Database connection successful'); | ||
|
|
||
| // Run migrations to ensure schema is up to date | ||
| console.log('Running database migrations...'); | ||
| const migrationRunner = new MigrationRunner(globalDb); | ||
|
|
||
| try { | ||
| // First, clean up any existing schema | ||
| try { | ||
| await migrationRunner.down(); | ||
| console.log('✓ Cleaned up existing schema'); | ||
| } catch (error) { | ||
| // Ignore errors if schema doesn't exist | ||
| console.log('No existing schema to clean up'); | ||
| } | ||
|
|
||
| // Apply fresh schema | ||
| await migrationRunner.up(); | ||
| console.log('✓ Database schema initialized'); | ||
| } catch (error) { | ||
| console.error('✗ Migration failed:', error); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
globalDb is created in globalSetup but never closed. Because DatabaseClient owns a pg.Pool, leaving it open can keep Jest from exiting (open handle) and leak connections in CI. Consider making the setup client short-lived (run migrations, then await globalDb.close()), or refactor so the same pool is reused and closed in teardown (e.g., via a shared singleton).
There was a problem hiding this comment.
Already fixed in commit d5464bb. The globalDb connection is now properly closed after migrations (line 81) and in the error path (line 76) to prevent connection leaks and Jest hanging.
| // Teardown grace period to allow async operations to complete | ||
| const TEARDOWN_GRACE_PERIOD_MS = 2000; |
There was a problem hiding this comment.
TEARDOWN_GRACE_PERIOD_MS is hardcoded, but the PR description states this grace period is configurable via TEARDOWN_GRACE_PERIOD_MS. To match the intended behavior, read it from process.env.TEARDOWN_GRACE_PERIOD_MS with a safe numeric default and validation.
| echo "Waiting for PostgreSQL to be ready..." | ||
| for i in {1..30}; do | ||
| if pg_isready -h localhost -p 5432 -U vexel_test_user; then | ||
| echo "PostgreSQL is ready!" | ||
| break | ||
| fi | ||
| echo "Attempt $i: PostgreSQL not ready yet..." | ||
| sleep 2 | ||
| done |
There was a problem hiding this comment.
The "Wait for PostgreSQL" loop doesn't fail the step if the database never becomes ready (it just finishes the loop and continues). Add an explicit failure after the loop (e.g., track readiness and exit 1 if not ready) so the workflow fails fast with a clear message.
| echo "Waiting for PostgreSQL to be ready..." | |
| for i in {1..30}; do | |
| if pg_isready -h localhost -p 5432 -U vexel_test_user; then | |
| echo "PostgreSQL is ready!" | |
| break | |
| fi | |
| echo "Attempt $i: PostgreSQL not ready yet..." | |
| sleep 2 | |
| done | |
| echo "Waiting for PostgreSQL to be ready..." | |
| postgres_ready=false | |
| for i in {1..30}; do | |
| if pg_isready -h localhost -p 5432 -U vexel_test_user; then | |
| echo "PostgreSQL is ready!" | |
| postgres_ready=true | |
| break | |
| fi | |
| echo "Attempt $i: PostgreSQL not ready yet..." | |
| sleep 2 | |
| done | |
| if [ "$postgres_ready" != "true" ]; then | |
| echo "PostgreSQL did not become ready after 30 attempts. Failing the workflow." | |
| exit 1 | |
| fi |
| - name: Initialize database schema | ||
| run: | | ||
| echo "Running database migrations..." | ||
| psql -h localhost -p 5432 -U vexel_test_user -d vexel_test -f database/schema.sql | ||
| echo "Database schema initialized successfully" | ||
|
|
||
| - name: Verify database schema | ||
| run: | | ||
| echo "Verifying database tables..." | ||
| psql -h localhost -p 5432 -U vexel_test_user -d vexel_test -c "\dt" | ||
| psql -h localhost -p 5432 -U vexel_test_user -d vexel_test -c "\dT" | ||
|
|
There was a problem hiding this comment.
This workflow initializes the schema via psql -f database/schema.sql, but jest.integration.setup.ts also performs a down/up cycle using the same schema/rollback scripts. This duplication adds time and increases the chance of drift between CI and local behavior. Consider removing the explicit schema initialization step here and rely on Jest globalSetup for a single source of truth (keeping only a simple connection/health check).
| - name: Initialize database schema | |
| run: | | |
| echo "Running database migrations..." | |
| psql -h localhost -p 5432 -U vexel_test_user -d vexel_test -f database/schema.sql | |
| echo "Database schema initialized successfully" | |
| - name: Verify database schema | |
| run: | | |
| echo "Verifying database tables..." | |
| psql -h localhost -p 5432 -U vexel_test_user -d vexel_test -c "\dt" | |
| psql -h localhost -p 5432 -U vexel_test_user -d vexel_test -c "\dT" |
| await this.db.query('DELETE FROM capability_vectors'); | ||
| await this.db.query('DELETE FROM agent_status_history'); | ||
| await this.db.query('DELETE FROM ipfs_metadata'); | ||
| await this.db.query('DELETE FROM agents'); | ||
| } | ||
|
|
||
| /** | ||
| * Seed test agents | ||
| */ |
There was a problem hiding this comment.
cleanAll() issues multiple DELETE statements every time it runs. Given the FK relationships are ON DELETE CASCADE, you can simplify this and speed up tests by using a single TRUNCATE agents RESTART IDENTITY CASCADE (or TRUNCATE all tables) instead of multiple deletes. This will significantly reduce test runtime as the integration suite grows.
| await this.db.query('DELETE FROM capability_vectors'); | |
| await this.db.query('DELETE FROM agent_status_history'); | |
| await this.db.query('DELETE FROM ipfs_metadata'); | |
| await this.db.query('DELETE FROM agents'); | |
| } | |
| /** | |
| * Seed test agents | |
| */ | |
| // Use TRUNCATE with CASCADE to efficiently clear agents and all dependent tables | |
| await this.db.query('TRUNCATE TABLE agents RESTART IDENTITY CASCADE'); | |
| } | |
| /** | |
| * Seed test agents | |
| */ | |
| * Seed test agents | |
| */ |
| // Generate unique DID with UUID to prevent collisions | ||
| const uniqueId = `${Date.now()}-${Math.random().toString(36).substring(2, 15)}`; | ||
|
|
There was a problem hiding this comment.
Comment says "Generate unique DID with UUID" but the implementation uses Date.now() + Math.random(). Either update the comment to match the actual approach, or switch to a real UUID generator to align with the stated intent.
|
@copilot apply changes based on the comments in this thread |
…mize cleanAll Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
Applied all review feedback in commit d5464bb:
|
Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
Co-authored-by: colleenpridemore <4281084+colleenpridemore@users.noreply.github.com>
|
Superseded by #100 |
Integration Test Workflow with PostgreSQL Service
Implementation Complete ✅
All acceptance criteria met and review feedback addressed.
Latest Refinements
Remove Unnecessary RESTART IDENTITY
RESTART IDENTITYclause is unnecessary since all tables use UUID PKs, not sequencesTRUNCATE TABLE agents CASCADE(removed RESTART IDENTITY)Document Grace Period Validation
Review Feedback Addressed ✅
1. Database Connection Leak Fixed (jest.integration.setup.ts)
globalDbcreated but never closed, causing connection leaks and Jest exit issuesawait globalDb.close()after migrations and in error path2. Configurable Grace Period (jest.integration.teardown.ts)
TEARDOWN_GRACE_PERIOD_MSwas hardcoded despite PR description saying it's configurableprocess.env.TEARDOWN_GRACE_PERIOD_MSwith safe fallback to 2000msTEARDOWN_GRACE_PERIOD_MS=5000 npm run test:integration3. Fail-Fast PostgreSQL Check (.github/workflows/integration-tests.yml)
4. Remove Duplicate Schema Initialization (.github/workflows/integration-tests.yml)
5. Optimize cleanAll() (src/database/test-seeder.ts)
TRUNCATE TABLE agents CASCADE(no RESTART IDENTITY since UUIDs don't use sequences)6. Fix Comment Accuracy (src/database/test-seeder.ts)
Test Results
Files Modified (4 files)
jest.integration.setup.ts- Close connection after migrationsjest.integration.teardown.ts- Configurable grace period with validation and documentation.github/workflows/integration-tests.yml- Fail-fast check, remove duplicate schema initsrc/database/test-seeder.ts- TRUNCATE CASCADE optimization (no RESTART IDENTITY), fix commentReady for Merge ✅
All review feedback addressed, connection leaks fixed, workflow optimized, validation robust, code accurate to schema.
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.