Draft
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 a new Cypress page object module packages/cypress/cypress/pages/aiAssets.ts exporting an AiAssets class and singleton aiAssets for interacting with the Gen AI "AI asset endpoints" UI. Adds a findPageTitle() method to packages/cypress/cypress/pages/genAiPlayground.ts. Introduces a comprehensive end-to-end spec packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts covering navigation, redirects, namespace handling, broken-route checks, cross-session shared-link validation, project lifecycle setup/teardown, and conditional test skipping. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Actionable Issues
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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 |
toklumpp
force-pushed
the
RHOAIENG-37588
branch
from
59928c7 to
43b08aa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6944 +/- ##
==========================================
+ Coverage 64.98% 64.99% +0.01%
==========================================
Files 2446 2446
Lines 76161 76161
Branches 19213 19213
==========================================
+ Hits 49490 49502 +12
+ Misses 26671 26659 -12 see 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
toklumpp
force-pushed
the
RHOAIENG-37588
branch
3 times, most recently
from
1ec4635 to
4a2811a
Compare
toklumpp
force-pushed
the
RHOAIENG-37588
branch
4 times, most recently
from
ba72c1e to
a3c000a
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
packages/cypress/cypress/pages/aiAssets.ts (1)
11-13: Use a stable locator for the description.
cy.contains('Browse endpoints for models and MCP servers')is unscoped and tied to product copy, so a text tweak elsewhere on the page can make this pass or fail for the wrong reason. Prefer a dedicateddata-testid, or at least scope the assertion under a stable page-header container.As per coding guidelines, "Use data-testid selectors, not CSS classes (resilient to style changes)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/pages/aiAssets.ts` around lines 11 - 13, The findPageDescription() method uses an unscoped text matcher (cy.contains(...)) which is brittle; update findPageDescription() to use a stable selector instead—prefer a dedicated data-testid on the page header (e.g. data-testid="ai-assets-page-description") and change the locator to cy.get('[data-testid="ai-assets-page-description"]'), or at minimum scope the contains() call under the page header container (e.g. cy.get('[data-testid="ai-assets-page-header"]').contains(...)); modify the app markup to include the new data-testid if needed and update the test to reference the new attribute.
🤖 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/e2e/gen-ai/testGenAiNavigation.cy.ts`:
- Around line 16-57: The suite setup in retryableBefore() must early-exit when
running as a non-admin to avoid leaving projectName undefined or flip skipTest
incorrectly; add a guard that checks Cypress.env('IS_NON_ADMIN_RUN') at the top
of the hook and set skipTest = true (or return) before any logic that assigns
projectName or calls createCleanProject/enableGenAiFeatures; ensure the guard
runs prior to the getCustomResource(...) and project creation steps so
createCleanProject and enableGenAiFeatures are never invoked when
IS_NON_ADMIN_RUN is truthy.
- Around line 59-69: The after() hook currently calls disableGenAiFeatures()
which only patches resources but does not wait for the UI/feature flag to
actually clear; update the teardown to wait until Gen AI is fully disabled
before finishing the suite by adding a polling/wait step after
disableGenAiFeatures() that verifies the sidebar/feature flag is gone (e.g.,
implement or call a helper like
waitForGenAiDisabled()/waitForGenAiFeatureToggleToClear that polls the API or
checks the UI element disappears), then proceed to
deleteOpenShiftProject(projectName, { wait: false, ignoreNotFound: true }) —
ensure the new wait targets the same signals disableGenAiFeatures() modifies so
subsequent specs start with Gen AI disabled.
- Around line 304-305: The test currently uses a hard-coded invalidNamespace
('non-existent-project-12345') which can collide on shared clusters; change the
testGenAiNavigation.cy.ts code that sets invalidNamespace to instead generate a
unique value at runtime (e.g., use a uuid/UUID generator to build the string) so
the value used in cy.visit(`/gen-ai-studio/playground/${invalidNamespace}`) is
unique per run; update the variable named invalidNamespace and any related
usages in that test to use the generated UUID-based name.
- Around line 109-120: The tests labeled "Data scientist accesses Gen AI
resources via direct URL links" and the "Developer shares direct link" journey
are both reusing HTPASSWD_CLUSTER_ADMIN_USER and the same browser session, so
update the authentication to use the intended persona constants (e.g.,
DATASCIENTIST_USER, DEVELOPER_USER, COLLEAGUE_USER) instead of
HTPASSWD_CLUSTER_ADMIN_USER, and ensure each persona uses a fresh session (use
cy.session or clear cookies/localStorage between logins and call
cy.visitWithLogin for each distinct user) so RBAC and cross-user link access are
actually exercised; update occurrences of cy.visitWithLogin, the test titles in
testGenAiNavigation.cy.ts, and any related setup that currently references
HTPASSWD_CLUSTER_ADMIN_USER or reuses the same session.
- Around line 16-24: The uncaught exception handler inside retryableBefore that
currently checks err.message.includes('expected expression') ||
includes('Unexpected token') is too broad; update the
Cypress.on('uncaught:exception') predicate to only ignore the specific
federation/dev-server parse errors (e.g., check for exact substrings like
"Unexpected token '<'", "Unexpected token ':'", or known remote entry
URLs/module federation identifiers or module names appearing in err.message or
err.stack) and ensure other syntax errors still bubble up; apply the same
narrower check to the other occurrence in testGenAi.cy.ts so both handlers only
suppress the targeted federation/dev-server fallback errors.
- Around line 77-80: In testGenAiNavigation.cy.ts the current arrow-function
it() callbacks return early when skipTest is true which marks tests as passed
instead of skipped; change each affected it(...) callback from an arrow function
to a function() { ... } form and, at the top of the body before any Cypress
commands, call this.skip() when skipTest is true (e.g., if (skipTest) {
this.skip(); }), applying this change to the similar blocks referenced (around
the current branches at lines ~77, 114, 147, 172, 198, 223, 256, 295) so tests
are properly marked skipped instead of passing.
---
Nitpick comments:
In `@packages/cypress/cypress/pages/aiAssets.ts`:
- Around line 11-13: The findPageDescription() method uses an unscoped text
matcher (cy.contains(...)) which is brittle; update findPageDescription() to use
a stable selector instead—prefer a dedicated data-testid on the page header
(e.g. data-testid="ai-assets-page-description") and change the locator to
cy.get('[data-testid="ai-assets-page-description"]'), or at minimum scope the
contains() call under the page header container (e.g.
cy.get('[data-testid="ai-assets-page-header"]').contains(...)); modify the app
markup to include the new data-testid if needed and update the test to reference
the new attribute.
🪄 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
Run ID: d8e4beab-f2fc-410c-a3b7-1e0cd30bf93c
📥 Commits
Reviewing files that changed from the base of the PR and between 55a7893 and a3c000adb9a2aee75c3f44070533ca787214ecd7.
📒 Files selected for processing (3)
packages/cypress/cypress/pages/aiAssets.tspackages/cypress/cypress/pages/genAiPlayground.tspackages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts
Comment on lines
+109
to
+120
| 'Data scientist accesses Gen AI resources via direct URL links', | ||
| { | ||
| tags: ['@Sanity', '@GenAI', '@Navigation'], | ||
| }, | ||
| () => { | ||
| if (skipTest) { | ||
| cy.log('Skipping test - Gen AI is RHOAI-specific and not available on ODH.'); | ||
| return; | ||
| } | ||
|
|
||
| cy.step('Log into the application as Data scientist'); | ||
| cy.visitWithLogin('/', HTPASSWD_CLUSTER_ADMIN_USER); |
Contributor
There was a problem hiding this comment.
These persona stories never leave the admin session.
Both the "Data scientist" journey and the "Developer shares direct link" journey authenticate as HTPASSWD_CLUSTER_ADMIN_USER, and the "colleague" step just revisits the URL in the same browser session. That leaves RBAC and cross-user link access completely untested unless these cases use the intended personas and a fresh session.
Also applies to: 251-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts` around
lines 109 - 120, The tests labeled "Data scientist accesses Gen AI resources via
direct URL links" and the "Developer shares direct link" journey are both
reusing HTPASSWD_CLUSTER_ADMIN_USER and the same browser session, so update the
authentication to use the intended persona constants (e.g., DATASCIENTIST_USER,
DEVELOPER_USER, COLLEAGUE_USER) instead of HTPASSWD_CLUSTER_ADMIN_USER, and
ensure each persona uses a fresh session (use cy.session or clear
cookies/localStorage between logins and call cy.visitWithLogin for each distinct
user) so RBAC and cross-user link access are actually exercised; update
occurrences of cy.visitWithLogin, the test titles in testGenAiNavigation.cy.ts,
and any related setup that currently references HTPASSWD_CLUSTER_ADMIN_USER or
reuses the same session.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts (4)
60-69:⚠️ Potential issue | 🟠 MajorWait for Gen AI disablement to converge before finishing teardown.
disableGenAiFeatures()is invoked, but teardown does not wait for the feature state to fully clear, so following specs can start against stale enabled state.As per coding guidelines, "Test data cleanup: delete resources created during tests in after() hooks." and "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/e2e/gen-ai/testGenAiNavigation.cy.ts` around lines 60 - 69, Teardown calls disableGenAiFeatures() but does not wait for the feature state to actually clear, causing flakiness; update the after() to await disableGenAiFeatures() completion and then poll the feature state (e.g., via a new helper waitForGenAiDisabled() that repeatedly calls getGenAiFeatureState() or your existing status API until it returns "disabled") before proceeding to deleteOpenShiftProject(projectName, ...); ensure the after() returns a Promise (or uses Cypress commands properly) so teardown waits for the disablement to converge.
112-118:⚠️ Potential issue | 🟠 MajorPersona stories are still executed as admin and in one session.
The “Data scientist” and “Developer shares direct link” journeys both log in as
HTPASSWD_CLUSTER_ADMIN_USER, and the “colleague” step reuses the same authenticated session. This does not validate RBAC or cross-user link access.Also applies to: 229-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts` around lines 112 - 118, The tests "Data scientist accesses Gen AI resources via direct URL links" and the "Developer shares direct link" flow currently both call cy.visitWithLogin with HTPASSWD_CLUSTER_ADMIN_USER and reuse the same authenticated session (e.g., the "colleague" step), which fails to validate RBAC/cross-user access; update the tests to log in as distinct personas by replacing the second and subsequent cy.visitWithLogin/visit steps to use the correct non-admin credential constants (e.g., a DATA_SCIENTIST_USER and a DEVELOPER_USER) and ensure you start a fresh session between personas (use cy.clearCookies()/cy.clearLocalStorage() or a distinct cy.session call) so each journey runs under the intended user identity (look for uses of cy.visitWithLogin, HTPASSWD_CLUSTER_ADMIN_USER, and the "colleague" step to modify).
19-24:⚠️ Potential issue | 🟠 MajorNarrow exception suppression to federation-specific signatures only.
The current filter ignores any
"expected expression"/"Unexpected token"parse error, which can hide real regressions unrelated to missing federated modules.Proposed patch
Cypress.on('uncaught:exception', (err) => { - // Ignore SyntaxError from missing federated modules - if (err.message.includes('expected expression') || err.message.includes('Unexpected token')) { + const message = err.message ?? ''; + const stack = err.stack ?? ''; + const isKnownFederationFallback = + message.includes("Unexpected token '<'") || + message.includes("Unexpected token ':'") || + stack.includes('remoteEntry') || + stack.includes('module federation'); + + if (isKnownFederationFallback) { return false; } return true; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts` around lines 19 - 24, The uncaught exception handler currently suppresses any parse errors containing "expected expression" or "Unexpected token"; change this to only suppress errors that match those messages AND also indicate a federated-module failure (e.g., the error message or stack contains federation-specific signatures like "remoteEntry", "federation", "federated", "__webpack_require__", or a remote URL pattern). Update the Cypress.on('uncaught:exception', (err) => ...) predicate to use a combined check (regex or chained includes) so only federation-related parse errors are returned false while all other parse errors are allowed to bubble.
17-37:⚠️ Potential issue | 🟠 MajorAdd an explicit non-admin early skip guard before any setup commands.
This hook still proceeds without checking
Cypress.env('IS_NON_ADMIN_RUN'), so restricted runs can enter partial setup paths before being skipped.Proposed patch
retryableBefore(() => { + if (Cypress.env('IS_NON_ADMIN_RUN')) { + skipTest = true; + cy.log('Non-admin run detected; skipping Gen AI navigation suite.'); + return; + } + // Ignore module federation loading errors (for clusters without Gen AI modules deployed) Cypress.on('uncaught:exception', (err) => {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/e2e/gen-ai/testGenAiNavigation.cy.ts` around lines 17 - 37, Add an explicit early non-admin guard at the top of the retryableBefore hook: check Cypress.env('IS_NON_ADMIN_RUN') immediately inside retryableBefore and, if true, set skipTest = true and short-circuit the rest of the setup (return early) so no further Cypress commands (Cypress.on, getCustomResource, etc.) run for non-admin runs; update references to retryableBefore, Cypress.env('IS_NON_ADMIN_RUN'), and skipTest to ensure the guard runs before any other setup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts`:
- Around line 60-69: Teardown calls disableGenAiFeatures() but does not wait for
the feature state to actually clear, causing flakiness; update the after() to
await disableGenAiFeatures() completion and then poll the feature state (e.g.,
via a new helper waitForGenAiDisabled() that repeatedly calls
getGenAiFeatureState() or your existing status API until it returns "disabled")
before proceeding to deleteOpenShiftProject(projectName, ...); ensure the
after() returns a Promise (or uses Cypress commands properly) so teardown waits
for the disablement to converge.
- Around line 112-118: The tests "Data scientist accesses Gen AI resources via
direct URL links" and the "Developer shares direct link" flow currently both
call cy.visitWithLogin with HTPASSWD_CLUSTER_ADMIN_USER and reuse the same
authenticated session (e.g., the "colleague" step), which fails to validate
RBAC/cross-user access; update the tests to log in as distinct personas by
replacing the second and subsequent cy.visitWithLogin/visit steps to use the
correct non-admin credential constants (e.g., a DATA_SCIENTIST_USER and a
DEVELOPER_USER) and ensure you start a fresh session between personas (use
cy.clearCookies()/cy.clearLocalStorage() or a distinct cy.session call) so each
journey runs under the intended user identity (look for uses of
cy.visitWithLogin, HTPASSWD_CLUSTER_ADMIN_USER, and the "colleague" step to
modify).
- Around line 19-24: The uncaught exception handler currently suppresses any
parse errors containing "expected expression" or "Unexpected token"; change this
to only suppress errors that match those messages AND also indicate a
federated-module failure (e.g., the error message or stack contains
federation-specific signatures like "remoteEntry", "federation", "federated",
"__webpack_require__", or a remote URL pattern). Update the
Cypress.on('uncaught:exception', (err) => ...) predicate to use a combined check
(regex or chained includes) so only federation-related parse errors are returned
false while all other parse errors are allowed to bubble.
- Around line 17-37: Add an explicit early non-admin guard at the top of the
retryableBefore hook: check Cypress.env('IS_NON_ADMIN_RUN') immediately inside
retryableBefore and, if true, set skipTest = true and short-circuit the rest of
the setup (return early) so no further Cypress commands (Cypress.on,
getCustomResource, etc.) run for non-admin runs; update references to
retryableBefore, Cypress.env('IS_NON_ADMIN_RUN'), and skipTest to ensure the
guard runs before any other setup logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1130552b-4fb6-45b5-9b53-f7303b7715ef
📥 Commits
Reviewing files that changed from the base of the PR and between a3c000adb9a2aee75c3f44070533ca787214ecd7 and 7de3756facaf959fd37a5905413c52475d5c25e7.
📒 Files selected for processing (1)
packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts
toklumpp
force-pushed
the
RHOAIENG-37588
branch
2 times, most recently
from
14dc955 to
dfc557f
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts (3)
65-75:⚠️ Potential issue | 🟠 MajorTeardown is non-deterministic and can leak state into following specs.
Lines 70-74 disable features and delete the project without waiting (
wait: false), so subsequent suites can start before cleanup is complete.Proposed fix
after(() => { if (skipTest) { return; } - disableGenAiFeatures(); + disableGenAiFeatures(); if (projectName) { - deleteOpenShiftProject(projectName, { wait: false, ignoreNotFound: true }); + deleteOpenShiftProject(projectName, { wait: true, ignoreNotFound: true }); } });As per coding guidelines, "Test data cleanup: delete resources created during tests in after() hooks." and "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/e2e/gen-ai/testGenAiNavigation.cy.ts` around lines 65 - 75, The after() teardown is non-deterministic: make the hook async and ensure deterministic cleanup by awaiting disableGenAiFeatures() (if it is async) and awaiting deleteOpenShiftProject(projectName, { wait: true, ignoreNotFound: true }) instead of using wait: false; also keep the existing skipTest guard and only call deleteOpenShiftProject when projectName is set so the hook reliably completes before the next spec runs.
117-124:⚠️ Potential issue | 🟠 MajorPersona journeys are not exercising persona behavior.
Line 123 and Line 240 log in as
HTPASSWD_CLUSTER_ADMIN_USERfor stories labeled "Data scientist" and "Developer/Colleague", and Lines 250-253 reuse the same session. This does not validate role-specific or cross-user link behavior.Proposed fix
- cy.step('Log into the application as Data scientist'); - cy.visitWithLogin('/', HTPASSWD_CLUSTER_ADMIN_USER); + cy.step('Log into the application as Data scientist'); + cy.visitWithLogin('/', DATASCIENTIST_USER); ... - cy.step('Log into the application as first user (Developer)'); - cy.visitWithLogin('/', HTPASSWD_CLUSTER_ADMIN_USER); + cy.step('Log into the application as first user (Developer)'); + cy.visitWithLogin('/', DEVELOPER_USER); ... cy.step('Colleague accesses the shared link'); cy.then(() => { + cy.clearCookies(); + cy.clearLocalStorage(); + cy.visitWithLogin('/', COLLEAGUE_USER); cy.visit(sharedUrl); });Also applies to: 234-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts` around lines 117 - 124, The tests labeled for different personas are all logging in with HTPASSWD_CLUSTER_ADMIN_USER and reusing the same session (via cy.visitWithLogin and the shared session block), so update the persona-specific tests (e.g., the test titled 'Data scientist accesses Gen AI resources via direct URL links' and the Developer/Colleague story) to log in with distinct test accounts that reflect their roles instead of HTPASSWD_CLUSTER_ADMIN_USER, and stop reusing the single session for cross-user validation by creating separate cy.visitWithLogin/cy.session calls per persona; locate usages of cy.visitWithLogin and any shared session helper in this spec and replace the admin user with role-specific constants (e.g., HTPASSWD_DATA_SCIENTIST_USER, HTPASSWD_DEVELOPER_USER) and ensure each persona test creates its own session.
24-30:⚠️ Potential issue | 🟠 MajorUncaught-exception suppression is too broad and can hide real regressions.
Line 26 ignores any parse error containing generic text (
expected expression/Unexpected token), which can suppress unrelated app failures. Narrow this to known federation/dev-server signatures only.Proposed fix
- Cypress.on('uncaught:exception', (err) => { - // Ignore SyntaxError from missing federated modules - if (err.message.includes('expected expression') || err.message.includes('Unexpected token')) { - return false; - } - return true; - }); + Cypress.on('uncaught:exception', (err) => { + const message = err.message ?? ''; + const stack = err.stack ?? ''; + const knownFederationParseError = + message.includes("Unexpected token '<'") || + message.includes("Unexpected token ':'") || + /remoteEntry|module federation|federated/i.test(`${message}\n${stack}`); + + if (knownFederationParseError) { + return false; + } + return true; + });#!/bin/bash set -euo pipefail # Compare exception-ignore patterns used in Gen AI specs and global Cypress support config. rg -n --type ts -C3 "uncaught:exception|Unexpected token|expected expression|remoteEntry|module federation" \ packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts \ packages/cypress/cypress/tests/e2e/gen-ai/testGenAi.cy.ts \ packages/cypress/cypress/support/e2e.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts` around lines 24 - 30, The uncaught-exception handler registered via Cypress.on('uncaught:exception', (err) => ...) is too broad — replace the generic string checks for 'expected expression' / 'Unexpected token' with narrowly targeted patterns that match known federation/dev-server failure signatures (e.g., 'remoteEntry', 'module federation', a dev-server URL fragment or the exact error message emitted by the federated module loader) so only those known, benign parse errors are suppressed; update the handler to check err.message for those specific identifiers and return false only when they match, otherwise rethrow/return true to allow real regressions to surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts`:
- Around line 65-75: The after() teardown is non-deterministic: make the hook
async and ensure deterministic cleanup by awaiting disableGenAiFeatures() (if it
is async) and awaiting deleteOpenShiftProject(projectName, { wait: true,
ignoreNotFound: true }) instead of using wait: false; also keep the existing
skipTest guard and only call deleteOpenShiftProject when projectName is set so
the hook reliably completes before the next spec runs.
- Around line 117-124: The tests labeled for different personas are all logging
in with HTPASSWD_CLUSTER_ADMIN_USER and reusing the same session (via
cy.visitWithLogin and the shared session block), so update the persona-specific
tests (e.g., the test titled 'Data scientist accesses Gen AI resources via
direct URL links' and the Developer/Colleague story) to log in with distinct
test accounts that reflect their roles instead of HTPASSWD_CLUSTER_ADMIN_USER,
and stop reusing the single session for cross-user validation by creating
separate cy.visitWithLogin/cy.session calls per persona; locate usages of
cy.visitWithLogin and any shared session helper in this spec and replace the
admin user with role-specific constants (e.g., HTPASSWD_DATA_SCIENTIST_USER,
HTPASSWD_DEVELOPER_USER) and ensure each persona test creates its own session.
- Around line 24-30: The uncaught-exception handler registered via
Cypress.on('uncaught:exception', (err) => ...) is too broad — replace the
generic string checks for 'expected expression' / 'Unexpected token' with
narrowly targeted patterns that match known federation/dev-server failure
signatures (e.g., 'remoteEntry', 'module federation', a dev-server URL fragment
or the exact error message emitted by the federated module loader) so only those
known, benign parse errors are suppressed; update the handler to check
err.message for those specific identifiers and return false only when they
match, otherwise rethrow/return true to allow real regressions to surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8401546d-df58-41ef-9e95-7804154010fd
📥 Commits
Reviewing files that changed from the base of the PR and between 14dc95579c81292932ab540bf8348a542e5596f4 and dfc557f54d1cba2a2e41793823fd84a00978fa49.
📒 Files selected for processing (3)
packages/cypress/cypress/pages/aiAssets.tspackages/cypress/cypress/pages/genAiPlayground.tspackages/cypress/cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cypress/cypress/pages/aiAssets.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cypress/cypress/pages/genAiPlayground.ts
toklumpp
force-pushed
the
RHOAIENG-37588
branch
6 times, most recently
from
44e6024 to
7406dfd
Compare
Contributor
|
[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 |
toklumpp
changed the title
openshift-ci
Bot
added
needs-rebase
toklumpp
force-pushed
the
RHOAIENG-37588
branch
5 times, most recently
from
b42c06c to
b32825b
Compare
Implements 8 user-story focused E2E tests for Gen AI navigation: - Administrator navigates between Gen AI sections - Data scientist accesses resources via direct URLs - User gets automatic redirect to default page - User redirected when accessing without namespace - User receives clear guidance for broken links - User understands current location while navigating - Developer shares direct links for team collaboration - User receives feedback for invalid namespace New files: - cypress/pages/aiAssets.ts - AI Assets page object - cypress/tests/e2e/gen-ai/testGenAiNavigation.cy.ts - Navigation tests Updated: - cypress/pages/genAiPlayground.ts - Added findPageTitle() method Tests follow user-story focused pattern, representing real user workflows rather than technical UI checks. Tagged with @sanity, @genai, @Navigation. RHOAI-only with proper skip handling for ODH. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixes "Property 'then' does not exist on type 'void'" error. createCleanProject returns void, not a Chainable, so it cannot be chained with .then(). Split the logic into separate .then() blocks to match the pattern used in testGenAi.cy.ts. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Use unique UUID for invalidNamespace to prevent collisions on shared clusters - Implement proper test skipping with beforeEach hook using this.skip() - Remove redundant skipTest checks from individual test cases - Tests now correctly report as skipped instead of passed when Gen AI is unavailable Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add early guard to set skipTest=true when running in non-admin mode - Prevents undefined projectName errors when retryableBefore skips setup - Ensures tests properly skip in non-admin CI runs instead of failing The retryableHooks framework returns early when IS_NON_ADMIN_RUN=true, preventing the test setup callback from executing. This guard ensures skipTest is set before retryableBefore runs. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Improve test determinism and accuracy in Gen AI navigation E2E tests: 1. **Deterministic cleanup**: Make after() hook async and await cleanup - Changed deleteOpenShiftProject to use wait: true for reliable teardown - Added await for disableGenAiFeatures() to ensure complete cleanup 2. **Correct persona validation**: Use role-appropriate user accounts - Data scientist test now uses LDAP_CONTRIBUTOR_USER - Developer collaboration test now uses LDAP_CONTRIBUTOR_USER - Validates actual role-based access instead of all admin users 3. **Narrow exception suppression**: Target federation-specific errors only - Replace generic syntax error patterns with federation identifiers - Only suppress 'remoteEntry', 'module federation', and dynamic import errors - Allow other parse errors to surface as real regressions Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…istener accumulation
Move Cypress.on('uncaught:exception') registration from inside retryableBefore()
to describe block level to avoid accumulating multiple event listeners on retries.
Problem: Registering the handler inside retryableBefore causes multiple
identical listeners to be created on each retry attempt, leading to:
- Memory leaks from accumulated event handlers
- Potential race conditions or unexpected behavior
- Violation of Cypress best practices
Solution: Register the handler once at the describe block level, ensuring
it is only created once for the entire test suite.
Based on review feedback from PR opendatahub-io#6229 by andrewballantyne.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Remove emoji character from cy.log statement to maintain consistent code style without decorative characters. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
toklumpp
force-pushed
the
RHOAIENG-37588
branch
from
b32825b to
08897c0
Compare
The Gen AI playground test was failing with a timeout waiting for a tabpanel element. Root cause was a race condition where the test navigated to the assets page before the project selector finished loading. The assets page requires a loaded project context to render its content. Without waiting for the project selector, the page would get stuck in a loading state with the selector showing "Loading... No projects", preventing the Gen AI module from rendering the tabpanel. Changes: - Added explicit wait for project-selector-toggle in waitForAssetsPageLoad() - Ensures the selector is not disabled and not showing "Loading..." - This wait happens between the page title check and tabpanel check This fix applies to all tests using navigateToAssets(), making them more robust against timing issues. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Dashboard requires status.components.llamastackoperator.managementState to be 'Managed', not just the LlamaStackOperatorReady condition. Added polling to wait for the status field to reconcile after patching the spec. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://redhat.atlassian.net/browse/RHOAIENG-37588
Description
Implements 8 E2E tests for Gen AI navigation:
Tests follow user-story focused pattern, representing real user workflows rather than technical UI checks.
How Has This Been Tested?
test working
Test Impact
Tests
Request review criteria:
Self checklist (all need to be checked):
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:
mainSummary by CodeRabbit