Feat/test improvements

[amikofalvy]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7974331

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
agents-api	Ready	Preview, Comment	Jan 24, 2026 6:15am
agents-docs	Error		Jan 24, 2026 6:15am
agents-manage-ui	Ready	Preview, Comment	Jan 24, 2026 6:15am

[claude]

Claude finished @amikofalvy's task —— View job

---

TypeScript Staff Engineer & System Architect Review

Reviewing this comprehensive test infrastructure overhaul. This PR introduces significant changes to testing strategy, CI/CD, and build processes.

Todo List:

• Analyze test separation strategy (slow vs fast)
• Review CI/CD pipeline modifications
• Examine database setup and snapshot generation
• Evaluate parallel testing implementation
• Check for architectural and design considerations
• Assess potential impacts and risks
• Provide comprehensive feedback and recommendations

Executive Summary

This PR implements a sophisticated testing infrastructure overhaul with three key innovations:

1. Test categorization (fast vs slow) with separate configurations
2. Database snapshot generation for faster test initialization
3. Affected-only testing in CI to reduce build times

While the technical execution is solid, there are several architectural concerns and potential risks that warrant attention before merging.

🔥 Critical Issues

1. Database Snapshot Management & Race Conditions

Location: packages/agents-core/src/db/manage/test-manage-client.ts:34-82

Issue: The snapshot loading strategy has multiple concerning patterns:

if (snapshotPath) {
  const snapshotData = readFileSync(snapshotPath); // Synchronous file I/O
  const blob = new Blob([snapshotData], { type: 'application/gzip' });
  client = new PGlite({ loadDataDir: blob });
} else {
  client = new PGlite();
  // Falls back to migration
}

Problems:

• Race condition risk: Multiple test files loading the same snapshot simultaneously could cause file contention
• Memory overhead: Each test loads the entire compressed snapshot into memory (potentially 10-50MB per test instance)
• Synchronous I/O: Blocking reads in test setup violates Node.js best practices
• Snapshot versioning: No mechanism to detect stale snapshots after schema changes

Recommendation: Implement snapshot versioning with checksums and async loading:

// Add version checking
const snapshotVersion = await getSnapshotVersion(snapshotPath);
const currentSchemaVersion = await getCurrentSchemaVersion();
if (snapshotVersion !== currentSchemaVersion) {
  // Regenerate or fall back to migrations
}

2. Parallel Test Resource Management

Location: agents-api/vitest.fast.config.ts:24-27

Issue: Both fast and slow configs use identical threading parameters:

poolOptions: {
  threads: {
    maxThreads: 10,
    minThreads: 4,
  },
}

Problems:

• Resource contention: Fast tests (5s timeout) and slow tests (60s timeout) competing for the same thread pool
• Memory scaling: 10 threads × database snapshots × test isolation = potential 500MB-1GB memory usage
• No resource bounds: No memory limits or CPU throttling for database operations

Recommendation: Differentiate resource allocation by test type:

// Fast tests - CPU bound, low memory
maxThreads: 8, minThreads: 2

// Slow tests - I/O bound, higher memory
maxThreads: 4, minThreads: 1

⚠️ High-Impact Concerns

3. CI Strategy Complexity vs. Value Trade-off

Location: .github/workflows/ci.yml:81-99

Issue: The affected-only testing strategy adds significant complexity:

if [ "${{ github.event_name }}" == "pull_request" ]; then
  git fetch origin main --depth=1 2>/dev/null || git fetch origin master --depth=1 2>/dev/null || true
  pnpm turbo run lint typecheck test --filter='...[origin/main]'

Concerns:

• Silent failures: The || true could hide legitimate git fetch failures
• Inconsistent behavior: Different execution paths for PRs vs pushes increases CI debugging complexity
• False positives: Turbo's change detection might miss transitive dependencies (e.g., schema changes affecting tests in other packages)

Alternative approach: Consider GitHub's built-in changed files API:

- uses: dorny/paths-filter@v2
  with:
    filters: |
      agents-api: 'agents-api/**'
      agents-core: 'packages/agents-core/**'

4. Test File Naming Convention Inconsistency

Location: 39 renamed files from .test.ts to .slow.test.ts

Issue: The mass renaming creates maintenance overhead without clear categorization criteria.

Missing guidelines:

• What qualifies as a "slow" test? (Database I/O? Duration? Setup complexity?)
• How do developers know which config to use for new tests?
• What's the migration strategy for existing tests that become slow over time?

Recommendation: Document explicit criteria in CONTRIBUTING.md:

## Test Categories
- **Fast tests** (`.test.ts`): < 5s, no database, pure logic
- **Slow tests** (`.slow.test.ts`): Database operations, external APIs, file I/O
- **Integration tests** (`.integration.test.ts`): End-to-end workflows

🧐 Architectural Design Concerns

5. Database Setup Strategy Anti-pattern

Location: agents-api/src/__tests__/setup.ts:127-167

Issue: Complex migration logic in global setup with path resolution heuristics:

const isInPackageDir = process.cwd().includes('agents-api');
const manageMigrationsPath = isInPackageDir
  ? '../packages/agents-core/drizzle/manage'
  : './packages/agents-core/drizzle/manage';

Problems:

• Fragile path resolution: Breaks if directory structure changes or tests run from different contexts
• Hidden dependencies: Migration paths are implicit, making the system harder to debug
• Monorepo coupling: Tight coupling between test setup and repository structure

Better approach: Use explicit configuration or environment variables:

const MANAGE_MIGRATIONS_DIR = process.env.MANAGE_MIGRATIONS_DIR 
  || resolve(__dirname, '../../packages/agents-core/drizzle/manage');

6. Lack of Test Performance Monitoring

Location: CI configuration uploads test timing but no analysis

Missing capabilities:

• No alerting for test performance regressions
• No trending analysis of slow tests over time
• No automatic promotion of fast tests that become slow

Recommendation: Implement performance budgets:

# In CI
- name: Check test performance budget
  run: |
    if [ "$(cat test-results/timing.json | jq '.maxDuration')" -gt "300000" ]; then
      echo "Test duration exceeded 5 minute budget"
      exit 1
    fi

🔍 Implementation Quality Issues

7. Error Handling Gaps

Location: Multiple files lack comprehensive error handling

Examples:

• scripts/generate-test-db-snapshot.ts:42-47: Migration failures only log but don't provide recovery guidance
• packages/agents-core/src/db/manage/test-manage-client.ts:110-111: Database cleanup errors silently ignored

8. Missing Integration Tests for New Infrastructure

Critical gap: No tests validate that:

• Snapshot generation produces valid databases
• Fast/slow test separation works correctly
• Affected-only testing catches real issues

💡 Positive Aspects

The PR demonstrates solid engineering practices:

1. Comprehensive documentation: The CLI parallelization analysis in TEST_PARALLELIZATION_NOTES.md shows thoughtful analysis
2. Incremental adoption: Allowing fallback to migrations when snapshots aren't available
3. CI optimization: Affected-only testing will significantly reduce CI times for focused changes
4. Resource management: Thoughtful thread pool configuration for different test types

🎯 Recommendations

Immediate Actions (Before Merge):

1. Add snapshot versioning to prevent stale snapshot issues
2. Implement resource budgets for memory and CPU usage
3. Add integration tests for the new testing infrastructure
4. Document test categorization criteria for future maintainers

Follow-up Work (Post-merge):

1. Monitor test performance and adjust resource allocation
2. Implement test performance budgets in CI
3. Consider test sharding if the test suite continues growing

Long-term Considerations:

1. Database test strategy evolution: Consider containerized databases vs in-memory for better production parity
2. Test infrastructure as code: Move configuration to a dedicated directory for better maintenance

Summary

This is a well-engineered solution to real performance problems, but the complexity introduced requires careful management. The database snapshot strategy is innovative but needs safeguards against race conditions and stale data. The affected-only CI testing is valuable but adds debugging complexity.

Recommendation: Approve with required changes - address the snapshot versioning and resource management issues before merge, then monitor closely in the first few weeks to ensure the new infrastructure performs as expected.

---