Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a comprehensive Newman/Postman e2e test suite: multiple Postman collections and per-provider environment fixtures, sample fixtures and docs, plus several Bash runners that discover provider envs, run collections (single or parallel), capture IDs, post-process logs, and emit HTML/JSON/CLI reports. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Test Runner (bash)
participant EnvStore as Provider Env files
participant Newman as Newman CLI
participant Server as Bifrost API
participant Reports as Reports/Logs
Runner->>EnvStore: discover provider envs (bifrost-v1-*.postman_environment.json)
Runner->>Newman: spawn per-provider Newman run (collection + env + injected vars)
Newman->>Server: execute HTTP requests against {{base_url}} (provider-targeted)
Server-->>Newman: responses (IDs, status, errors)
Newman-->>Reports: write CLI/HTML/JSON reports and raw logs
Newman-->>Runner: exit code/result
Runner->>Reports: post-process logs (format JSON, aggregate failures)
Runner-->>Runner: display live status table and final summary
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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 |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@tests/e2e/api/bifrost-v1-complete.postman_collection.json`:
- Around line 622-647: The "Delete File" request named "Delete File" currently
runs before the "Create Container File (Reference)" request and removes the
shared {{file_id}}, causing the container reference to fail; fix by relocating
the "Delete File" request to the end of the collection (after Containers
cleanup) or alternatively add an extra file upload request that sets a different
collection variable (e.g., file_id_ref) and update "Create Container File
(Reference)" to use that new variable so the referenced file remains available;
target the "Delete File" request's prerequest script and the "Create Container
File (Reference)" request to make this change.
In `@tests/e2e/api/fixtures/sample.jsonl`:
- Around line 1-2: The fixture tests/e2e/api/fixtures/sample.jsonl hardcodes
"model": "openai/gpt-4o-mini", causing non-OpenAI providers to fail; fix by
making the inner request model dynamic or provider-specific: either (A) replace
the hardcoded model string with a templated value like "{{provider}}/{{model}}"
so the batch inner requests match the top-level provider parameter, or (B) add
per-provider fixture variants under provider_config (e.g.,
provider_config/<provider>/sample.jsonl) and update test harness to load the
provider-specific file, or (C) mark this fixture as OpenAI-only and update the
batch test logic to skip it for other providers — locate references to the
sample.jsonl content and the "model" field to implement the chosen approach.
In `@tests/e2e/api/provider_config/bifrost-v1-anthropic.postman_environment.json`:
- Around line 7-11: The environment sets embedding_model, speech_model,
transcription_model, and image_model to the Claude chat model which is invalid;
change those keys ("embedding_model", "speech_model", "transcription_model",
"image_model") to a sentinel value (e.g., "UNSUPPORTED" or empty string) or
otherwise mark them disabled so tests know to skip unsupported endpoints, and
leave "model" set to "claude-sonnet-4-5-20250929" for chat completions; ensure
whatever sentinel you choose is recognized by the test harness so those
endpoints are skipped for Anthropic.
In
`@tests/e2e/api/provider_config/bifrost-v1-huggingface.postman_environment.json`:
- Around line 7-11: Update the HuggingFace Postman environment entries so each
task uses the appropriate model: set "model" to a chat/text model (e.g.,
groq/meta-llama/Llama-3.3-70B-Instruct), set "embedding_model" to an embedding
model (e.g., sambanova/intfloat/e5-mistral-7b-instruct), set "speech_model" to a
speech synthesis model (e.g., fal-ai/hexgrad/Kokoro-82M), set
"transcription_model" to a transcription model (e.g.,
fal-ai/openai/whisper-large-v3), and leave "image_model" as
fal-ai/fal-ai/flux/dev; verify values against
core/providers/huggingface/huggingface_test.go and update the keys "model",
"embedding_model", "speech_model", "transcription_model", and "image_model" in
the JSON accordingly.
In `@tests/e2e/api/provider_config/bifrost-v1-nebius.postman_environment.json`:
- Around line 7-11: The environment incorrectly uses the image model
"black-forest-labs/flux-schnell" for the primary "model" and several other keys;
update the JSON so "model" is a Nebius-hosted chat model (replace the "model"
value with the correct Nebius chat model name), set "image_model" to
"black-forest-labs/flux-schnell" only, and change "embedding_model",
"speech_model", and "transcription_model" to appropriate Nebius-supported models
or sentinel values (e.g., "none" or "unsupported") for capabilities not provided
by the provider; ensure you update the entries for the keys "model",
"embedding_model", "speech_model", "transcription_model", and leave
"image_model" as the image model.
In
`@tests/e2e/api/provider_config/bifrost-v1-openrouter.postman_environment.json`:
- Around line 7-11: The environment uses the same chat model value for all
specialized keys which will break capability-specific tests; update the keys
"embedding_model", "speech_model", "transcription_model", and "image_model" to
provider-appropriate models (e.g., set "embedding_model" to an embedding-capable
model like openai/text-embedding-3-small, "speech_model"/"transcription_model"
to a speech/transcription-capable model, and "image_model" to an image-capable
model) or replace unsupported endpoints with a sentinel/value documenting they
are expected to fail for this provider, keeping the "model" (chat) entry as-is
and leaving enabled flags consistent.
In `@tests/e2e/api/provider_config/bifrost-v1-parasail.postman_environment.json`:
- Around line 7-11: The environment uses the chat model value
"parasail-llama-33-70b-fp8" for every capability; update the entries for
"embedding_model", "speech_model", "transcription_model", and "image_model" to
either appropriate provider-specific models for
embeddings/speech/transcription/images or replace them with a clear
sentinel/placeholder (e.g., "unsupported" or "none") to indicate those
capabilities are not supported; leave the chat model key "model" as the chat
model and ensure each key name ("model", "embedding_model", "speech_model",
"transcription_model", "image_model") maps to a correct type of model for its
capability.
In `@tests/e2e/api/run-newman-tests.sh`:
- Around line 155-184: The run_newman function currently uses eval $cmd which,
under set -e, will immediately exit the script on a failing newman run and skip
the subsequent success/failure/report printing; modify run_newman so it
temporarily disables errexit, runs eval $cmd, captures the exit code, then
re-enables errexit and returns that exit code (e.g., use set +e before eval,
rc=$? after, set -e to restore, and return or echo rc). Update callers that rely
on run_newman's outcome to check that returned exit code and print the "✓ All
tests passed" / "✗ Some tests failed" messages and report listing based on the
captured rc.
- Around line 1-6: The ENVIRONMENT variable check and fallback logic
(ENVIRONMENT and the lines referencing bifrost-v1.postman_environment.json) are
unused because the script always uses provider-specific env files from
provider_config/; either remove the ENVIRONMENT variable definition and the
conditional fallback block entirely from run-newman-tests.sh to simplify the
script, or add the missing bifrost-v1.postman_environment.json (copied/synced
from docs/openapi/) and ensure ENVIRONMENT is actually referenced when selecting
the Newman environment so the fallback becomes meaningful.
🧹 Nitpick comments (8)
tests/e2e/api/README.md (1)
55-63: Manual sync fromdocs/openapi/is a drift risk.The manual copy-and-refresh workflow between
docs/openapi/andtests/e2e/api/will inevitably lead to the two copies diverging. Consider either:
- Using symlinks or a CI check that diffs the two directories, or
- Keeping only one canonical location and pointing the test runner at it.
This isn't blocking, but worth tracking to avoid stale tests down the line.
tests/e2e/api/bifrost-v1-complete.postman_collection.json (3)
7-28: Test assertions only validate status codes — no response body or schema validation.The global test script treats any 2xx or
unsupported_operationerror as a pass. This means the e2e suite can't catch regressions in response structure, missing fields, incorrect types, or data corruption — it only verifies the server returns something without crashing. Consider adding per-endpoint assertions for at least the most critical paths (e.g., verifyingchoicesarray in chat completions,dataarray in embeddings,idfield on created resources).
514-523: Missingtry/catcharoundpm.response.json()— inconsistent with Upload File script.The "Upload File" test script (line 421) wraps
pm.response.json()in atry/catch, but this "Create Batch (Inline Requests)" test script does not. If the 2xx response isn't valid JSON (e.g., empty body), this will throw an unhandled exception in the test sandbox.Proposed fix
"exec": [ "var code = pm.response.code;", "if (code >= 200 && code <= 299) {", + " try {", " var j = pm.response.json();", " if (j && j.id) { pm.collectionVariables.set('batch_id', j.id); if (pm.environment) { pm.environment.set('batch_id', j.id); } }", + " } catch (e) {}", "}"
87-105: No tests for streaming responses.All chat/text completion requests explicitly set
"stream": false. Streaming is a critical code path for LLM providers and a common source of regressions. Consider adding at least one streaming variant.tests/e2e/api/run-newman-tests.sh (4)
155-183:eval $cmdis fragile — use an array to build the command instead.Building a command as a single string and
eval-ing it is error-prone: variables with spaces, quotes, or shell metacharacters (e.g., a folder name likeChat Completions) rely on careful manual escaping. An array-based approach is more robust and idiomatic.Sketch of array-based approach
run_newman() { - local cmd="newman run $COLLECTION" - if [ -n "${2:-}" ] && [ -f "${2}" ]; then - cmd="$cmd -e ${2}" - else + local cmd=(newman run "$COLLECTION") + if [ -n "${2:-}" ] && [ -f "${2}" ]; then + cmd+=(-e "${2}") + else local base_url="${BIFROST_BASE_URL:-http://localhost:8080}" # ... (same local vars) ... if [ -n "$ENV_FLAG" ]; then - cmd="$cmd $ENV_FLAG" + cmd+=(-e "$ENVIRONMENT") fi - cmd="$cmd --env-var \"base_url=$base_url\" ..." + cmd+=(--env-var "base_url=$base_url" --env-var "provider=$provider" ...) fi - [ -n "$FOLDER" ] && cmd="$cmd $FOLDER" + [ -n "$FOLDER_NAME" ] && cmd+=(--folder "$FOLDER_NAME") # ... build rest of array ... - eval $cmd + "${cmd[@]}" }This also means
FOLDERshould store only the name (not the flag), andENV_FLAGcan be replaced by a boolean check.
88-91:FOLDERembeds escaped quotes foreval— this breaks if the folder name itself contains quotes or special characters.
FOLDER="--folder \"$2\""is a workaround forevalstring-based command execution. With an array-based approach (recommended above), this would simply be:- FOLDER="--folder \"$2\"" + FOLDER_NAME="$2"
280-344: ANSI cursor-repositioning won't render correctly in CI/non-interactive terminals.
printf '\033[%dA'(cursor up) on line 338 produces garbage in CI logs (GitHub Actions, Jenkins, etc.). Consider detecting[ -t 1 ](stdout is a terminal) and falling back to simple append-only output when non-interactive.
295-304: No timeout for parallel provider runs — a hanging provider blocks the script indefinitely.If a single provider's newman run hangs (e.g., server is unresponsive), the polling loop on line 322 will never complete. Consider adding a
timeoutwrapper around each subshell or a wall-clock deadline in the polling loop.Example using `timeout`
- ( run_newman "$name" "$jsonfile" ) > "$logfile" 2>&1 & + ( timeout 600 bash -c "$(declare -f run_newman); run_newman \"$name\" \"$jsonfile\"" ) > "$logfile" 2>&1 &Or more cleanly, just wrap the newman invocation inside
run_newmanitself with a configurableTIMEOUTvariable.
tests/e2e/api/provider_config/bifrost-v1-anthropic.postman_environment.json
Show resolved
Hide resolved
tests/e2e/api/provider_config/bifrost-v1-huggingface.postman_environment.json
Outdated
Show resolved
Hide resolved
tests/e2e/api/provider_config/bifrost-v1-nebius.postman_environment.json
Outdated
Show resolved
Hide resolved
tests/e2e/api/provider_config/bifrost-v1-openrouter.postman_environment.json
Outdated
Show resolved
Hide resolved
tests/e2e/api/provider_config/bifrost-v1-parasail.postman_environment.json
Outdated
Show resolved
Hide resolved
Radheshg04
force-pushed
the
graphite-base/1564
branch
from
fa16b7f to
b7ce786
Compare
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
from
e221f78 to
c0fb384
Compare
Radheshg04
changed the base branch from
graphite-base/1564
to
02-03-feat_added_e2e_tests
There was a problem hiding this comment.
Actionable comments posted: 16
🤖 Fix all issues with AI agents
In `@tests/e2e/api/bifrost-anthropic-integration.postman_collection.json`:
- Around line 79-105: The test collection uses fallback variables "batch_id" and
"file_id" which mask failures from the Create Batch and Upload File requests;
update the Create Batch and Upload File request test scripts to assert and set
pm.environment/pm.collectionVariables only on success and add pre-request or
test-time guards in dependent requests (Retrieve Batch, Cancel Batch, Get File
Content, Delete File) that verify the existence and non-default values of
"batch_id" and "file_id" (fail the test with pm.test or skip the downstream
request via postman.setNextRequest when missing), and tighten the global test
regex so it does not silently swallow these specific "not found" errors. Ensure
the checks reference the exact variable names "batch_id" and "file_id" and the
request names Create Batch and Upload File so failures are surfaced rather than
using the hardcoded fallbacks "batch_123" and "file_123".
- Around line 58-74: The global test is too permissive: tighten the acceptance
logic in the script that defines pass (variables: pass, code, errCode, errType,
msg) so only legitimately expected server-side unsupported errors are
allowed—remove the errType === 'invalid_request_error' branch and drop broad
regex matches; instead accept only errCode === 'unsupported_operation' or
explicit HTTP 405/method-not-allowed cases (keep the existing special-casing of
code === 405 if you want) and replace the big regex with a narrow pattern that
only matches true "not implemented/unsupported" messages (e.g., "not
implemented|unsupported operation|method not allowed"); update the pm.test
description ('Status is 2xx or unsupported operation') if needed to reflect the
narrower criteria.
In `@tests/e2e/api/bifrost-api-management.postman_collection.json`:
- Around line 113-117: The collection currently hardcodes a developer-local
absolute path in the "plugin_path" value which will break on other machines;
update the "plugin_path" entry in
tests/e2e/api/bifrost-api-management.postman_collection.json to use a
repository-relative path (for example
../../../examples/plugins/hello-world/build/hello-world.so) or reference a
Postman/environment variable (e.g.,
{{base_path}}/examples/plugins/hello-world/build/hello-world.so) that is set at
runtime; make sure to update any test setup or README to document the new
base_path variable if you choose that approach.
- Around line 1131-1145: Replace the hardcoded credentials in the "Login"
request body with collection/environment variables (use "{{admin_username}}" and
"{{admin_password}}" instead of "admin" and "password") and add corresponding
collection variables named "admin_username" (default "admin") and
"admin_password" (default empty) so CI or local env files can supply real
secrets; update the POST request body in the Login request to reference those
variables and ensure the collection's variable list includes keys
"admin_username" and "admin_password".
In `@tests/e2e/api/bifrost-bedrock-integration.postman_collection.json`:
- Around line 58-73: The current global test script sets a permissive pass flag
using a broad regex inside the block that inspects pm.response.json() (the code
that sets and mutates the variable pass and the regex check used to auto-allow
many error messages), which hides real failures; update the logic so you
explicitly whitelist known unsupported error codes/messages (e.g., only
'unsupported_operation' and a small list of exact msg strings or exact errType
values) instead of the broad regex, set a distinct flag like
unsupportedError=true when a response matches that precise whitelist, and then
change the final assertion (the pm.test using pm.expect(pass).to.be.true) to
fail on unexpected errors while reporting unsupportedError responses as
skips/warnings (e.g., use a separate pm.test or console.warn to mark skipped
cases) so real unexpected errors are not swallowed.
In `@tests/e2e/api/bifrost-composite-integrations.postman_collection.json`:
- Around line 58-74: The postman test currently marks many responses as passing
via the broad match on msg, errCode and errType (variables: pass, msg, errCode,
errType and the final pm.test 'Status is 2xx or unsupported operation'); narrow
this by replacing the permissive regex and the blanket acceptance of errType ===
'invalid_request_error' with a strict whitelist of known unsupported error
codes/types (e.g., ['unsupported_operation', ...]) or specific exact messages,
or split the check into two separate tests (one "must-pass" that requires
2xx/405 for core endpoints and a second "best-effort" that allows only a small,
explicit set of supported-provider errors), and update the pm.test assertion
accordingly so only truly expected unsupported responses are treated as pass.
In `@tests/e2e/api/bifrost-openai-integration.postman_collection.json`:
- Around line 1149-1177: The "Create Container File (Reference)" request uses
the variable {{file_id}} but that file was already removed in the Files section
(the "Delete File" step), causing the container reference creation to fail; fix
by ensuring a valid file exists before this request—either insert/re-run the
file upload step that sets {{file_id}} immediately before the "Create Container
File (Reference)" request (the POST to
openai/v1/containers/{{container_id}}/files), or move the "Delete File" step to
the end of the collection so {{file_id}} remains valid when referenced.
- Around line 976-1014: In the Postman collection's Files section the "Delete
File" request is placed before "Get File Content", causing the content retrieval
to run after deletion; update the item array so the "Get File Content" entry
appears before the "Delete File" entry (i.e., swap the positions of the "Get
File Content" and "Delete File" items) to ensure the GET runs prior to the
DELETE.
In `@tests/e2e/api/bifrost-v1-complete.postman_collection.json`:
- Around line 13-46: The global Postman test is too permissive: tighten the
logic around the `pass` variable and the `pm.test('Status is 2xx or unsupported
operation', ...)` assertion by (1) removing the unconditional pass for 500
responses (drop the `else if (code == 500 ... ) { pass = true; }` branch and
treat 5xx as failures), (2) narrowing/removing overly generic regex tokens in
the error-message check (remove terms like "file", "provider", "configuration",
"bad request", "invalid" and instead whitelist specific, known unsupported
message patterns), (3) restrict the 400/other-code auto-pass rules to only very
specific request types and known error codes (keep the explicit list like
401/402/404/405/409/422 only if truly intentional), and (4) split the single
`pm.test` into two assertions: one strict test that requires 2xx for requests
expected to succeed and a separate "may be unsupported" test that only applies
when a request is explicitly allowed to return a documented unsupported error;
update uses of `reqName`, `isBatchReq`, `isAudioReq`, `isImageReq`,
`isEmbeddingReq`, `isCompletionReq`, and the `msg/errCode/errType` checks
accordingly.
In `@tests/e2e/api/run-all-integration-tests.sh`:
- Around line 56-63: The orchestrator forwards the full ARGS array to every
child in TEST_SCRIPTS which breaks run-newman-api-tests.sh when it receives
provider flags like --env; update the invocation logic so that before calling
run-newman-api-tests.sh you create a filtered args array that removes any
occurrence of --env and its following value (e.g., iterate ARGS, skip "--env"
and the next element), then call run-newman-api-tests.sh with the filtered array
instead of "${ARGS[@]}"; keep other scripts unchanged and preserve quoting/array
semantics when passing arguments.
In `@tests/e2e/api/run-newman-anthropic-integration.sh`:
- Around line 3-4: Update the top-of-file header comment in
run-newman-anthropic-integration.sh: replace the incorrect reference "OpenAI"
with "Anthropic" so the second comment line correctly reads that this script
runs the Anthropic integration API test suite using Newman; locate the file
header comment (the two-line description at the top) and make the single-word
correction.
In `@tests/e2e/api/run-newman-api-tests.sh`:
- Line 6: The script uses `set -e` but not `pipefail`, so failures from the
`eval $CMD | tee ...` pipeline (where `eval $CMD` is the Newman run) can be
masked by `tee`; add `set -o pipefail` near the top next to the existing `set
-e` and keep the existing immediate use of `PIPESTATUS[0]` after the pipeline to
capture the real exit code of `eval $CMD`. This ensures the pipeline returns a
non-zero status on Newman failures and that `PIPESTATUS` reflects the correct
command exit code.
In `@tests/e2e/api/run-newman-bedrock-integration.sh`:
- Around line 3-4: Update the top-of-file comment that incorrectly mentions
"OpenAI" to refer to "Bedrock" instead; locate the header lines in the script
(the two comment lines containing "Bifrost Bedrock Integration API Newman Test
Runner" and the second line that currently reads "This script runs the OpenAI
integration API test suite using Newman") and change the second comment to
accurately read that the script runs the Bedrock integration API test suite
using Newman.
In `@tests/e2e/api/run-newman-composite-integration.sh`:
- Around line 1-466: The PR duplicates ~450 lines across five scripts; extract
shared logic into a sourced library to avoid repeated maintenance: create a new
_newman-lib.sh containing the common functions and logic (run_newman,
post_process_log, print_provider_report, draw_table, parallel execution and arg
parsing core) and update each runner script to only set per-script config
variables (COLLECTION, REPORT_DIR, BANNER, VERBOSE, PROVIDER_CONFIG_DIR if
needed) and then source _newman-lib.sh; ensure exported/local variable handling
matches (e.g., FOLDER, VERBOSE, REPORTERS) so run_newman and post_process_log
work unchanged and preserve existing behavior for single-provider vs
all-provider flows.
- Around line 3-4: Update the header comment in the script so it correctly
describes the test runner: replace the phrase "OpenAI integration API test
suite" with "Composite integration API test suite" in the script's top comment
(the second comment line in tests/e2e/api/run-newman-composite-integration.sh)
to remove the copy-paste error and accurately reflect this is the Composite
integration Newman runner.
In `@tests/e2e/api/run-newman-tests.sh`:
- Line 162: The default model in the script uses local
model="${BIFROST_MODEL:-gpt-4}" and the help text still references "gpt-4",
causing inconsistency with other integration scripts that default to gpt-4o;
change the fallback value to gpt-4o (local model="${BIFROST_MODEL:-gpt-4o}") and
update the help text string that mentions "gpt-4" to "gpt-4o" so the script and
its usage message match the other runner scripts.
🧹 Nitpick comments (6)
tests/e2e/api/bifrost-composite-integrations.postman_collection.json (1)
290-306: Hardcoded model identifiers will rot when models are deprecated.The LiteLLM "Messages (Anthropic Routing)" request hardcodes
anthropic/claude-sonnet-4-5-20250929instead of using{{provider}}/{{model}}variables. The same pattern repeats in LangChain (line 439) and PydanticAI (line 586) Anthropic routing requests, and Bedrock Converse requests hardcodeus.anthropic.claude-3-5-sonnet-20241022-v2:0(lines 323, 470, 617).These should either be parameterized via environment variables (e.g.,
anthropic_model,bedrock_model) or documented with a comment explaining why they are intentionally pinned.tests/e2e/api/bifrost-bedrock-integration.postman_collection.json (1)
233-331: Batch job tests depend on a validjob_arnfrom "Create Batch Job", but the flow continues regardless of creation failure.If "Create Batch Job" fails (returns an error), the
job_arnvariable retains its placeholder valuearn:aws:bedrock:us-east-1:123456789012:model-invocation-job/abc123. Subsequent "Retrieve Batch Job" and "Stop Batch Job" requests will then hit the API with this fake ARN, and any resulting error will likely be swallowed by the global error regex. This reduces test effectiveness to near-zero for the batch job flow when creation doesn't succeed.Consider adding a pre-request script to "Retrieve Batch Job" and "Stop Batch Job" that skips them (via
postman.setNextRequest) ifjob_arnstill holds the placeholder value, similar to the pattern used forfile_idin the V1 complete collection.tests/e2e/api/run-newman-api-tests.sh (2)
107-125:eval $CMDis fragile — prefer an array-based command construction.Building the command as a string and using
evalis error-prone if any argument contains spaces, quotes, or special characters (e.g., file paths with spaces in report directories). Use a Bash array instead.♻️ Proposed refactor
-# Build Newman command -CMD="newman run $COLLECTION" -CMD="$CMD -r $REPORTERS" - -if [[ "$REPORTERS" == *"html"* ]]; then - CMD="$CMD --reporter-html-export $REPORT_DIR/report.html" -fi - -if [[ "$REPORTERS" == *"json"* ]]; then - CMD="$CMD --reporter-json-export $REPORT_DIR/report.json" -fi - -[ -n "$VERBOSE" ] && CMD="$CMD $VERBOSE" -[ -n "$BAIL" ] && CMD="$CMD $BAIL" - -# Run Newman and save output to log file while displaying to console (using tee) -LOG_FILE="$LOG_DIR/api-management.log" -eval $CMD 2>&1 | tee "$LOG_FILE" -EXIT_CODE=${PIPESTATUS[0]} +# Build Newman command +CMD=(newman run "$COLLECTION" -r "$REPORTERS") + +if [[ "$REPORTERS" == *"html"* ]]; then + CMD+=(--reporter-html-export "$REPORT_DIR/report.html") +fi + +if [[ "$REPORTERS" == *"json"* ]]; then + CMD+=(--reporter-json-export "$REPORT_DIR/report.json") +fi + +[ -n "$VERBOSE" ] && CMD+=("$VERBOSE") +[ -n "$BAIL" ] && CMD+=("$BAIL") + +# Run Newman and save output to log file while displaying to console (using tee) +LOG_FILE="$LOG_DIR/api-management.log" +"${CMD[@]}" 2>&1 | tee "$LOG_FILE" +EXIT_CODE=${PIPESTATUS[0]}
37-40: Unnecessaryparallel_logsdirectory — likely copy-paste artifact.This script runs a single Newman invocation, not parallel jobs. The
parallel_logssubdirectory name is misleading. Consider using just$REPORT_DIRfor the log file, or renaming the directory tologs.tests/e2e/api/run-all-integration-tests.sh (1)
1-6:set -emay cause premature exit despite theifguard.The
if "./$script" "${ARGS[@]}"on line 93 correctly suppressesset -efor the child process. However, if any other command between theifblocks fails unexpectedly (e.g.,echoto a broken pipe), the script will abort without printing the summary. Consider usingset +eat the top (since you're already manually tracking exit codes) or trapping errors to ensure the summary always prints.tests/e2e/api/run-newman-composite-integration.sh (1)
155-184:eval $cmdis fragile — prefer an array-based command construction.Building a command string and using
evalis error-prone with special characters in paths or variable values, and makes debugging harder. Consider building the Newman command as an array:Sketch of array-based approach
run_newman() { local cmd=(newman run "$COLLECTION") if [ -n "${2:-}" ] && [ -f "${2}" ]; then cmd+=(-e "${2}") else # ... env var setup ... [ -n "$ENV_FLAG" ] && cmd+=($ENV_FLAG) cmd+=(--env-var "base_url=$base_url" --env-var "provider=$provider" ...) fi [ -n "$FOLDER" ] && cmd+=(--folder "$FOLDER_NAME") cmd+=(-r "$REPORTERS") # ... reporter flags ... [ -n "$VERBOSE" ] && cmd+=(--verbose) [ -n "$BAIL" ] && cmd+=(--bail) "${cmd[@]}" }This also eliminates the need for
evalentirely and avoids theset -einteraction issue since the command runs directly.
tests/e2e/api/bifrost-bedrock-integration.postman_collection.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@tests/e2e/api/bifrost-api-management.postman_collection.json`:
- Around line 1131-1145: The "Login" POST request in the collection currently
hardcodes credentials in the request body ("username": "admin", "password":
"password"); replace those literal values with collection/environment variables
(e.g., {{test_username}} and {{test_password}}) in the request body for the
"Login" item and ensure corresponding variables are added to the collection or
environment so tests can run against different environments without leaking
defaults. Update the "Login" request's raw body to reference those variables and
add/describe the variables in the collection/environment variables list.
- Around line 51-62: The current postman test logic (vars code, pass, msg and
the large regex) is too permissive and treats common real errors like "not
found", "invalid" or "failed" as acceptable; update the check to only accept
explicit unsupported-operation conditions by either (A) testing for a specific
machine-readable error code (e.g., body.error.code === 'unsupported_operation'
or body.error.code === 'feature_not_enabled') before setting pass = true, or (B)
if you must match text, tighten the regex used on msg to only match precise
phrases such as /\b(not supported|unsupported|not enabled|not configured)\b/i
and remove general tokens like not found, invalid, failed, cannot find, does not
exist, no data, empty; ensure the code uses body.error.code when available and
falls back to the narrowed regex only as a last resort.
- Around line 164-200: The "Update Config" item's prerequest script is nested
inside the request object so Newman/Postman will ignore it; move the event block
(the prerequest script that sets pm.request.body.raw with the explicit body) out
of the "request" object and place it as a sibling to "request" at the item level
so the prerequest event runs correctly.
- Around line 113-117: The postman collection has a hardcoded absolute path in
the JSON key "plugin_path" which leaks a developer-specific filesystem and will
break tests for others; change the "plugin_path" value to a portable placeholder
or relative path (e.g., use a Postman variable like "{{PLUGIN_PATH}}" or a
relative path such as "./examples/plugins/hello-world/build/hello-world.so") and
document that consumers must set PLUGIN_PATH in their environment or collection
variables; ensure any CI/test configs reference the same variable so the path is
not committed as an absolute local path.
In `@tests/e2e/api/bifrost-composite-integrations.postman_collection.json`:
- Around line 58-74: The test's pass-through logic (vars code, pass, msg,
errCode, errType) is too permissive and masks real failures; update the
condition inside the post-response script to only accept explicit
unsupported-capability signals: allow errCode === 'unsupported_operation' and
narrow the regex on msg to exact phrases like /not
supported|unsupported|batch.*api|tool.*not.*found/i while removing broad
patterns such as /not.*found|does.*not.*exist|not.*implemented/ and stop
treating any errType === 'invalid_request_error' as acceptable; ensure pm.test
still asserts pass but only when these tighter checks match.
In `@tests/e2e/api/run-newman-anthropic-integration.sh`:
- Around line 3-4: The header comment at the top of the script incorrectly
references "OpenAI"; update the comment text in
tests/e2e/api/run-newman-anthropic-integration.sh so it correctly says
"Anthropic" (e.g., change "This script runs the OpenAI integration API test
suite using Newman" to mention the Anthropic integration runner) to avoid the
copy-paste error and keep the file description accurate.
- Around line 1-466: The script duplicates a large shared runner; extract the
common logic (argument parsing, run_newman(), post_process_log(), draw_table(),
parallel/single provider flows, PROVIDER_CONFIG_DIR handling,
REPORTERS/VERBOSE/BAIL handling) into a single parameterized script (e.g.,
run-newman-integration.sh) that accepts flags like --collection, --report-dir,
--default-provider, --default-model and honors existing env overrides
(BIFROST_*); then convert this file's hard-coded COLLECTION, REPORT_DIR, banner
text and default provider/model constants into parameters and remove duplicated
blocks here, and replace the provider-specific scripts with tiny wrappers that
simply exec the shared script with the provider-specific
--collection/--report-dir/--default-provider/--default-model plus "$@". Ensure
functions referenced (run_newman, post_process_log, draw_table) are moved
unchanged into the shared script and that this script still supports providing
either a direct env file or a provider name as before.
In `@tests/e2e/api/run-newman-bedrock-integration.sh`:
- Line 4: Update the top-of-file comment in the
run-newman-bedrock-integration.sh script: replace the phrase "OpenAI integration
API test suite" with "Bedrock integration API test suite" so the header
accurately reflects this Bedrock runner; locate the file's initial comment line
(the one reading "This script runs the OpenAI integration API test suite using
Newman") and change "OpenAI" to "Bedrock".
- Around line 155-184: The run_newman() function uses OpenAI defaults when no
env file is provided; change the default provider and model variables (currently
set in run_newman as provider and model) from openai/gpt-4o to bedrock and the
Bedrock model id (e.g., us.anthropic.claude-3-5-sonnet-20241022-v2:0) so the
Bedrock integration runner matches the collection's expected defaults; update
the local variables provider and model inside run_newman() accordingly and keep
the other default vars (embedding_model, speech_model, etc.) unchanged.
- Line 6: The script currently uses set -e which causes the whole script to
abort when run_newman returns non-zero, skipping cleanup and summary; change the
two places where run_newman writes to "$TEMP_LOG" so that you temporarily
disable error-exit before invoking run_newman, capture its exit status into
EXIT_CODE immediately after the command, then restore the original set -e
behavior (or simply avoid set -e entirely); specifically update the run_newman
invocations that redirect to "$TEMP_LOG" to ensure you run: disable error-exit,
call run_newman > "$TEMP_LOG" 2>&1, set EXIT_CODE=$?, and then re-enable
error-exit (or remove set -e) so cleanup and summary always run.
In `@tests/e2e/api/run-newman-composite-integration.sh`:
- Line 6: The script tests/e2e/api/run-newman-composite-integration.sh sets "set
-e" which causes the whole script or subshells to abort on Newman non-zero exits
and prevents EXIT_CODE capture, post_process_log and cleanup; remove "set -e"
from the top of the script or, if you must keep it, wrap each Newman invocation
(the run_newman calls in the single-provider path, default-provider path, and
the parallel subshell) with explicit error handling so you capture the exit code
(e.g., run_newman ... || true; EXIT_CODE=$? immediately after) and then always
call post_process_log and rm -f .tmp regardless of the command result; locate
and fix the run_newman call sites and ensure post_process_log and cleanup always
run.
- Around line 3-4: Update the incorrect script description line that currently
reads "This script runs the OpenAI integration API test suite using Newman" to
reference the composite integrations runner instead; change that second header
comment to something like "This script runs the Composite Integrations API test
suite using Newman" so it matches the first line "# Bifrost Composite
Integrations API Newman Test Runner" and avoids the OpenAI copy-paste error.
In `@tests/e2e/api/run-newman-openai-integration.sh`:
- Line 6: The script currently uses "set -e" which causes the whole test runner
to abort when Newman returns non-zero; remove or disable the global "set -e" and
instead handle Newman exit codes explicitly: locate the "set -e" declaration and
the three Newman invocation sites (the single-provider invocation, the
default-provider invocation, and the parallel subshells invocation where "newman
run ..." is called) and either (a) append guarded handling like capturing the
exit code ($?) and deciding pass/fail without exiting the script, or (b) wrap
each newman call with a conditional (e.g., "newman run ... || true" and then
inspect the exit code) so test failures don't abort the whole script; ensure you
implement consistent error-status aggregation/reporting after those Newman calls
so the script can report failures but continue through all execution paths.
🧹 Nitpick comments (9)
tests/e2e/api/run-newman-tests.sh (1)
255-278: Newman output parsing is fragile but acceptable for this use case.Line 263 parses the Newman CLI summary table by splitting on
│— this will break if Newman changes its output format. Line 265 greps for Unicode✗. Both are reasonable for a test runner, just worth noting as maintenance risk.tests/e2e/api/run-newman-api-tests.sh (3)
6-6: Addset -o pipefailalongsideset -e.Without
pipefail,set -ewon't trigger on Newman failures in the pipeline at line 124 (eval $CMD 2>&1 | tee ...). WhilePIPESTATUS[0]is correctly used for the final exit code, any commands between the pipe and the explicitexitcould still execute unexpectedly after a failure.Proposed fix
-set -e +set -euo pipefail
124-125: Prefer an array overevalfor command construction.Building a command as a string and using
evalis fragile — spaces or special characters in paths can cause breakage. Using a Bash array is safer and more idiomatic.Proposed refactor
-# Build Newman command -CMD="newman run $COLLECTION" -CMD="$CMD -r $REPORTERS" - -if [[ "$REPORTERS" == *"html"* ]]; then - CMD="$CMD --reporter-html-export $REPORT_DIR/report.html" -fi - -if [[ "$REPORTERS" == *"json"* ]]; then - CMD="$CMD --reporter-json-export $REPORT_DIR/report.json" -fi - -[ -n "$VERBOSE" ] && CMD="$CMD $VERBOSE" -[ -n "$BAIL" ] && CMD="$CMD $BAIL" - -# Run Newman and save output to log file while displaying to console (using tee) -LOG_FILE="$LOG_DIR/api-management.log" -eval $CMD 2>&1 | tee "$LOG_FILE" -EXIT_CODE=${PIPESTATUS[0]} +# Build Newman command as array +CMD=(newman run "$COLLECTION" -r "$REPORTERS") + +if [[ "$REPORTERS" == *"html"* ]]; then + CMD+=(--reporter-html-export "$REPORT_DIR/report.html") +fi + +if [[ "$REPORTERS" == *"json"* ]]; then + CMD+=(--reporter-json-export "$REPORT_DIR/report.json") +fi + +[ -n "$VERBOSE" ] && CMD+=("$VERBOSE") +[ -n "$BAIL" ] && CMD+=("$BAIL") + +# Run Newman and save output to log file while displaying to console (using tee) +LOG_FILE="$LOG_DIR/api-management.log" +"${CMD[@]}" 2>&1 | tee "$LOG_FILE" +EXIT_CODE=${PIPESTATUS[0]}
39-40: Misleading directory nameparallel_logsfor a single-provider, non-parallel runner.This script runs a single Newman invocation, so naming the log directory
parallel_logsis confusing. Consider using justlogsor renaming to match the actual usage.tests/e2e/api/bifrost-composite-integrations.postman_collection.json (1)
290-293: Hardcoded Anthropic model in routing tests — intentional cross-provider routing, but may break if the model is deprecated.
anthropic/claude-sonnet-4-5-20250929is hardcoded in LiteLLM, LangChain, and PydanticAI Anthropic routing tests (lines 292, 439, 586). While this is understandable for cross-provider routing, the dated model identifier will eventually become invalid. Consider extracting this into a collection variable (e.g.,anthropic_model) for easier maintenance.tests/e2e/api/run-newman-bedrock-integration.sh (1)
183-183:eval $cmdis fragile with user-supplied values.Building the command as a string and using
evalcan break or behave unexpectedly if any variable (e.g., folder name, base URL) contains spaces or shell metacharacters. Consider using arrays to build the command instead. This applies to all runner scripts in the PR.tests/e2e/api/run-newman-composite-integration.sh (2)
362-385:print_provider_report()is defined but never called.This function is dead code. Either integrate it into the final summary (e.g., after the parallel loop completes) or remove it to avoid confusion.
155-184:run_newman()is duplicated verbatim across multiple scripts.This function is identical to the one in
run-newman-openai-integration.sh(this PR) andrun-newman-tests.sh(from the relevant snippets). Consider extracting it into a shared library file (e.g.,_newman-common.sh) that each runner sources. The same applies topost_process_log(),draw_table(),print_provider_report(), and the parallel orchestration logic — the entire file structure is copy-pasted.This would significantly reduce maintenance burden when changes are needed (e.g., the
set -efix above would need to be applied in every copy independently).tests/e2e/api/run-newman-openai-integration.sh (1)
362-385:print_provider_report()is defined but never called — same as in the composite script.Remove or wire it into the summary output.
tests/e2e/api/bifrost-composite-integrations.postman_collection.json
Outdated
Show resolved
Hide resolved
akshaydeo
changed the base branch from
02-03-feat_added_e2e_tests
to
graphite-base/1564
Radheshg04
force-pushed
the
graphite-base/1564
branch
from
b7ce786 to
3761626
Compare
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
from
c0fb384 to
da25de7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/e2e/api/bifrost-openai-integration.postman_collection.json`:
- Around line 58-74: The global Postman assertion uses a permissive regex and
blanket errType check (see variables pass, msg, errCode, errType and the big
regex containing not.*found) which causes legitimate 404s and malformed payloads
to be treated as passes; tighten this by replacing the broad
`not.*found|does.*not.*exist|cannot.*find` fragment with more specific phrases
(e.g.
`endpoint.*not.*supported|operation.*not.*supported|feature.*not.*available`),
remove or scope `errType === 'invalid_request_error'` so it only auto-passes for
known-unsupported endpoints, and consider moving permissive logic into
per-request test scripts for endpoints that may legitimately be unsupported
rather than keeping it in the global assertion that sets `pass`.
In `@tests/e2e/api/provider_config/bifrost-v1-groq.postman_environment.json`:
- Around line 7-11: Update the Postman environment values so only the supported
chat model remains set and unsupported capability fields are cleared: change the
values for embedding_model, speech_model, transcription_model, and image_model
from "llama-3.3-70b-versatile" to an empty string "" while leaving model (chat)
as "llama-3.3-70b-versatile"; update the JSON entries for the keys
"embedding_model", "speech_model", "transcription_model", and "image_model"
accordingly to reflect Groq's supported capabilities.
🧹 Nitpick comments (3)
tests/e2e/api/run-newman-tests.sh (1)
155-184:run_newmanusesevalwith string-concatenated arguments — fragile with special characters.If any environment variable value (e.g.,
BIFROST_BASE_URL) contains spaces or shell metacharacters,eval $cmdwill misinterpret them. Consider using arrays instead of string concatenation + eval, which is safer and more idiomatic in Bash.This is a lower-priority concern since the inputs are controlled, but worth noting for robustness across all runner scripts.
tests/e2e/api/run-newman-bedrock-integration.sh (1)
1-466: Massive code duplication across per-provider runner scripts — extract a shared runner.This 466-line script is nearly identical to
run-newman-openai-integration.sh(and presumably the Anthropic and other provider runners in this PR stack). The only differences areCOLLECTION,REPORT_DIR, and the banner text. This violates DRY and means every bug fix (e.g., theset -eissue) must be applied N times.Consider extracting the shared logic into a single parameterized script (e.g.,
run-newman-integration.sh) that accepts the collection name and report directory as arguments, then having thin per-provider wrappers or simply using--collection/--report-dirflags.# Example: single shared script invocation ./run-newman-integration.sh \ --collection bifrost-bedrock-integration.postman_collection.json \ --report-dir newman-reports/bedrock-integration \ --env bedrocktests/e2e/api/run-newman-openai-integration.sh (1)
1-466: Near-identical copy of the Bedrock runner — same DRY concern applies.See the refactor suggestion on
run-newman-bedrock-integration.sh. This file differs only inCOLLECTION,REPORT_DIR, and the banner. A shared parameterized script would eliminate the duplication.
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
from
da25de7 to
3c1893c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@tests/e2e/api/bifrost-anthropic-integration.postman_collection.json`:
- Around line 58-74: Update the Anthropic collection's global test that builds
the "pass" boolean (the block using pm.response.code, msg, errCode, errType and
the regex check) to match the OpenAI collection: remove the condition errType
=== 'invalid_request_error' and remove the broad regex fragments that match
not.*found, does.*not.*exist, cannot.*find, and tool.*not.*found (the
alternations inside the /.../i test); keep the remaining checks
(unsupported_operation, invalid model/tool/mcp/batch patterns, etc.) so the
pm.test('Status is 2xx or unsupported operation', ...) behaves the same as the
OpenAI collection.
In `@tests/e2e/api/bifrost-openai-integration.postman_collection.json`:
- Around line 120-139: The collection uses placeholder defaults for batch_id,
file_id, container_id, container_file_id which can mask failed create
operations; update the create-step requests ("Create Batch", "Upload File",
"Create Container") to validate the response status is 2xx and only set the
corresponding environment variables when successful, and add per-request tests
that assert the captured ID is not the placeholder string (e.g. not
"batch_123"/"file_123"/"container_123") so downstream requests are skipped or
fail fast when creates fail; alternatively, add guard tests on dependent
requests to check the env var differs from the placeholder before executing.
In `@tests/e2e/api/run-newman-anthropic-integration.sh`:
- Around line 155-170: The run_newman function uses OpenAI-specific defaults;
update the default env vars inside run_newman so
provider="${BIFROST_PROVIDER:-anthropic}" and
model="${BIFROST_MODEL:-claude-sonnet-4-5-20250929}" (and adjust any other
OpenAI-specific defaults if needed), and also update the related help text and
fallback message to reflect Anthropic defaults (references: run_newman, the
provider and model local vars, ENV_FLAG, and COLLECTION used by the script) so
the Anthropic integration runner uses the correct default provider and model
names.
- Around line 308-310: The script currently runs
run_newman("$SINGLE_PROVIDER_NAME", "$SINGLE_JSON_ENV") under set -e so any
non-zero exit aborts the script and prevents capturing EXIT_CODE and running the
summary/cleanup; wrap the call to run_newman (the lines creating TEMP_LOG and
invoking run_newman) with set +e before executing and then restore set -e after
so EXIT_CODE=$? is always reached and the later summary/report logic executes,
ensuring TEMP_LOG and EXIT_CODE are available for the summary; make the change
around the run_newman invocation referencing the TEMP_LOG variable and the
run_newman function name.
- Around line 338-340: The script currently exits prematurely because of set -e
when running run_newman for the default/no-provider path; wrap the run_newman
invocation so the shell doesn't exit on a nonzero status: temporarily do set +e
before calling run_newman > "$TEMP_LOG" 2>&1, capture EXIT_CODE=$?, and then
restore set -e after (same pattern used around the other run_newman invocation).
Make the change around the TEMP_LOG/run_newman/EXIT_CODE sequence so the
TEMP_LOG and EXIT_CODE behavior is identical to the other tested path.
🧹 Nitpick comments (4)
tests/e2e/api/bifrost-composite-integrations.postman_collection.json (1)
280-306: Hardcoded Anthropic model in routing tests — intentional but fragile.The Anthropic routing requests (LiteLLM line 292, LangChain line 439, PydanticAI line 586) hardcode
"anthropic/claude-sonnet-4-5-20250929". Similarly, Bedrock routing hardcodes"us.anthropic.claude-3-5-sonnet-20241022-v2:0". While this makes sense for testing specific provider routing paths, these model identifiers will become stale as providers deprecate models.Consider extracting these into collection variables (e.g.,
{{anthropic_routing_model}},{{bedrock_routing_model}}) so they can be updated in one place.tests/e2e/api/bifrost-openai-integration.postman_collection.json (1)
58-74: Global test assertion is tighter than the Anthropic collection but still includesinvalid.*model.The improvements here are good — removing
invalid_request_errortype matching and narrowing the regex. However,invalid.*modelon line 69 will auto-pass responses where the configured model name is wrong (e.g., a typo in the environment). This could silently mask real configuration errors.Consider narrowing to patterns that unambiguously indicate the endpoint/feature is unsupported rather than the input being invalid.
tests/e2e/api/run-newman-composite-integration.sh (1)
366-389:print_provider_reportis defined but never called.This function is dead code — the parallel execution path uses
draw_tablefor status display and inline logic for pass/fail reporting. Either integrate it into the results summary or remove it.tests/e2e/api/run-newman-openai-integration.sh (1)
366-389:print_provider_reportis defined but never called — same as in the composite script.Dead code. The parallel path uses
draw_tableand inline pass/fail logic instead.
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
from
3c1893c to
a449558
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/e2e/api/bifrost-composite-integrations.postman_collection.json`:
- Around line 292-298: The Postman collection contains hardcoded
Anthropic/Bedrock provider model strings (e.g.,
"anthropic/claude-sonnet-4-5-20250929" used in the request body for
LiteLLM/LangChain/PydanticAI Anthropic Routing and Bedrock Routing requests)
which will fail when the test environment lacks those provider credentials;
either document that these composite tests require all routed providers
configured, or update the test expectations by adding "not configured" / "no
config found" (or similar) to the allowedMessagePattern so
provider-not-configured errors are accepted; locate occurrences by searching for
the literal model string and the allowedMessagePattern entry in the collection
and apply the chosen change consistently across all repeated requests.
In `@tests/e2e/api/README.md`:
- Around line 127-135: Update the "Syncing from OpenAPI docs" section to either
include the integration Postman collections and runners or clarify their
canonical location: add mappings for the integration collection filenames (e.g.
bifrost-openai-integration.postman_collection.json and the other integration
collections) and their runner scripts to mirror how V1 files are listed, or add
a short note stating that those integration collections/runners are maintained
only in this tests/e2e/api directory (not in docs/openapi/) so maintainers know
not to sync them from docs/openapi/.
In `@tests/e2e/api/run-newman-composite-integration.sh`:
- Around line 155-184: The run_newman function currently builds a single command
string and uses eval, which is fragile; replace the string-based build with a
Bash array (e.g., cmd_arr=()) and append each token as a separate element
(newman, run, "$COLLECTION"), handle the optional env-file case by appending -e
"$2" when present, otherwise append each --env-var as separate elements with
values quoted (e.g., --env-var, "base_url=$base_url"), append FOLDER as its
value (not as a preformatted flag), add reporter flags and reporter-export paths
as separate elements, include optional flags like "$ENV_FLAG", "$VERBOSE",
"$BAIL" as elements only when non-empty, and execute safely with "${cmd_arr[@]}"
instead of eval; update the places referencing FOLDER so it supplies only the
folder name to the array.
🧹 Nitpick comments (4)
tests/e2e/api/bifrost-anthropic-integration.postman_collection.json (1)
300-315:setNextRequest(null)terminates the entire collection run — consider skipping to the next independent group instead.When
batch_idis missing, the prerequest guard callspostman.setNextRequest(null), which halts the entire Newman run after this request. This means the entire Files section (Upload, List, Get, Delete) is also skipped even though it doesn't depend on batch operations.If you want batch failures to be isolated from file tests, you could use
postman.setNextRequest('Upload File')(or the first item of the next folder) to skip remaining batch-dependent requests but continue with files. The current behavior is acceptable for a focused per-provider run, but worth noting for maintainability.tests/e2e/api/run-newman-openai-integration.sh (1)
6-6:set -eis still at the top — guarded at call sites but fragile.While the
set +e/set -eguards around Newman calls (lines 309–312, 341–344) andset +ein the parallel subshell (line 414) address the previously flagged issue, havingset -eat the top means any other command that fails unexpectedly (e.g.,post_process_logreturning non-zero, agrepwith no matches) will also abort the script. Consider removing the globalset -eentirely and relying on explicit error handling throughout, as the script already does for Newman calls.tests/e2e/api/run-all-integration-tests.sh (2)
74-85: The|| trueguards on arithmetic are a symptom ofset -efragility.Lines 79–80 and 84 use
((i++)) || truebecause underset -e,((0))(wheniis 0) evaluates to false and would abort the script. This is a well-knownset -efootgun. Since the child scripts are already invoked insideif(line 111) which naturally captures exit codes, you could simply dropset -efrom line 6 and remove all the|| trueguards — the script already handles errors explicitly everywhere.
56-72: TEST_SCRIPTS and TEST_NAMES arrays must be kept in sync manually.If a new test suite is added, both arrays need to be updated with matching indices. Consider using an associative array or a single array of
script:namepairs to keep them coupled.Example
declare -a TEST_ENTRIES=( "run-newman-openai-integration.sh|OpenAI Integration" "run-newman-anthropic-integration.sh|Anthropic Integration" "run-newman-bedrock-integration.sh|Bedrock Integration" "run-newman-composite-integration.sh|Composite Integrations" "run-newman-api-tests.sh|API Management & Health" ) for entry in "${TEST_ENTRIES[@]}"; do script="${entry%%|*}" name="${entry##*|}" # ... done
| "body": { | ||
| "mode": "raw", | ||
| "raw": "{\n \"model\": \"anthropic/claude-sonnet-4-5-20250929\",\n \"messages\": [\n {\n \"role\": \"user\",\n \"content\": \"Hello, how are you?\"\n }\n ],\n \"max_tokens\": 1024\n}" | ||
| }, | ||
| "url": { | ||
| "raw": "{{base_url}}/litellm/anthropic/v1/messages", | ||
| "host": [ |
There was a problem hiding this comment.
Hardcoded anthropic/claude-sonnet-4-5-20250929 won't be covered by the environment's provider variable.
The LiteLLM, LangChain, and PydanticAI "Anthropic Routing" and "Bedrock Routing" requests use hardcoded provider/model strings (repeated at lines 294, 325, 441, 472, 588, 619). This is intentional for testing specific routing paths, but if the Bifrost instance under test doesn't have Anthropic or Bedrock credentials configured, these requests will fail with provider-not-configured errors that don't match the allowedMessagePattern (which only covers not supported, unsupported operation/feature, method not allowed, not implemented).
If the intent is that these composite tests require all routed providers to be configured, consider documenting that prerequisite. If some providers may be absent, add not configured or no config found to allowedMessagePattern to match the Anthropic integration collection's handling.
🤖 Prompt for AI Agents
In `@tests/e2e/api/bifrost-composite-integrations.postman_collection.json` around
lines 292 - 298, The Postman collection contains hardcoded Anthropic/Bedrock
provider model strings (e.g., "anthropic/claude-sonnet-4-5-20250929" used in the
request body for LiteLLM/LangChain/PydanticAI Anthropic Routing and Bedrock
Routing requests) which will fail when the test environment lacks those provider
credentials; either document that these composite tests require all routed
providers configured, or update the test expectations by adding "not configured"
/ "no config found" (or similar) to the allowedMessagePattern so
provider-not-configured errors are accepted; locate occurrences by searching for
the literal model string and the allowedMessagePattern entry in the collection
and apply the chosen change consistently across all repeated requests.
447aa2e to
ec335df
Compare
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
from
ec335df to
ba0eae9
Compare
Radheshg04
changed the base branch from
graphite-base/1564
to
02-11-fix_fix_embedcontent_in_genai_integration
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
2 times, most recently
from
7b5021d to
5f05d97
Compare
Radheshg04
changed the base branch from
02-11-fix_fix_embedcontent_in_genai_integration
to
graphite-base/1564
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
from
5f05d97 to
cc43469
Compare
Radheshg04
force-pushed
the
graphite-base/1564
branch
from
46fe451 to
14ce35f
Compare
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
4 times, most recently
from
d64d0fc to
51186ab
Compare
Radheshg04
force-pushed
the
02-06-feat_added_e2e_tests_for_api
branch
from
51186ab to
9aa145b
Compare
1 participant
Summary
Add end-to-end API tests for the Bifrost V1 API using Newman/Postman to validate API functionality across providers.
Changes
bifrost-v1-complete.postman_collection.json) covering all V1 endpointsrun-newman-tests.sh) that can run tests for a single provider or all providers in parallelType of change
Affected areas
How to test
Related issues
Adds automated API testing to complement existing Go-based tests.
Security considerations
Tests require a running Bifrost server with configured provider API keys.
Checklist