Repository navigation
test(gateway): port-agnostic e2e, opt-in ocr - #3514
Merged
Merged
Conversation
The video log-content assertions hardcoded http://localhost:4001, so videos.spec.ts failed for any worktree running an isolated stack on an offset GATEWAY_PORT. Assert against the same helpers the gateway uses to build the URL instead. ocr.e2e.ts hardcoded mistral-ocr-latest and only skipped when LLM_MISTRAL_API_KEY was absent, so a key without the OCR entitlement failed every scoped e2e run with an upstream 401 unrelated to the change under test. Drive it from the catalogue like the rerank/speech suites via a new ocrModels list, and mark the mapping test: "skip" so it runs only under TEST_MODELS="mistral/mistral-ocr-latest". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe PR adds filtered OCR model configurations for parameterized gateway E2E tests, disables Mistral OCR tests by default, and updates video log URL assertions to use shared gateway URL helpers. ChangesOCR and gateway E2E updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OCRTestSuite
participant GatewayAPI
participant OCRProvider
OCRTestSuite->>GatewayAPI: Send extraction request with configured OCR model
GatewayAPI->>OCRProvider: Execute OCR extraction
OCRProvider-->>GatewayAPI: Return extraction result
GatewayAPI-->>OCRTestSuite: Return test response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two unrelated ways a scoped test run reports failures that have nothing to do with the change under test. Both showed up while verifying #3492.
1. Video log-content assertions hardcode
http://localhost:4001.gateway-api-test-harness.tsand twovideos.spec.tsassertions compare against a literalhttp://localhost:4001, but the gateway builds that URL fromgetGatewayPublicBaseUrl()(i.e.GATEWAY_URL). Any worktree following AGENTS.md's "isolated stack per worktree" guidance runs on an offsetGATEWAY_PORT, so three video tests fail:The tests were asserting the port, not the behaviour.
2.
ocr.e2e.tsruns unconditionally and hardcodesmistral-ocr-latest.It only skipped when
LLM_MISTRAL_API_KEYwas entirely absent, so a Mistral key without the separate OCR entitlement fails with an upstream{"detail":"Invalid API Key"}→ 401 on every e2e run, including ones scoped to an unrelated model:OCR is also billed per page, so it should not run on every unrelated scoped run in the first place.
Approach
Port: assert against the same helpers the gateway itself uses —
getGatewayPublicBaseUrl()in the harness,buildGatewayVideoLogContentUrl()invideos.spec.ts. The tests now verify the URL matches what the app produces, on any port, and keep working on thehttp://localhost:4001default.OCR: OCR models already live in the catalogue (
output: ["ocr"],ocr: true), so drive the suite from it like the rerank/speech/transcription suites do. Adds anocrModelslist tochat-helpers.e2e.tsmirroringrerankModels(same TEST_MODELS/TEST_PROVIDERS, deactivation, env-var and stability filters), andocr.e2e.tsbecomestest.each(ocrModels).The mapping is marked
test: "skip", which makes the suite opt-in —TEST_MODELSoverridestest: "skip", so it runs exactly when you ask for it:TEST_MODELS="mistral/mistral-ocr-latest" pnpm test:e2eThe "rejects an unknown model with 400" case is pure request validation, rejected before any provider is contacted, so it no longer needs a key gate and runs always.
Verification
videos.spec.ts— 50/50 pass on all three configurations (previously 3 failures on the first):GATEWAY_URLhttp://localhost:4801(offset stack)http://localhost:4001(explicit)ocr.e2e.ts:The scoped run that previously failed is now clean:
Gates:
pnpm build17/17 ·pnpm lint17/17 ·packages/models131/131 · fullapps/gatewayunit suite 2171 passed. One failure there —openai-content-filter.spec.ts > logs missing moderation credentials— reproduces identically on unmodifiedmainwith these changes stashed; it is the known local.envleakage (the spec assumes no OpenAI key is configured) and is untouched by this PR.🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores