feat: enhance e2e API tests with DB verification and new endpoints#1899
Merged
feat: enhance e2e API tests with DB verification and new endpoints#1899
Conversation
Contributor
📝 WalkthroughWalkthroughMultiple Newman test runner scripts update timeout configurations (shorter per-script, longer overall). A comprehensive database verification reporter module is introduced with supporting infrastructure. The main E2E API test runner adds DB verification, MCP server setup, and plugin path handling. The Postman API collection is expanded with new Logs and Governance endpoints, updated provider references, and refined error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Newman as Newman Test Runner
participant API as API Server
participant Reporter as DB Verify Reporter
participant DB as Database<br/>(PostgreSQL/SQLite)
Newman->>Newman: Execute API test request
Newman->>API: Send API call (POST/GET/PUT/DELETE)
API-->>Newman: Return 2xx response + body
Newman->>Reporter: After hook: emit request complete
Reporter->>Reporter: Map endpoint to DB table(s)
Reporter->>Reporter: Extract entity ID from response
alt DB Connection Ready
Reporter->>DB: Execute verification query
DB-->>Reporter: Return query result
Reporter->>Reporter: Validate expected fields
Reporter->>Reporter: Queue verification result
else DB Connection Pending
Reporter->>Reporter: Queue verification for later
end
Reporter->>Reporter: On DB ready: drain queued verifications
Reporter->>DB: Execute queued SQL queries
DB-->>Reporter: Return results
Reporter->>Reporter: Generate ASCII summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 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 |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/run-newman-composite-integration.sh (1)
175-206:⚠️ Potential issue | 🟠 MajorAvoid
evalinrun_newman; this is injection-prone.Lines [175-206] build a shell command string from interpolated values (including
--folder) and execute it witheval, which can lead to command injection and quoting bugs.🔒 Safer array-based rewrite
run_newman() { - local cmd="newman run $COLLECTION" + local -a cmd=(newman run "$COLLECTION") if [ -n "${2:-}" ] && [ -f "${2}" ]; then - cmd="$cmd -e ${2}" + cmd+=(-e "${2}") else local base_url="${BIFROST_BASE_URL:-http://localhost:8080}" local provider="${BIFROST_PROVIDER:-openai}" local model="${BIFROST_MODEL:-gpt-4o}" local embedding_model="${BIFROST_EMBEDDING_MODEL:-text-embedding-3-small}" local speech_model="${BIFROST_SPEECH_MODEL:-tts-1}" local transcription_model="${BIFROST_TRANSCRIPTION_MODEL:-whisper-1}" local image_model="${BIFROST_IMAGE_MODEL:-dall-e-3}" if [ -n "$ENV_FLAG" ]; then - cmd="$cmd $ENV_FLAG" + cmd+=(-e "$ENVIRONMENT") fi - cmd="$cmd --env-var \"base_url=$base_url\" --env-var \"provider=$provider\" --env-var \"model=$model\" --env-var \"embedding_model=$embedding_model\" --env-var \"speech_model=$speech_model\" --env-var \"transcription_model=$transcription_model\" --env-var \"image_model=$image_model\"" + cmd+=(--env-var "base_url=$base_url" --env-var "provider=$provider" --env-var "model=$model" --env-var "embedding_model=$embedding_model" --env-var "speech_model=$speech_model" --env-var "transcription_model=$transcription_model" --env-var "image_model=$image_model") fi - [ -n "$FOLDER" ] && cmd="$cmd $FOLDER" - cmd="$cmd --timeout-script 120000 --timeout 900000 -r $REPORTERS" + [ -n "$FOLDER" ] && cmd+=(--folder "$FOLDER") + cmd+=(--timeout-script 120000 --timeout 900000 -r "$REPORTERS") if [[ "$REPORTERS" == *"html"* ]]; then - cmd="$cmd --reporter-html-export $REPORT_DIR/report_${1:-run}.html" + cmd+=(--reporter-html-export "$REPORT_DIR/report_${1:-run}.html") fi if [[ "$REPORTERS" == *"json"* ]]; then - cmd="$cmd --reporter-json-export $REPORT_DIR/report_${1:-run}.json" + cmd+=(--reporter-json-export "$REPORT_DIR/report_${1:-run}.json") fi - [ -n "$VERBOSE" ] && cmd="$cmd $VERBOSE" - [ -n "$BAIL" ] && cmd="$cmd $BAIL" + [ -n "$VERBOSE" ] && cmd+=("$VERBOSE") + [ -n "$BAIL" ] && cmd+=("$BAIL") if [ "$ci_normalized" = "1" ] || [ "$ci_normalized" = "true" ]; then - cmd="$cmd --env-var \"CI=1\"" + cmd+=(--env-var "CI=1") fi - - eval $cmd + "${cmd[@]}" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/run-newman-composite-integration.sh` around lines 175 - 206, The current run_newman code builds a single string in cmd and executes it with eval, which is injection-prone; replace this with a safely quoted array-based invocation: create an array (e.g., cmd_args) instead of the string cmd, append program name "newman" then push each argument as separate elements (use cmd_args+=( "-e" "$2" ) when a file is provided, otherwise append each --env-var as separate elements with their values quoted, and handle ENV_FLAG, FOLDER, REPORTERS, REPORT_DIR, VERBOSE, BAIL, and ci_normalized by appending appropriate elements like "--timeout-script" "120000" and reporter export flags), and finally run newman with: "${cmd_args[@]}" (no eval). Ensure you stop using eval $cmd and keep all variable expansions quoted to prevent injection.
🧹 Nitpick comments (1)
tests/e2e/api/run-newman-anthropic-integration.sh (1)
188-188: Make timeout values configurable once to reduce stack-wide drift.Line [188] hardcodes values that are duplicated across multiple runner scripts. Consider env-backed defaults so future stack changes are single-point updates.
♻️ Proposed refactor
+# Timeout configuration (override in CI if needed) +NEWMAN_TIMEOUT_SCRIPT_MS="${NEWMAN_TIMEOUT_SCRIPT_MS:-120000}" +NEWMAN_TIMEOUT_MS="${NEWMAN_TIMEOUT_MS:-900000}" + run_newman() { @@ - cmd+=(--timeout-script 120000 --timeout 900000) + cmd+=(--timeout-script "$NEWMAN_TIMEOUT_SCRIPT_MS" --timeout "$NEWMAN_TIMEOUT_MS")As per coding guidelines, "always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/run-newman-anthropic-integration.sh` at line 188, The hardcoded timeouts in the cmd array (the tokens '--timeout-script' and '--timeout') inside run-newman-anthropic-integration.sh should be replaced with env-backed defaults so all runner scripts share a single configuration point; update the script to read NEWMAN_TIMEOUT_SCRIPT and NEWMAN_TIMEOUT (or a single NEWMAN_TIMEOUTS JSON/CSV) with sensible defaults and use those variables when appending to cmd, and extract the same env defaults into a common sourced file (or CI variable) so other runner scripts reference the same values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/e2e/api/run-newman-composite-integration.sh`:
- Around line 175-206: The current run_newman code builds a single string in cmd
and executes it with eval, which is injection-prone; replace this with a safely
quoted array-based invocation: create an array (e.g., cmd_args) instead of the
string cmd, append program name "newman" then push each argument as separate
elements (use cmd_args+=( "-e" "$2" ) when a file is provided, otherwise append
each --env-var as separate elements with their values quoted, and handle
ENV_FLAG, FOLDER, REPORTERS, REPORT_DIR, VERBOSE, BAIL, and ci_normalized by
appending appropriate elements like "--timeout-script" "120000" and reporter
export flags), and finally run newman with: "${cmd_args[@]}" (no eval). Ensure
you stop using eval $cmd and keep all variable expansions quoted to prevent
injection.
---
Nitpick comments:
In `@tests/e2e/api/run-newman-anthropic-integration.sh`:
- Line 188: The hardcoded timeouts in the cmd array (the tokens
'--timeout-script' and '--timeout') inside run-newman-anthropic-integration.sh
should be replaced with env-backed defaults so all runner scripts share a single
configuration point; update the script to read NEWMAN_TIMEOUT_SCRIPT and
NEWMAN_TIMEOUT (or a single NEWMAN_TIMEOUTS JSON/CSV) with sensible defaults and
use those variables when appending to cmd, and extract the same env defaults
into a common sourced file (or CI variable) so other runner scripts reference
the same values.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/e2e/api/run-newman-anthropic-integration.shtests/e2e/api/run-newman-api-tests.shtests/e2e/api/run-newman-bedrock-integration.shtests/e2e/api/run-newman-composite-integration.shtests/e2e/api/run-newman-openai-integration.shtests/e2e/api/run-newman-tests.sh
Radheshg04
force-pushed
the
03-03-feat_added_timeout_for_api_tests
branch
from
571ae65 to
d09527d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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/bifrost-api-management.postman_collection.json (1)
284-330:⚠️ Potential issue | 🟠 MajorDon't mutate shared
openaigovernance state from this suite.The collection now creates a disposable
custom-123provider, but the provider-governance CRUD still writes to and deletes/api/governance/providers/openai. That overwrites real operator-managed config instead of test-owned state. Reuse the disposable provider here and move its delete until after this folder, or snapshot/restore the existingopenaidocument.🔧 Safer target for the provider-governance requests
- "raw": "{{base_url}}/api/governance/providers/openai", + "raw": "{{base_url}}/api/governance/providers/custom-123", "host": ["{{base_url}}"], - "path": ["api", "governance", "providers", "openai"] + "path": ["api", "governance", "providers", "custom-123"]Apply that to both the update and delete requests, and defer the earlier
Delete Providerstep until this governance cleanup is finished.Also applies to: 1155-1178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/bifrost-api-management.postman_collection.json` around lines 284 - 330, The tests are mutating the shared governance provider "openai"; change the governance CRUD requests to target the disposable provider created earlier ("custom-123") instead of "/api/governance/providers/openai" (look for requests referencing "/api/governance/providers/openai") and apply the same change to both the governance Update and Delete steps; additionally, defer the earlier "Delete Provider" request that removes "/api/providers/custom-123" until after the governance cleanup folder completes (or implement snapshot/restore of the original "openai" doc) so the suite only touches test-owned state (see "Get Provider (After Create)", "Update Provider", "Delete Provider" and their "/api/providers/custom-123" endpoints to find where to adjust ordering and URLs).
🧹 Nitpick comments (5)
tests/e2e/api/newman-reporter-dbverify/index.js (2)
769-774: Same forEach implicit return issue.Apply the same fix as above to avoid the Biome warning.
Proposed fix
.catch((e) => { logsDbReady = false; console.warn(`[dbverify] Logs DB not reachable, skipping logs DB checks: ${e.message}`); - earlyLogsDbQueue.forEach((item) => results.push({ name: item.name, result: 'SKIP', detail: 'Logs DB not connected' })); + earlyLogsDbQueue.forEach((item) => { + results.push({ name: item.name, result: 'SKIP', detail: 'Logs DB not connected' }); + }); earlyLogsDbQueue.length = 0; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/newman-reporter-dbverify/index.js` around lines 769 - 774, The forEach callback uses an implicit arrow return which triggers the Biome warning; replace the earlyLogsDbQueue.forEach((item) => results.push({ ... })) inside the catch block with an explicit loop (e.g., for (const item of earlyLogsDbQueue) { results.push({ name: item.name, result: 'SKIP', detail: 'Logs DB not connected' }); }) so there is no implicit return, then clear earlyLogsDbQueue.length = 0 as before; locate the callback in the catch block that sets logsDbReady = false and console.warn to apply this change.
752-757: Remove implicit return from forEach callbacks.The
results.push()call returns a value (the new array length), which is implicitly returned from the forEach callback. While not a bug (forEach ignores return values), this triggers static analysis warnings and is a minor code smell.Proposed fix
.catch((e) => { dbReady = false; console.warn(`[dbverify] Main DB not reachable, skipping DB checks: ${e.message}`); - earlyMainDbQueue.forEach((item) => results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected' })); + earlyMainDbQueue.forEach((item) => { + results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected' }); + }); earlyMainDbQueue.length = 0; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/newman-reporter-dbverify/index.js` around lines 752 - 757, The forEach callback in earlyMainDbQueue.forEach currently uses an expression that implicitly returns the result of results.push, triggering static-analysis warnings; change the callback to a statement body that does not return a value (e.g., use a block with results.push(...) followed by a semicolon or replace with a for/of loop) so the push is executed but nothing is returned; update the code around earlyMainDbQueue.forEach((item) => results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected' })); to a non-returning callback or explicit loop, keeping the same objects and clearing earlyMainDbQueue afterward.tests/e2e/api/run-newman-api-tests.sh (1)
193-203: Unused loop variableiflagged by shellcheck.The loop variable
iis unused. This is a minor warning but easy to fix for cleaner code.Proposed fix
# Wait up to 10 s for it to accept connections - for i in $(seq 1 10); do + for _ in $(seq 1 10); do sleep 1 if lsof -ti tcp:3001 &>/dev/null 2>&1; then echo " http-no-ping-server ready (PID $HTTP_SERVER_PID)" return 0 fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/run-newman-api-tests.sh` around lines 193 - 203, The loop uses an unused variable `i`; change the loop to use a throwaway variable (e.g. `_`) so shellcheck warning is resolved: update the loop declaration (currently `for i in $(seq 1 10); do`) to use `_` (or an equivalent unused name) and leave the body (the lsof readiness check, echo and return) unchanged; refer to the loop around the lsof readiness check in run-newman-api-tests.sh.merged-prs.md (1)
1-32: Consider this file's purpose and placement.This appears to be a personal tracking file for merged PRs by a specific author. A few observations:
- The static analysis warnings (MD018) are false positives - the
#symbols are PR references, not markdown headings.- Consider whether this file belongs in the repository root, or if it should be in a
.github/directory, a personal notes location, or excluded from version control entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merged-prs.md` around lines 1 - 32, merged-prs.md looks like a personal PR-tracking note that triggers markdownlint MD018; either relocate or exclude it and suppress the linter: move the file out of the repo root into a non-repo personal/notes location or into a .github/ or docs/ folder if it must be stored in-repo, or add it to .gitignore to keep it out of version control; if you must keep it in the repo, suppress the false positive by adding a per-file markdownlint directive (e.g., add <!-- markdownlint-disable MD018 --> at the top of merged-prs.md) or add an exclusion rule for this filename in the repository’s markdownlint configuration so the MD018 warnings stop.tests/e2e/api/bifrost-api-management.postman_collection.json (1)
63-75: Narrow the collection-wide 4xx bypass.This now runs for every request in the collection, so a mandatory management endpoint can go green if it happens to return
unsupported_operation,feature_not_enabled, or a genericnot configuredmessage. Keeping that bypass behind an explicit request/folder allowlist would preserve the portability win without weakening the new coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/bifrost-api-management.postman_collection.json` around lines 63 - 75, The current collection-level 4xx bypass unconditionally sets pass=true for responses with certain error codes/messages (code block using pm.response.json(), errCode and regex checks) which is too broad; restrict this by adding an explicit allowlist check (e.g., an array of allowed request names or folder names) and only run the existing errCode/regex logic when the current request is on that allowlist (use pm.request.name or pm.info.folderName to look up the current request and gate the existing if (!pass && code >= 400 && code < 500) { ... } block accordingly), so non-allowed requests cannot silently pass due to those error codes/messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/api/package.json`:
- Around line 6-10: The test runner skips installing runtime deps required by
the custom reporter, so update the run-newman-api-tests.sh script to install
dependencies before invoking Newman: ensure the script (run-newman-api-tests.sh)
runs a reproducible install (e.g., cd tests/e2e/api && npm ci || npm install)
prior to exporting NODE_PATH and running newman with the
newman-reporter-dbverify reporter; this guarantees the required modules pg and
better-sqlite3 are present and avoids the "[dbverify] ... not found" warnings
(alternatively, add a short README note in tests/e2e/api documenting the npm
install prerequisite).
---
Outside diff comments:
In `@tests/e2e/api/bifrost-api-management.postman_collection.json`:
- Around line 284-330: The tests are mutating the shared governance provider
"openai"; change the governance CRUD requests to target the disposable provider
created earlier ("custom-123") instead of "/api/governance/providers/openai"
(look for requests referencing "/api/governance/providers/openai") and apply the
same change to both the governance Update and Delete steps; additionally, defer
the earlier "Delete Provider" request that removes "/api/providers/custom-123"
until after the governance cleanup folder completes (or implement
snapshot/restore of the original "openai" doc) so the suite only touches
test-owned state (see "Get Provider (After Create)", "Update Provider", "Delete
Provider" and their "/api/providers/custom-123" endpoints to find where to
adjust ordering and URLs).
---
Nitpick comments:
In `@merged-prs.md`:
- Around line 1-32: merged-prs.md looks like a personal PR-tracking note that
triggers markdownlint MD018; either relocate or exclude it and suppress the
linter: move the file out of the repo root into a non-repo personal/notes
location or into a .github/ or docs/ folder if it must be stored in-repo, or add
it to .gitignore to keep it out of version control; if you must keep it in the
repo, suppress the false positive by adding a per-file markdownlint directive
(e.g., add <!-- markdownlint-disable MD018 --> at the top of merged-prs.md) or
add an exclusion rule for this filename in the repository’s markdownlint
configuration so the MD018 warnings stop.
In `@tests/e2e/api/bifrost-api-management.postman_collection.json`:
- Around line 63-75: The current collection-level 4xx bypass unconditionally
sets pass=true for responses with certain error codes/messages (code block using
pm.response.json(), errCode and regex checks) which is too broad; restrict this
by adding an explicit allowlist check (e.g., an array of allowed request names
or folder names) and only run the existing errCode/regex logic when the current
request is on that allowlist (use pm.request.name or pm.info.folderName to look
up the current request and gate the existing if (!pass && code >= 400 && code <
500) { ... } block accordingly), so non-allowed requests cannot silently pass
due to those error codes/messages.
In `@tests/e2e/api/newman-reporter-dbverify/index.js`:
- Around line 769-774: The forEach callback uses an implicit arrow return which
triggers the Biome warning; replace the earlyLogsDbQueue.forEach((item) =>
results.push({ ... })) inside the catch block with an explicit loop (e.g., for
(const item of earlyLogsDbQueue) { results.push({ name: item.name, result:
'SKIP', detail: 'Logs DB not connected' }); }) so there is no implicit return,
then clear earlyLogsDbQueue.length = 0 as before; locate the callback in the
catch block that sets logsDbReady = false and console.warn to apply this change.
- Around line 752-757: The forEach callback in earlyMainDbQueue.forEach
currently uses an expression that implicitly returns the result of results.push,
triggering static-analysis warnings; change the callback to a statement body
that does not return a value (e.g., use a block with results.push(...) followed
by a semicolon or replace with a for/of loop) so the push is executed but
nothing is returned; update the code around earlyMainDbQueue.forEach((item) =>
results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected'
})); to a non-returning callback or explicit loop, keeping the same objects and
clearing earlyMainDbQueue afterward.
In `@tests/e2e/api/run-newman-api-tests.sh`:
- Around line 193-203: The loop uses an unused variable `i`; change the loop to
use a throwaway variable (e.g. `_`) so shellcheck warning is resolved: update
the loop declaration (currently `for i in $(seq 1 10); do`) to use `_` (or an
equivalent unused name) and leave the body (the lsof readiness check, echo and
return) unchanged; refer to the loop around the lsof readiness check in
run-newman-api-tests.sh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b5d7d56-6f09-412a-9844-75b54d4a0ee1
📒 Files selected for processing (12)
merged-prs.mdtests/e2e/api/bifrost-api-management.postman_collection.jsontests/e2e/api/newman-reporter-dbverify/index.jstests/e2e/api/newman-reporter-dbverify/package.jsontests/e2e/api/package.jsontests/e2e/api/run-newman-anthropic-integration.shtests/e2e/api/run-newman-api-tests.shtests/e2e/api/run-newman-bedrock-integration.shtests/e2e/api/run-newman-composite-integration.shtests/e2e/api/run-newman-openai-integration.shtests/e2e/api/run-newman-tests.shtransports/bifrost-http/handlers/plugins.go
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/api/run-newman-openai-integration.sh
- tests/e2e/api/run-newman-anthropic-integration.sh
- tests/e2e/api/run-newman-tests.sh
- tests/e2e/api/run-newman-composite-integration.sh
Apply changes from code browser
Radheshg04
force-pushed
the
03-03-feat_added_timeout_for_api_tests
branch
from
e572635 to
a5c596f
Compare
Contributor
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/bifrost-api-management.postman_collection.json (1)
4-4:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency: description references stale provider name.
The collection description still mentions
test_provideras a fixed resource name, but the collection now usescustom-123for the provider tests (see lines 284, 299, etc.).📝 Suggested fix
- "description": "E2E tests for Bifrost management API endpoints (/api/*) and health endpoint. Uses fixed resource names (e.g. test_provider, hello-world); do not run this collection in parallel against the same server.", + "description": "E2E tests for Bifrost management API endpoints (/api/*) and health endpoint. Uses fixed resource names (e.g. custom-123, hello-world); do not run this collection in parallel against the same server.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/bifrost-api-management.postman_collection.json` at line 4, Update the collection description string so it accurately reflects the current fixed resource name used in the requests: replace the stale "test_provider" mention with the actual provider identifier "custom-123" (the description field at the top of the JSON and any other metadata description entries), and scan request bodies/variables that the description references to ensure consistency with the provider name used in requests (e.g., occurrences tied to provider tests such as the requests referencing "custom-123").
🧹 Nitpick comments (5)
tests/e2e/api/run-newman-api-tests.sh (2)
195-201: Unused loop variable (minor).The variable
iinfor i in $(seq 1 10)is unused within the loop body. This is a false positive from ShellCheck since you're just iterating, but you can use_to indicate an intentionally unused variable.♻️ Proposed fix
- for i in $(seq 1 10); do + for _ in $(seq 1 10); do sleep 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/run-newman-api-tests.sh` around lines 195 - 201, Replace the unused loop variable `i` in the for-loop in run-newman-api-tests.sh with `_` to indicate it's intentionally unused: locate the loop `for i in $(seq 1 10); do` (the block that sleeps, checks `lsof -ti tcp:3001`, echoes readiness including `HTTP_SERVER_PID` and returns) and change the loop variable to `_` so it reads `for _ in $(seq 1 10); do`.
169-204: Consider portability oflsoffor port checking.
lsofmay not be available on all systems (e.g., minimal Docker images). Consider adding a fallback usingncorcurlfor the readiness check.♻️ Alternative readiness check using curl
# Wait up to 10 s for it to accept connections - for i in $(seq 1 10); do + for _ in $(seq 1 10); do sleep 1 - if lsof -ti tcp:3001 &>/dev/null 2>&1; then + if curl -s --max-time 1 http://localhost:3001/ >/dev/null 2>&1 || lsof -ti tcp:3001 &>/dev/null 2>&1; then echo " http-no-ping-server ready (PID $HTTP_SERVER_PID)" return 0 fi done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/run-newman-api-tests.sh` around lines 169 - 204, The start_http_mcp_server function currently relies solely on lsof to check port 3001; make the checks portable by adding a fallback path when lsof is not present: detect if lsof exists and use it as now for the initial "port in use" check and the readiness loop, otherwise use a lightweight alternative such as nc -z localhost 3001 (or curl -sSf http://127.0.0.1:3001) to test connectivity; update all places referencing lsof in start_http_mcp_server (the initial skip-if-in-use block and the for-loop readiness check) to try lsof first, then fall back to nc/curl, and keep setting HTTP_SERVER_PID and using HTTP_SERVER_BIN/HTTP_SERVER_DIR as before.tests/e2e/api/newman-reporter-dbverify/index.js (3)
140-159: Consider adding connection timeout for PostgreSQL client.The
pg.Clientconnection doesn't specify a connection timeout. If the database is unreachable, this could hang indefinitely.♻️ Proposed fix to add connection timeout
if (type === 'postgres') { let pg; try { pg = require('pg'); } catch (_) { console.warn('[dbverify] pg module not found. Run: npm install in tests/e2e/api/'); return null; } - const pgClient = new pg.Client({ connectionString: url }); + const pgClient = new pg.Client({ + connectionString: url, + connectionTimeoutMillis: 10000, // 10 second timeout + }); await pgClient.connect();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/newman-reporter-dbverify/index.js` around lines 140 - 159, The createDbClient function currently constructs a new pg.Client without any timeout, so connect() can hang if the DB is unreachable; update the pg.Client instantiation in createDbClient to include a connection timeout (e.g., connectionTimeoutMillis) in the client config and optionally a query timeout (e.g., statement_timeout or query_timeout) to prevent indefinite hangs, and ensure any thrown timeout error is propagated/handled the same way as other connection errors; refer to the createDbClient function and the pg.Client constructor when making the change.
755-756: Avoid returning values from forEach callbacks.The
results.push()call returns the new array length, which is inadvertently returned from the forEach callback. While harmless, this is flagged by static analysis and can be cleaner.♻️ Proposed fix
- earlyMainDbQueue.forEach((item) => results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected' })); + for (const item of earlyMainDbQueue) { + results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected' }); + } earlyMainDbQueue.length = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/newman-reporter-dbverify/index.js` around lines 755 - 756, The forEach callback is implicitly returning the value from results.push which static analysis flags; update the block around earlyMainDbQueue and results.push to avoid returning from the callback — for example, replace earlyMainDbQueue.forEach((item) => results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected' })); with an explicit loop (e.g., for (const item of earlyMainDbQueue) { results.push({ name: item.name, result: 'SKIP', detail: 'Main DB not connected' }); }) and keep the existing earlyMainDbQueue.length = 0; so no value is returned from a callback.
772-773: Same forEach callback return issue as above.♻️ Proposed fix
- earlyLogsDbQueue.forEach((item) => results.push({ name: item.name, result: 'SKIP', detail: 'Logs DB not connected' })); + for (const item of earlyLogsDbQueue) { + results.push({ name: item.name, result: 'SKIP', detail: 'Logs DB not connected' }); + } earlyLogsDbQueue.length = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/api/newman-reporter-dbverify/index.js` around lines 772 - 773, The forEach callback on earlyLogsDbQueue is causing the same callback/return pattern problem as earlier; replace the forEach with an explicit for...of loop over earlyLogsDbQueue and push each item into results using results.push({ name: item.name, result: 'SKIP', detail: 'Logs DB not connected' }), then set earlyLogsDbQueue.length = 0 after the loop to clear it; reference earlyLogsDbQueue and results.push to locate the code to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/e2e/api/bifrost-api-management.postman_collection.json`:
- Line 4: Update the collection description string so it accurately reflects the
current fixed resource name used in the requests: replace the stale
"test_provider" mention with the actual provider identifier "custom-123" (the
description field at the top of the JSON and any other metadata description
entries), and scan request bodies/variables that the description references to
ensure consistency with the provider name used in requests (e.g., occurrences
tied to provider tests such as the requests referencing "custom-123").
---
Nitpick comments:
In `@tests/e2e/api/newman-reporter-dbverify/index.js`:
- Around line 140-159: The createDbClient function currently constructs a new
pg.Client without any timeout, so connect() can hang if the DB is unreachable;
update the pg.Client instantiation in createDbClient to include a connection
timeout (e.g., connectionTimeoutMillis) in the client config and optionally a
query timeout (e.g., statement_timeout or query_timeout) to prevent indefinite
hangs, and ensure any thrown timeout error is propagated/handled the same way as
other connection errors; refer to the createDbClient function and the pg.Client
constructor when making the change.
- Around line 755-756: The forEach callback is implicitly returning the value
from results.push which static analysis flags; update the block around
earlyMainDbQueue and results.push to avoid returning from the callback — for
example, replace earlyMainDbQueue.forEach((item) => results.push({ name:
item.name, result: 'SKIP', detail: 'Main DB not connected' })); with an explicit
loop (e.g., for (const item of earlyMainDbQueue) { results.push({ name:
item.name, result: 'SKIP', detail: 'Main DB not connected' }); }) and keep the
existing earlyMainDbQueue.length = 0; so no value is returned from a callback.
- Around line 772-773: The forEach callback on earlyLogsDbQueue is causing the
same callback/return pattern problem as earlier; replace the forEach with an
explicit for...of loop over earlyLogsDbQueue and push each item into results
using results.push({ name: item.name, result: 'SKIP', detail: 'Logs DB not
connected' }), then set earlyLogsDbQueue.length = 0 after the loop to clear it;
reference earlyLogsDbQueue and results.push to locate the code to change.
In `@tests/e2e/api/run-newman-api-tests.sh`:
- Around line 195-201: Replace the unused loop variable `i` in the for-loop in
run-newman-api-tests.sh with `_` to indicate it's intentionally unused: locate
the loop `for i in $(seq 1 10); do` (the block that sleeps, checks `lsof -ti
tcp:3001`, echoes readiness including `HTTP_SERVER_PID` and returns) and change
the loop variable to `_` so it reads `for _ in $(seq 1 10); do`.
- Around line 169-204: The start_http_mcp_server function currently relies
solely on lsof to check port 3001; make the checks portable by adding a fallback
path when lsof is not present: detect if lsof exists and use it as now for the
initial "port in use" check and the readiness loop, otherwise use a lightweight
alternative such as nc -z localhost 3001 (or curl -sSf http://127.0.0.1:3001) to
test connectivity; update all places referencing lsof in start_http_mcp_server
(the initial skip-if-in-use block and the for-loop readiness check) to try lsof
first, then fall back to nc/curl, and keep setting HTTP_SERVER_PID and using
HTTP_SERVER_BIN/HTTP_SERVER_DIR as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f30a8c3-179c-4e36-8b03-8e77156c8896
📒 Files selected for processing (11)
tests/e2e/api/bifrost-api-management.postman_collection.jsontests/e2e/api/newman-reporter-dbverify/index.jstests/e2e/api/newman-reporter-dbverify/package.jsontests/e2e/api/package.jsontests/e2e/api/run-newman-anthropic-integration.shtests/e2e/api/run-newman-api-tests.shtests/e2e/api/run-newman-bedrock-integration.shtests/e2e/api/run-newman-composite-integration.shtests/e2e/api/run-newman-openai-integration.shtests/e2e/api/run-newman-tests.shtransports/bifrost-http/handlers/plugins.go
✅ Files skipped from review due to trivial changes (2)
- tests/e2e/api/package.json
- transports/bifrost-http/handlers/plugins.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/api/run-newman-bedrock-integration.sh
- tests/e2e/api/run-newman-composite-integration.sh
- tests/e2e/api/run-newman-openai-integration.sh
- tests/e2e/api/run-newman-anthropic-integration.sh
- tests/e2e/api/newman-reporter-dbverify/package.json
Radheshg04
changed the title
|
|
Contributor
Merge activity
|
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
3 participants
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.
Changes
This pull request enhances the e2e API test infrastructure with comprehensive database verification capabilities and improved test reliability.
Database Verification Reporter
newman-reporter-dbverifycustom Newman reporter that verifies CRUD operations against PostgreSQL and SQLite databasesTest Infrastructure Improvements
--timeout-scriptfrom 300000ms to 120000ms and added--timeoutof 900000ms across all Newman test scripts for better timeout handlingPostman Collection Updates
enable_loggingconfiguration option andReconnect MCP ClientendpointConfiguration and Error Handling
The changes significantly improve test reliability and provide comprehensive database verification to ensure API operations correctly persist to the database layer.