Skip to content

fix(gen-ai): Fix flaky mcpTab test by re-querying server rows after modal closes#7030

Draft
toklumpp wants to merge 9 commits intoopendatahub-io:mainfrom
toklumpp:RHOAIENG-49598-gen-ai-test-fix
Draft

fix(gen-ai): Fix flaky mcpTab test by re-querying server rows after modal closes#7030
toklumpp wants to merge 9 commits intoopendatahub-io:mainfrom
toklumpp:RHOAIENG-49598-gen-ai-test-fix

Conversation

@toklumpp
Copy link
Copy Markdown
Contributor

@toklumpp toklumpp commented Apr 2, 2026

Description

This PR fixes flaky Gen AI mock tests in the MCP Servers tab by addressing stale element references.

Root Cause

When modals close in the MCP Servers test, the underlying DOM can be re-rendered, causing previously queried Cypress elements to become stale. Attempting to interact with these stale references causes intermittent test failures.

Changes Made

Test Reliability Fix

  • Re-query server rows after modals close instead of reusing stale element references
  • Ensures tests interact with fresh, valid DOM elements after modal interactions

How Has This Been Tested?

Local Testing

  • Ran Gen AI mock tests: cd packages/gen-ai/frontend && npm run test:cypress-ci
  • Verified tests pass consistently with fresh element queries after modal closes

CI Testing

  • All tests passing after /retest

Test Impact

Tests Modified:

  • packages/gen-ai/frontend/src/__tests__/cypress/cypress/tests/mocked/playground/mcpTab.cy.ts
    • Re-query server row after closing success modal (3 locations)
    • Prevents stale element references in tools button interactions

Test Coverage:

  • MCP server modal workflows
  • Stale element reference prevention

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)
  • The code follows our Best Practices (React coding standards, PatternFly usage, performance considerations)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress This PR is in WIP state label Apr 2, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 2, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 2, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Cypress test modifications in the MCP tab tests address stale DOM element references. Instead of reusing previously captured serverRow element references across multiple operations, the test now re-queries the server row using freshServerRow or getServerRow() lookups after modal closures and between operations. This applies to tool button interactions in both the auto-unlock workflow and comprehensive tools workflow sections. Total modifications: 7 lines added, 4 lines removed across one test file.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed PR description comprehensively covers the issue, root cause, specific changes, testing methodology, and all self-checklist items are marked complete.
Title check ✅ Passed The title accurately describes the main change: fixing a flaky MCP tab test by re-querying server rows after modal closes, which directly matches the changeset modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot added area/frontend needs-rebase PR needs to be rebased area/gen-ai and removed needs-rebase PR needs to be rebased labels Apr 2, 2026
@toklumpp toklumpp changed the title fix(gen-ai): Improve test reliability and add missing test infrastruc… Flaky test fix mockMCP Apr 2, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.96%. Comparing base (4dbfc83) to head (5c8b184).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7030      +/-   ##
==========================================
- Coverage   64.98%   64.96%   -0.03%     
==========================================
  Files        2446     2447       +1     
  Lines       76161    76159       -2     
  Branches    19213    19216       +3     
==========================================
- Hits        49490    49473      -17     
- Misses      26671    26686      +15     

see 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46ad36a...5c8b184. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch 7 times, most recently from a19ed6f to 2598bc4 Compare April 9, 2026 09:18
@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch 5 times, most recently from 5c7f865 to 8dbcdca Compare April 10, 2026 13:46
@rhods-ci-bot
Copy link
Copy Markdown

@toklumpp: The following test has Failed:

OCI Artifact Browser URL

View in Artifact Browser

Inspecting Test Artifacts Manually

To inspect your test artifacts manually, follow these steps:

  1. Install ORAS (see the ORAS installation guide).
  2. Download artifacts with the following commands:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:odh-pr-test-trainer-m2m2v

@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch from 8dbcdca to 0f45758 Compare April 10, 2026 15:07
@toklumpp toklumpp changed the title Flaky test fix mockMCP fix(gen-ai): Fix flaky mock tests by correcting MCP API structure and improving test infrastructure Apr 10, 2026
@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch from 0f45758 to 75f4c81 Compare April 13, 2026 11:22
@toklumpp toklumpp marked this pull request as ready for review April 13, 2026 12:22
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress This PR is in WIP state label Apr 13, 2026
@openshift-ci openshift-ci Bot requested a review from antowaddle April 13, 2026 12:22
@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch 2 times, most recently from 47f5d9b to 1933412 Compare April 15, 2026 14:22
@toklumpp
Copy link
Copy Markdown
Contributor Author

/retest

@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch from 1933412 to e097a88 Compare April 15, 2026 16:51
@NickGagan NickGagan self-assigned this Apr 15, 2026
@toklumpp toklumpp changed the title fix(gen-ai): Fix flaky mock tests by correcting MCP API structure and improving test infrastructure fix(gen-ai): Fix flaky mcpTab test by re-querying server rows after modal closes Apr 15, 2026
@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch from e097a88 to 0ae37c8 Compare April 16, 2026 09:34
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 16, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from nickgagan. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@toklumpp
Copy link
Copy Markdown
Contributor Author

/retest

1 similar comment
@toklumpp
Copy link
Copy Markdown
Contributor Author

/retest

@NickGagan NickGagan self-requested a review April 16, 2026 14:01
Copy link
Copy Markdown
Contributor

@NickGagan NickGagan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reran the job a few times and it's failing: https://github.com/opendatahub-io/odh-dashboard/actions/runs/24505080643/job/71651562272

Can you try temporarily updating the github workflow to create artifacts from the saved videos so we can get more information on what's causing the failures?

@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch 5 times, most recently from 4dddc19 to 93d6572 Compare April 20, 2026 09:45
toklumpp and others added 8 commits April 20, 2026 14:29
…ferences

The comprehensive tool selection test was failing because after closing the success
modal, the table component remounts and the old serverRow reference becomes stale.

Re-querying the server row after the modal closes ensures we get a fresh reference
to the DOM element, fixing the 'element not found' timeout error.

This matches the pattern used in other tests that don't have issues with stale
references because they re-query elements after significant UI changes.
…nces

Fix flaky test by re-querying the server row after modal closes to avoid
stale DOM element references.

After the success modal closes, the table component may remount, making
the old serverRow reference stale. Re-querying ensures we get a fresh
reference to the DOM element before interacting with the tools button.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… to avoid stale references

Apply the same stale element fix to the "comprehensive tool selection operations" test that was previously applied to the auto-unlock test. Prevents Cypress timeout errors when clicking tools button after modal closes and components remount.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Apply stale element fixes to two more locations:
- First test: Re-query server row after success modal closes before clicking tools button
- Auto-unlock test: Use freshServerRow instead of old serverRow when re-opening tools modal

These follow the same pattern as previous fixes to prevent Cypress timeout errors from stale DOM references.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…n in comprehensive test

Add .should('exist').and('not.have.attr', 'aria-disabled') assertions before clicking the tools button in the comprehensive tool selection test. This ensures Cypress waits for the element to be ready before attempting to click, preventing timeout errors when the element is still mounting.

Follows the same defensive pattern used in the auto-unlock test.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…al closes

Replace direct findMCPServersTable().should('be.visible') calls with
verifyMCPTabVisible() method which includes a 30-second timeout to
handle component remount delays after success modal closes.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…dal in comprehensive test

Add verifyMCPTabVisible() call before querying server row when re-opening
the tools modal to verify selection persistence. This prevents timeout errors
when the MCP table hasn't fully remounted after closing the previous modal.

Completes the fix started in previous commits by applying the same defensive
pattern to the final location that was missing the visibility check.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…es in MCP tool selection test

Forces immediate row query after table visibility check using .then()
callback, eliminating stale element references without explicit timeouts.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@toklumpp toklumpp force-pushed the RHOAIENG-49598-gen-ai-test-fix branch from 5043251 to b9916a3 Compare April 20, 2026 13:29
@toklumpp
Copy link
Copy Markdown
Contributor Author

/retest

…s in MCP tools button

After modal closes and MCP table remounts, directly query for the tools
button using its testID instead of going through the page object's lazy
selector. This avoids stale element references and ensures the element
is found after the DOM updates.

The testID uses serverUrl which matches server.id (set in
transformMCPServerData), so direct query should reliably find the element.

Related: RHOAIENG-49598
@toklumpp toklumpp marked this pull request as draft April 21, 2026 12:05
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress This PR is in WIP state label Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants