Add e2e tests for MCP server runtime config endpoint display fix#1379
Add e2e tests for MCP server runtime config endpoint display fix#1379Saadha123 wants to merge 2 commits into
Conversation
Saadha123
requested review from
HeshanSudarshana,
Krishanx92,
PasanT9,
RakhithaRR,
ashera96,
hisanhunais,
lasanthaS,
piyumaldk and
tharikaGitHub
as code owners
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 Walkthrough📝 Walkthrough🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/cypress/e2e/publisher/024-mcp-server/00-runtime-config-endpoint-display.cy.js (1)
122-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
cy.contains('-')is a substring match, not an exact one — weakens the regression check.Cypress's
cy.contains()performs a case-sensitive substring match by default (confirmed against Cypress docs), so this assertion passes if any text node inside#endpointsmerely contains a hyphen (e.g. a URL fragment, date, or unrelated label), not necessarily the sandbox placeholder itself. Since the entire point of this regression test is to verify the sandbox section specifically renders-(not a stale URL or a crash), this loose match could pass even if the underlying rendering regresses.Recommend scoping the assertion more tightly, e.g. anchoring to the sandbox row/section and using an exact-match regex:
🔧 Proposed fix
// Sandbox section must show "-" (not crash) because it was deleted. cy.contains('Sandbox').should('be.visible'); - cy.contains('-').should('be.visible'); + cy.contains('Sandbox').parent().within(() => { + cy.contains(/^-$/).should('be.visible'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cypress/e2e/publisher/024-mcp-server/00-runtime-config-endpoint-display.cy.js` around lines 122 - 129, The Sandbox placeholder check in the `#endpoints` Cypress assertion is too loose because cy.contains('-') matches any hyphen substring anywhere in the container. Tighten the regression test in the cy.get('`#endpoints`').within() block by scoping the assertion to the Sandbox row/section and using an exact match for the placeholder, so the test only passes when the Sandbox value itself renders as "-".tests/cypress/support/utils.js (2)
236-334: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo readiness poll before resolving with the new MCP server id.
addAPI/addAPIfromSwaggerexplicitly callUtils.waitForApiRetrievablebefore resolving, because "create can return an id before the API is retrievable, causing 500s on follow-ups".addMCPServerandaddMCPServerFromExistingAPIresolve immediately withparsed.id, and the consuming specs immediately hitmcp-servers/{id}/backendsright after. If MCP servers share the same eventhub-propagation characteristics as APIs (likely, same platform), this can cause intermittent flakiness in CI for these new regression tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cypress/support/utils.js` around lines 236 - 334, Both MCP server helpers return the new id too early, which can make follow-up backend calls race the server creation flow. Update addMCPServer and addMCPServerFromExistingAPI in Utils to wait for the MCP server to become retrievable before resolving, using the same pattern as addAPI/addAPIfromSwagger with the relevant MCP retrieval/wait helper or equivalent polling. Keep the existing parsed.id validation, then only resolve after the readiness check succeeds so specs can safely call mcp-servers/{id}/backends.
236-286: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFragile shell escaping for
additionalProperties.
additionalProps.replace(/"/g, '\\"')only escapes embedded double quotes. Inside the outer double-quoted-Fargument, characters like`,$, and\inname/version/contextare still shell-significant and can corrupt the request or (per static analysis) enable command injection. The file already has a safer pattern for this — thespecJsonwrite on line 261 and thebodyinaddMCPServerFromExistingAPI(line 315) use single-quote wrapping with'\\''escaping, which is the correct technique here too.🔧 Proposed fix
- -F "additionalProperties=${additionalProps.replace(/"/g, '\\"')}" \ + -F "additionalProperties=${additionalProps.replace(/'/g, "'\\''")}" \Note: since this value is embedded via
-F(which Cypress/curl expects unquoted or single-quoted form), wrap the whole flag value in single quotes rather than double quotes for consistency with the rest of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cypress/support/utils.js` around lines 236 - 286, The `addMCPServer` curl command is still vulnerable because `additionalProps` is only escaping double quotes, leaving shell-significant characters in `name`, `version`, and `context` unsafe. Update the `cy.exec(curl)` construction in `Utils.addMCPServer` to use the same single-quote wrapping and `'\''` escaping pattern already used for `specJson` in this method (and the `body` handling in `addMCPServerFromExistingAPI`), so the `-F "additionalProperties=..."` payload is passed safely without command injection risk.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/cypress/e2e/publisher/024-mcp-server/00-runtime-config-endpoint-display.cy.js`:
- Line 43: The spec in the Cypress test currently uses an exclusive test via
it.only, which will skip other tests in the file. Remove the .only from the it
declaration in the runtime configuration test so the suite runs normally
alongside any future tests in this spec. Keep the test name and behavior
unchanged, and only eliminate the exclusivity marker.
In
`@tests/cypress/e2e/publisher/024-mcp-server/01-runtime-config-from-existing-api.cy.js`:
- Line 40: The Cypress spec is using it.only in the runtime configuration test,
which will exclude other tests in this file from running. Update the test in the
existing API MCP server spec to remove the .only call from the it declaration so
the full suite executes normally; use the runtime configuration test block as
the place to make this change.
---
Nitpick comments:
In
`@tests/cypress/e2e/publisher/024-mcp-server/00-runtime-config-endpoint-display.cy.js`:
- Around line 122-129: The Sandbox placeholder check in the `#endpoints` Cypress
assertion is too loose because cy.contains('-') matches any hyphen substring
anywhere in the container. Tighten the regression test in the
cy.get('`#endpoints`').within() block by scoping the assertion to the Sandbox
row/section and using an exact match for the placeholder, so the test only
passes when the Sandbox value itself renders as "-".
In `@tests/cypress/support/utils.js`:
- Around line 236-334: Both MCP server helpers return the new id too early,
which can make follow-up backend calls race the server creation flow. Update
addMCPServer and addMCPServerFromExistingAPI in Utils to wait for the MCP server
to become retrievable before resolving, using the same pattern as
addAPI/addAPIfromSwagger with the relevant MCP retrieval/wait helper or
equivalent polling. Keep the existing parsed.id validation, then only resolve
after the readiness check succeeds so specs can safely call
mcp-servers/{id}/backends.
- Around line 236-286: The `addMCPServer` curl command is still vulnerable
because `additionalProps` is only escaping double quotes, leaving
shell-significant characters in `name`, `version`, and `context` unsafe. Update
the `cy.exec(curl)` construction in `Utils.addMCPServer` to use the same
single-quote wrapping and `'\''` escaping pattern already used for `specJson` in
this method (and the `body` handling in `addMCPServerFromExistingAPI`), so the
`-F "additionalProperties=..."` payload is passed safely without command
injection risk.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5a89c394-a921-4e19-9dd4-5822f0cc1953
📒 Files selected for processing (3)
tests/cypress/e2e/publisher/024-mcp-server/00-runtime-config-endpoint-display.cy.jstests/cypress/e2e/publisher/024-mcp-server/01-runtime-config-from-existing-api.cy.jstests/cypress/support/utils.js
Saadha123
force-pushed
the
fix/mcp-server-runtime-endpoint-tests
branch
from
d79c021 to
7b06f43
Compare
|
2 participants
Description:
Tests cover two MCP server creation types:
then verifies the Runtime Configuration page renders correctly with "-" shown for the sandbox section
Utilities added to cypress/support/utils.js:
Related Issue:
wso2/api-manager#5096