OLS-1555: Add Cypress tests for HITL tool approval flow#1909
OLS-1555: Add Cypress tests for HITL tool approval flow#1909kyoto wants to merge 1 commit intoopenshift:mainfrom
Conversation
kyoto
added
the
approved
📝 WalkthroughWalkthroughAdds Cypress test support and end-to-end tests for human-in-the-loop (HITL) tool approvals: new test commands stub streaming-query responses that require approval and stub approval decision submissions; test cases validate both approve and reject UI flows. ChangesHITL Tool Approval Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described 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.
🧹 Nitpick comments (2)
cypress/support/commands.ts (1)
198-202: ⚡ Quick winAdd request-shape assertion parity with
interceptQuery.Line 200 only asserts
query. Adding amedia_typeassertion here (same asinterceptQuery) will catch malformed request bodies in this path too.Proposed patch
Cypress.Commands.add('interceptQueryWithApproval', (alias: string, query: string) => { cy.intercept('POST', getApiUrl('/v1/streaming_query'), (request) => { + expect(request.body.media_type).to.equal('application/json'); expect(request.body.query).to.include(query); request.reply({ body: MOCK_STREAMED_RESPONSE_WITH_APPROVAL_BODY, delay: 500 }); }).as(alias); });🤖 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 198 - 202, The interceptQueryWithApproval command only asserts request.body.query and misses the media_type check present in interceptQuery; update the handler inside Cypress.Commands.add('interceptQueryWithApproval', (alias, query) => { ... }) so that the intercepted request callback also asserts request.body.media_type is the same expected value used by interceptQuery (e.g., 'text' or the shared constant), then leave the existing query assertion and reply behavior intact so malformed request bodies with wrong media_type are caught the same way as interceptQuery.tests/tests/lightspeed-install.cy.ts (1)
36-37: ⚡ Quick winScope
toolLabelselector tighter to reduce flake risk.Line 37 currently targets any PatternFly label inside the popover. If additional labels appear, assertions/clicks may hit the wrong element.
Proposed patch
-const toolLabel = `${popover} .pf-v6-c-label`; +const toolLabel = `${aiChatEntry} .pf-v6-c-label`;🤖 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 36 - 37, The toolLabel selector is too broad and may match any PatternFly label inside the popover; tighten it so interactions target the specific label associated with the tool approval card (e.g., scope by relation to toolApprovalCard, a unique modifier/class, data-* attribute, or text selector) — update the toolLabel constant so it selects the label adjacent to or inside the `${popover} .ols-plugin__tool-call` element rather than every `.pf-v6-c-label` in the popover.
🤖 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.
Nitpick comments:
In `@cypress/support/commands.ts`:
- Around line 198-202: The interceptQueryWithApproval command only asserts
request.body.query and misses the media_type check present in interceptQuery;
update the handler inside Cypress.Commands.add('interceptQueryWithApproval',
(alias, query) => { ... }) so that the intercepted request callback also asserts
request.body.media_type is the same expected value used by interceptQuery (e.g.,
'text' or the shared constant), then leave the existing query assertion and
reply behavior intact so malformed request bodies with wrong media_type are
caught the same way as interceptQuery.
In `@tests/tests/lightspeed-install.cy.ts`:
- Around line 36-37: The toolLabel selector is too broad and may match any
PatternFly label inside the popover; tighten it so interactions target the
specific label associated with the tool approval card (e.g., scope by relation
to toolApprovalCard, a unique modifier/class, data-* attribute, or text
selector) — update the toolLabel constant so it selects the label adjacent to or
inside the `${popover} .ols-plugin__tool-call` element rather than every
`.pf-v6-c-label` in the popover.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fa03a085-6a6b-4f30-9ee6-e517f219be9e
📒 Files selected for processing (2)
cypress/support/commands.tstests/tests/lightspeed-install.cy.ts
|
/cherry-pick release-4.19 |
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cypress/support/commands.ts (1)
192-192: ⚡ Quick winExtract
approval_idmagic string as a named constant.
"abc"appears in both the mock SSE body (line 192) and the assertion ininterceptToolApproval(line 208). If either changes without the other, the test fails with a confusing mismatch rather than a clear signal. A single named constant makes the coupling explicit.♻️ Proposed refactor
+const MOCK_APPROVAL_ID = 'abc'; + /* eslint-disable camelcase */ const MOCK_STREAMED_RESPONSE_WITH_APPROVAL_BODY = `data: {"event": "start", ...} ... -data: {"event": "approval_required", "data": {"approval_id": "abc", "tool_name": "mock_tool", ...}} +data: {"event": "approval_required", "data": {"approval_id": "${MOCK_APPROVAL_ID}", "tool_name": "mock_tool", ...}} ... `; /* eslint-enable camelcase */ Cypress.Commands.add('interceptToolApproval', (alias: string, approved: boolean) => { cy.intercept('POST', getApiUrl('/v1/tool-approvals/decision'), (request) => { - expect(request.body.approval_id).to.equal('abc'); + expect(request.body.approval_id).to.equal(MOCK_APPROVAL_ID); expect(request.body.approved).to.equal(approved); request.reply({ statusCode: 200, body: {} }); }).as(alias); });Also applies to: 206-212
🤖 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` at line 192, The test uses the literal approval_id "abc" in the mock SSE payload and again in the assertion inside interceptToolApproval, causing a brittle coupling; extract that magic string into a single named constant (e.g., const MOCK_APPROVAL_ID = "abc") declared near the top of cypress/support/commands.ts and replace the inline "abc" occurrences in the SSE mock (the data JSON used for the approval_required event) and in the interceptToolApproval assertion to reference MOCK_APPROVAL_ID so both the mock and the assertion stay in sync.
🤖 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.
Nitpick comments:
In `@cypress/support/commands.ts`:
- Line 192: The test uses the literal approval_id "abc" in the mock SSE payload
and again in the assertion inside interceptToolApproval, causing a brittle
coupling; extract that magic string into a single named constant (e.g., const
MOCK_APPROVAL_ID = "abc") declared near the top of cypress/support/commands.ts
and replace the inline "abc" occurrences in the SSE mock (the data JSON used for
the approval_required event) and in the interceptToolApproval assertion to
reference MOCK_APPROVAL_ID so both the mock and the assertion stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7587fdc7-96cd-4cf0-a6df-9d96b6c2ba80
📒 Files selected for processing (2)
cypress/support/commands.tstests/tests/lightspeed-install.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/tests/lightspeed-install.cy.ts
|
/retest |
|
@kyoto: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
kyoto
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@kyoto: This pull request references OLS-1555 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
3 participants
Summary by CodeRabbit