tests: add e2e routing wiring test harness - #4249
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds Postman-based E2E wiring tests for model-catalog routing: a reusable collection-builder library, a scenario-matrix generator that emits routing collections, a Newman runner that executes them with seed-env handling and polling, and README updates documenting scenarios and regeneration. ChangesE2E Routing Wiring Test Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Generator as build-routing-wiring.mjs
participant CollectionBuilder as collection-builder.mjs
participant Newman as run-newman-routing-wiring-tests.sh
participant Bifrost as Bifrost API
Generator->>CollectionBuilder: buildCollection(expandedScenarios, extraVariables)
CollectionBuilder->>Generator: writes Postman collection JSON
Newman->>Bifrost: executes collection requests (mutations, routing, reads)
Bifrost->>Newman: responses and async logs
Newman->>Newman: pollTest / exponential backoff for eventual consistency
Newman->>Reports: generates HTML reports and exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Confidence Score: 4/5Safe to merge after fixing the assertRoutingTrail polling loop — on a trail-assertion failure the test hangs for 900 s instead of retrying a bounded number of times and proceeding to cleanup. The assertRoutingTrail step 2 omits its pollPrerequest, leaving __cur_poll_attempt stale. Every retry reads the same counter value so the loop never exits cleanly. The two scenarios using assertRoutingTrail will hang rather than fail cleanly on any assertion error. The rest of the collection and runner are correct. tests/e2e/api/runners/build-routing-wiring.mjs — the assertRoutingTrail step 2 item construction at line ~485. Important Files Changed
Reviews (3): Last reviewed commit: "tests: add e2e routing wiring test harne..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ci_normalized="$(printf '%s' "${CI:-}" | tr '[:upper:]' '[:lower:]')" | ||
| if [ "$ci_normalized" = "1" ] || [ "$ci_normalized" = "true" ]; then | ||
| cmd+=(--reporter-cli-no-failures false) | ||
| fi |
There was a problem hiding this comment.
--reporter-cli-no-failures false is a no-op, comment is misleading
The comment says "In CI keep going past failures so every scenario's cleanup folder runs", but --reporter-cli-no-failures false is Newman's default behavior — it only controls whether the CLI reporter prints failure output, not whether Newman bails on failures. Newman only stops on failures if --bail is passed, which this script never does. Cleanup folders already run unconditionally because there is no --bail flag in the command.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const arr = (ids) => "[" + (ids || []).map((kid) => jsKeyName(sid, kid)).join(", ") + "]"; | ||
| const lines = [`var seen = (pm.collectionVariables.get('dist_${sid}') || '').split(',').filter(Boolean);`]; | ||
| if (step.expectKeyIds) { | ||
| lines.push(`${"var"} mustServe = ${arr(step.expectKeyIds)};`); |
There was a problem hiding this comment.
${"var"} in template literal is an unusual pattern
`${"var"} mustServe = …` embeds the string literal "var" via template interpolation, producing the same output as writing var directly. All adjacent lines emit var declarations as regular string content without this workaround; consider aligning the style.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| while IFS= read -r line || [ -n "$line" ]; do | ||
| [[ "$line" =~ ^[[:space:]]*# ]] && continue | ||
| [[ -z "${line//[[:space:]]/}" ]] && continue | ||
| [[ "$line" != *=* ]] && continue | ||
| key="${line%%=*}" | ||
| value="${line#*=}" | ||
| [[ ! "$key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]] && continue | ||
| # Unwrap outer single quotes written by the seed writer and undo its | ||
| # '\''-escape; also tolerate plain double-quoted values. | ||
| if [[ "$value" == \'*\' ]]; then | ||
| value="${value:1:${#value}-2}" | ||
| value="${value//\'\"\'\"\'/\'}" | ||
| elif [[ "$value" == \"*\" ]]; then | ||
| value="${value:1:${#value}-2}" | ||
| fi | ||
| seed_env_values["$key"]="$value" | ||
| done < "$SEED_ENV_PATH" |
There was a problem hiding this comment.
The new runner silently skips malformed seed-env lines (missing =, invalid variable name) with continue, while run-newman-api-tests.sh prints a red error and exits for the same conditions. A misconfigured seed file will cause the routing tests to silently ignore credentials and fail later with an opaque upstream error rather than a clear diagnostic message.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/api/README.md (1)
29-188:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDocumentation mismatch: README documents the wrong test suite.
The README section "Model Catalog Wiring Tests" (lines 29-36, 134-188) documents files that don't exist in this PR:
bifrost-model-catalog-wiring.postman_collection.json(documented)build-model-catalog-wiring-collection.py(documented, Python)run-newman-model-catalog-wiring-tests.sh(documented)But this PR implements different files:
bifrost-routing-wiring.postman_collection.json(actual)build-routing-wiring.mjs(actual, JavaScript)run-newman-routing-wiring-tests.sh(actual)The test focus is also different:
- Model Catalog Wiring (documented): Tests that management-API mutations (add/update/delete provider and key) propagate into the catalog read endpoints (
/api/models,/api/models/details).- Routing Wiring (implemented): Tests that governance × model-catalog rules produce correct routing decisions (VK allow/deny, key gating, aliases, distribution, cross-provider load balancing) verified via
extra_fields.routing_infoand stored logs.The section title, file names, and scenario descriptions need to be updated to match the actual "Routing Wiring Tests" implemented in this PR.
Suggested fix
Replace the section title and file references:
-### Model Catalog Wiring Tests +### Routing Wiring Tests | Path | Description | |------|-------------| -| `collections/bifrost-model-catalog-wiring.postman_collection.json` | Generated collection asserting that management-API mutations (add/update/delete provider and key, toggle key, alias) propagate into the model catalog read endpoints. **Generated — do not hand-edit.** | -| `runners/build-model-catalog-wiring-collection.py` | Generator for the collection above. Holds the scenario spec (the source of truth) and emits the JSON. | -| `runners/individual/run-newman-model-catalog-wiring-tests.sh` | Script to run the model-catalog wiring collection. | +| `collections/bifrost-routing-wiring.postman_collection.json` | Generated collection asserting that governance × model-catalog routing rules produce correct routing decisions (VK gates, key gating, aliases, distribution, cross-provider LB). **Generated — do not hand-edit.** | +| `runners/build-routing-wiring.mjs` | Generator for the collection above. Holds the scenario spec (the source of truth) and emits the JSON. | +| `runners/individual/run-newman-routing-wiring-tests.sh` | Script to run the routing wiring collection. |And update the detailed section starting at line 134:
-### Model Catalog Wiring Tests +### Routing Wiring Tests -These tests cover the path **HTTP mutation → config write → server-side catalog -hook → read endpoint**: the wiring that keeps the model catalog (`/api/models`, -`/api/models/details`) in sync with provider and key changes made through the -management API. Each scenario stands up an isolated custom provider backed by a -real upstream (OpenAI), drives a sequence of mutations, and asserts the catalog -reflects each one. +These tests cover **governance × model-catalog routing decisions**: verifying +that virtual key gates, key-level model restrictions, aliases, and cross-provider +load balancing produce correct routing outcomes. Each scenario stands up an +isolated custom provider backed by a real upstream (OpenAI), configures keys +and optional virtual keys, drives inference requests, and asserts the route via +`extra_fields.routing_info` (synchronous) and stored logs (asynchronous). -What it covers (one scenario per contract): +What it covers (30+ scenarios): -- **Add provider + key** — a gated key surfaces its allowed model. -- **Update key model set** — changing a key's allow-list re-gates the catalog. -- **Disable / re-enable key** — a disabled key drops its models; re-enabling restores them. -- **Delete one of two keys** — only the deleted key's models drop; the sibling's survive. -- **Delete provider** — the provider and its models disappear from the catalog. -- **Alias resolution** — an inference call via a key alias routes to the underlying model. +- **VK allow/deny** — VK allowed_models whitelist/blacklist controls routing. +- **VK key restriction** — VK key_ids pins routing to specific keys. +- **Key gating** — key-level models/blacklist filters routing candidates. +- **Distribution** — weighted key selection across multiple keys. +- **Cross-provider LB** — governance load-balances bare models across VK providers. +- **Aliases** — key aliases resolve to underlying model IDs. +- **Standard providers** — Azure/Bedrock/Vertex routing (serial-only, global providers). Run locally (from this directory): ```bash -./runners/individual/run-newman-model-catalog-wiring-tests.sh +./runners/individual/run-newman-routing-wiring-tests.shAnd update lines 177-182: ```diff -- To change or extend the scenarios, edit - `runners/build-model-catalog-wiring-collection.py` and re-run it, then commit - both the script and the regenerated collection: +- To change or extend the scenarios, edit `runners/build-routing-wiring.mjs` + and re-run it, then commit both the script and the regenerated collection: ```bash - python3 runners/build-model-catalog-wiring-collection.py + node runners/build-routing-wiring.mjs</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@tests/e2e/api/README.mdaround lines 29 - 188, Docs currently describe
"Model Catalog Wiring Tests" and reference
bifrost-model-catalog-wiring.postman_collection.json,
build-model-catalog-wiring-collection.py, and
run-newman-model-catalog-wiring-tests.sh, but the PR implements routing wiring
assets; update the README to rename the section/title to "Routing Wiring Tests",
replace file references with bifrost-routing-wiring.postman_collection.json,
runners/build-routing-wiring.mjs, and
runners/individual/run-newman-routing-wiring-tests.sh, and revise the scenario
description to reflect routing-focused assertions (extra_fields.routing_info,
stored logs, VK allow/deny, key gating, aliases, distribution, cross-provider
load balancing) and change the example invocation from "python3
runners/build-model-catalog-wiring-collection.py" to "node
runners/build-routing-wiring.mjs".</details> <!-- cr-comment:v1:87198bd0f0bf19df3849ecf0 --> </blockquote></details> </blockquote></details>🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Outside diff comments: In `@tests/e2e/api/README.md`: - Around line 29-188: Docs currently describe "Model Catalog Wiring Tests" and reference bifrost-model-catalog-wiring.postman_collection.json, build-model-catalog-wiring-collection.py, and run-newman-model-catalog-wiring-tests.sh, but the PR implements routing wiring assets; update the README to rename the section/title to "Routing Wiring Tests", replace file references with bifrost-routing-wiring.postman_collection.json, runners/build-routing-wiring.mjs, and runners/individual/run-newman-routing-wiring-tests.sh, and revise the scenario description to reflect routing-focused assertions (extra_fields.routing_info, stored logs, VK allow/deny, key gating, aliases, distribution, cross-provider load balancing) and change the example invocation from "python3 runners/build-model-catalog-wiring-collection.py" to "node runners/build-routing-wiring.mjs".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID:
c84be7bb-162f-4276-a232-bce8c76bca41📒 Files selected for processing (6)
tests/e2e/api/README.mdtests/e2e/api/collections/bifrost-routing-wiring.postman_collection.jsontests/e2e/api/runners/build-routing-wiring.mjstests/e2e/api/runners/individual/run-newman-routing-wiring-tests.shtests/e2e/api/runners/lib/collection-builder.mjstests/e2e/api/runners/run-newman-api-tests.sh
0e59caa to
d742bbb
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/api/README.md`:
- Around line 152-182: README contains incorrect script and regeneration
instructions: replace the local run command reference
"run-newman-model-catalog-wiring-tests.sh" with
"run-newman-routing-wiring-tests.sh", change the regeneration script reference
"build-model-catalog-wiring-collection.py" to "build-routing-wiring.mjs", and
update the regeneration command from "python3 ... .py" to "node ... .mjs" so the
README uses the Node CLI and correct script names.
In `@tests/e2e/api/runners/build-routing-wiring.mjs`:
- Around line 249-254: The template literal `${"var"}` in function
distAssertLines is redundant; replace the expression when pushing the
declaration for mustServe so it uses a plain keyword (e.g., change
lines.push(`${"var"} mustServe = ${arr(step.expectKeyIds)};` to lines.push("var
mustServe = " + arr(step.expectKeyIds) + ";")) so the code reads the same as the
later `var only = ...` line and avoids the needless template expression; update
the lines.push call that builds `mustServe` accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f69226a7-25f5-4511-958f-654f901815a4
📒 Files selected for processing (6)
tests/e2e/api/README.mdtests/e2e/api/collections/bifrost-routing-wiring.postman_collection.jsontests/e2e/api/runners/build-routing-wiring.mjstests/e2e/api/runners/individual/run-newman-routing-wiring-tests.shtests/e2e/api/runners/lib/collection-builder.mjstests/e2e/api/runners/run-newman-api-tests.sh
| Run locally (from this directory): | ||
|
|
||
| ```bash | ||
| ./runners/individual/run-newman-model-catalog-wiring-tests.sh | ||
| ``` | ||
|
|
||
| Requirements: | ||
|
|
||
| - Bifrost running at `{{base_url}}` (default `http://localhost:8080`), ideally | ||
| against a clean config store so no pre-existing `catwiring-*` providers linger. | ||
| - `openai_api_key` available — either in the seed env file (`generated/seed.env` | ||
| or `$BIFROST_E2E_SEED_ENV`) or exported in the shell. Scenarios whose required | ||
| credentials are missing skip themselves rather than fail. | ||
|
|
||
| Notes: | ||
|
|
||
| - Every resource is named `catwiring-openai-<scenario>-<run-id>`, where the | ||
| run-id is built once per run from `e2e_seed_prefix` plus a timestamp nonce, so | ||
| parallel runs never collide and a failed run leaves no blocking state. | ||
| - The catalog's live-model cache is populated asynchronously by the key hooks, so | ||
| every post-mutation read polls with exponential backoff (up to 8 attempts) | ||
| instead of asserting immediately. | ||
| - Each scenario has a cleanup folder that deletes its provider (cascading to its | ||
| keys); it runs even when a mid-scenario step fails, and accepts 200/204/404. | ||
| - To change or extend the scenarios, edit | ||
| `runners/build-model-catalog-wiring-collection.py` and re-run it, then commit | ||
| both the script and the regenerated collection: | ||
|
|
||
| ```bash | ||
| python3 runners/build-model-catalog-wiring-collection.py | ||
| ``` |
There was a problem hiding this comment.
Regeneration instructions reference wrong filename and language.
The regeneration workflow and local run instructions have the same filename mismatches:
- Line 155: Script path should be
./runners/individual/run-newman-routing-wiring-tests.sh(notrun-newman-model-catalog-wiring-tests.sh). - Line 177: Comment references editing
runners/build-model-catalog-wiring-collection.pybut should referencerunners/build-routing-wiring.mjs. - Line 181: Regeneration command shows
python3 runners/build-model-catalog-wiring-collection.pybut should benode runners/build-routing-wiring.mjsper the stack context (Node.js CLI, not Python).
These instructions are non-functional as written.
📝 Proposed fix for regeneration instructions
- To change or extend the scenarios, edit
- `runners/build-model-catalog-wiring-collection.py` and re-run it, then commit
+ `runners/build-routing-wiring.mjs` and re-run it, then commit
both the script and the regenerated collection:
```bash
- python3 runners/build-model-catalog-wiring-collection.py
+ node runners/build-routing-wiring.mjs
```And for the local run command:
```bash
- ./runners/individual/run-newman-model-catalog-wiring-tests.sh
+ ./runners/individual/run-newman-routing-wiring-tests.sh</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @tests/e2e/api/README.md around lines 152 - 182, README contains incorrect
script and regeneration instructions: replace the local run command reference
"run-newman-model-catalog-wiring-tests.sh" with
"run-newman-routing-wiring-tests.sh", change the regeneration script reference
"build-model-catalog-wiring-collection.py" to "build-routing-wiring.mjs", and
update the regeneration command from "python3 ... .py" to "node ... .mjs" so the
README uses the Node CLI and correct script names.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- cr-comment:v1:7a1bfbdaacc8eecff983a87d -->
<!-- This is an auto-generated comment by CodeRabbit -->
| function distAssertLines(sid, step) { | ||
| const arr = (ids) => "[" + (ids || []).map((kid) => jsKeyName(sid, kid)).join(", ") + "]"; | ||
| const lines = [`var seen = (pm.collectionVariables.get('dist_${sid}') || '').split(',').filter(Boolean);`]; | ||
| if (step.expectKeyIds) { | ||
| lines.push(`${"var"} mustServe = ${arr(step.expectKeyIds)};`); | ||
| lines.push("mustServe.forEach(function (e) { if (seen.indexOf(e) < 0) throw new Error('key ' + e + ' never served; observed ' + JSON.stringify(seen)); });"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove unnecessary template literal.
Line 253 uses ${"var"} which evaluates to "var" — the template expression is redundant. Compare to line 257 which correctly uses var only = ... directly.
♻️ Simplify
- lines.push(`${"var"} mustServe = ${arr(step.expectKeyIds)};`);
+ lines.push(`var mustServe = ${arr(step.expectKeyIds)};`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/api/runners/build-routing-wiring.mjs` around lines 249 - 254, The
template literal `${"var"}` in function distAssertLines is redundant; replace
the expression when pushing the declaration for mustServe so it uses a plain
keyword (e.g., change lines.push(`${"var"} mustServe =
${arr(step.expectKeyIds)};` to lines.push("var mustServe = " +
arr(step.expectKeyIds) + ";")) so the code reads the same as the later `var only
= ...` line and avoids the needless template expression; update the lines.push
call that builds `mustServe` accordingly.
|
|
Merge activity
|
d742bbb to
fe0a474
Compare
## Summary Briefly explain the purpose of this PR and the problem it solves. ## Changes - What was changed and why - Any notable design decisions or trade-offs ## Type of change - [ ] Bug fix - [ ] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [ ] Core (Go) - [ ] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [ ] Docs ## How to test Describe the steps to validate this change. Include commands and expected outcomes. ```sh # Core/Transports go version go test ./... # UI cd ui pnpm i || npm i pnpm test || npm test pnpm build || npm run build ``` If adding new configs or environment variables, document them here. ## Screenshots/Recordings If UI changes, add before/after screenshots or short clips. ## Breaking changes - [ ] Yes - [ ] No If yes, describe impact and migration instructions. ## Related issues Link related issues and discussions. Example: Closes #123 ## Security considerations Note any security implications (auth, secrets, PII, sandboxing, etc.). ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [ ] I added/updated tests where appropriate - [ ] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Enhanced e2e API testing docs with a new Model Catalog Wiring Tests section, detailed scenarios, run/seed instructions, skipping behavior, async polling/cleanup, and guidance for merging extra Postman collections. * **Tests** * Added comprehensive end-to-end routing/wiring test generation and runners covering provider/key configs, virtual-key routing, distribution/load-balancing, polling/cleanup, reports, and CI-friendly execution. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit