Skip to content

feat(tests): parallelize E2E tests - #818

Merged
steebchen merged 7 commits into
mainfrom
terragon/parallelize-chat-api-e2e-tests
Sep 15, 2025
Merged

steebchen merged 7 commits into
mainfrom
terragon/parallelize-chat-api-e2e-tests

Conversation

@steebchen

@steebchen steebchen commented Sep 14, 2025

Copy link
Copy Markdown
Member

Summary

  • Introduces parallel execution for chat API end-to-end (E2E) tests using Vitest's thread pool
  • Splits E2E tests into two files: api.e2e.ts for parallelizable .each() tests and api-individual.e2e.ts for isolated individual tests
  • Enables concurrent mode in Vitest config and main test suite for improved test performance
  • Removes redundant tests from api.e2e.ts that are moved to api-individual.e2e.ts
  • Adds detailed setup and validation logic in individual tests to ensure test isolation and reliability
  • Updates documentation to explain the new E2E test structure and parallelization approach

Changes

Test Structure and Execution

  • Created apps/gateway/src/api-individual.e2e.ts with 649 lines of isolated individual test cases
  • Modified apps/gateway/src/api.e2e.ts to run with { concurrent: true } and removed individual tests now in api-individual.e2e.ts
  • Configured vitest/vitest.e2e.config.mts to use a thread pool with up to 10 threads and enabled concurrent mode

Test Implementation

  • Added comprehensive setup in individual tests including database cleanup and provider key insertion
  • Implemented helper functions for request ID generation, log validation, and response validation
  • Covered various scenarios including JSON output mode errors, credits mode completions, prompt token handling, streaming, and multi-provider model requests

Documentation

  • Updated CLAUDE.md to describe the new E2E test structure:
    • Parallel execution with up to 10 threads
    • Split test files for parallel and isolated tests
    • Concurrent mode enabled for .each() tests

Test plan

  • Run E2E tests to verify parallel execution and isolation
  • Confirm individual tests run correctly with proper setup and teardown
  • Validate logs and responses for various test scenarios
  • Ensure documentation accurately reflects the new test structure and usage

This change significantly improves test suite performance and maintainability by leveraging parallelism and clear test separation.

🌿 Generated by Terry


ℹ️ Tag @terragon-labs to ask questions and address PR feedback

📎 Task: https://www.terragonlabs.com/task/c2efea21-eb8c-4682-8cc0-f31122851aa2

Summary by CodeRabbit

  • Documentation

    • Expanded E2E Test Options with a new Test Structure section describing parallel execution, split suites for parallelizable vs. isolated tests, and environment-driven provider key setup.
  • Tests

    • Enabled concurrent E2E execution with a threads pool (min 8, max 16) to speed runs.
    • Split suites to isolate tests needing single-threaded runs while parallelizing the rest.
    • Added a shared setup to seed fixtures and provider keys for cross-provider scenarios.
    • Introduced a streaming test helper for robust SSE/stream validation and adjusted test coverage to focus on core flows.

- Split .each() tests and individual tests into separate files
- Configure Vitest to run up to 10 tests in parallel
- Add concurrent mode for .each() test suites
- Update CLAUDE.md with new test structure documentation

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 14, 2025

Copy link
Copy Markdown
Contributor

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Enables parallel E2E runs (Vitest threads), restructures the main e2e suite to use shared beforeAll seeding and concurrent tests, adds an isolated individual e2e file for tests requiring isolation, adds a streaming-read helper, and updates docs and Vitest config to describe/enable the parallel split.

Changes

