OLS-2654 - adding MCP apps tests#1877
Conversation
openshift-ci
Bot
added
the
needs-rebase
|
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 two Cypress commands and typings to mock MCP streaming queries and MCP resources (including pass-through for non-matching resource requests), and refactors Lightspeed operator install/uninstall and test setup to be idempotent and grant an extra cluster-role; adds a new Cypress suite that validates MCP iframe rendering with the mocks. ChangesCypress commands + Lightspeed install/idempotency
Sequence Diagram(s)sequenceDiagram
participant Test
participant Browser
participant MockAPI
participant Iframe
Test->>MockAPI: register interceptMCPQuery(alias, query, toolName, uiResourceUri, conversationId)
Test->>MockAPI: register interceptMCPResources(alias, htmlContent, serverName, uiResourceUri)
Test->>Browser: navigate to page with MCP iframe
Browser->>MockAPI: POST /v1/streaming_query (query payload)
MockAPI-->>Browser: SSE stream (mocked SSE chunks)
Iframe->>MockAPI: POST /v1/mcp-apps/resources (resource request)
MockAPI-->>Iframe: HTML response (htmlContent) or pass-through/error
Browser->>Test: iframe loaded or error state reported
Test->>Browser: validate iframe sandbox/srcDoc, visibility, and UI states
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
JoaoFula
force-pushed
the
test-mcp-apps
branch
2 times, most recently
from
990f31a to
a5ba52e
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/tests/lightspeed-install.cy.ts (1)
254-271: Extract duplicated OLSConfig YAML into a single helper constant/function.The manifest string is duplicated in both branches, which makes edits error-prone.
♻️ Refactor sketch
+ const buildOLSConfigManifest = () => `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`; ... - const config = `apiVersion: ...` + const config = buildOLSConfigManifest(); ... - const config = `apiVersion: ...` + const config = buildOLSConfigManifest();Also applies to: 282-302
🤖 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 254 - 271, Duplicate OLS manifest YAML string assigned to config is used in multiple branches; extract it into a single helper (e.g., a constant OLS_CONFIG_YAML or a function buildOLSConfig()) and replace the in-place multi-line strings in both places with a reference to that helper to avoid duplication; ensure the helper reproduces the same interpolations (name, kind, model/provider values) used by the current config assignment so callers (the two branches around the current config usage) can reuse it without changing behavior.
🤖 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`:
- Around line 179-192: The CSV selection uses raw result.stdout (assigned to
csvName) which may include trailing newlines or multiple lines; update the code
around where result.stdout is read (the promise callback that sets csvName and
the later occurrences around lines 217-218) to normalize the value by trimming
whitespace and picking the first non-empty line before using it in
cy.exec/cy.exec(... `oc patch ${csvName}` / `oc get ${csvName}`) so shell
commands receive a single valid resource name.
---
Nitpick comments:
In `@tests/tests/lightspeed-install.cy.ts`:
- Around line 254-271: Duplicate OLS manifest YAML string assigned to config is
used in multiple branches; extract it into a single helper (e.g., a constant
OLS_CONFIG_YAML or a function buildOLSConfig()) and replace the in-place
multi-line strings in both places with a reference to that helper to avoid
duplication; ensure the helper reproduces the same interpolations (name, kind,
model/provider values) used by the current config assignment so callers (the two
branches around the current config usage) can reuse it without changing
behavior.
🪄 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: 04789863-b9e1-450c-b3aa-6295a71e9efa
📒 Files selected for processing (2)
cypress/support/commands.tstests/tests/lightspeed-install.cy.ts
JoaoFula
force-pushed
the
test-mcp-apps
branch
2 times, most recently
from
0cdcfa9 to
d7c4cd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/tests/lightspeed-install.cy.ts (1)
197-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSelect the active CSV deterministically before patching it.
oc get clusterserviceversion -o namecan return more than one CSV in an OLM namespace. Taking the first line here makes the console-image patch nondeterministic, so this can end up modifying an inactive/replaced CSV and leave the actual operator unchanged.🤖 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/tests/lightspeed-install.cy.ts` around lines 197 - 208, The current selection of csvName from the first line of `oc get clusterserviceversion -o name` is nondeterministic; change the lookup to deterministically select the active CSV (for example query JSON and pick the CSV whose status.phase === 'Succeeded' or whose metadata.name matches the subscription status.currentCSV) instead of taking the first stdout line. Update the code that builds csvName (the block that calls `oc get clusterserviceversion --namespace=${OLS.namespace} -o name`) to use `oc get clusterserviceversion -o json` and parse result.stdout to find the CSV with status.phase === 'Succeeded' (or match subscription.status.currentCSV) and assign that to csvName before running the oc patch and related commands.
🧹 Nitpick comments (1)
cypress/support/commands.ts (1)
240-259: ⚡ Quick winParameterize
server_nameconsistently across the MCP helpers.
interceptMCPQuery()always emits"test-server", whileinterceptMCPResources()lets callers overrideserverNamebut still defaultsuiResourceUrito atest-serverURI. A caller that changes onlyserverNamegets a mismatched mock pair, which makes these helpers harder to reuse outside this spec.Also applies to: 266-273
🤖 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 `@cypress/support/commands.ts` around lines 240 - 259, interceptMCPQuery and interceptMCPResources are emitting mismatched "test-server" values causing inconsistent mocks; update both helpers to accept the same serverName parameter (or rename to server_name consistently) and derive the default uiResourceUri from that serverName instead of hardcoding 'test-server', then use that variable when replacing 'UI_RESOURCE_URI' and when setting the mocked server_name in the response template so callers who override serverName get consistent paired mocks; update any default parameter values and the places that build the uiResourceUri in both interceptMCPQuery and interceptMCPResources to use the unified serverName variable.
🤖 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/tests/lightspeed-install.cy.ts`:
- Around line 254-287: Currently when OLSConfig exists and isn't deleting the
test skips updating it and inherits any prior spec; modify the logic in the
existing block that handles a successful oc get (where variables/functions
referenced include OLS.config, existingConfig, and OLS_CONFIG_YAML) to reconcile
the resource to the desired spec instead of leaving it as-is: call oc apply (or
oc create --save-config --dry-run=client followed by oc apply) with
OLS_CONFIG_YAML (same invocation used for creation) so the cluster resource
matches the test's desired YAML on every run, ensuring idempotence and
deterministic setup; keep the deletion-wait path intact and only switch the
"already exists and is not deleting" branch to run an apply/replace of
OLS_CONFIG_YAML.
---
Duplicate comments:
In `@tests/tests/lightspeed-install.cy.ts`:
- Around line 197-208: The current selection of csvName from the first line of
`oc get clusterserviceversion -o name` is nondeterministic; change the lookup to
deterministically select the active CSV (for example query JSON and pick the CSV
whose status.phase === 'Succeeded' or whose metadata.name matches the
subscription status.currentCSV) instead of taking the first stdout line. Update
the code that builds csvName (the block that calls `oc get clusterserviceversion
--namespace=${OLS.namespace} -o name`) to use `oc get clusterserviceversion -o
json` and parse result.stdout to find the CSV with status.phase === 'Succeeded'
(or match subscription.status.currentCSV) and assign that to csvName before
running the oc patch and related commands.
---
Nitpick comments:
In `@cypress/support/commands.ts`:
- Around line 240-259: interceptMCPQuery and interceptMCPResources are emitting
mismatched "test-server" values causing inconsistent mocks; update both helpers
to accept the same serverName parameter (or rename to server_name consistently)
and derive the default uiResourceUri from that serverName instead of hardcoding
'test-server', then use that variable when replacing 'UI_RESOURCE_URI' and when
setting the mocked server_name in the response template so callers who override
serverName get consistent paired mocks; update any default parameter values and
the places that build the uiResourceUri in both interceptMCPQuery and
interceptMCPResources to use the unified serverName variable.
🪄 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: f941f958-6deb-4685-b749-8be2052b2f93
📒 Files selected for processing (2)
cypress/support/commands.tstests/tests/lightspeed-install.cy.ts
| // 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) { | ||
| // OLSConfig exists, check if it's being deleted | ||
| const existingConfig = JSON.parse(configCheck.stdout); | ||
| if (existingConfig.metadata.deletionTimestamp) { | ||
| cy.task( | ||
| 'log', | ||
| `OLSConfig is being deleted. Waiting for deletion to complete before recreating...`, | ||
| ); | ||
| // Wait for deletion to complete (check every 5 seconds for up to 3 minutes) | ||
| cy.exec( | ||
| `timeout 180 bash -c 'until ! oc get ${OLS.config.kind} ${OLS.config.name} --kubeconfig ${Cypress.env('KUBECONFIG_PATH')} 2>/dev/null; do echo "Waiting for deletion..."; sleep 5; done'`, | ||
| { timeout: 4 * MINUTE }, | ||
| ).then(() => { | ||
| cy.task('log', 'OLSConfig deleted. Creating new OLSConfig...'); | ||
| cy.exec( | ||
| `oc scale --replicas=1 deployment/lightspeed-operator-controller-manager --namespace=${OLS.namespace} --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| `echo '${OLS_CONFIG_YAML}' | oc create -f - --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
| }); | ||
| } else { | ||
| throw new Error(`Getting CSV name failed | ||
| Exit code: ${result.exitCode} | ||
| Stdout:\n${result.stdout} | ||
| Stderr:\n${result.stderr}`); | ||
| cy.task('log', `OLSConfig already exists and is not deleting. Using existing config.`); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| const config = `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`; | ||
| cy.exec(`echo '${config}' | oc create -f - --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`); | ||
| } else { | ||
| // OLSConfig doesn't exist, create it | ||
| cy.task('log', 'OLSConfig not found. Creating new OLSConfig...'); | ||
| cy.exec( | ||
| `echo '${OLS_CONFIG_YAML}' | oc create -f - --kubeconfig ${Cypress.env('KUBECONFIG_PATH')}`, | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Reconcile OLSConfig to the expected spec instead of reusing it as-is.
When the config already exists and is not deleting, setup now skips any update and inherits whatever spec was left by a previous run. That makes this suite environment-dependent: reruns can pick up a stale provider/model config even though OLS_CONFIG_YAML above defines the desired test state. Applying the YAML on every run would keep setup idempotent and deterministic.
🧰 Tools
🪛 OpenGrep (1.20.0)
[ERROR] 255-258: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 268-271: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 273-275: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 283-285: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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/tests/lightspeed-install.cy.ts` around lines 254 - 287, Currently when
OLSConfig exists and isn't deleting the test skips updating it and inherits any
prior spec; modify the logic in the existing block that handles a successful oc
get (where variables/functions referenced include OLS.config, existingConfig,
and OLS_CONFIG_YAML) to reconcile the resource to the desired spec instead of
leaving it as-is: call oc apply (or oc create --save-config --dry-run=client
followed by oc apply) with OLS_CONFIG_YAML (same invocation used for creation)
so the cluster resource matches the test's desired YAML on every run, ensuring
idempotence and deterministic setup; keep the deletion-wait path intact and only
switch the "already exists and is not deleting" branch to run an apply/replace
of OLS_CONFIG_YAML.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/tests/lightspeed-install.cy.ts (1)
254-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply
OLS_CONFIG_YAMLin the “already exists” branch as well.This still reuses whatever
OLSConfigspec was left behind and only recreates when deletion is in progress. That makes reruns depend on cluster state even though this file already defines the desired config inline.🤖 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/tests/lightspeed-install.cy.ts` around lines 254 - 279, The test currently skips applying OLS_CONFIG_YAML when an OLSConfig exists and is not deleting, causing runs to depend on leftover cluster spec; in the else branch where you log "OLSConfig already exists and is not deleting. Using existing config.", invoke a cy.exec to apply the desired manifest (use OLS_CONFIG_YAML) against the cluster (e.g., pipe OLS_CONFIG_YAML into oc apply -f - with the same --kubeconfig Cypress.env('KUBECONFIG_PATH')) so the desired spec is enforced even when the resource already exists; keep the existing logging (cy.task('log', ...)) and reference OLS.config.kind / OLS.config.name / existingConfig to locate the branch to modify.
🧹 Nitpick comments (1)
cypress/support/commands.ts (1)
45-57: ⚡ Quick winLet
interceptMCPQuerymodel the no-uiResourceUricase too.The helper currently requires
uiResourceUriand always emitstool_meta.ui.resourceUri, so the spec has to hand-roll a second SSE fixture for the missing-URI scenario. Making this argument optional and omitting that field when absent would keep all MCP streaming mocks on one code path and reduce fixture drift.Also applies to: 240-264
🤖 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 `@cypress/support/commands.ts` around lines 45 - 57, Change interceptMCPQuery to accept uiResourceUri as optional and when composing the emitted SSE payload only add tool_meta.ui.resourceUri when uiResourceUri is defined; update any overloads or declarations (including the duplicate signatures around interceptMCPResources area) so the parameter is optional and the code path omits that field when absent, keeping the rest of the SSE structure identical to the existing happy-path behavior.
🤖 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/tests/lightspeed-install.cy.ts`:
- Around line 151-156: The current branch returns early when
operatorAlreadyInstalled is true and skips the CSV patching that reconciles
CONSOLE_IMAGE; update the logic so that even if operatorAlreadyInstalled is true
you still perform the CSV patch/image-substitution steps: after logging the skip
for installation (the block that references OLS.namespace), invoke the same
CSV-patching routine or inline the CSV image-replacement code (the code used
later to patch the operator CSV and set CONSOLE_IMAGE) so the CSV is patched
regardless of installation state; ensure you reuse the existing CSV patch
function/steps and do not duplicate installation logic.
---
Duplicate comments:
In `@tests/tests/lightspeed-install.cy.ts`:
- Around line 254-279: The test currently skips applying OLS_CONFIG_YAML when an
OLSConfig exists and is not deleting, causing runs to depend on leftover cluster
spec; in the else branch where you log "OLSConfig already exists and is not
deleting. Using existing config.", invoke a cy.exec to apply the desired
manifest (use OLS_CONFIG_YAML) against the cluster (e.g., pipe OLS_CONFIG_YAML
into oc apply -f - with the same --kubeconfig Cypress.env('KUBECONFIG_PATH')) so
the desired spec is enforced even when the resource already exists; keep the
existing logging (cy.task('log', ...)) and reference OLS.config.kind /
OLS.config.name / existingConfig to locate the branch to modify.
---
Nitpick comments:
In `@cypress/support/commands.ts`:
- Around line 45-57: Change interceptMCPQuery to accept uiResourceUri as
optional and when composing the emitted SSE payload only add
tool_meta.ui.resourceUri when uiResourceUri is defined; update any overloads or
declarations (including the duplicate signatures around interceptMCPResources
area) so the parameter is optional and the code path omits that field when
absent, keeping the rest of the SSE structure identical to the existing
happy-path behavior.
🪄 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: 0e9ef084-cd5d-473c-a9b4-48559b80c367
📒 Files selected for processing (2)
cypress/support/commands.tstests/tests/lightspeed-install.cy.ts
| if (operatorAlreadyInstalled) { | ||
| cy.task( | ||
| 'log', | ||
| `Operator subscription already exists in ${OLS.namespace} namespace. Skipping installation and image substitution.`, | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Reconcile CONSOLE_IMAGE even when the operator is already installed.
When operatorAlreadyInstalled is true, this branch exits before the CSV patching code runs. A rerun with CONSOLE_IMAGE set will therefore keep whatever console image was already in the cluster, so these tests can silently exercise the wrong build.
Also applies to: 194-250
🤖 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/tests/lightspeed-install.cy.ts` around lines 151 - 156, The current
branch returns early when operatorAlreadyInstalled is true and skips the CSV
patching that reconciles CONSOLE_IMAGE; update the logic so that even if
operatorAlreadyInstalled is true you still perform the CSV
patch/image-substitution steps: after logging the skip for installation (the
block that references OLS.namespace), invoke the same CSV-patching routine or
inline the CSV image-replacement code (the code used later to patch the operator
CSV and set CONSOLE_IMAGE) so the CSV is patched regardless of installation
state; ensure you reuse the existing CSV patch function/steps and do not
duplicate installation logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/tests/lightspeed-install.cy.ts (2)
1161-1164: 💤 Low value
MCP_CONVERSATION_IDduplicates the existingCONVERSATION_IDconstant.It has the same value as the file-level
CONVERSATION_IDdeclared at line 65 and as the fallback UUID baked intointerceptMCPQuery. Just reuseCONVERSATION_IDto keep them from drifting.♻️ Proposed fix
- const MCP_CONVERSATION_ID = '5f424596-a4f9-4a3a-932b-46a768de3e7c'; const MCP_PROMPT = 'Show me the dashboard';…and replace the single use on line 1369 with
CONVERSATION_ID.🤖 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/tests/lightspeed-install.cy.ts` around lines 1161 - 1164, MCP_CONVERSATION_ID duplicates the file-level CONVERSATION_ID constant; remove the MCP_CONVERSATION_ID declaration and update the single usage (where MCP_CONVERSATION_ID is passed to the MCP query at its usage site) to use CONVERSATION_ID instead so the test and interceptMCPQuery fallback share the same UUID.
1275-1329: ⚡ Quick winInline intercepts diverge from the helper-command URL/assertion conventions.
Three inline intercepts in this suite use
'**/v1/streaming_query'/'**/v1/mcp-apps/resources'glob patterns andto.equal(MCP_PROMPT), while the newinterceptMCPQuery/interceptMCPResourceshelpers usegetApiUrl('/v1/...')andto.include(query). Two concrete consequences:
- If
getApiUrlever returns an origin-qualified or proxy-prefixed URL, the helpers keep matching but these globs may not — and the FE silently hits the real endpoint.to.equal(MCP_PROMPT)(line 1327) breaks the moment the chatbot prepends/appends any context toquery(the existinginterceptQueryhelper deliberately usesto.includefor that reason).Consider routing all three through
getApiUrl(...)(importing it as the helper does) and usingto.includeforquery.🤖 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/tests/lightspeed-install.cy.ts` around lines 1275 - 1329, The inline cy.intercept calls using '**/v1/streaming_query' and '**/v1/mcp-apps/resources' and the assertion expect(request.body.query).to.equal(MCP_PROMPT) diverge from the helper conventions (interceptMCPQuery/interceptMCPResources) and can fail if getApiUrl() returns origin-qualified URLs or the query is augmented; change those inline intercepts to use getApiUrl('/v1/streaming_query') and getApiUrl('/v1/mcp-apps/resources') respectively (import getApiUrl like the helpers do) and change the body assertion to use to.include(MCP_PROMPT) instead of to.equal so the match is resilient to added context.cypress/support/commands.ts (2)
254-259: 💤 Low valueUse
replaceAllfor placeholder substitution to be safe against future template changes.
String.prototype.replacewith a string argument only replaces the first occurrence. Today each placeholder appears once inMOCK_MCP_STREAMED_RESPONSE_BODY_TEMPLATE, so it works, but adding a secondtool_call/tool_resultblock (a likely extension for multi-iframe scenarios) would silently leave later occurrences unsubstituted and produce a broken SSE stream.♻️ Proposed fix
- const responseBody = MOCK_MCP_STREAMED_RESPONSE_BODY_TEMPLATE.replace( - 'CONVERSATION_ID', - conversationId || '5f424596-a4f9-4a3a-932b-46a768de3e7c', - ) - .replace('TOOL_NAME', toolName) - .replace('UI_RESOURCE_URI', uiResourceUri); + const responseBody = MOCK_MCP_STREAMED_RESPONSE_BODY_TEMPLATE + .replaceAll('CONVERSATION_ID', conversationId || '5f424596-a4f9-4a3a-932b-46a768de3e7c') + .replaceAll('TOOL_NAME', toolName) + .replaceAll('UI_RESOURCE_URI', uiResourceUri);🤖 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 `@cypress/support/commands.ts` around lines 254 - 259, The current substitution uses String.prototype.replace which only replaces the first match; update the creation of responseBody (using MOCK_MCP_STREAMED_RESPONSE_BODY_TEMPLATE and the variables conversationId, toolName, uiResourceUri) to perform global replacements (e.g., use String.prototype.replaceAll or global-regex replace) for each placeholder so all occurrences of 'CONVERSATION_ID', 'TOOL_NAME', and 'UI_RESOURCE_URI' are replaced even if the template contains multiple blocks; keep the same conversationId fallback value and preserve the order of substitutions.
274-291: 💤 Low valueDocument the intercept registration order dependency more explicitly.
The handler returns without calling
request.reply()orrequest.continue()whenresource_uridoesn't match (line 278), relying on Cypress's LIFO fall-through to a previously registered intercept. While the inline comment acknowledges this is intentional, and the "handles multiple MCP iframes in conversation" test depends on this exact behavior with twointerceptMCPResourcescalls for different URIs, the contract is fragile: any refactor that callsinterceptMCPResourcesonly once or changes intercept registration order will silently let mismatched requests fall through to the network. Consider an explicitrequest.continue()instead of implicit fall-through, or strengthen the documentation comment to make the ordering dependency impossible to miss.🤖 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 `@cypress/support/commands.ts` around lines 274 - 291, The intercept handler for MCP resources currently returns early when request.body.resource_uri !== uiResourceUri, relying on Cypress's LIFO fall-through; update the handler in interceptMCPResources to call request.continue() instead of returning (so unmatched requests are explicitly forwarded) and keep the existing Cypress.log and assertions for the matched branch; also update the inline comment to state explicitly that unmatched requests are continued (not dropped) to prevent fragile ordering assumptions.
🤖 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/tests/lightspeed-install.cy.ts`:
- Around line 206-208: The test currently patches a hardcoded relatedImages
index (`/spec/relatedImages/1/image`) which is brittle; change the flow in the
test (around csvName) to first fetch the CSV JSON (use the existing code that
reads the CSV), locate the relatedImages array entry whose name matches the
console image key used by the operator bundle (the same approach used for args
detection like arg.startsWith('--console-image')), compute its index, then apply
a single oc patch that updates both the discovered relatedImages[index].image
and the console-image arg (use Cypress.env('CONSOLE_IMAGE') and OLS.namespace in
the patch) so you aren't relying on hardcoded index values and both edits happen
atomically.
---
Nitpick comments:
In `@cypress/support/commands.ts`:
- Around line 254-259: The current substitution uses String.prototype.replace
which only replaces the first match; update the creation of responseBody (using
MOCK_MCP_STREAMED_RESPONSE_BODY_TEMPLATE and the variables conversationId,
toolName, uiResourceUri) to perform global replacements (e.g., use
String.prototype.replaceAll or global-regex replace) for each placeholder so all
occurrences of 'CONVERSATION_ID', 'TOOL_NAME', and 'UI_RESOURCE_URI' are
replaced even if the template contains multiple blocks; keep the same
conversationId fallback value and preserve the order of substitutions.
- Around line 274-291: The intercept handler for MCP resources currently returns
early when request.body.resource_uri !== uiResourceUri, relying on Cypress's
LIFO fall-through; update the handler in interceptMCPResources to call
request.continue() instead of returning (so unmatched requests are explicitly
forwarded) and keep the existing Cypress.log and assertions for the matched
branch; also update the inline comment to state explicitly that unmatched
requests are continued (not dropped) to prevent fragile ordering assumptions.
In `@tests/tests/lightspeed-install.cy.ts`:
- Around line 1161-1164: MCP_CONVERSATION_ID duplicates the file-level
CONVERSATION_ID constant; remove the MCP_CONVERSATION_ID declaration and update
the single usage (where MCP_CONVERSATION_ID is passed to the MCP query at its
usage site) to use CONVERSATION_ID instead so the test and interceptMCPQuery
fallback share the same UUID.
- Around line 1275-1329: The inline cy.intercept calls using
'**/v1/streaming_query' and '**/v1/mcp-apps/resources' and the assertion
expect(request.body.query).to.equal(MCP_PROMPT) diverge from the helper
conventions (interceptMCPQuery/interceptMCPResources) and can fail if
getApiUrl() returns origin-qualified URLs or the query is augmented; change
those inline intercepts to use getApiUrl('/v1/streaming_query') and
getApiUrl('/v1/mcp-apps/resources') respectively (import getApiUrl like the
helpers do) and change the body assertion to use to.include(MCP_PROMPT) instead
of to.equal so the match is resilient to added context.
🪄 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: 80642326-4946-4ab9-8c20-3e525fbcc490
📒 Files selected for processing (2)
cypress/support/commands.tstests/tests/lightspeed-install.cy.ts
removing only and fixing lint some further changes some further changes changing logic for checking tour changing back to include since we provide the current pod as context removing reference to own pod to prevent infinite loop removing reference to own pod to prevent infinite loop
| const mcpAppLoading = `${mcpAppCard} .pf-v6-c-spinner`; | ||
| const mcpAppError = '.ols-plugin__alert'; | ||
|
|
||
| const MCP_CONVERSATION_ID = '5f424596-a4f9-4a3a-932b-46a768de3e7c'; |
There was a problem hiding this comment.
This is the same as CONVERSATION_ID, so let's delete this and just use CONVERSATION_ID below
| const MCP_TOOL_NAME = 'dashboard'; | ||
| const MCP_UI_RESOURCE_URI = 'mcp://test-server/resources/dashboard'; | ||
|
|
||
| const SAMPLE_MCP_HTML = `<!DOCTYPE html> |
There was a problem hiding this comment.
Let's keep this HTML as minimal as possible to make it obvious what we are actually testing. I think we can make it just
<html>
<body>
<h1>MCP Dashboard</h1>
<h2>Resource Dashboard</h2>
<p>CPU Usage</p>
<p>45%</p>
</body>
</html>
|
|
||
| const responseBody = MOCK_MCP_STREAMED_RESPONSE_BODY_TEMPLATE.replace( | ||
| 'CONVERSATION_ID', | ||
| conversationId || '5f424596-a4f9-4a3a-932b-46a768de3e7c', |
There was a problem hiding this comment.
To avoid hardcoding this ID in multiple places, let's export const CONVERSATION_ID = '5f424596-a4f9-4a3a-932b-46a768de3e7c' in this file and import it in lightspeed-install.cy.ts
2 participants
Adding MCP apps tests with a mock MCP server.
Summary by CodeRabbit