refactor(tests): improve e2e tests, move provider funcs - #849
Conversation
![terragon-labs[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
- Introduced getConcurrentTestOptions helper in test-helpers.ts to provide consistent test options with concurrency and CI retry. - Updated all e2e test files to use getConcurrentTestOptions instead of inline concurrent: true. - Simplified test options management and improved consistency across test suites. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
WalkthroughReplaces scattered, static e2e test options with centralized, dynamic concurrency via getConcurrentTestOptions. Consolidates provider env var helpers under @llmgateway/models and re-exports provider utilities from packages/models. Refactors chat-basic.e2e.ts to use shared helpers. Minor import-path updates and a .npmrc key order change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Runner as Vitest Runner
participant Suite as e2e Suite (describe)
participant Helpers as chat-helpers.e2e
participant TestUtils as test-utils/test-helpers
participant Models as @llmgateway/models
Runner->>Suite: start
Suite->>Helpers: import getConcurrentTestOptions()
Helpers->>TestUtils: getConcurrentTestOptions(opts?)
TestUtils->>Models: read models, provider mappings
TestUtils-->>Helpers: { concurrent: true, retry/skip }
Helpers-->>Suite: options object
Note over Suite,Helpers: describe("e2e", options)
Suite->>Helpers: beforeAllHook
Helpers->>Models: getProviderEnvVar(providerId)
Helpers-->>Suite: setup complete
Runner-->>Suite: execute tests with derived concurrency
sequenceDiagram
autonumber
participant Test as e2e Test
participant Models as @llmgateway/models
Test->>Models: getProviderEnvVar(providerId)
Models-->>Test: ENV var name (e.g., OPENAI_API_KEY)
Test->>Test: check process.env[ENV]
alt missing key
Test-->>Test: skip test
else has key
Test-->>Test: run test
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit 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. Comment |
github-actions
Bot
changed the title
- Removed redundant code in e2e test files by centralizing utility functions. - Updated test configuration methods with simplified, reusable utilities. - Enhanced test options logic to include dynamic retries and concurrency controls.
- Replaced incorrect relative import with an explicit relative path. - Removed unused exports and cleaned up imports for better clarity.
- Moved `getProviderEnvVar` import from test-helpers to a shared library. - Updated references in relevant E2E test files to use the new path. - Removed unused import from test-helpers to streamline module usage.
…-concurrent-tests
- Moved `getProviderEnvVar` and related logic to shared models directory. - Updated all references to use the new shared location. - Simplified imports and removed redundant exports across files.
steebchen
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/routes/keys-provider.e2e.ts (2)
71-73: Parse theSet-Cookieheader into a validCookieheader.Sending the raw
Set-Cookiestring (with attributes) asCookieis incorrect and may fail.- const token = auth.headers.get("set-cookie")!; + const setCookie = auth.headers.get("set-cookie")!; + // Extract "name=value" from the first cookie + const cookie = setCookie.split(",")[0].split(";")[0]; ... - Cookie: token, + Cookie: cookie,Also applies to: 114-121
127-129: Do not log provider tokens in CI/test output.
jsonincludes the provider key token; logging it leaks secrets.- console.log("json", json); + // Avoid logging sensitive key material + // console.log("json", { ...json, providerKey: { ...json.providerKey, token: "***redacted***" } });apps/gateway/src/log-queue.e2e.ts (1)
63-66: Replace bracketed array indexes in toHaveProperty with dot-index notationUse numeric dot segments (choices.0.message.content) instead of choices.[0] or choices[0]. Replace the following occurrences:
- apps/gateway/src/log-queue.e2e.ts:64 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/chat-json.e2e.ts:55 — expect(json).toHaveProperty("choices[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/chat-helpers.e2e.ts:337 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api-individual.e2e.ts:93 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api-individual.e2e.ts:244 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api-individual.e2e.ts:286 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api-individual.e2e.ts:353 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api.spec.ts:132 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api.spec.ts:247 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api.spec.ts:451 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api.spec.ts:576 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
- apps/gateway/src/api.spec.ts:661 — expect(json).toHaveProperty("choices.[0].message.content") → expect(json).toHaveProperty("choices.0.message.content")
🧹 Nitpick comments (13)
apps/gateway/src/api-individual.e2e.ts (2)
20-71: Add cleanup or unique IDs to avoid DB pollution across runs.
createTestData uses deterministic IDs (e.g., user-json-error). Re-runs can violate PKs and bloat state.Consider:
- Add a cleanup helper and call it in afterEach/afterAll, or
- Suffix testId with a short unique token.
Example (minimal change):
-async function createTestData(testId: string) { +async function createTestData(testId: string) { + const uniq = `${testId}-${Date.now().toString(36)}`; - const userId = `user-${testId}`; + const userId = `user-${uniq}`; - const orgId = `org-${testId}`; + const orgId = `org-${uniq}`; - const projectId = `project-${testId}`; + const projectId = `project-${uniq}`; - const userOrgId = `user-org-${testId}`; + const userOrgId = `user-org-${uniq}`; - const token = `real-token-${testId}`; + const token = `real-token-${uniq}`;If you prefer cleanup, I can draft a
cleanupTestData(...)and wire it into hooks.
92-98: Fix path in toHaveProperty for array index.
Use dot index to avoid fragile bracket parsing.-function validateResponse(json: any) { - expect(json).toHaveProperty("choices.[0].message.content"); +function validateResponse(json: any) { + expect(json).toHaveProperty("choices.0.message.content");apps/gateway/src/chat/chat.ts (1)
35-38: Consolidate imports from @llmgateway/models.
Minor readability nit: merge into a single import block.-import { - getProviderEnvVar, - hasProviderEnvironmentToken, -} from "@llmgateway/models"; -import { +import { + getProviderEnvVar, + hasProviderEnvironmentToken, getCheapestFromAvailableProviders, getModelStreamingSupport, getProviderEndpoint, getProviderHeaders, type Model, type ModelDefinition, type ProviderModelMapping, models, prepareRequestBody, type BaseMessage, type Provider, type ProviderRequestBody, type OpenAIToolInput, hasMaxTokens, providers, } from "@llmgateway/models";Also applies to: 40-55
apps/gateway/src/log-queue.e2e.ts (1)
11-15: Adopt shared concurrency helper and drop localgetTestOptions.Use the centralized helpers for consistency with the rest of the suite and to enable CI retry + concurrent runs.
Apply:
import { beforeAllHook, beforeEachHook, generateTestRequestId, + getConcurrentTestOptions, + getTestOptions, } from "@/chat-helpers.e2e"; -// Helper function to get test options with retry for CI environment -function getTestOptions(): TestOptions { - return process.env.CI ? { retry: 3 } : {}; -} +// use shared helpers from chat-helpers.e2e -describe("Log Queue Processing E2E", () => { +describe("Log Queue Processing E2E", getConcurrentTestOptions(), () => { beforeAll(beforeAllHook); beforeEach(beforeEachHook);Also applies to: 23-25, 27-29
apps/gateway/src/chat-helpers.e2e.ts (2)
24-26: Avoid duplicategetTestOptionsimplementations.This local version diverges from the richer one in
test-utils/test-helpers. Re-export the shared one to prevent drift.-import { describe, expect, it, type TestOptions } from "vitest"; +import { describe, expect, it } from "vitest"; ... -// Helper function to get test options with retry for CI environment -export function getTestOptions(): TestOptions { - return process.env.CI ? { retry: 3 } : {}; -} +export { getTestOptions } from "./test-utils/test-helpers";
428-433: Remove the empty test in a helpers module.This creates a noisy, non-informative test suite entry.
-describe("e2e", getConcurrentTestOptions(), () => { - it("empty", () => { - expect(true).toBe(true); - }); -});apps/gateway/src/test-utils/test-helpers.ts (3)
17-30: Clarify skip behavior whenTEST_MODELSis set (infra tests may be skipped unintentionally).With
opts.completions: false, non‑completion tests (e.g., keys/provider) get skipped ifTEST_MODELSis set locally. Provide an escape hatch.-export function getTestOptions( - opts: { completions?: boolean } = { - completions: true, - }, -): TestOptions { +export function getTestOptions( + opts: { completions?: boolean; infra?: boolean } = { completions: true }, +): TestOptions { const hasTestOnly = models.some((model) => model.providers.some( (provider: ProviderModelMapping) => provider.test === "only", ), ); - return process.env.CI || opts?.completions - ? { retry: 3 } - : { skip: hasTestOnly || !!process.env.TEST_MODELS }; + if (process.env.CI || opts?.completions) return { retry: 3 }; + if (opts?.infra) return {}; + return { skip: hasTestOnly || !!process.env.TEST_MODELS }; }Follow‑up: pass
{ infra: true }from infra-only tests (e.g., provider-keys).
55-66: Gate verbose polling logs behind a flag.These logs can spam CI; consider honoring
LOG_MODE.- console.log(`Waiting for ${expectedCount} logs (timeout: ${maxWaitMs}ms)...`); + if (process.env.LOG_MODE) { + console.log(`Waiting for ${expectedCount} logs (timeout: ${maxWaitMs}ms)...`); + }Repeat similarly for other
console.log/console.warnin this helper.
165-207: Handle SSE line splits across chunks.Current splitter can drop events if a line is split between reads. Buffer partial lines across iterations.
- const chunk = new TextDecoder().decode(value); - fullContent += chunk; - const lines = chunk.split("\n"); + const chunk = new TextDecoder().decode(value); + fullContent += chunk; + // Preserve partial lines across chunks + buffered = (buffered ?? "") + chunk; + const lines = buffered.split("\n"); + buffered = lines.pop() ?? ""; for (const line of lines) { if (line.startsWith("data: ")) { eventCount++; hasValidSSE = true; if (line === "data: [DONE]") { continue; } try { const data = JSON.parse(line.substring(6)); chunks.push(data);Add
let buffered = "";above the loop.apps/gateway/src/chat-basic.e2e.ts (4)
19-21: Use crypto.randomUUID() and avoid legacy substrGenerates safer, simpler IDs; removes deprecated String.prototype.substr usage.
Apply this diff:
-export function generateTestRequestId(): string { - return `test-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`; -} +export function generateTestRequestId(): string { + return `test-${randomUUID()}`; +}Add import (outside selected lines):
import { randomUUID } from "node:crypto";
107-110: Remove empty text content to avoid provider 4xx/validation flakesSome providers reject empty text chunks in a content array. Since the comment already notes special handling, safest is to drop this element.
Apply this diff:
- { - type: "text", - text: "", // empty text – note this may need special handling - },
145-148: Avoid hard-coded 'zai' exception; use capability metadataInstead of branching on providerId, push this into provider metadata (e.g., provider.promptTokensMayBeZero) or a helper assertion to keep tests declarative.
66-76: DRY up usage assertions into a helperThese identical checks appear in multiple tests. Consider an assertUsage(json, { allowZeroPrompt?: boolean }) in chat-helpers.e2e to reduce duplication and ease future changes.
Example:
export function assertUsage(json: any, opts?: { allowZeroPrompt?: boolean }) { expect(json).toHaveProperty("usage"); const { prompt_tokens, completion_tokens, total_tokens } = json.usage; expect(typeof prompt_tokens).toBe("number"); expect(typeof completion_tokens).toBe("number"); expect(typeof total_tokens).toBe("number"); if (!opts?.allowZeroPrompt) expect(prompt_tokens).toBeGreaterThan(0); expect(completion_tokens).toBeGreaterThan(0); expect(total_tokens).toBeGreaterThan(0); }Also applies to: 199-209
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.npmrc(1 hunks)apps/api/src/routes/keys-provider.e2e.ts(2 hunks)apps/gateway/src/api-individual.e2e.ts(1 hunks)apps/gateway/src/chat-basic.e2e.ts(1 hunks)apps/gateway/src/chat-full.e2e.ts(2 hunks)apps/gateway/src/chat-helpers.e2e.ts(4 hunks)apps/gateway/src/chat-json.e2e.ts(1 hunks)apps/gateway/src/chat-reasoning.e2e.ts(2 hunks)apps/gateway/src/chat-rs.e2e.ts(2 hunks)apps/gateway/src/chat-streaming.e2e.ts(2 hunks)apps/gateway/src/chat-toolcalls-result.e2e.ts(1 hunks)apps/gateway/src/chat-toolcalls.e2e.ts(1 hunks)apps/gateway/src/chat/chat.ts(1 hunks)apps/gateway/src/log-queue.e2e.ts(1 hunks)apps/gateway/src/test-utils/test-helpers.ts(1 hunks)packages/models/src/index.ts(1 hunks)packages/models/src/provider.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always use top-level import; never use require() or dynamic imports
Files:
packages/models/src/index.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tspackages/models/src/provider.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts
{apps/api,apps/gateway,apps/ui,apps/docs,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Always use top-level import; never use require() or dynamic imports (e.g., import(), next/dynamic)
Files:
packages/models/src/index.tsapps/gateway/src/chat/chat.tsapps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tspackages/models/src/provider.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts
{apps/{api,gateway}/src,packages/db}/**/*.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Use Drizzle ORM with the latest object syntax
Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts
apps/{api,gateway}/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
For read queries, use db().query.
.findMany() or db().query..findFirst() Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts{apps/api,apps/gateway,packages/db}/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
{apps/api,apps/gateway,packages/db}/**/*.ts: Use Drizzle ORM with the latest object syntax for database access
For reads, use db().query..findMany() or db().query.
.findFirst() Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts**/*.e2e.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place end-to-end tests in files matching *.e2e.ts
Place end-to-end tests in files named *.e2e.ts
Files:
apps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.tsapps/gateway/src/api-individual.e2e.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Put isolated E2E tests in apps/gateway/src/api-individual.e2e.ts
Put isolated E2E test cases in apps/gateway/src/api-individual.e2e.ts
Files:
apps/gateway/src/api-individual.e2e.ts🧠 Learnings (7)
📓 Common learnings
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-15T13:15:00.724Z Learning: Applies to apps/gateway/src/api.e2e.ts : Put parallelized .each() E2E tests in apps/gateway/src/api.e2e.ts (uses concurrent mode)Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-09-15T13:16:05.355Z Learning: Applies to apps/gateway/src/api.e2e.ts : Put parallelizable .each() E2E tests in apps/gateway/src/api.e2e.ts (uses concurrent mode)Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-15T13:15:00.724Z Learning: Applies to apps/gateway/src/api-individual.e2e.ts : Put isolated E2E tests in apps/gateway/src/api-individual.e2e.tsLearnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-09-15T13:16:05.355Z Learning: Applies to apps/gateway/src/api-individual.e2e.ts : Put isolated E2E test cases in apps/gateway/src/api-individual.e2e.ts📚 Learning: 2025-09-15T13:15:00.724Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-15T13:15:00.724Z Learning: Applies to apps/gateway/src/api.e2e.ts : Put parallelized .each() E2E tests in apps/gateway/src/api.e2e.ts (uses concurrent mode)Applied to files:
apps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts📚 Learning: 2025-09-15T13:16:05.355Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-09-15T13:16:05.355Z Learning: Applies to apps/gateway/src/api.e2e.ts : Put parallelizable .each() E2E tests in apps/gateway/src/api.e2e.ts (uses concurrent mode)Applied to files:
apps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts📚 Learning: 2025-09-15T13:15:00.724Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-15T13:15:00.724Z Learning: Applies to apps/gateway/src/api-individual.e2e.ts : Put isolated E2E tests in apps/gateway/src/api-individual.e2e.tsApplied to files:
apps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-json.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tsapps/gateway/src/test-utils/test-helpers.tsapps/gateway/src/chat-toolcalls.e2e.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts📚 Learning: 2025-09-15T13:16:05.355Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-09-15T13:16:05.355Z Learning: Applies to apps/gateway/src/api-individual.e2e.ts : Put isolated E2E test cases in apps/gateway/src/api-individual.e2e.tsApplied to files:
apps/gateway/src/chat-helpers.e2e.tsapps/gateway/src/api-individual.e2e.tsapps/gateway/src/chat-reasoning.e2e.tsapps/gateway/src/chat-toolcalls-result.e2e.tsapps/gateway/src/chat-full.e2e.tsapps/gateway/src/chat-streaming.e2e.tsapps/gateway/src/log-queue.e2e.tsapps/gateway/src/test-utils/test-helpers.tsapps/api/src/routes/keys-provider.e2e.tsapps/gateway/src/chat-rs.e2e.tsapps/gateway/src/chat-basic.e2e.ts📚 Learning: 2025-09-15T13:16:05.355Z
Learnt from: CR PR: theopenco/llmgateway#0 File: AGENTS.md:0-0 Timestamp: 2025-09-15T13:16:05.355Z Learning: Applies to **/*.e2e.ts : Place end-to-end tests in files named *.e2e.tsApplied to files:
apps/gateway/src/chat-streaming.e2e.ts📚 Learning: 2025-09-15T13:15:00.724Z
Learnt from: CR PR: theopenco/llmgateway#0 File: CLAUDE.md:0-0 Timestamp: 2025-09-15T13:15:00.724Z Learning: Applies to **/*.e2e.ts : Place end-to-end tests in files matching *.e2e.tsApplied to files:
apps/gateway/src/chat-streaming.e2e.ts🧬 Code graph analysis (11)
apps/gateway/src/chat-helpers.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/chat-reasoning.e2e.ts (2)
apps/gateway/src/chat-helpers.e2e.ts (1)
getConcurrentTestOptions(30-30)apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/chat-toolcalls-result.e2e.ts (2)
apps/gateway/src/chat-helpers.e2e.ts (1)
getConcurrentTestOptions(30-30)apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/chat-json.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/chat-full.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/chat-streaming.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/test-utils/test-helpers.ts (3)
apps/gateway/src/chat-helpers.e2e.ts (1)
getTestOptions(25-27)apps/gateway/src/models/index.ts (1)
models(7-7)packages/models/src/models.ts (2)
models(159-175)ProviderModelMapping(23-100)apps/gateway/src/chat-toolcalls.e2e.ts (2)
apps/gateway/src/chat-helpers.e2e.ts (1)
getConcurrentTestOptions(30-30)apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/api/src/routes/keys-provider.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/chat-rs.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)apps/gateway/src/chat-basic.e2e.ts (1)
apps/gateway/src/test-utils/test-helpers.ts (1)
getConcurrentTestOptions(36-40)🪛 ESLint
packages/models/src/provider.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/KoUbPAlwnj'
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). (3)
- GitHub Check: build / run
- GitHub Check: e2e-shards (1)
- GitHub Check: e2e-shards (2)
🔇 Additional comments (17)
.npmrc (1)
2-2: LGTM: reorder only; no semantic change.packages/models/src/index.ts (1)
2-2: Re‑export looks good; watch for cycles with provider.ts.
If provider.ts imports from the index (.), ensure there’s no type-resolution cycle. See my note on provider.ts.apps/gateway/src/chat-toolcalls-result.e2e.ts (1)
8-16: LGTM: unified concurrent test options.
Simplifies config and keeps CI behavior consistent.apps/gateway/src/chat-reasoning.e2e.ts (1)
8-19: LGTM: switched to getConcurrentTestOptions.
Keeps concurrency policy centralized.apps/gateway/src/chat-full.e2e.ts (1)
10-12: LGTM: centralized concurrency via helper.
Consistent with other e2e suites.Also applies to: 21-21
apps/gateway/src/log-queue.e2e.ts (1)
17-21: Import source changes look good.Moving
getProviderEnvVarto@llmgateway/modelsand keepingwaitForLogByRequestIdlocal is consistent with the PR direction.apps/gateway/src/chat-streaming.e2e.ts (1)
17-17: Good switch to centralized concurrency options.
describe("e2e", getConcurrentTestOptions(), ...)aligns with the new helper.apps/gateway/src/chat-toolcalls.e2e.ts (1)
16-16: Concurrency helper usage LGTM.Consistent with other e2e files.
apps/gateway/src/chat-rs.e2e.ts (1)
19-19: Concurrency helper usage LGTM.Matches suite-wide pattern.
apps/gateway/src/chat-json.e2e.ts (1)
16-16: Concurrency helper usage LGTM.Consistent with shared helper adoption.
apps/gateway/src/chat-helpers.e2e.ts (2)
7-11: ImportinggetProviderEnvVarfrom models is correct.Centralizing provider utilities under
@llmgateway/modelsis the right direction.
29-31: Re-exportinggetConcurrentTestOptionshere is good.Keeps a single import surface for tests.
apps/gateway/src/test-utils/test-helpers.ts (2)
36-41: Concurrency wrapper looks good.Simple composition over
getTestOptions.
102-110: Use Drizzle query API for reads (replace select().from() with db.query.*)File: apps/gateway/src/test-utils/test-helpers.ts (lines 102–110). Prefer db.query.log.findFirst over the raw select().from() per project guidelines.
- // Query for the specific log entry by request ID - const logs = await db - .select() - .from(tables.log) - .where(eq(tables.log.requestId, requestId)) - .limit(1); - - const log = logs[0] || null; + // Query for the specific log entry by request ID (Drizzle query API) + const log = await db.query.log.findFirst({ + where: eq(tables.log.requestId, requestId), + });Verification attempted with rg returned no output; cannot confirm other occurrences — manual verification required.
apps/gateway/src/chat-basic.e2e.ts (3)
5-14: Align helper import path with relocation to test-helpers.tsPer PR summary, getConcurrentTestOptions/getTestOptions moved to test-helpers.ts. This file still imports them from "@/chat-helpers.e2e". Confirm re‑export, or switch to importing from "@/test-utils/test-helpers" to avoid drift/cycles.
Apply if not re‑exported:
import { beforeAllHook, beforeEachHook, - getConcurrentTestOptions, - getTestOptions, logMode, providerModels, testModels, validateLogByRequestId, validateResponse, } from "@/chat-helpers.e2e"; +import { getConcurrentTestOptions, getTestOptions } from "@/test-utils/test-helpers";
23-23: Confirm describe() options are honored; fallback to describe.concurrent if notVitest supports per‑test options; describe‑level options support can vary. Ensure getConcurrentTestOptions() at suite level is effective. If not, prefer describe.concurrent(...) and keep per‑test getTestOptions().
Optional change if needed:
-describe("e2e", getConcurrentTestOptions(), () => { +describe.concurrent("e2e", () => {
28-31: Nice centralization of test optionsSwitching to shared helpers for retries/concurrency reduces boilerplate and keeps CI knobs consistent.
| ? { retry: 3 } | ||
| : { skip: hasTestOnly || !!process.env.TEST_MODELS }; | ||
| } | ||
| import { getConcurrentTestOptions } from "../../../gateway/src/test-utils/test-helpers"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid cross-app relative import for test helpers.
../../../gateway/src/test-utils/test-helpers is brittle and likely violates lint rules. Move helpers to a shared package (e.g., packages/test-helpers) and import from there, or re-export via a workspace alias.
Follow‑up: if rehoming is out of scope for this PR, duplicate a minimal wrapper locally and plan a follow-up refactor.
🤖 Prompt for AI Agents
In apps/api/src/routes/keys-provider.e2e.ts around line 11, the test imports a
helper via a brittle cross-app relative path
("../../../gateway/src/test-utils/test-helpers"); replace this by either moving
the helper into a shared test-helpers package (e.g., packages/test-helpers) and
updating the import to that package, or, if rehoming is out of scope for this
PR, create a minimal local wrapper copy of the required helper in
apps/api/src/test-utils (or similar) and import it from there, then add a
follow-up ticket to re-export or deduplicate into a workspace-shared module so
lint rules and repo boundaries are respected.
| describe( | ||
| "e2e tests for provider keys", | ||
| { concurrent: true, ...getTestOptions() }, | ||
| getConcurrentTestOptions({ completions: false }), | ||
| () => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check skip semantics with getConcurrentTestOptions({ completions: false }).
With the current helper, local runs with TEST_MODELS may skip these infra tests. Consider an infra: true option (see suggested change in helpers) and use getConcurrentTestOptions({ completions: false, infra: true }).
🏁 Script executed:
#!/bin/bash
# Grep helper implementation to confirm current skip logic
rg -n "getConcurrentTestOptions|getTestOptions" apps/gateway/src/test-utils/test-helpers.ts -n -A3 -B3Length of output: 535
Add an infra flag to test helpers and opt infra tests into it.
getConcurrentTestOptions currently only accepts { completions?: boolean } (apps/gateway/src/test-utils/test-helpers.ts); extend getTestOptions/getConcurrentTestOptions to accept infra?: boolean (default false) and update the call in apps/api/src/routes/keys-provider.e2e.ts (around lines 18–21) to getConcurrentTestOptions({ completions: false, infra: true }).
🤖 Prompt for AI Agents
In apps/api/src/routes/keys-provider.e2e.ts around lines 18 to 21, the test uses
getConcurrentTestOptions({ completions: false }) but the test helpers need to
support an infra flag so infra tests can be opted-in; update the test-helpers
(apps/gateway/src/test-utils/test-helpers.ts) to extend both getTestOptions and
getConcurrentTestOptions signatures to accept an optional infra?: boolean
(default false) and wire that flag through to any internal option objects or
behavior, then change this call to getConcurrentTestOptions({ completions:
false, infra: true }) so the e2e file opts into infra tests.
| @@ -1,4 +1,4 @@ | |||
| import type { Provider } from "@llmgateway/models"; | |||
| import type { Provider } from "."; | |||
There was a problem hiding this comment.
Avoid barrel import here to prevent circular dependency.
provider.ts imports types from ".", and index.ts now re‑exports "./provider", creating a cycle. Use a direct types import.
Apply this diff:
-import type { Provider } from ".";
+import type { Provider } from "./types";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { Provider } from "."; | |
| import type { Provider } from "./types"; |
🧰 Tools
🪛 ESLint
[error] 1-1: Resolve error: EACCES: permission denied, open '/KoUbPAlwnj'
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)
🤖 Prompt for AI Agents
In packages/models/src/provider.ts around line 1, the file currently uses a
barrel import (import type { Provider } from ".") which creates a circular
dependency because index.ts re-exports "./provider"; replace the barrel import
with a direct type import from the module that actually declares the Provider
type (e.g., import type { Provider } from "./types" or the exact file path where
Provider is defined), ensuring the new import uses a relative path to that
declaration to break the cycle.
1 participant
Summary
getConcurrentTestOptionshelper for enabling concurrency and CI retrieskeys-provider.e2e.tswith a dedicatedsetupTestDatafunction to create isolated test data including user, account, organization, and projectkeys-provider.e2e.tstest-helpers.tsfor consistent retry and concurrency configurationChanges
Test Options and Concurrency
getConcurrentTestOptionsintest-helpers.tsto combine concurrency with CI retry logicchat-basic.e2e.ts,chat-full.e2e.ts,chat-helpers.e2e.ts,chat-json.e2e.ts,chat-reasoning.e2e.ts,chat-rs.e2e.ts,chat-streaming.e2e.ts,chat-toolcalls-result.e2e.ts,chat-toolcalls.e2e.ts) to usegetConcurrentTestOptionsin theirdescribeblockskeys-provider.e2e.ts
getConcurrentTestOptionsfor concurrencysetupTestDataasync function to create unique test users, accounts, organizations, and projects for each test runTest Helpers
getTestOptionsandgetConcurrentTestOptionstotest-helpers.tsfor reuseTest plan
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/8abc6a5b-b87a-4fc4-b06f-3af35d626c89
Summary by CodeRabbit