Cohort / File(s) Summary
E2E Test Docs
CLAUDE.md
Added "E2E Test Structure" explaining split between parallelizable and isolated tests, notes on Vitest concurrent mode and thread pool (up to 16, min 8), and guidance for DB/test environment operations.
Vitest E2E Config
vitest/vitest.e2e.config.mts
Enabled thread pool execution: pool: "threads", poolOptions.threads.maxThreads = 16, minThreads = 8, and concurrent: true.
Concurrent E2E Suite / Shared Setup
apps/gateway/src/api.e2e.ts
Converted suite to concurrent (describe(..., { concurrent: true })); added beforeAll to clear caches/tables and seed shared fixtures (user/org/project/apiKey) and providerKey rows (env-driven); adjusted beforeEach for lighter isolation; removed many multi-provider/json-mode tests; updated imports to use shared test helpers.
Isolated Individual E2E Tests
apps/gateway/src/api-individual.e2e.ts
Added detailed isolated E2E tests for individual scenarios (error handling, JSON output mode, credits, model-specific behavior, zero prompt tokens) with helpers for unique test data and per-test logging.
Test Helpers (streaming)
apps/gateway/src/test-utils/test-helpers.ts
Added exported readAll helper to consume SSE/streaming ReadableStreams, parse data: lines into chunks, and return aggregated flags (fullContent, eventCount, hasValidSSE, hasOpenAIFormat, hasContent, hasUsage, chunks).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Dev
  participant Vitest as Vitest Runner
  participant Suite as e2e Suite (api.e2e.ts)
  participant DB as Database
  participant ProviderKeys as Provider Keys

  Dev->>Vitest: Run E2E (concurrent, thread pool)
  Vitest->>Suite: initialize suite
  Suite->>DB: beforeAll — clear caches/tables, seed user/org/project/apiKey
  Suite->>ProviderKeys: create providerKey rows (env-driven)
  Note over Suite,DB: Shared fixtures persist across parallel tests

  par Parallel tests
    Vitest->>Suite: Test A
    Suite->>DB: beforeEach — clear caches
    Suite-->>Vitest: assertions
  and
    Vitest->>Suite: Test B
    Suite->>DB: beforeEach — clear caches
    Suite-->>Vitest: assertions
  end
Loading
sequenceDiagram
  autonumber
  actor Dev as Dev
  participant Vitest as Vitest Runner
  participant Indv as Individual Suite (api-individual.e2e.ts)
  participant DB as Database

  Dev->>Vitest: Run isolated E2E
  Vitest->>Indv: initialize individual suite
  loop Each isolated test
    Indv->>DB: create unique test data / seed per-test artifacts
    Indv->>Indv: execute scenario, read stream via readAll
    Indv-->>Vitest: validate results & logs
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "feat(tests): parallelize E2E tests" is concise, specific, and accurately summarizes the primary change in the changeset—enabling parallel/concurrent execution of the end-to-end test suite. It clearly indicates scope (tests) and intent (parallelize) so a reviewer scanning history understands the main purpose.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch terragon/parallelize-chat-api-e2e-tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot changed the title Parallelize Chat API E2E Tests with Split Structure and Concurrent Execution feat(tests): parallelize E2E tests Sep 14, 2025
- Change main e2e tests to use shared database setup with beforeAll
- Individual tests now create unique test data per test using testId
- Only clear logs between tests to avoid conflicts
- Import beforeAll from vitest to fix reference error

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@steebchen
steebchen marked this pull request as ready for review September 15, 2025 00:34
cursor[bot]

This comment was marked as outdated.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
apps/gateway/src/api.e2e.ts (1)

1493-1589: Extract duplicate readAll function to shared test helpers.

This function is duplicated in both api.e2e.ts and api-individual.e2e.ts. As mentioned in the review of api-individual.e2e.ts, this should be moved to the shared test helpers.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc392a9 and 3b4789d.

📒 Files selected for processing (4)
  • CLAUDE.md (1 hunks)
  • apps/gateway/src/api-individual.e2e.ts (1 hunks)
  • apps/gateway/src/api.e2e.ts (5 hunks)
  • vitest/vitest.e2e.config.mts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Always use top-level import; never use require() or dynamic import()

Always use top-level import; never use require or dynamic imports

Files:

  • apps/gateway/src/api-individual.e2e.ts
  • apps/gateway/src/api.e2e.ts
{apps/{api,gateway}/**/*.ts,packages/db/**/*.ts}

📄 CodeRabbit inference engine (CLAUDE.md)

{apps/{api,gateway}/**/*.ts,packages/db/**/*.ts}: For database reads, use Drizzle’s db().query.

.findMany() or db().query.
.findFirst()
Use Drizzle ORM with the latest object syntax

Files:

  • apps/gateway/src/api-individual.e2e.ts
  • apps/gateway/src/api.e2e.ts
**/*.e2e.ts

📄 CodeRabbit inference engine (AGENTS.md)

Name end-to-end test files with the .e2e.ts suffix

Files:

  • apps/gateway/src/api-individual.e2e.ts
  • apps/gateway/src/api.e2e.ts
apps/{api,gateway}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle ORM with the latest object syntax for database access
For reads, use db().query.

.findMany() or db().query.
.findFirst()
After API route changes, run pnpm generate to update OpenAPI schemas

Files:

  • apps/gateway/src/api-individual.e2e.ts
  • apps/gateway/src/api.e2e.ts
🧠 Learnings (2)
📚 Learning: 2025-09-13T16:25:00.705Z
Learnt from: CR
PR: theopenco/llmgateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-13T16:25:00.705Z
Learning: Run `pnpm test:unit` and `pnpm test:e2e` after adding features

Applied to files:

  • CLAUDE.md
📚 Learning: 2025-09-13T16:48:49.136Z
Learnt from: CR
PR: theopenco/llmgateway#0
File: AGENTS.md:0-0
Timestamp: 2025-09-13T16:48:49.136Z
Learning: Applies to **/*.e2e.ts : Name end-to-end test files with the .e2e.ts suffix

Applied to files:

  • CLAUDE.md
🧬 Code graph analysis (2)
apps/gateway/src/api-individual.e2e.ts (5)
packages/db/src/db.ts (1)
  • db (15-19)
packages/db/src/index.ts (1)
  • tables (10-12)
packages/models/src/providers.ts (1)
  • providers (19-236)
apps/gateway/src/test-utils/test-helpers.ts (3)
  • clearCache (8-10)
  • waitForLogByRequestId (58-98)
  • waitForLogs (19-49)
packages/db/src/schema.ts (1)
  • log (324-386)
apps/gateway/src/api.e2e.ts (3)
apps/gateway/src/test-utils/test-helpers.ts (1)
  • clearCache (8-10)
packages/db/src/db.ts (1)
  • db (15-19)
packages/db/src/index.ts (1)
  • tables (10-12)
🪛 ESLint
apps/gateway/src/api-individual.e2e.ts

[error] 1-1: Resolve error: EACCES: permission denied, open '/PEQJfVbxkI'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import@2.32.0_eslint@9.34.0_jiti@2.5.1_/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:233:12)
at resolveImportType (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1___504bceda7a403f00735dcb51d6a813c7/node_modules/eslint-plugin-import/lib/core/importType.js:126:2822)
at computeRank (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1___504bceda7a403f00735dcb51d6a813c7/node_modules/eslint-plugin-import/lib/rules/order.js:529:43)

(import/order)


[error] 1-1: Resolve error: EACCES: permission denied, open '/BkAzYSrYvo'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import@2.32.0_eslint@9.34.0_jiti@2.5.1_/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:233:12)
at checkSourceValue (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1___504bceda7a403f00735dcb51d6a813c7/node_modules/eslint-plugin-import/lib/rules/no-useless-path-segments.js:85:53)
at checkSourceValue (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/moduleVisitor.js:32:5)

(import/no-useless-path-segments)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
vitest/vitest.e2e.config.mts (1)

18-27: LGTM! Thread pool configuration aligns with the documented parallelization strategy.

The configuration properly implements parallel execution with a thread pool of up to 10 threads, matching the documented approach in CLAUDE.md.

CLAUDE.md (1)

42-51: LGTM! Documentation clearly describes the new E2E test structure.

The documentation effectively explains the split structure and parallelization approach, providing clear guidance for developers on the new testing architecture.

apps/gateway/src/api-individual.e2e.ts (3)

24-73: Good test data isolation approach.

The createTestData function properly creates unique test data for each test using a test ID, which prevents data conflicts in isolated test scenarios. This is well-suited for the individual test file's purpose.


1-14: No code changes required — imports resolve correctly.
Confirmed exports: apps/gateway/src/index.ts -> app; apps/gateway/src/test-utils/test-helpers.ts -> getProviderEnvVar, clearCache, waitForLogs, waitForLogByRequestId. ESLint "permission denied" resolution errors are environmental (CI/sandbox).


316-380: Enable or document the skipped custom model test.

The test is intentionally guarded by an OpenAI API key check (apps/gateway/src/api-individual.e2e.ts:316–380); other tests filter out "custom"/"auto" models (apps/gateway/src/api.e2e.ts) and chat logic has special handling for "custom" (apps/gateway/src/chat/chat.ts). Either enable it in CI (provide credentials) or convert it to a mocked test; if it must remain skipped, add a one-line comment explaining why and the conditions to enable it.

apps/gateway/src/api.e2e.ts (4)

315-315: Excellent use of Vitest's concurrent mode for parallel test execution.

The concurrent mode configuration properly enables parallel execution of tests, which should significantly improve test execution time.


317-382: Well-structured shared test setup in beforeAll.

The beforeAll hook properly sets up shared data that all tests can use, which is essential for concurrent test execution. The cleanup of existing data before seeding ensures a clean slate.


384-389: Good isolation strategy in beforeEach.

Only clearing logs and cache while preserving shared test data is the right approach for concurrent tests. This prevents tests from interfering with each other while maintaining the shared setup.


373-381: Verify provider key creation doesn't cause race conditions.

  • createProviderKey is async and awaited per-provider in beforeAll (apps/gateway/src/api.e2e.ts:373–404); the file also deletes providerKey early (line 324).
  • api-individual creates test-scoped keys (apps/gateway/src/api-individual.e2e.ts:81–94).
  • Risk: api.e2e uses fixed IDs ("provider-key-{provider}", "env-{provider}") which can collide across test files/workers if tests run in parallel against a shared DB, causing duplicate-key errors or flakes.
  • Remediation: scope keys to the test (append a testId), make inserts idempotent (upsert / ON CONFLICT), or ensure test isolation (per-worker DB or serial test execution).

Comment thread apps/gateway/src/api-individual.e2e.ts Outdated
steebchen and others added 2 commits September 15, 2025 00:44
- Remove log deletion from beforeEach hooks in both parallel and individual tests
- Tests now use unique request IDs to identify their logs without interfering
- Only clear cache between tests, not logs that other concurrent tests might need
- Resolves race condition where concurrent tests deleted each other's logs

Fixes the bug identified by cursor[bot] in PR #818

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
…ve duplicates

- Extracted readAll function from api.e2e.ts and api-individual.e2e.ts
- Moved readAll to test-utils/test-helpers.ts for reuse
- Removed duplicate readAll implementations from test files

This improves code reuse and maintainability by centralizing the stream reading helper.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
cursor[bot]

This comment was marked as outdated.

Refactored the e2e individual tests to generate and use a unique requestId for each request. This requestId is included in the request headers and used to retrieve logs via waitForLogByRequestId instead of waiting for all logs. Removed waitForLogs usage to improve test precision and reliability.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
cursor[bot]

This comment was marked as outdated.

- Updated Vitest E2E config to use a thread pool of 16 (min 8 threads) instead of 10
- Updated CLAUDE.md documentation to reflect the new parallel execution thread count

This change aims to improve E2E test performance by increasing parallelism.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
@steebchen
steebchen merged commit 1090aae into main Sep 15, 2025
11 of 12 checks passed
@steebchen
steebchen deleted the terragon/parallelize-chat-api-e2e-tests branch September 15, 2025 01:34

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
apps/gateway/src/api-individual.e2e.ts (1)

15-18: Consider extracting shared test utilities.

The generateTestRequestId function appears to be duplicated in both api.e2e.ts and api-individual.e2e.ts. Since the readAll function was already extracted to avoid duplication, this helper could also be moved to the shared test utilities.

🧹 Nitpick comments (4)
apps/gateway/src/api-individual.e2e.ts (1)

95-111: Consider extracting common validation functions.

The validateLogByRequestId function is duplicated in both test files. Consider moving it to the shared test helpers alongside validateResponse.

Move these validation functions to apps/gateway/src/test-utils/test-helpers.ts:

+export async function validateLogByRequestId(requestId: string, logMode?: string) {
+	const log = await waitForLogByRequestId(requestId);
+
+	if (logMode) {
+		console.log("log", JSON.stringify(log, null, 2));
+	}
+
+	expect(log.usedProvider).toBeTruthy();
+	expect(log.errorDetails).toBeNull();
+	expect(log.finishReason).not.toBeNull();
+	expect(log.unifiedFinishReason).not.toBeNull();
+	expect(log.unifiedFinishReason).toBeTruthy();
+	expect(log.usedModel).toBeTruthy();
+	expect(log.requestedModel).toBeTruthy();
+
+	return log;
+}
+
+export function validateResponse(json: any) {
+	expect(json).toHaveProperty("choices.[0].message.content");
+	expect(json).toHaveProperty("usage.prompt_tokens");
+	expect(json).toHaveProperty("usage.completion_tokens");
+	expect(json).toHaveProperty("usage.total_tokens");
+}
apps/gateway/src/test-utils/test-helpers.ts (1)

177-177: Empty catch block could mask errors.

The empty catch block silently ignores JSON parsing errors. While this might be intentional for malformed SSE data, consider at least logging these errors in debug mode.

-					} catch {}
+					} catch (e) {
+						// Silently ignore malformed JSON in SSE stream
+						// This can happen with partial chunks or non-JSON data
+					}
apps/gateway/src/api.e2e.ts (2)

431-484: Consider extracting common test assertions.

The token validation logic (lines 469-478) is repeated multiple times throughout the test file. Consider extracting it into a helper function to reduce duplication.

+function validateUsageTokens(usage: any) {
+	expect(usage).toHaveProperty("prompt_tokens");
+	expect(usage).toHaveProperty("completion_tokens");
+	expect(usage).toHaveProperty("total_tokens");
+	expect(typeof usage.prompt_tokens).toBe("number");
+	expect(typeof usage.completion_tokens).toBe("number");
+	expect(typeof usage.total_tokens).toBe("number");
+	expect(usage.prompt_tokens).toBeGreaterThan(0);
+	expect(usage.completion_tokens).toBeGreaterThan(0);
+	expect(usage.total_tokens).toBeGreaterThan(0);
+}

Then use it in tests:

-expect(json).toHaveProperty("usage");
-expect(json.usage).toHaveProperty("prompt_tokens");
-expect(json.usage).toHaveProperty("completion_tokens");
-expect(json.usage).toHaveProperty("total_tokens");
-expect(typeof json.usage.prompt_tokens).toBe("number");
-expect(typeof json.usage.completion_tokens).toBe("number");
-expect(typeof json.usage.total_tokens).toBe("number");
-expect(json.usage.prompt_tokens).toBeGreaterThan(0);
-expect(json.usage.completion_tokens).toBeGreaterThan(0);
-expect(json.usage.total_tokens).toBeGreaterThan(0);
+expect(json).toHaveProperty("usage");
+validateUsageTokens(json.usage);

1423-1426: Document the special case for zai provider.

The code has a special case where zai provider may have "weird prompt tokens". This should be documented with more context about why this exception exists.

if (provider.providerId !== "zai") {
-	// zai may have weird prompt tokens
+	// zai provider sometimes returns 0 or unexpected prompt token values
+	// TODO: Investigate root cause and potentially fix in provider adapter
	expect(json.usage.prompt_tokens).toBeGreaterThan(0);
}
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3b4789d and 7100639.

📒 Files selected for processing (5)
  • CLAUDE.md (1 hunks)
  • apps/gateway/src/api-individual.e2e.ts (1 hunks)
  • apps/gateway/src/api.e2e.ts (6 hunks)
  • apps/gateway/src/test-utils/test-helpers.ts (1 hunks)
  • vitest/vitest.e2e.config.mts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • vitest/vitest.e2e.config.mts
  • CLAUDE.md
🧰 Additional context used
📓 Path-based instructions (6)
apps/{api,gateway}/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

For reads, use db().query.

.findMany() or db().query.
.findFirst() with Drizzle ORM

Files:

  • apps/gateway/src/test-utils/test-helpers.ts
  • apps/gateway/src/api.e2e.ts
  • apps/gateway/src/api-individual.e2e.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Always use top-level import; never use require() or dynamic imports

Files:

  • apps/gateway/src/test-utils/test-helpers.ts
  • apps/gateway/src/api.e2e.ts
  • apps/gateway/src/api-individual.e2e.ts
apps/{gateway,api}/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

apps/{gateway,api}/**/*.{ts,tsx}: In Gateway (apps/gateway) and API (apps/api), use Hono with Zod and OpenAPI for routing, validation, and documentation
For DB reads, use db().query.

.findMany() or db().query.
.findFirst()

Files:

  • apps/gateway/src/test-utils/test-helpers.ts
  • apps/gateway/src/api.e2e.ts
  • apps/gateway/src/api-individual.e2e.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Always use top-level import; never use require or dynamic imports

Files:

  • apps/gateway/src/test-utils/test-helpers.ts
  • apps/gateway/src/api.e2e.ts
  • apps/gateway/src/api-individual.e2e.ts
**/*.e2e.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Name end-to-end tests with the .e2e.ts extension

Name end-to-end test files with the .e2e.ts suffix

Files:

  • apps/gateway/src/api.e2e.ts
  • apps/gateway/src/api-individual.e2e.ts
**/*.{spec,e2e}.ts

📄 CodeRabbit inference engine (AGENTS.md)

Use Vitest as the test runner in tests

Files:

  • apps/gateway/src/api.e2e.ts
  • apps/gateway/src/api-individual.e2e.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: theopenco/llmgateway#0
File: AGENTS.md:0-0
Timestamp: 2025-09-15T00:49:48.327Z
Learning: Applies to **/*.{spec,e2e}.ts : Use Vitest as the test runner in tests
Learnt from: CR
PR: theopenco/llmgateway#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-15T00:49:15.218Z
Learning: Applies to **/*.e2e.ts : Name end-to-end tests with the .e2e.ts extension
Learnt from: CR
PR: theopenco/llmgateway#0
File: AGENTS.md:0-0
Timestamp: 2025-09-15T00:49:48.327Z
Learning: Applies to **/*.e2e.ts : Name end-to-end test files with the .e2e.ts suffix
📚 Learning: 2025-09-15T00:49:48.327Z
Learnt from: CR
PR: theopenco/llmgateway#0
File: AGENTS.md:0-0
Timestamp: 2025-09-15T00:49:48.327Z
Learning: Applies to **/*.{spec,e2e}.ts : Use Vitest as the test runner in tests

Applied to files:

  • apps/gateway/src/api.e2e.ts
🧬 Code graph analysis (2)
apps/gateway/src/api.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
  • clearCache (8-10)
apps/gateway/src/api-individual.e2e.ts (5)
packages/db/src/db.ts (1)
  • db (15-19)
packages/db/src/index.ts (1)
  • tables (10-12)
packages/models/src/providers.ts (1)
  • providers (19-236)
apps/gateway/src/test-utils/test-helpers.ts (4)
  • getProviderEnvVar (6-6)
  • clearCache (8-10)
  • waitForLogByRequestId (58-98)
  • readAll (105-193)
packages/db/src/schema.ts (1)
  • log (324-386)
🪛 ESLint
apps/gateway/src/api-individual.e2e.ts

[error] 1-1: Resolve error: EACCES: permission denied, open '/rgHpInVAEg'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import@2.32.0_eslint@9.34.0_jiti@2.5.1_/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:233:12)
at resolveImportType (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1___504bceda7a403f00735dcb51d6a813c7/node_modules/eslint-plugin-import/lib/core/importType.js:126:2822)
at computeRank (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1___504bceda7a403f00735dcb51d6a813c7/node_modules/eslint-plugin-import/lib/rules/order.js:529:43)

(import/order)


[error] 1-1: Resolve error: EACCES: permission denied, open '/wMnIVlTyxb'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import@2.32.0_eslint@9.34.0_jiti@2.5.1_/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/resolve.js:233:12)
at checkSourceValue (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1___504bceda7a403f00735dcb51d6a813c7/node_modules/eslint-plugin-import/lib/rules/no-useless-path-segments.js:85:53)
at checkSourceValue (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.5.1__t_0245afd672d291ebf09c60ebb405cf51/node_modules/eslint-module-utils/moduleVisitor.js:32:5)

(import/no-useless-path-segments)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: e2e / run
  • GitHub Check: Cursor Bugbot
🔇 Additional comments (13)
apps/gateway/src/api-individual.e2e.ts (9)

1-13: LGTM!

The imports and file naming correctly follow the coding guidelines (.e2e.ts suffix for E2E tests and using Vitest as the test runner).


24-73: LGTM! Well-structured test data isolation.

The createTestData function provides excellent test isolation by creating unique entities for each test. This prevents interference between concurrent tests and ensures deterministic test execution.


76-78: LGTM! Appropriate isolation for concurrent tests.

Only clearing the cache (not logs) in beforeEach is the correct approach for concurrent test execution, as this prevents race conditions where one test might delete logs that another concurrent test is waiting to validate.


121-155: LGTM! Comprehensive error handling test.

The test properly validates that models without JSON output support correctly return a 400 error with an appropriate error message.


382-427: Clever approach to ensure prompt tokens are never zero.

The test validates that even when providers return 0 prompt tokens, the system correctly calculates and returns a non-zero value. This prevents misleading usage metrics.


429-463: LGTM! Consistent token calculation for streaming.

The test ensures that prompt tokens are properly calculated even in streaming mode when providers return 0, maintaining consistency with the non-streaming behavior.


465-521: LGTM! Comprehensive reasoning model testing.

The test properly validates GPT-5-nano with reasoning parameters and verifies the model mapping works correctly.


523-558: LGTM! Multi-provider model routing validation.

The test ensures that models available from multiple providers can be requested without specifying a provider prefix and the system correctly routes to an available provider.


315-380: Confirm or remove skipped '/v1/chat/completions with bare custom model' test

apps/gateway/src/api-individual.e2e.ts (lines 315–380) contains a skipped test that inserts a providerKey for provider llmgateway and asserts requestedModel/usedModel === "custom". Automated search was inconclusive; verify whether the gateway actually supports a bare "custom" model — if not, remove the test; if yes, unskip and add/verify provider→model mapping tests.

apps/gateway/src/test-utils/test-helpers.ts (1)

105-193: LGTM! Well-structured SSE stream parser.

The readAll function properly handles SSE stream parsing with comprehensive validation of OpenAI format, usage information, and content detection. The implementation correctly handles edge cases like null streams and [DONE] markers.

apps/gateway/src/api.e2e.ts (3)

316-316: LGTM! Concurrent test execution enabled.

The test suite correctly uses { concurrent: true } to enable parallel execution as documented in the PR objectives.


385-388: LGTM! Correct cache-only clearing for concurrent tests.

The beforeEach hook correctly only clears the cache and avoids clearing logs, which prevents race conditions between concurrent tests waiting for their logs.


318-383: No in-file tests modify the shared entities — only cleanup runs in beforeAll (lines 322–337).

rg search of apps/gateway/src/api.e2e.ts found only the initial db.delete calls in beforeAll and no db.update/db.insert referencing org-id, project-id, user-id, or token-id.

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.

1 participant