OLS-2654 - adding MCP apps tests#1877
Conversation
openshift-ci
Bot
added
the
needs-rebase
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces Cypress helper commands for intercepting MCP-related API endpoints with request validation and mocked SSE streaming responses, refactors operator installation setup with conditional logic for idempotency, and adds a new MCP iframe rendering test suite with mocked endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Browser
participant MockAPI as Mock API Intercepts
participant Iframe as MCP Iframe
Test->>Test: interceptMCPQuery()
Test->>Test: interceptMCPResources()
Test->>Browser: Load page with MCP iframe
Browser->>MockAPI: POST /v1/streaming_query
MockAPI-->>Browser: SSE stream (mocked)
Iframe->>MockAPI: POST /v1/mcp-apps/resources
MockAPI-->>Iframe: { content: htmlContent }
Test->>Iframe: Validate sandbox attribute
Test->>Iframe: Validate srcDoc rendering
Test->>Browser: Assert iframe visibility & state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 (4)
tests/tests/lightspeed-install.cy.ts (3)
178-198: TrimcsvNameto avoid trailing-newline foot-guns.
oc get ... -o nameproduces stdout with a trailing newline. While the shell typically word-splits it away in unquoted interpolation, this is fragile if anyone later quotes the variable or uses it for string concatenation/logging. A simple.trim()makes the intent explicit.♻️ Proposed fix
- if (result.stderr === '') { - const csvName = result.stdout; + if (result.stderr === '') { + const csvName = result.stdout.trim();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tests/lightspeed-install.cy.ts` around lines 178 - 198, The variable csvName captures stdout from the oc get command and may include a trailing newline; update the assignment of csvName (where it is set from result.stdout) to use .trim() to remove whitespace so subsequent uses in cy.exec (the oc patch and oc get calls that reference csvName) won't break if quoted or concatenated later.
1170-1173: Reuse the existingCONVERSATION_IDconstant.
MCP_CONVERSATION_IDat line 1170 has the same UUID asCONVERSATION_IDdeclared at line 46 and the default fallback insideinterceptMCPQuery(commands.ts line 256). Reusing the existing constant avoids three sources of truth drifting if the mock UUID is ever changed.♻️ Proposed fix
- const MCP_CONVERSATION_ID = '5f424596-a4f9-4a3a-932b-46a768de3e7c'; const MCP_PROMPT = 'Show me the dashboard';…and replace usage at line 1373 with
CONVERSATION_ID, and the${MCP_CONVERSATION_ID}interpolation in the SSE template at line 1318 likewise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tests/lightspeed-install.cy.ts` around lines 1170 - 1173, MCP_CONVERSATION_ID duplicates the existing CONVERSATION_ID constant; remove MCP_CONVERSATION_ID and replace all references with the single source-of-truth CONVERSATION_ID (including the `${MCP_CONVERSATION_ID}` interpolation inside the SSE template and the usage where the mock conversation id is passed to the MCP call), ensuring interceptMCPQuery’s fallback remains consistent with CONVERSATION_ID so the mock UUID is not defined in multiple places.
235-304: Extract the duplicated OLSConfig YAML into a single constant.Lines 254-271 and 282-299 contain the identical OLSConfig YAML literal. Extracting to a top-level constant (or a small helper that returns the create command) avoids the risk of the two blobs drifting apart in future edits.
♻️ Proposed fix
+ const OLS_CONFIG_YAML = `apiVersion: ols.openshift.io/v1alpha1 +kind: ${OLS.config.kind} +metadata: + name: ${OLS.config.name} +spec: + llm: + providers: + - type: openai + name: openai + credentialsSecretRef: + name: openai-api-keys + url: https://api.openai.com/v1 + models: + - name: gpt-4o-mini + ols: + defaultModel: gpt-4o-mini + defaultProvider: openai + logLevel: INFO`; + // Check if OLSConfig exists and handle accordingly cy.exec( `oc get ${OLS.config.kind} ${OLS.config.name} -o json --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, { failOnNonZeroExit: false }, ).then((configCheck) => { if (configCheck.exitCode === 0) { ... - const config = `apiVersion: ols.openshift.io/v1alpha1 -... - logLevel: INFO`; cy.exec( - `echo '${config}' | oc create -f - --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, + `echo '${OLS_CONFIG_YAML}' | oc create -f - --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, ); ... - const config = `apiVersion: ols.openshift.io/v1alpha1 -... - logLevel: INFO`; cy.exec( - `echo '${config}' | oc create -f - --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, + `echo '${OLS_CONFIG_YAML}' | oc create -f - --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tests/lightspeed-install.cy.ts` around lines 235 - 304, The two identical OLSConfig YAML literals used when creating the resource should be pulled out into a single top-level constant or small helper (e.g., OLS_CONFIG_YAML or buildOlsConfig()) and reused in both branches instead of duplicating the string; update the create commands that currently echo the inline `config` (the blocks that reference OLS.config.kind and OLS.config.name) to use that constant/helper when piping to `oc create`, ensuring the value still interpolates OLS.config.* and Cypress.env('KUBECONFIG_PATH').cypress/support/commands.ts (1)
266-295: Use.as(alias)on the intercept and convert conditional branching to assertions.The current pattern of setting
request.aliasconditionally only when the request matches is not aligned with Cypress best practices and creates a silent failure risk:
req.aliasis designed for dynamic scenarios (e.g., distinguishing multiple requests to the same URL or GraphQL mutations), not for conditional request acceptance. Using it this way, if the request doesn't match the expectedserverName/uiResourceUri, the alias is never set andcy.wait('@alias')will silently timeout.- Letting unmatched requests pass through via
request.continue()to the real backend can cause confusing errors in test environments without the actual service.- The multi-line
ifcondition should be reformatted per Prettier rules.♻️ Proposed refactor
Cypress.Commands.add( 'interceptMCPResources', ( alias: string, htmlContent: string, serverName: string = 'test-server', uiResourceUri: string = 'mcp://test-server/resources/dashboard', ) => { cy.intercept('POST', getApiUrl('/v1/mcp-apps/resources'), (request) => { /* eslint-disable camelcase */ Cypress.log({ name: 'MCP Resources Request', message: `server_name: ${request.body.server_name}, resource_uri: ${request.body.resource_uri}`, }); - // Only intercept requests that match this specific resource URI - if ( - request.body.server_name === serverName && - request.body.resource_uri === uiResourceUri - ) { - request.alias = alias; - request.reply({ body: { content: htmlContent }, delay: 500 }); - } else { - // Let other intercepts handle this request - request.continue(); - } + expect(request.body.server_name).to.equal(serverName); + expect(request.body.resource_uri).to.equal(uiResourceUri); + request.reply({ body: { content: htmlContent }, delay: 500 }); /* eslint-enable camelcase */ - }); + }).as(alias); }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/support/commands.ts` around lines 266 - 295, The intercept in interceptMCPResources should use cy.intercept(...).as(alias) instead of setting request.alias conditionally; change the Cypress.Commands.add('interceptMCPResources', ...) implementation so the cy.intercept call is given .as(alias) immediately, remove the conditional setting of request.alias and request.continue(), and inside the intercept handler assert the incoming request body values (e.g., expect(request.body.server_name).to.equal(serverName) and expect(request.body.resource_uri).to.equal(uiResourceUri)) before replying with request.reply({ body: { content: htmlContent }, delay: 500 }); also reformat the multi-line condition per Prettier.
🤖 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/tests/lightspeed-install.cy.ts`:
- Line 1360: The call to interceptMCPResources on line with
cy.interceptMCPResources('mcpResources1', SAMPLE_MCP_HTML, 'test-server',
MCP_UI_RESOURCE_URI) must be reformatted to satisfy Prettier: break each
argument onto its own line with a trailing comma and proper indentation; update
the call site (interceptMCPResources invocation) so the arguments are on
separate lines and the final argument ends with a comma to match the project's
formatter rules.
- Around line 1245-1382: The six MCP tests currently use it.only which disables
all other specs; locate the tests by their titles ('renders iframe when MCP
response includes uiResourceUri', 'iframe srcDoc contains expected HTML
content', 'displays loading state while fetching MCP resources', 'displays error
when resource fetch fails', 'does not render iframe when uiResourceUri is
missing', 'handles multiple MCP iframes in conversation') and change each
it.only(...) back to plain it(...), ensuring the test declarations (the it
calls) no longer include .only so the full test suite runs in CI.
---
Nitpick comments:
In `@cypress/support/commands.ts`:
- Around line 266-295: The intercept in interceptMCPResources should use
cy.intercept(...).as(alias) instead of setting request.alias conditionally;
change the Cypress.Commands.add('interceptMCPResources', ...) implementation so
the cy.intercept call is given .as(alias) immediately, remove the conditional
setting of request.alias and request.continue(), and inside the intercept
handler assert the incoming request body values (e.g.,
expect(request.body.server_name).to.equal(serverName) and
expect(request.body.resource_uri).to.equal(uiResourceUri)) before replying with
request.reply({ body: { content: htmlContent }, delay: 500 }); also reformat the
multi-line condition per Prettier.
In `@tests/tests/lightspeed-install.cy.ts`:
- Around line 178-198: The variable csvName captures stdout from the oc get
command and may include a trailing newline; update the assignment of csvName
(where it is set from result.stdout) to use .trim() to remove whitespace so
subsequent uses in cy.exec (the oc patch and oc get calls that reference
csvName) won't break if quoted or concatenated later.
- Around line 1170-1173: MCP_CONVERSATION_ID duplicates the existing
CONVERSATION_ID constant; remove MCP_CONVERSATION_ID and replace all references
with the single source-of-truth CONVERSATION_ID (including the
`${MCP_CONVERSATION_ID}` interpolation inside the SSE template and the usage
where the mock conversation id is passed to the MCP call), ensuring
interceptMCPQuery’s fallback remains consistent with CONVERSATION_ID so the mock
UUID is not defined in multiple places.
- Around line 235-304: The two identical OLSConfig YAML literals used when
creating the resource should be pulled out into a single top-level constant or
small helper (e.g., OLS_CONFIG_YAML or buildOlsConfig()) and reused in both
branches instead of duplicating the string; update the create commands that
currently echo the inline `config` (the blocks that reference OLS.config.kind
and OLS.config.name) to use that constant/helper when piping to `oc create`,
ensuring the value still interpolates OLS.config.* and
Cypress.env('KUBECONFIG_PATH').
🪄 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: Enterprise
Run ID: a91f7676-6558-4bac-9357-fd6dd7137a0e
📒 Files selected for processing (2)
cypress/support/commands.tstests/tests/lightspeed-install.cy.ts
removing only and fixing lint
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
1 participant
Adding MCP apps tests with a mock MCP server.
Summary by CodeRabbit