Add mock tests for mlflow experiments and prompts pages#7371
Add mock tests for mlflow experiments and prompts pages#7371openshift-merge-bot[bot] merged 3 commits intoopendatahub-io:mainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 28 seconds. ⌛ 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe changes reorganize Cypress page-object infrastructure for testing MLflow and Prompt Management features. Two theme toggle methods are moved from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes IssuesNone identified. The changes follow established test infrastructure patterns without introducing security concerns, hardcoded credentials, or architectural conflicts. localStorage access for 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ 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. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cypress/cypress/tests/e2e/promptManagement/testPromptManagement.cy.ts (1)
114-115:⚠️ Potential issue | 🟡 MinorStep label doesn't match the action.
The step says "Navigate away to home page" but the code only asserts
appChrome.findMainContent().should('be.visible')— it doesn't actually navigate anywhere. Either remove the misleading step or perform the navigation (e.g.cy.visit('/')/ click a home nav item) before the assertion, otherwise the subsequent "Navigate back to Prompt Management page" step is meaningless because we never left.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/promptManagement/testPromptManagement.cy.ts` around lines 114 - 115, The step label "Navigate away to home page" is misleading because the test only asserts appChrome.findMainContent().should('be.visible') and never performs navigation; either remove or change the cy.step label to reflect a visibility assertion, or actually navigate away before asserting (e.g. perform a visit or click the app's home nav) so that the later "Navigate back to Prompt Management page" step is meaningful; update the cy.step text and/or add the navigation action immediately before appChrome.findMainContent().should('be.visible') in this test.
🧹 Nitpick comments (3)
packages/cypress/cypress/tests/mocked/mlflow/mlflowExperiments.cy.ts (1)
44-53: Asserting onpf-m-currentcouples the test to a PatternFly internal class.
findNavItem().should('have.class', 'pf-m-current')will break silently on any PatternFly major bump that renames the "current" modifier (it has already changed between v4/v5/v6). Prefer an accessibility/semantic assertion such asshould('have.attr', 'aria-current', 'page')if the nav exposes it, which is both more stable and closer to what users/AT actually observe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/mocked/mlflow/mlflowExperiments.cy.ts` around lines 44 - 53, The test currently asserts PatternFly's internal CSS modifier by calling mlflowExperiments.findNavItem().should('have.class', 'pf-m-current'); change this to a semantic/accessibility assertion by checking the navigation item's ARIA state (e.g., mlflowExperiments.findNavItem().should('have.attr', 'aria-current', 'page')) so the test relies on stable semantics rather than PF internals; update the assertion in the "should navigate via sidebar and show active nav item" spec where findNavItem() is used and remove the pf-m-current class check (or add an aria-current fallback assert if the nav component supports it).packages/cypress/cypress/tests/mocked/mlflow/promptManagement.cy.ts (1)
65-81: RedundantinitIntercepts()re-invocation afterbeforeEach.
beforeEachalready raninitIntercepts()with defaults. Calling it again inside theitregisters a second set ofcy.intercepthandlers (Cypress uses the last match, so it works, but it also re-invokesasProductAdminUser()and stacks intercepts). Prefer passing overrides via a single setup path, e.g. movinginitInterceptsinto a per-test setup and skippingbeforeEach, or exposing an override helper that replaces rather than layers.Not blocking, but it obscures which mock wins and makes future debugging harder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/mocked/mlflow/promptManagement.cy.ts` around lines 65 - 81, The test re-calls initIntercepts() inside the it block causing layered cy.intercept handlers; instead remove the in-test initIntercepts({ genAiStudio: false }) call and supply the override via the existing beforeEach setup (or add a per-test setup helper) so only one initIntercepts invocation runs; reference initIntercepts, beforeEach, and the it case that currently calls initIntercepts({ genAiStudio: false }) and for the MLflow test use mockDscStatus + cy.interceptOdh to set the MLflow Removed state without re-invoking initIntercepts.packages/cypress/cypress/pages/mlflowExperiments.ts (1)
83-85: Replace brittle substring selector with exact-name matcher or explicit data-testid.The selector
cy.contains('[role="button"][aria-pressed]', label)uses substring matching, risking false positives (e.g.,'GenAI'matching'GenAI (beta)') and interference from unrelated toggle groups on the page. Usecy.findAllByRole('button', { pressed: true, name: /^${label}$/ })for exact-match semantics, or add and target adata-testidon the toggle items if the upstream MLflow component supports it. This aligns with the E2E test standards requiringdata-testidselectors for resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/pages/mlflowExperiments.ts` around lines 83 - 85, The current findExperimentTypeToggleItem uses a brittle substring selector; update it to use exact-role matching or a test id: replace cy.contains('[role="button"][aria-pressed]', label) in findExperimentTypeToggleItem with cy.findAllByRole('button', { pressed: true, name: new RegExp(`^${label}$`) }) for an exact name match, or change the MLflow toggle to expose a stable data-testid and select by cy.get('[data-testid="..."]', { timeout: ... }) to target the specific toggle item reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cypress/cypress/tests/mocked/mlflow/mlflowExperiments.cy.ts`:
- Around line 71-81: Update the test in the Dark mode toggle spec to be
idempotent and accurate: before interacting, call cy.clearLocalStorage() (or
assert a known baseline via mlflowExperiments.getMlflowDarkModeStorageValue())
so the first click cannot rely on prior state; change the it title to remove
"class" or add an assertion for the HTML class (e.g. use
cy.get('html').should('have.class', 'pf-v6-theme-dark')) alongside the existing
localStorage checks; keep the existing calls to
mlflowExperiments.visit(PROJECT_A),
appChrome.findDarkThemeToggle()/findLightThemeToggle(), and
mlflowExperiments.getMlflowDarkModeStorageValue() but add the precondition
clear/assert and the html class assertion if you want the title to remain
unchanged.
In `@packages/cypress/cypress/tests/mocked/mlflow/promptManagement.cy.ts`:
- Around line 46-56: Ensure the test sets a deterministic MLflow dark-mode
precondition before exercising the toggles: before calling
promptManagement.visit(PROJECT_A) or before clicking, explicitly set or clear
the localStorage key `_mlflow_dark_mode_toggle_enabled` via promptManagement
helpers (or use promptManagement.clearMlflowDarkModeStorage /
promptManagement.setMlflowDarkModeStorage) so the initial state is known, then
assert the toggle changes the value using
appChrome.findDarkThemeToggle().click(),
promptManagement.getMlflowDarkModeStorageValue(), and
appChrome.findLightThemeToggle().click(); this makes the test idempotent and
removes leakage across specs.
---
Outside diff comments:
In
`@packages/cypress/cypress/tests/e2e/promptManagement/testPromptManagement.cy.ts`:
- Around line 114-115: The step label "Navigate away to home page" is misleading
because the test only asserts appChrome.findMainContent().should('be.visible')
and never performs navigation; either remove or change the cy.step label to
reflect a visibility assertion, or actually navigate away before asserting (e.g.
perform a visit or click the app's home nav) so that the later "Navigate back to
Prompt Management page" step is meaningful; update the cy.step text and/or add
the navigation action immediately before
appChrome.findMainContent().should('be.visible') in this test.
---
Nitpick comments:
In `@packages/cypress/cypress/pages/mlflowExperiments.ts`:
- Around line 83-85: The current findExperimentTypeToggleItem uses a brittle
substring selector; update it to use exact-role matching or a test id: replace
cy.contains('[role="button"][aria-pressed]', label) in
findExperimentTypeToggleItem with cy.findAllByRole('button', { pressed: true,
name: new RegExp(`^${label}$`) }) for an exact name match, or change the MLflow
toggle to expose a stable data-testid and select by
cy.get('[data-testid="..."]', { timeout: ... }) to target the specific toggle
item reliably.
In `@packages/cypress/cypress/tests/mocked/mlflow/mlflowExperiments.cy.ts`:
- Around line 44-53: The test currently asserts PatternFly's internal CSS
modifier by calling mlflowExperiments.findNavItem().should('have.class',
'pf-m-current'); change this to a semantic/accessibility assertion by checking
the navigation item's ARIA state (e.g.,
mlflowExperiments.findNavItem().should('have.attr', 'aria-current', 'page')) so
the test relies on stable semantics rather than PF internals; update the
assertion in the "should navigate via sidebar and show active nav item" spec
where findNavItem() is used and remove the pf-m-current class check (or add an
aria-current fallback assert if the nav component supports it).
In `@packages/cypress/cypress/tests/mocked/mlflow/promptManagement.cy.ts`:
- Around line 65-81: The test re-calls initIntercepts() inside the it block
causing layered cy.intercept handlers; instead remove the in-test
initIntercepts({ genAiStudio: false }) call and supply the override via the
existing beforeEach setup (or add a per-test setup helper) so only one
initIntercepts invocation runs; reference initIntercepts, beforeEach, and the it
case that currently calls initIntercepts({ genAiStudio: false }) and for the
MLflow test use mockDscStatus + cy.interceptOdh to set the MLflow Removed state
without re-invoking initIntercepts.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 591fd2a9-3bd8-45bf-898c-945c52b1a36a
📒 Files selected for processing (6)
packages/cypress/cypress/pages/appChrome.tspackages/cypress/cypress/pages/mlflowExperiments.tspackages/cypress/cypress/pages/promptManagement.tspackages/cypress/cypress/tests/e2e/promptManagement/testPromptManagement.cy.tspackages/cypress/cypress/tests/mocked/mlflow/mlflowExperiments.cy.tspackages/cypress/cypress/tests/mocked/mlflow/promptManagement.cy.ts
💤 Files with no reviewable changes (1)
- packages/cypress/cypress/pages/promptManagement.ts
| describe('Dark mode toggle', () => { | ||
| it('should sync dark mode class and localStorage on toggle', () => { | ||
| mlflowExperiments.visit(PROJECT_A); | ||
|
|
||
| appChrome.findDarkThemeToggle().click(); | ||
| mlflowExperiments.getMlflowDarkModeStorageValue().should('equal', 'true'); | ||
|
|
||
| appChrome.findLightThemeToggle().click(); | ||
| mlflowExperiments.getMlflowDarkModeStorageValue().should('equal', 'false'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Same idempotency gap as in promptManagement.cy.ts, plus a misleading title.
- The title says "should sync dark mode class and localStorage on toggle" but the body only asserts
localStorage. Either drop "class" from the title or also assert the<html>class (e.g.cy.get('html').should('have.class', 'pf-v6-theme-dark')). localStorageis not cleared between tests, so the firstclick()may be toggling from an already-dark baseline and the'true'assertion passes trivially. Addcy.clearLocalStorage()(or assert the pre-toggle baseline) to make the test prove a real round-trip.
🔧 Proposed fix
describe('Dark mode toggle', () => {
- it('should sync dark mode class and localStorage on toggle', () => {
+ it('should sync localStorage on toggle', () => {
+ cy.clearLocalStorage();
mlflowExperiments.visit(PROJECT_A);
+ mlflowExperiments.getMlflowDarkModeStorageValue().should('not.equal', 'true');
appChrome.findDarkThemeToggle().click();
mlflowExperiments.getMlflowDarkModeStorageValue().should('equal', 'true');
appChrome.findLightThemeToggle().click();
mlflowExperiments.getMlflowDarkModeStorageValue().should('equal', 'false');
});
});As per coding guidelines: "Tests must be idempotent: runnable in any order without shared state."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cypress/cypress/tests/mocked/mlflow/mlflowExperiments.cy.ts` around
lines 71 - 81, Update the test in the Dark mode toggle spec to be idempotent and
accurate: before interacting, call cy.clearLocalStorage() (or assert a known
baseline via mlflowExperiments.getMlflowDarkModeStorageValue()) so the first
click cannot rely on prior state; change the it title to remove "class" or add
an assertion for the HTML class (e.g. use cy.get('html').should('have.class',
'pf-v6-theme-dark')) alongside the existing localStorage checks; keep the
existing calls to mlflowExperiments.visit(PROJECT_A),
appChrome.findDarkThemeToggle()/findLightThemeToggle(), and
mlflowExperiments.getMlflowDarkModeStorageValue() but add the precondition
clear/assert and the html class assertion if you want the title to remain
unchanged.
| describe('Dark mode toggle', () => { | ||
| it('should sync localStorage on toggle', () => { | ||
| promptManagement.visit(PROJECT_A); | ||
|
|
||
| appChrome.findDarkThemeToggle().click(); | ||
| promptManagement.getMlflowDarkModeStorageValue().should('equal', 'true'); | ||
|
|
||
| appChrome.findLightThemeToggle().click(); | ||
| promptManagement.getMlflowDarkModeStorageValue().should('equal', 'false'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Dark-mode test leaks localStorage across specs/tests.
The MLflow dark-mode flag is written to localStorage (key _mlflow_dark_mode_toggle_enabled). Cypress 13 does not automatically clear localStorage between tests, so if any prior test (or a previous run in --no-exit/open mode) left 'true' behind, the click on findDarkThemeToggle() may toggle into an already-dark state and the subsequent assertion should('equal', 'true') becomes a no-op rather than a genuine round-trip check. Worse, the findLightThemeToggle().click() assertion flips brittle in the reverse order.
Make the pre-conditions explicit so the test is idempotent and actually proves the toggle behavior.
🔧 Proposed fix
describe('Dark mode toggle', () => {
it('should sync localStorage on toggle', () => {
+ cy.clearLocalStorage();
promptManagement.visit(PROJECT_A);
+ // Baseline: light theme
+ promptManagement.getMlflowDarkModeStorageValue().should('not.equal', 'true');
+
appChrome.findDarkThemeToggle().click();
promptManagement.getMlflowDarkModeStorageValue().should('equal', 'true');
appChrome.findLightThemeToggle().click();
promptManagement.getMlflowDarkModeStorageValue().should('equal', 'false');
});
});As per coding guidelines: "Tests must be idempotent: runnable in any order without shared state."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('Dark mode toggle', () => { | |
| it('should sync localStorage on toggle', () => { | |
| promptManagement.visit(PROJECT_A); | |
| appChrome.findDarkThemeToggle().click(); | |
| promptManagement.getMlflowDarkModeStorageValue().should('equal', 'true'); | |
| appChrome.findLightThemeToggle().click(); | |
| promptManagement.getMlflowDarkModeStorageValue().should('equal', 'false'); | |
| }); | |
| }); | |
| describe('Dark mode toggle', () => { | |
| it('should sync localStorage on toggle', () => { | |
| cy.clearLocalStorage(); | |
| promptManagement.visit(PROJECT_A); | |
| // Baseline: light theme | |
| promptManagement.getMlflowDarkModeStorageValue().should('not.equal', 'true'); | |
| appChrome.findDarkThemeToggle().click(); | |
| promptManagement.getMlflowDarkModeStorageValue().should('equal', 'true'); | |
| appChrome.findLightThemeToggle().click(); | |
| promptManagement.getMlflowDarkModeStorageValue().should('equal', 'false'); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cypress/cypress/tests/mocked/mlflow/promptManagement.cy.ts` around
lines 46 - 56, Ensure the test sets a deterministic MLflow dark-mode
precondition before exercising the toggles: before calling
promptManagement.visit(PROJECT_A) or before clicking, explicitly set or clear
the localStorage key `_mlflow_dark_mode_toggle_enabled` via promptManagement
helpers (or use promptManagement.clearMlflowDarkModeStorage /
promptManagement.setMlflowDarkModeStorage) so the initial state is known, then
assert the toggle changes the value using
appChrome.findDarkThemeToggle().click(),
promptManagement.getMlflowDarkModeStorageValue(), and
appChrome.findLightThemeToggle().click(); this makes the test idempotent and
removes leakage across specs.
nananosirova
force-pushed
the
RHOAIENG-59124
branch
from
89c41d4 to
429ddb2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7371 +/- ##
==========================================
+ Coverage 65.07% 65.09% +0.02%
==========================================
Files 2458 2466 +8
Lines 76491 76565 +74
Branches 19317 19338 +21
==========================================
+ Hits 49775 49842 +67
- Misses 26716 26723 +7 see 22 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
DaoDaoNoCode
left a comment
DaoDaoNoCode
left a comment
There was a problem hiding this comment.
Nice set of mock tests for the MLflow experiments and prompts page wrappers. Good call moving the dark mode toggle methods to appChrome since they're app-level controls, and moving the dark mode testing from E2E to mock tests where it's faster and more appropriate. Structure looks clean -- page objects for all selectors, no direct cy commands in tests, proper beforeEach setup, mock data from __mocks__.
Local checks: lint ✅ (0 errors, only pre-existing warnings) · CI ✅ (all 43 checks pass including the new mlflow mock test suite)
Left a couple of nits inline but nothing blocking.
nananosirova
force-pushed
the
RHOAIENG-59124
branch
from
cc0b588 to
4ccf24a
Compare
Signed-off-by: Nana Nosirova <10577112+nananosirova@users.noreply.github.com>
….cy.ts Co-authored-by: Juntao Wang <37624318+DaoDaoNoCode@users.noreply.github.com>
….cy.ts Co-authored-by: Juntao Wang <37624318+DaoDaoNoCode@users.noreply.github.com>
nananosirova
force-pushed
the
RHOAIENG-59124
branch
from
4ccf24a to
2bd43f5
Compare
DaoDaoNoCode
left a comment
DaoDaoNoCode
left a comment
There was a problem hiding this comment.
Re-reviewed after the two fix commits. Both nits from the initial review are addressed: the nav assertion now uses aria-current="page" instead of pf-m-current, and the dark mode test title was updated to match what the test actually asserts. The getHtmlDarkModeClass helper and per-page theme toggle methods were also cleaned up and consolidated into appChrome, which is a nice improvement. Lint is clean, all CI checks pass. Nothing else to flag.
Local checks: lint (0 errors, pre-existing warnings only), CI mlflow mock tests pass.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DaoDaoNoCode 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 |
openshift-merge-bot
Bot
merged commit e302dcb
into
opendatahub-io:main
2 participants
https://redhat.atlassian.net/browse/RHOAIENG-59124
Description
Added Cypress mock tests for the MLflow Experiments and Prompt Management page wrappers.
How Has This Been Tested?
Mock tests cover:
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit