Skip to content

Conversation

@Gaurav1998-dev
Copy link
Collaborator

@Gaurav1998-dev Gaurav1998-dev commented Dec 12, 2025

This PR adds end-to-end testing for the inkeep push command in the CLI.

What's Added

  • A new E2E test suite (push.test.ts) that validates the entire push workflow by:
  • Testing project creation: Verifies projects, agents, sub-agents, tools, and their relationships are correctly saved to the database
  • Testing idempotency: Ensures pushing the same project twice updates rather than duplicates records
  • Validating database constraints: Confirms all foreign key relationships and multi-tenant constraints are properly maintained
  • Proper cleanup: Automatically removes test data after execution

Test Coverage

The test suite validates:
✅ Project metadata (name, description, models configuration)
✅ Agent and sub-agent creation with correct attributes
✅ Tool configuration (MCP server URLs)
✅ Tool-to-sub-agent relationship linking
✅ Foreign key constraint integrity across all tables
✅ Update behavior (no duplicate records on re-push)

Requirements
⚠️ Important: The Management API must be running locally for this test to pass, as it tests against the real database using DATABASE_URL.

@changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2025

⚠️ No Changeset found

Latest commit: 713d054

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
Copy link

vercel bot commented Dec 12, 2025

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

Project Deployment Review Updated (UTC)
agents-docs Ready Ready Preview, Comment Dec 16, 2025 8:18pm
agents-manage-api Ready Ready Preview, Comment Dec 16, 2025 8:18pm
agents-manage-ui Ready Ready Preview, Comment Dec 16, 2025 8:18pm
agents-run-api Ready Ready Preview, Comment Dec 16, 2025 8:18pm

@claude
Copy link

claude bot commented Dec 12, 2025

Claude finished @Gaurav1998-dev's task —— View job


Code Review Complete

I've thoroughly reviewed this E2E testing implementation for the push command. Here are my findings and recommendations:

Summary

This PR adds comprehensive end-to-end testing for the inkeep push command, testing the complete flow from project directory to database persistence. The implementation is generally well-structured but has several areas for improvement.


🔧 Critical Issues

1. Hard-coded Configuration Dependencies

The test relies on hardcoded localhost URLs and a specific database setup without proper environment isolation.

// agents-cli/src/__tests__/e2e/src/projects/inkeep.config.ts:5-10
agentsManageApi: {
  url: 'http://localhost:3002',
},
agentsRunApi: {
  url: 'http://localhost:3003',
},

Recommendation: Use environment variables or mock services to avoid external dependencies:

agentsManageApi: {
  url: process.env.TEST_AGENTS_MANAGE_API_URL || 'http://localhost:3002',
},

2. Real Database Usage in E2E Tests

The test connects to a real database using DATABASE_URL, which could cause issues in CI/CD environments and introduce flakiness.

// Line 29-30: Creates real database connection
db = createDatabaseClient();
await createTestOrganization(db, tenantId);

Recommendation: Consider using the existing createTestDatabaseClient() pattern from @inkeep/agents-core/db/test-client which provides isolated PGlite instances, or ensure DATABASE_URL points to a test-only database.


🚨 Architecture & Design Concerns

3. Inconsistent Test Data Structure

The E2E test project duplicates files already in /test-agents/ directory, creating maintenance overhead.

Files duplicated:

  • inkeep.config.ts (identical content)
  • slack-digest/ project structure (identical content)

Recommendation:

  • Use symlinks or references to the existing /test-agents/ directory
  • Or extract common test fixtures to a shared location
  • This follows DRY principles and reduces maintenance burden

4. Manual Cleanup Logic

The afterAll cleanup has complex manual deletion logic that's brittle and could miss edge cases.

// Lines 42-91: Manual cleanup with multiple delete operations
await db.delete(subAgentToolRelations)...
await db.delete(subAgents)...
await db.delete(agents)...
// etc.

Recommendation: Leverage existing test utilities like cleanupTestDatabase() or implement a cascade delete helper.


⚠️ Code Quality Issues

5. Missing Error Boundaries

Tests lack proper error handling for network failures or database connection issues.

Recommendation: Add try-catch blocks around network-dependent operations and provide meaningful error messages.

6. Hard-coded Test Data

Magic strings like 'slack-digest', 'default' are scattered throughout without constants.

const PROJECT_ID = 'slack-digest';
const TENANT_ID = 'default';

7. Insufficient Test Coverage

Missing edge cases:

  • Invalid project directories
  • Network failures
  • Database constraint violations
  • Malformed configuration files

🔍 Testing Strategy Concerns

8. E2E vs Integration Test Confusion

This test might be better classified as an integration test rather than E2E, as it tests internal API interactions rather than user-facing workflows.

Recommendation: Consider whether this should be in an integration directory and whether true E2E tests (testing the CLI command end-to-end) would be more valuable.

9. Test Reliability

The comment "Important: have your management API running locally for this test to pass" indicates brittle test dependencies.

Recommendation: Mock external API dependencies or use test containers to ensure test isolation and reliability.


Positive Aspects

  1. Comprehensive Database Verification: Excellent coverage of all database entities and relationships
  2. Foreign Key Constraint Testing: Good validation of referential integrity (lines 229-279)
  3. Idempotency Testing: Tests that multiple pushes don't create duplicates
  4. Proper Vitest Configuration: Uses appropriate timeouts and test isolation settings

📝 Additional Recommendations

Security Considerations

  • Ensure test credentials don't leak into version control
  • Consider using dedicated test tenant IDs to prevent cross-contamination

Performance & Scalability

  • Consider running E2E tests in parallel-safe manner
  • Add test timing metrics to identify slow tests

Maintainability

  • Extract test constants to a shared configuration
  • Add JSDoc comments explaining the test scenarios
  • Consider using test builders/factories for complex test data

🎯 Action Items

High Priority:

  1. ✅ Fix hardcoded configuration dependencies
  2. ✅ Resolve test data duplication with /test-agents/
  3. ✅ Improve cleanup logic reliability

Medium Priority:

  1. Add error boundary testing
  2. Extract magic constants
  3. Add more edge case coverage

Low Priority:

  1. Consider reclassifying as integration test
  2. Add JSDoc documentation
  3. Performance monitoring

This is a solid foundation for E2E testing with room for improvement in reliability and maintainability. The comprehensive database verification is particularly valuable for ensuring data integrity.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants