Open
Conversation
Contributor
|
cc @etmurasaki |
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
etmurasaki
reviewed
Feb 2, 2026
There was a problem hiding this comment.
all these elements are related to monitoring-plugin and I believe you should only the ones used in your test
Contributor
Author
There was a problem hiding this comment.
Yes. I'd like to re-use these elements. I will add more troubleshoot and Logging elements and Consider backporting to monitoring-plugin someday.
etmurasaki
reviewed
Feb 2, 2026
openshift-merge-robot
added
the
needs-rebase
anpingli
force-pushed
the
web-test
branch
2 times, most recently
from
6a5b5e6 to
809e3ea
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
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:
WalkthroughMigration of end-to-end testing framework from integration-tests to cypress directory structure, introducing Cypress configuration, custom commands for authentication and UI interaction, test fixtures, and test execution infrastructure with OpenShift integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
web/cypress/README.md-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorFix heading/language to satisfy markdownlint and improve clarity.
Line [1]violates MD018 (missing space after#) and has typos.✍️ Proposed fix
-#The following file are copyed from https://github.com/openshift/logging-view-plugin/tree/main/web. These files will be updated Ad hoc +# The following files are copied from https://github.com/openshift/logging-view-plugin/tree/main/web. These files will be updated ad hoc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/README.md` at line 1, Fix the markdown heading text "#The following file are copyed from https://github.com/openshift/logging-view-plugin/tree/main/web. These files will be updated Ad hoc" by adding the required space after '#' to satisfy MD018 and correct typos: change "file" to "files", "copyed" to "copied", and make "Ad hoc" lowercase "ad hoc" (or "ad-hoc" per style); update the heading line (the string starting with "#The following file are copyed...") in web/cypress/README.md accordingly.web/scripts/run-e2e.sh-18-18 (1)
18-18:⚠️ Potential issue | 🟡 Minor
SCRIPT_DIRis unused.
SCRIPT_DIRis assigned at line 18 but never referenced;script_dir(lowercase) is redeclared at line 158 and used instead. Remove the duplicate declaration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` at line 18, Remove the unused duplicate environment variable by deleting the unnecessary uppercase declaration SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"; keep the existing lowercase script_dir declaration used later (script_dir at the later block) and ensure any references use that single variable name to avoid duplication and confusion.web/scripts/run-e2e.sh-4-14 (1)
4-14:⚠️ Potential issue | 🟡 MinorStale description references Logging UI and irrelevant prerequisites.
Lines 4–8 describe "Logging UI" test prerequisites (clusterlogging, lokistack, log-test-app namespaces) that have nothing to do with the Troubleshooting Panel plugin. Update the header comment to accurately describe this script's purpose and prerequisites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 4 - 14, The header comment currently references "Logging UI" and unrelated prerequisites (clusterlogging, lokistack, log-test-app namespaces); update the top-of-script comment in run-e2e.sh to describe this script runs Troubleshooting Panel plugin E2E tests and list correct prerequisites: exported KUBECONFIG, optional CYPRESS_SPEC and CYPRESS_TAG, how to supply login users via CYPRESS_LOGIN_IDP/CYPRESS_LOGIN_USERS or enable_idp_htpasswd, and any namespace/test data requirements specific to the Troubleshooting Panel rather than the old Logging UI details; edit the comment block (the script header) to remove references to clusterlogging, lokistack, and log-test-app namespaces and replace them with accurate Troubleshooting Panel prerequisites and usage notes.web/scripts/run-e2e.sh-53-53 (1)
53-53:⚠️ Potential issue | 🟡 MinorEcho message has a duplicated IDP name.
"uiauto-htpasswd-idpuiauto-htpasswd-idp" is a copy-paste artifact.
🐛 Proposed fix
- echo "Create new users and add uiauto-htpasswd-idpuiauto-htpasswd-idp" + echo "Create new users and add uiauto-htpasswd-idp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` at line 53, The echo message in run-e2e.sh contains a duplicated IDP name ("uiauto-htpasswd-idpuiauto-htpasswd-idp"); update the string in the echo statement to the correct single IDP name (e.g., "uiauto-htpasswd-idp") so the message reads "Create new users and add uiauto-htpasswd-idp", ensuring you modify the echo line accordingly.web/scripts/run-e2e.sh-158-159 (1)
158-159:⚠️ Potential issue | 🟡 Minor
cdwithout a failure guard; also re-declaresSCRIPT_DIRunder a different name.If
cdfails the remainder of the script runs in the wrong directory. Also,SCRIPT_DIRwas already defined at line 18; reusing it would eliminate this duplicate.🛡️ Proposed fix
-script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -cd $script_dir/../ +cd "$SCRIPT_DIR/../" || exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 158 - 159, The script re-defines script_dir and performs an unguarded cd which can leave the script running in the wrong directory; instead reuse the existing SCRIPT_DIR variable (defined earlier) and perform the cd with a failure guard. Replace the second declaration of script_dir with a cd to "$SCRIPT_DIR/../" and ensure the cd is followed by a short-circuit error handler (e.g., cd ... || { echo "Failed to change directory"; exit 1; }) so the script exits if the directory change fails.web/scripts/run-e2e.sh-84-97 (1)
84-97:⚠️ Potential issue | 🟡 MinorLoop iterates ≤ 6 × 30 s = 3 minutes, not 5 as stated in the log message.
Either change the echo to "3 minutes" or increase the bound to
10(5 minutes).🐛 Proposed fix (adjust bound to 10 for ~5 minutes)
- echo "Wait up to 5 minutes for new idp take effect" - ... - while [[ $count -le 6 ]]; do + echo "Wait up to 5 minutes for new idp take effect" + ... + while [[ $count -le 10 ]]; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 84 - 97, The log says "Wait up to 5 minutes" but the loop using count with while [[ $count -le 6 ]] sleeps 30s per iteration (6×30s = 3 minutes); either update the echo to "3 minutes" or extend the loop bound to 10 to reach ~5 minutes—modify the while condition from count -le 6 to count -le 10 (or change the echo message) in the block that references expected_replicas, available_replicas, gen_number, new_gen_number and count so the logged wait matches actual behavior.
🧹 Nitpick comments (7)
web/cypress/e2e/acceptance.cy.ts (2)
9-10: Remove the emptybeforeEachhook.No behavior is defined there currently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` around lines 9 - 10, Remove the empty beforeEach hook in the test file (the beforeEach defined in acceptance.cy.ts) since it defines no behavior; delete the entire beforeEach( function() { }); block so there is no no-op hook left in the test suite.
39-39: Avoid positional button selector for refresh control.
Line [39](eq(2)) is fragile and likely to break with UI changes. Use a stable selector (data-test or explicit attribute/text) for the refresh button.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` at line 39, The test uses a fragile positional selector cy.get('button').eq(2) to target the refresh control; replace it with a stable, explicit selector (e.g. a data attribute like data-test/data-testid, an aria-label, or the button text) and update the test to query that selector instead of eq(2); add the corresponding attribute to the refresh button in the app (or use cy.contains('Refresh') / cy.get('button[aria-label="Refresh"]') if accessible) so the selector for the refresh control is stable and resilient to DOM reordering.Makefile (1)
18-20: Prefer deterministic install intest-e2etarget.
Line [20]usesnpm installinline on every run. Prefernpm ci(or a dedicated install prerequisite) for reproducible E2E execution.♻️ Proposed refactor
.PHONY: test-e2e test-e2e: - cd web && npm install && npm run test:e2e + cd web && npm ci && npm run test:e2e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 18 - 20, The test-e2e Makefile target currently runs "npm install" which is non-deterministic; change it to use a reproducible install (e.g., replace "npm install" with "npm ci") or create a dedicated prerequisite target (e.g., "web-install" that runs "cd web && npm ci") and have test-e2e depend on that prerequisite; update the test-e2e recipe to "cd web && npm ci && npm run test:e2e" or "cd web && npm run test:e2e" with the new prerequisite to ensure deterministic E2E installs.web/cypress/views/utils.ts (1)
1-5: TightenclickIfExistto avoid re-query flakiness and add typing.Current logic checks one element set, then clicks another via a second query. Keep it scoped to the element already found and type the input as
string.♻️ Proposed refactor
-export function clickIfExist(element) { +export function clickIfExist(element: string) { cy.get('body').then((body) => { - if (body.find(element).length > 0) { - cy.get(element).click(); + const $el = body.find(element); + if ($el.length > 0) { + cy.wrap($el.first()).click(); } }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/utils.ts` around lines 1 - 5, The helper clickIfExist lacks a string type and re-queries the DOM causing flakiness; change its signature to accept element: string (e.g., clickIfExist(element: string)) and after locating the node via body.find(element) reuse that found jQuery element instead of calling cy.get(element) again — wrap the found element with cy.wrap(...) and call .click() so the click is scoped to the previously queried element and avoids a second query.web/cypress/views/nav.ts (1)
9-14: Indentation inconsistency and redundant intermediate assertions.The body of
changePerspectiveTois not indented relative to its function signature (line 10 is at the same level as line 9). Additionally, lines 11–12 and 13–14 each run the samescrollIntoView().should('be.visible')chain twice in a row before acting, which is redundant — a single chain can chain directly to.click().♻️ Proposed fix
changePerspectiveTo: (perspective: string) => { - cy.log('Switch perspective - ' + `${perspective}`); - cy.byLegacyTestID('perspective-switcher-toggle').scrollIntoView().should('be.visible'); - cy.byLegacyTestID('perspective-switcher-toggle').scrollIntoView().should('be.visible').click({force: true}); - cy.byLegacyTestID('perspective-switcher-menu-option').contains(perspective).should('be.visible'); - cy.byLegacyTestID('perspective-switcher-menu-option').contains(perspective).should('be.visible').click({force: true}); + cy.log('Switch perspective - ' + `${perspective}`); + cy.byLegacyTestID('perspective-switcher-toggle').scrollIntoView().should('be.visible').click({force: true}); + cy.byLegacyTestID('perspective-switcher-menu-option').contains(perspective).should('be.visible').click({force: true}); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/nav.ts` around lines 9 - 14, Indent the body of the changePerspectiveTo function so its statements are nested under the function signature and remove redundant duplicate assertions: for the 'perspective-switcher-toggle' and the 'perspective-switcher-menu-option' chains, call scrollIntoView().should('be.visible') only once each and then chain .click({ force: true }) (and keep the cy.log and contains(perspective).should('be.visible') checks), using the changePerspectiveTo function name and the 'perspective-switcher-toggle' and 'perspective-switcher-menu-option' test IDs to locate the lines to update.web/cypress/support/commands/selector-commands.ts (1)
43-91: Inconsistentreturnusage across custom commands — some yieldundefinedto callers.Commands like
byTestID,byLegacyTestID,byButtonText,byDataID,byTestSelector,byTestDropDownMenu,byTestOperatorRow,byTestSectionHeading, andbyTestOperandLinkcallcy.get(...)orcy.contains(...)without returning the result. The later commands (byOUIAID,byClass,bySemanticElement,byAriaLabel,byPFRole) correctly usereturn. When a caller chains.then(...)directly on one of the non-returning commands, they receiveundefinedrather than the jQuery subject.♻️ Proposed fix (apply `return` consistently)
Cypress.Commands.add( 'byTestID', (selector: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>) => { - cy.get(`[data-test="${selector}"]`, options); + return cy.get(`[data-test="${selector}"]`, options); }, ); Cypress.Commands.add('byTestActionID', (selector: string) => cy.get(`[data-test-action="${selector}"]:not([disabled])`), ); Cypress.Commands.add( 'byLegacyTestID', (selector: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>) => { - cy.get(`[data-test-id="${selector}"]`, options); + return cy.get(`[data-test-id="${selector}"]`, options); }, ); Cypress.Commands.add('byButtonText', (selector: string) => { - cy.get('button[type="button"]').contains(`${selector}`); + return cy.get('button[type="button"]').contains(`${selector}`); }); Cypress.Commands.add('byDataID', (selector: string) => { - cy.get(`[data-id="${selector}"]`); + return cy.get(`[data-id="${selector}"]`); });Apply the same pattern to
byTestSelector,byTestDropDownMenu,byTestOperatorRow,byTestSectionHeading, andbyTestOperandLink.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/selector-commands.ts` around lines 43 - 91, Several custom Cypress commands (byTestID, byLegacyTestID, byButtonText, byDataID, byTestSelector, byTestDropDownMenu, byTestOperatorRow, byTestSectionHeading, byTestOperandLink) call cy.get()/cy.contains() inside a block but don’t return the result, causing callers to receive undefined; update each Cypress.Commands.add callback to return the cy call (e.g., add "return cy.get(...)" or "return cy.get(...).contains(...)" respectively) so the jQuery subject is yielded and can be chained with .then(), keeping existing parameter signatures (options/types) unchanged.web/cypress.config.ts (1)
2-2: Unused import ofregisterCypressGrep; the inlinerequireat line 121 is correct.The named import
registerCypressGrepat line 2 is never invoked. The actual plugin registration on line 121 (require('@cypress/grep/src/plugin')(config)) is the correct documented API — pass onlyconfigto the plugin function. Remove the dead import.♻️ Proposed fix
import { defineConfig } from 'cypress'; -import registerCypressGrep from '@cypress/grep/src/plugin'; const fs = require('fs');Also applies to: 121-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress.config.ts` at line 2, Remove the unused named import registerCypressGrep at the top of the file; the plugin is correctly initialized via the inline require call require('@cypress/grep/src/plugin')(config) so keep that and delete the dead import to satisfy linting and avoid confusion, then run the linter/TS compile to confirm no remaining references to registerCypressGrep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress.config.ts`:
- Line 12: Remove the duplicate top-level "video" property in the exported
Cypress config object so only the intended setting remains; locate the exported
config (the object passed to defineConfig or module.exports) that contains two
"video" keys and delete the undesired one (keep whichever value is intended,
e.g., video: false) so the final config has a single "video" property.
- Line 43: The config defines viewportWidth twice (a 1600 entry earlier and a
1400 entry later), so the earlier key is silently overwritten; remove the
duplicate earlier definition (the `viewportWidth: 1600` entry) so only one
`viewportWidth` key remains (the intended `viewportWidth: 1400`) and verify
there are no other duplicate keys like `video` elsewhere in the same file;
update or delete the earlier `viewportWidth` occurrence referenced in the diff
so the single canonical setting (`viewportWidth`) is used.
- Around line 90-105: The on('after:screenshot') handler is not returning the
Promise created inside the fs.readdir callback, so the async rename (fs.rename)
is fire-and-forget; return the Promise from the handler and chain it to
fs.readdir (or use the Promise-based fs.readdir/fs.promises API) so Cypress
awaits the rename and resolves with { path: newPath }; update the
on('after:screenshot', (details) => { ... }) implementation to return the
Promise that wraps fs.readdir and fs.rename (or use async/await) referencing
details.path, path.parse(details.path), fs.readdir and fs.rename, and resolve({
path: newPath }) when rename succeeds.
In `@web/cypress/fixtures/data-test.ts`:
- Line 232: The fixture value for TroubleShootPanelPopText is missing the
leading CSS-class dot; update the TroubleShootPanelPopText entry in data-test.ts
so its string is a CSS selector (prepend a '.' to
'pf-v5-c-popover__title-text'); locate the TroubleShootPanelPopText symbol in
the file and change its value to include the '.' prefix to match how
LogDetail/LogToolbar are defined.
- Around line 216-217: The selectors for LogDetail and LogToolbar are missing
the leading dot so class-based queries like cy.get(Classes.LogDetail) won't
match; update the string values for LogDetail and LogToolbar in the Classes
object to include a '.' prefix (e.g., '.pf-v5-c-table__td
lv-plugin__table__details' → '.pf-v5-c-table__td lv-plugin__table__details' and
similarly for LogToolbar) so they follow the other entries and work with
cy.get(Classes.LogDetail)/cy.get(Classes.LogToolbar).
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 240-243: The LOGIN_USERS parsing is brittle because the chained
split and index access can throw; implement a single reusable parser function
(e.g., getLoginCredentials or parseLoginUser) that: reads
Cypress.env('LOGIN_USERS'), safely splits into entries, checks the requested
Env.Rank.toIndex[index] is within bounds, validates the selected entry contains
a ':' before splitting into username and password, and returns a safe {username,
password} fallback (or null) if malformed; replace the repeated chained parsing
at the locations using Env.Rank.toIndex[index] (the blocks assigning username
and userpassword at the shown diff and the other occurrences around lines
257-260, 277-280, 300-313) with calls to this helper so parsing is centralized
and errors are avoided.
- Around line 108-117: The login helper can pass an undefined oauthurl into
performLogin because the oauthurl parameter has no default; update the login
function signature (symbol: login) to provide a sensible default (e.g., from
Cypress.env('LOGIN_OAUTH_URL')) or make oauthurl optional and only forward it to
performLogin when defined, ensuring the call inside cy.session (where
performLogin is invoked) never receives undefined; adjust the parameter typing
accordingly and keep performLogin invocation unchanged except for the ensured
non-undefined argument.
- Around line 315-320: The empty-user branch currently calls this.skip() but
then continues to build fullusername; change this to a deterministic early exit:
after the this.skip() call return immediately (or wrap the skip in an explicit
return) and/or move the fullusername = Cypress.env('LOGIN_IDP') + ":" + username
assignment into the non-empty branch so fullusername is only constructed when
username is valid; refer to the username empty-check, this.skip(), and the
fullusername variable in auth-commands.ts to locate where to apply the early
return and/or move the assignment.
- Around line 194-200: The cliLogin helper currently interpolates
LOGIN_USERNAME/LOGIN_PASSWORD/HOST_API into a shell command (in
Cypress.Commands.add 'cliLogin' calling cy.exec) which is vulnerable to
shell-breaking credentials; change it to avoid shell interpolation by moving the
actual oc login invocation into a cy.task (e.g., create a task like 'ocLogin' in
your plugins/index that uses Node's child_process.spawn or execFile with an args
array [ 'login', '-u', username, '-p', password, hostapi,
'--insecure-skip-tls-verify=true' ]) and call that task from cliLogin with the
env values, or if keeping cy.exec, properly escape credentials using a robust
shell-escaping utility before interpolation; ensure you reference the same
symbols (cliLogin, cy.exec, LOGIN_USERNAME, LOGIN_PASSWORD, HOST_API) so callers
are unchanged while eliminating direct string interpolation.
In `@web/cypress/support/commands/env.ts`:
- Around line 1-2: The module exports admin_kubeconfig directly from
Cypress.env('KUBECONFIG_PATH') which can be undefined and cause failures; update
the top-level export logic in web/cypress/support/commands/env.ts (affecting
admin_kubeconfig and its usage by cy.readFile or shell commands) to validate
that Cypress.env('KUBECONFIG_PATH') is set and throw a clear Error immediately
if missing (fail-fast), leaving normal_kubeconfig unchanged; ensure the error
message mentions KUBECONFIG_PATH so callers can see why the module failed to
load.
In `@web/cypress/support/commands/troubeshoot-commands.ts`:
- Around line 70-72: The click call using an invalid option should be changed
from click({focus: true}) to click({force: true}) so the click will happen even
if the element is obscured; update the two occurrences in this file where
.click({focus: true}) is used (the selector chain ending with .eq(2).click(...)
near the call that follows
cy.get(dt.Classes.TroubleShootPanelTopologyContainer).should('be.visible') and
the similar occurrence around lines 78-79) to use click({force: true})
consistent with other clicks in this file.
- Around line 75-80: The custom Cypress command clickTroubleshootPanelAdvance is
implemented but not declared on the Cypress.Chainable interface, causing
TypeScript errors; add a declaration for clickTroubleshootPanelAdvance to the
Chainable interface (e.g., declare interface Chainable {
clickTroubleshootPanelAdvance(): Chainable<JQuery<HTMLElement>> } or similar
promise/void signature your project uses) so callers like
cy.clickTroubleshootPanelAdvance() are typed correctly and autocompleted; update
the same d.ts augmentation file where other commands are declared (where the
existing Chainable methods are defined) and export/augment Cypress accordingly.
- Line 5: Remove the unused and incorrect import "helperfuncs" (the import line
importing from '../views/utils') from the top of the file; replace the two
occurrences of .click({focus: true}) with .click({force: true}) to use a valid
Cypress click option (these are the calls that currently pass focus:true); and
add the missing TypeScript declaration clickTroubleshootPanelAdvance():
Chainable<JQuery<HTMLElement>> (or appropriate Subject) to the existing
Chainable<Subject> interface so the
Cypress.Commands.add('clickTroubleshootPanelAdvance', ...) function is properly
typed.
In `@web/cypress/support/commands/utility-commands.ts`:
- Around line 136-138: The code double-clicks the pod link by aliasing it then
clicking again when re-resolving '@podLink', causing navigation loops or stale
element errors; change the sequence to click the aliased element only once and
remove the second resolution-and-click. Specifically, either assert visibility
on the aliased element before the initial click (using
cy.get(...).eq(0).as('podLink').should('be.visible').click()) or remove the line
cy.get('@podLink').should('be.visible').click(); so that only the aliased click
happens, then proceed to cy.byPFRole('rowgroup').find('td').eq(1)... to verify
the destination content.
In `@web/cypress/support/e2e.ts`:
- Around line 33-36: The additional Cypress.on('uncaught:exception', (err) => {
... return false; }) handler unconditionally suppresses all exceptions and
overrides the allowlist behavior; remove this second uncaught:exception listener
so only the intended allowlist logic (the original handler that inspects
error/message/stack) runs and real application errors are not masked—locate and
delete the redundant Cypress.on('uncaught:exception', (err) => {
console.error(...); return false; }) block.
In `@web/cypress/tsconfig.json`:
- Around line 3-4: Update the TypeScript compilation target and library entries
in tsconfig.json so Cypress files can use modern APIs (e.g.,
String.prototype.includes used in web/cypress/support/e2e.ts,
web/cypress/support/auth-commands.ts, web/cypress/support/utility-commands.ts).
Change the "target" from "es5" to a modern ES version (for example "es2017" or
"es6") and update "lib" to include the corresponding ES library (e.g., replace
["es5", "dom"] with ["es2017", "dom"] or ["es6", "dom"]) so the TypeScript
compiler recognizes modern string/array methods and DOM typings. Ensure only
these tsconfig.json fields ("target" and "lib") are modified.
In `@web/cypress/views/tour.ts`:
- Around line 12-13: The close-button selector is global and should be scoped to
the welcome modal: locate the modal element found by
$body.find(`[aria-label="Welcome modal"]`) and perform the close action within
that modal (e.g., using .within or .find) instead of calling
cy.get('[aria-label="Close"]') globally; update the code surrounding the modal
check so the click targets the close button inside the modal element to avoid
capturing other close buttons.
In `@web/scripts/run-e2e.sh`:
- Line 2: Replace the lone "set +x" in the script with a strict shell mode so
errors fail fast: change the script initialization to use "set -euo pipefail"
(or at minimum "set -e") instead of "set +x" so any failing oc/command in
functions like the wait loop or login validation will abort the script
immediately and not continue in a broken state; keep any intentional tracing
separately if needed.
- Around line 148-149: Split declaration and assignment for both
CYPRESS_BASE_URL and CYPRESS_OPENSHIFT_VERSION: run the oc commands into
temporary variables, check their exit status, then export the variables with the
quoted values; also quote the jsonpath expressions (e.g. use '{.spec.host}' and
'{.openshiftVersion}') so the braces aren’t interpreted by the shell and ensure
you preserve error handling if the oc or jq calls fail before exporting
CYPRESS_BASE_URL and CYPRESS_OPENSHIFT_VERSION.
- Around line 110-128: The function enable_troubleshoot_plugin currently
unconditionally exits if the observability-operator isn't running; instead,
first check whether the UIPlugin resource "troubleshooting-panel" already exists
(use oc get uiplugin troubleshooting-panel) and if it does, simply return/skip
installation; otherwise, if it does not exist then proceed to check the operator
(oc get pod ...); if the operator is running apply the UIPlugin manifest as
before, and if the operator is not running and the UIPlugin is missing, emit a
warning/error message but do not call exit 1 so tests can continue or the caller
can decide how to handle failure. Ensure these changes are made inside
enable_troubleshoot_plugin to preserve behavior when installation is actually
required.
---
Minor comments:
In `@web/cypress/README.md`:
- Line 1: Fix the markdown heading text "#The following file are copyed from
https://github.com/openshift/logging-view-plugin/tree/main/web. These files will
be updated Ad hoc" by adding the required space after '#' to satisfy MD018 and
correct typos: change "file" to "files", "copyed" to "copied", and make "Ad hoc"
lowercase "ad hoc" (or "ad-hoc" per style); update the heading line (the string
starting with "#The following file are copyed...") in web/cypress/README.md
accordingly.
In `@web/scripts/run-e2e.sh`:
- Line 18: Remove the unused duplicate environment variable by deleting the
unnecessary uppercase declaration SCRIPT_DIR="$(cd "$(dirname
"${BASH_SOURCE[0]}")" && pwd)"; keep the existing lowercase script_dir
declaration used later (script_dir at the later block) and ensure any references
use that single variable name to avoid duplication and confusion.
- Around line 4-14: The header comment currently references "Logging UI" and
unrelated prerequisites (clusterlogging, lokistack, log-test-app namespaces);
update the top-of-script comment in run-e2e.sh to describe this script runs
Troubleshooting Panel plugin E2E tests and list correct prerequisites: exported
KUBECONFIG, optional CYPRESS_SPEC and CYPRESS_TAG, how to supply login users via
CYPRESS_LOGIN_IDP/CYPRESS_LOGIN_USERS or enable_idp_htpasswd, and any
namespace/test data requirements specific to the Troubleshooting Panel rather
than the old Logging UI details; edit the comment block (the script header) to
remove references to clusterlogging, lokistack, and log-test-app namespaces and
replace them with accurate Troubleshooting Panel prerequisites and usage notes.
- Line 53: The echo message in run-e2e.sh contains a duplicated IDP name
("uiauto-htpasswd-idpuiauto-htpasswd-idp"); update the string in the echo
statement to the correct single IDP name (e.g., "uiauto-htpasswd-idp") so the
message reads "Create new users and add uiauto-htpasswd-idp", ensuring you
modify the echo line accordingly.
- Around line 158-159: The script re-defines script_dir and performs an
unguarded cd which can leave the script running in the wrong directory; instead
reuse the existing SCRIPT_DIR variable (defined earlier) and perform the cd with
a failure guard. Replace the second declaration of script_dir with a cd to
"$SCRIPT_DIR/../" and ensure the cd is followed by a short-circuit error handler
(e.g., cd ... || { echo "Failed to change directory"; exit 1; }) so the script
exits if the directory change fails.
- Around line 84-97: The log says "Wait up to 5 minutes" but the loop using
count with while [[ $count -le 6 ]] sleeps 30s per iteration (6×30s = 3
minutes); either update the echo to "3 minutes" or extend the loop bound to 10
to reach ~5 minutes—modify the while condition from count -le 6 to count -le 10
(or change the echo message) in the block that references expected_replicas,
available_replicas, gen_number, new_gen_number and count so the logged wait
matches actual behavior.
---
Nitpick comments:
In `@Makefile`:
- Around line 18-20: The test-e2e Makefile target currently runs "npm install"
which is non-deterministic; change it to use a reproducible install (e.g.,
replace "npm install" with "npm ci") or create a dedicated prerequisite target
(e.g., "web-install" that runs "cd web && npm ci") and have test-e2e depend on
that prerequisite; update the test-e2e recipe to "cd web && npm ci && npm run
test:e2e" or "cd web && npm run test:e2e" with the new prerequisite to ensure
deterministic E2E installs.
In `@web/cypress.config.ts`:
- Line 2: Remove the unused named import registerCypressGrep at the top of the
file; the plugin is correctly initialized via the inline require call
require('@cypress/grep/src/plugin')(config) so keep that and delete the dead
import to satisfy linting and avoid confusion, then run the linter/TS compile to
confirm no remaining references to registerCypressGrep.
In `@web/cypress/e2e/acceptance.cy.ts`:
- Around line 9-10: Remove the empty beforeEach hook in the test file (the
beforeEach defined in acceptance.cy.ts) since it defines no behavior; delete the
entire beforeEach( function() { }); block so there is no no-op hook left in the
test suite.
- Line 39: The test uses a fragile positional selector cy.get('button').eq(2) to
target the refresh control; replace it with a stable, explicit selector (e.g. a
data attribute like data-test/data-testid, an aria-label, or the button text)
and update the test to query that selector instead of eq(2); add the
corresponding attribute to the refresh button in the app (or use
cy.contains('Refresh') / cy.get('button[aria-label="Refresh"]') if accessible)
so the selector for the refresh control is stable and resilient to DOM
reordering.
In `@web/cypress/support/commands/selector-commands.ts`:
- Around line 43-91: Several custom Cypress commands (byTestID, byLegacyTestID,
byButtonText, byDataID, byTestSelector, byTestDropDownMenu, byTestOperatorRow,
byTestSectionHeading, byTestOperandLink) call cy.get()/cy.contains() inside a
block but don’t return the result, causing callers to receive undefined; update
each Cypress.Commands.add callback to return the cy call (e.g., add "return
cy.get(...)" or "return cy.get(...).contains(...)" respectively) so the jQuery
subject is yielded and can be chained with .then(), keeping existing parameter
signatures (options/types) unchanged.
In `@web/cypress/views/nav.ts`:
- Around line 9-14: Indent the body of the changePerspectiveTo function so its
statements are nested under the function signature and remove redundant
duplicate assertions: for the 'perspective-switcher-toggle' and the
'perspective-switcher-menu-option' chains, call
scrollIntoView().should('be.visible') only once each and then chain .click({
force: true }) (and keep the cy.log and
contains(perspective).should('be.visible') checks), using the
changePerspectiveTo function name and the 'perspective-switcher-toggle' and
'perspective-switcher-menu-option' test IDs to locate the lines to update.
In `@web/cypress/views/utils.ts`:
- Around line 1-5: The helper clickIfExist lacks a string type and re-queries
the DOM causing flakiness; change its signature to accept element: string (e.g.,
clickIfExist(element: string)) and after locating the node via
body.find(element) reuse that found jQuery element instead of calling
cy.get(element) again — wrap the found element with cy.wrap(...) and call
.click() so the click is scoped to the previously queried element and avoids a
second query.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
Makefileweb/cypress.config.tsweb/cypress/README.mdweb/cypress/e2e/acceptance.cy.tsweb/cypress/fixtures/data-test.tsweb/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/env.tsweb/cypress/support/commands/selector-commands.tsweb/cypress/support/commands/troubeshoot-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/e2e.tsweb/cypress/tsconfig.jsonweb/cypress/views/nav.tsweb/cypress/views/tour.tsweb/cypress/views/utils.tsweb/integration-tests/.eslintrcweb/integration-tests/cypress.config.jsweb/integration-tests/fixtures/example.jsonweb/integration-tests/plugins/index.tsweb/integration-tests/reporter-config.jsonweb/integration-tests/tsconfig.jsonweb/package.jsonweb/scripts/run-e2e.sh
💤 Files with no reviewable changes (6)
- web/integration-tests/reporter-config.json
- web/integration-tests/cypress.config.js
- web/integration-tests/fixtures/example.json
- web/integration-tests/tsconfig.json
- web/integration-tests/plugins/index.ts
- web/integration-tests/.eslintrc
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (15)
web/cypress/support/commands/troubeshoot-commands.ts (3)
13-19:⚠️ Potential issue | 🟠 MajorDeclare
clickTroubleshootPanelAdvanceonCypress.Chainable.Line 75 registers
clickTroubleshootPanelAdvance, but it is missing from the interface on Line 13–Line 19, which causes TypeScript typing gaps forcy.clickTroubleshootPanelAdvance().🔧 Proposed fix
interface Chainable<Subject> { openTroubleshootPanel(); closeTroubleshootPanel(); focusTroubleshootPanel(); refreshTroubleshootPanel(); getTroubleshootPanelQueryText(); + clickTroubleshootPanelAdvance(); }Also applies to: 75-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/troubeshoot-commands.ts` around lines 13 - 19, The TypeScript Chainable interface is missing the declaration for the custom command clickTroubleshootPanelAdvance, causing typing errors for cy.clickTroubleshootPanelAdvance(); update the interface declaration (interface Chainable<Subject>) to include a method signature for clickTroubleshootPanelAdvance (matching other commands like openTroubleshootPanel/closeTroubleshootPanel) so the Cypress custom command registered around lines 75–80 is properly typed and available across the test suite.
71-72:⚠️ Potential issue | 🟠 MajorUse valid Cypress click options (
focusis ignored).Line 71 and Line 78 pass
{focus: true}to.click(), but Cypress doesn’t support afocusclick option. If you need actionability bypass, use{force: true}.🔧 Proposed fix
- .click({focus: true}); + .click({force: true}); @@ - .click({focus: true}); + .click({force: true});Also applies to: 78-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/troubeshoot-commands.ts` around lines 71 - 72, The .click calls are using an unsupported option ({focus: true}); locate the .click(...) usages (e.g., the call before the assertion on dt.Classes.TroubleShootPanelTopologyContainer and the duplicate at lines 78-79) and replace the invalid option with the appropriate Cypress option or remove it — if you intended to bypass actionability use {force: true}, otherwise call .click() with no options; update both occurrences so tests run without the ignored {focus: true} option.
5-5:⚠️ Potential issue | 🔴 CriticalRemove or correct the broken utility import path.
Line 5 imports from
../views/utils, which does not resolve relative toweb/cypress/support/commands/troubeshoot-commands.ts. This can break TS/module resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/troubeshoot-commands.ts` at line 5, The import for the utility module is broken: locate the import statement that brings in helperfuncs (the import of the utils module in troubeshoot-commands.ts) and either correct its relative path to the actual utils file location or remove the import if unused; ensure the file imports the exact exported symbols the file uses (referenced as helperfuncs) and verify TypeScript/module resolution compiles after updating the import.web/cypress.config.ts (2)
90-105:⚠️ Potential issue | 🟠 MajorReturn the async rename Promise from
after:screenshot.Line 93–Line 104 creates a Promise inside
fs.readdir, but the handler returnsundefined, so Cypress won’t await the rename/path update.🔧 Proposed fix
on('after:screenshot', (details) => { // Prepend "1_", "2_", etc. to screenshot filenames because they are sorted alphanumerically in CI's artifacts dir const pathObj = path.parse(details.path); - fs.readdir(pathObj.dir, (error, files) => { - const newPath = `${pathObj.dir}${path.sep}${files.length}_${pathObj.base}`; - return new Promise((resolve, reject) => { - // eslint-disable-next-line consistent-return - fs.rename(details.path, newPath, (err) => { - if (err) return reject(err); - // because we renamed and moved the image, resolve with the new path - // so it is accurate in the test results - resolve({ path: newPath }); - }); - }); - }); + return new Promise((resolve, reject) => { + fs.readdir(pathObj.dir, (error, files) => { + if (error) return reject(error); + const newPath = `${pathObj.dir}${path.sep}${files.length}_${pathObj.base}`; + fs.rename(details.path, newPath, (err) => { + if (err) return reject(err); + resolve({ path: newPath }); + }); + }); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress.config.ts` around lines 90 - 105, The on('after:screenshot' ...) handler builds a Promise inside the fs.readdir callback but doesn't return it to Cypress, so the rename/path update is not awaited; modify the handler to return the Promise produced by fs.readdir (or promisify fs.readdir) so Cypress receives the Promise and waits for fs.rename to complete and for resolve({ path: newPath }) to be returned; reference the on('after:screenshot') callback, the fs.readdir call that constructs the Promise, the fs.rename call that resolves/rejects, and the resolve({ path: newPath }) value to ensure the new path is returned.
12-12:⚠️ Potential issue | 🔴 CriticalRemove duplicate config keys that are silently overwritten.
Line 12 (
video: true) is overridden by Line 134 (video: false), and Line 43 (viewportWidth: 1600) is overridden by Line 135 (viewportWidth: 1400). Keep only one key for each.Also applies to: 43-43, 134-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress.config.ts` at line 12, The Cypress config contains duplicate keys (video and viewportWidth) so the latter values silently overwrite the earlier ones; in the cypress config object (e.g., the object passed to defineConfig or exported as default in web/cypress.config.ts) remove the duplicated entries so each key appears only once—decide which value you want to keep (either video: true or video: false and either viewportWidth: 1600 or 1400), keep that single entry for video and viewportWidth, and delete the other occurrences to avoid silent overrides.web/cypress/fixtures/data-test.ts (1)
216-217:⚠️ Potential issue | 🔴 CriticalFix invalid class selectors (missing
.prefix).Line 216, Line 217, and Line 232 are not valid class selectors as written, so
cy.get(Classes.…)won’t match the intended nodes.🐛 Proposed fix
- LogDetail: 'pf-v5-c-table__td lv-plugin__table__details', - LogToolbar: 'pf-v5-c-toolbar__content-section', + LogDetail: '.pf-v5-c-table__td .lv-plugin__table__details', + LogToolbar: '.pf-v5-c-toolbar__content-section', @@ - TroubleShootPanelPopText: 'pf-v5-c-popover__title-text', + TroubleShootPanelPopText: '.pf-v5-c-popover__title-text',Also applies to: 232-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/fixtures/data-test.ts` around lines 216 - 217, The class selector strings in the Classes object (e.g., LogDetail and LogToolbar and the entry at the other reported location) are missing the leading '.' so cy.get(Classes.<name>) won't match; update those values to be valid CSS class selectors by prefixing each class string with a dot (for example update LogDetail and LogToolbar entries in the Classes export and the other reported key to start with '.'), leaving the rest of the selector text unchanged.web/cypress/support/commands/auth-commands.ts (4)
111-115:⚠️ Potential issue | 🟠 MajorPrevent undefined OAuth URL in
cy.login()default usage.Line 114 has no default, so
cy.login()can passundefinedintoperformLoginon Line 119 and break the cross-origin auth path.🔧 Proposed fix
( provider: string = Cypress.env('LOGIN_IDP'), username: string = Cypress.env('LOGIN_USERNAME'), password: string = Cypress.env('LOGIN_PASSWORD'), - oauthurl: string, + oauthurl: string = getOauthUrl(), ) => {Also applies to: 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 111 - 115, The cy.login helper can pass an undefined oauthurl into performLogin—prevent this by giving the oauthurl parameter a safe default; update the cy.login signature so the oauthurl parameter defaults to a known value (e.g. Cypress.env('LOGIN_OAUTHURL') or an empty string) instead of being undefined, and keep the rest of the call to performLogin(oauthurl) unchanged so cross-origin auth path always gets a defined string.
250-256:⚠️ Potential issue | 🟠 MajorHarden
cliLoginagainst shell injection and shell-breaking credentials.Line 255 interpolates raw
username/password/hostapiinto a shell command. Special characters can break execution or inject shell tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 250 - 256, The cliLogin command currently interpolates raw credentials into a shell string (in Cypress.Commands.add('cliLogin') when calling cy.exec), which allows shell-breaking characters or injection; fix it by sanitizing/escaping the three inputs (loginUsername, loginPassword, hostapiurl) before building the oc login command: implement a small shell-escape helper that safely escapes single quotes and backslashes and wrap each value in single quotes (or otherwise properly escape all shell meta-characters), then use the escaped values when calling cy.exec; apply this escaping to the values resolved from username/password/hostapi (and env fallbacks) so the oc login invocation in cy.exec cannot be broken or injected into.
371-376:⚠️ Potential issue | 🟠 MajorAbort early when user lookup fails in impersonation flow.
Line 371–Line 374 logs/skip path but continues, and Line 375 still builds
fullusername. This should return/throw immediately to prevent invalid downstream actions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 371 - 376, The check for a missing username currently logs and calls this.skip() but execution continues and still constructs fullusername; modify the flow in the impersonation lookup so that after cy.log(`can not find the ${index} user.`) and this.skip() you immediately abort the function (e.g., return or throw) to prevent building fullusername and any downstream actions that rely on username; update the block that references username and fullusername to ensure no further code runs when username == "".
296-299:⚠️ Potential issue | 🟠 MajorCentralize and validate
LOGIN_USERSparsing before indexing.Line 297–Line 299 (and the repeated blocks) assumes the index exists and each entry contains
username:password. Malformed env input throws before fallback/exit logic runs.Also applies to: 314-315, 334-335, 357-357, 369-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 296 - 299, The LOGIN_USERS parsing is performed inline in multiple places (using Cypress.env('LOGIN_USERS') and Env.Rank.toIndex[index]) which can throw for malformed input; centralize parsing into a single validated helper that: reads Cypress.env('LOGIN_USERS'), splits into entries, validates each entry contains exactly one ':' and non-empty username/password, and only then uses Env.Rank.toIndex to lookup an entry; replace the repeated inline logic (where username and userpassword are assigned) with a call to that helper which returns a safe {username, password} or triggers the existing fallback/exit logic if validation fails; ensure uses of LOGIN_USERS at the locations mentioned (the blocks assigning username/userpassword at lines shown) reference the helper.web/cypress/views/tour.ts (1)
12-13:⚠️ Potential issue | 🟠 MajorScope the close click to the welcome modal container.
Line 13 uses a global close-button selector, so it can hit the wrong element when multiple dialogs are present. Since Line 12 already identifies the modal, click within that modal only.
🔧 Proposed fix
closeKubevirtTour: () => { cy.get('body').then(($body) => { - if ($body.find(`[aria-label="Welcome modal"]`).length > 0) { - cy.get('[aria-label="Close"]').should('be.visible').click(); + const $welcomeModal = $body.find('[aria-label="Welcome modal"]'); + if ($welcomeModal.length > 0) { + cy.wrap($welcomeModal).within(() => { + cy.get('[aria-label="Close"]').should('be.visible').click(); + }); } }); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/tour.ts` around lines 12 - 13, The close-button click is using a global selector and can target the wrong dialog; instead scope the click to the already-located modal node by finding the '[aria-label="Close"]' within the `[aria-label="Welcome modal"]` element (use the modal element returned by $body.find(`[aria-label="Welcome modal"]`) or use cy.get('[aria-label="Welcome modal"]').find('[aria-label="Close"]') ) and then assert visible and click that scoped element.web/cypress/support/e2e.ts (1)
33-36:⚠️ Potential issue | 🔴 CriticalRemove the blanket exception suppressor.
Line 33–Line 36 unconditionally returns
false, which suppresses all uncaught exceptions and overrides the allowlist behavior in Line 15–Line 31.🔧 Proposed fix
-Cypress.on('uncaught:exception', (err) => { - console.error("Uncaught error:", err.message); - return false; -});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/e2e.ts` around lines 33 - 36, The current Cypress.on('uncaught:exception', (err) => { ... return false; }) handler unconditionally suppresses all uncaught exceptions and overrides the allowlist; remove or replace it so suppression only occurs for whitelisted errors: reuse the existing allowlist logic in this file (the allowlist predicate/array defined earlier) inside the uncaught:exception handler, check err.message/err.stack against that allowlist, return false only when it matches, and otherwise let the exception propagate (do not return false).web/scripts/run-e2e.sh (3)
148-149:⚠️ Potential issue | 🟠 MajorSplit command-substitution exports and quote jsonpath expressions.
Lines 148-149 use
export VAR=$(...)(masks command failures) and unquoted jsonpath braces.🛠️ Proposed fix
-export CYPRESS_BASE_URL="https://$(oc get route console -n openshift-console -o jsonpath={.spec.host})" -export CYPRESS_OPENSHIFT_VERSION=$(oc version -o json |jq -r '.openshiftVersion'|cut -f 1,2 -d.) +base_host="$(oc get route console -n openshift-console -o jsonpath='{.spec.host}')" || exit 1 +CYPRESS_BASE_URL="https://${base_host}" +export CYPRESS_BASE_URL + +openshift_version="$(oc version -o json | jq -r '.openshiftVersion' | cut -f 1,2 -d.)" || exit 1 +CYPRESS_OPENSHIFT_VERSION="${openshift_version}" +export CYPRESS_OPENSHIFT_VERSION#!/bin/bash # Verify shell warnings for this segment. shellcheck -f gcc web/scripts/run-e2e.sh | rg -n 'SC2155|SC1083' || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 148 - 149, The two export lines (CYPRESS_BASE_URL and CYPRESS_OPENSHIFT_VERSION) should not use inline command substitution in the export (which masks failures) and the jsonpath expressions must be quoted; instead assign the command results to temporary variables first (e.g., base_url_host via `oc get route console -n openshift-console -o jsonpath='{.spec.host}'` and oc_version via `oc version -o json | jq -r '.openshiftVersion'`), check their exit status, then export CYPRESS_BASE_URL="https://$base_url_host" and CYPRESS_OPENSHIFT_VERSION="$(echo "$oc_version" | cut -f1,2 -d.)"; ensure the jsonpath braces are quoted ('{.spec.host}') and fail fast or log a clear error if any command fails before exporting.
110-127:⚠️ Potential issue | 🟠 MajorHandle pre-existing
UIPluginand avoid hard exit for non-install paths.Line 126 hard-exits when the operator is unavailable, even if the plugin is already present in pre-provisioned environments.
🧩 Proposed fix
function enable_troubleshoot_plugin() { + if oc get uiplugin troubleshooting-panel >/dev/null 2>&1; then + echo "troubleshooting-panel UIPlugin already exists, skipping install" + return 0 + fi + if oc get pod -l app.kubernetes.io/name=observability-operator --field-selector=status.phase=Running -A -o name | grep -q .; then echo "#deploy troubleshooting-panel" cat <<EOF|oc apply -f - @@ EOF else echo "observability-operator is not installed or not in running status" - exit 1 + return 0 fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 110 - 127, The enable_troubleshoot_plugin function currently exits with error if the observability-operator is not running and also unconditionally attempts to apply a UIPlugin which may already exist; change it to first check for the existence of the UIPlugin named "troubleshooting-panel" (using oc get uiplugin troubleshooting-panel) and only apply the manifest if it does not exist, and if the operator is not running avoid a hard exit—emit a warning and skip creating the plugin (or verify existence and succeed) instead of calling exit 1; update logic inside enable_troubleshoot_plugin to reference the function name and the resource "troubleshooting-panel" when performing these conditional checks.
2-2:⚠️ Potential issue | 🟠 MajorEnable strict shell mode at script start.
Line 2 uses only
set +x, so failed cluster/setup commands can be ignored and execution may continue in a broken state.🛡️ Proposed fix
-set +x +set -euo pipefail#!/bin/bash # Verify whether strict mode is missing/present in the script. rg -n '^\s*set\s+' web/scripts/run-e2e.sh rg -n '^\s*set\s+-e' web/scripts/run-e2e.sh || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` at line 2, Replace the lone "set +x" at the top of run-e2e.sh with strict-shell settings so failures stop the script: enable "set -euo pipefail" (and keep or add "set +x" / "set -x" only if you want to control xtrace separately). Specifically, update the script start where "set +x" appears to enable errexit, nounset and pipefail to ensure any failing command (cluster/setup etc.) aborts execution rather than being ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/README.md`:
- Line 1: Fix the malformed README heading by replacing the incorrect text "#The
following file are copyed from
https://github.com/openshift/logging-view-plugin/tree/main/web. These files will
be updated Ad hoc" with a properly formatted and spelled heading such as "# The
following files are copied from
https://github.com/openshift/logging-view-plugin/tree/main/web" followed by a
short sentence "These files will be updated ad hoc." Locate the string "#The
following file are copyed..." in web/cypress/README.md to make the edits.
In `@web/scripts/run-e2e.sh`:
- Around line 67-68: Remove the credential leak by stopping the script from
printing raw UI_USERS to stdout; replace the echo "Users are store in
${UI_USERS}" with a non-sensitive confirmation (e.g. echo "Users stored in
${uiusers_file}" or a redacted message) and ensure credentials are only written
to uiusers_file (echo "$UI_USERS" > "$uiusers_file") with restrictive
permissions (chmod 600 "$uiusers_file") if applicable; update references to the
UI_USERS variable and uiusers_file in run-e2e.sh accordingly so no
username:password pairs appear in logs.
---
Duplicate comments:
In `@web/cypress.config.ts`:
- Around line 90-105: The on('after:screenshot' ...) handler builds a Promise
inside the fs.readdir callback but doesn't return it to Cypress, so the
rename/path update is not awaited; modify the handler to return the Promise
produced by fs.readdir (or promisify fs.readdir) so Cypress receives the Promise
and waits for fs.rename to complete and for resolve({ path: newPath }) to be
returned; reference the on('after:screenshot') callback, the fs.readdir call
that constructs the Promise, the fs.rename call that resolves/rejects, and the
resolve({ path: newPath }) value to ensure the new path is returned.
- Line 12: The Cypress config contains duplicate keys (video and viewportWidth)
so the latter values silently overwrite the earlier ones; in the cypress config
object (e.g., the object passed to defineConfig or exported as default in
web/cypress.config.ts) remove the duplicated entries so each key appears only
once—decide which value you want to keep (either video: true or video: false and
either viewportWidth: 1600 or 1400), keep that single entry for video and
viewportWidth, and delete the other occurrences to avoid silent overrides.
In `@web/cypress/fixtures/data-test.ts`:
- Around line 216-217: The class selector strings in the Classes object (e.g.,
LogDetail and LogToolbar and the entry at the other reported location) are
missing the leading '.' so cy.get(Classes.<name>) won't match; update those
values to be valid CSS class selectors by prefixing each class string with a dot
(for example update LogDetail and LogToolbar entries in the Classes export and
the other reported key to start with '.'), leaving the rest of the selector text
unchanged.
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 111-115: The cy.login helper can pass an undefined oauthurl into
performLogin—prevent this by giving the oauthurl parameter a safe default;
update the cy.login signature so the oauthurl parameter defaults to a known
value (e.g. Cypress.env('LOGIN_OAUTHURL') or an empty string) instead of being
undefined, and keep the rest of the call to performLogin(oauthurl) unchanged so
cross-origin auth path always gets a defined string.
- Around line 250-256: The cliLogin command currently interpolates raw
credentials into a shell string (in Cypress.Commands.add('cliLogin') when
calling cy.exec), which allows shell-breaking characters or injection; fix it by
sanitizing/escaping the three inputs (loginUsername, loginPassword, hostapiurl)
before building the oc login command: implement a small shell-escape helper that
safely escapes single quotes and backslashes and wrap each value in single
quotes (or otherwise properly escape all shell meta-characters), then use the
escaped values when calling cy.exec; apply this escaping to the values resolved
from username/password/hostapi (and env fallbacks) so the oc login invocation in
cy.exec cannot be broken or injected into.
- Around line 371-376: The check for a missing username currently logs and calls
this.skip() but execution continues and still constructs fullusername; modify
the flow in the impersonation lookup so that after cy.log(`can not find the
${index} user.`) and this.skip() you immediately abort the function (e.g.,
return or throw) to prevent building fullusername and any downstream actions
that rely on username; update the block that references username and
fullusername to ensure no further code runs when username == "".
- Around line 296-299: The LOGIN_USERS parsing is performed inline in multiple
places (using Cypress.env('LOGIN_USERS') and Env.Rank.toIndex[index]) which can
throw for malformed input; centralize parsing into a single validated helper
that: reads Cypress.env('LOGIN_USERS'), splits into entries, validates each
entry contains exactly one ':' and non-empty username/password, and only then
uses Env.Rank.toIndex to lookup an entry; replace the repeated inline logic
(where username and userpassword are assigned) with a call to that helper which
returns a safe {username, password} or triggers the existing fallback/exit logic
if validation fails; ensure uses of LOGIN_USERS at the locations mentioned (the
blocks assigning username/userpassword at lines shown) reference the helper.
In `@web/cypress/support/commands/troubeshoot-commands.ts`:
- Around line 13-19: The TypeScript Chainable interface is missing the
declaration for the custom command clickTroubleshootPanelAdvance, causing typing
errors for cy.clickTroubleshootPanelAdvance(); update the interface declaration
(interface Chainable<Subject>) to include a method signature for
clickTroubleshootPanelAdvance (matching other commands like
openTroubleshootPanel/closeTroubleshootPanel) so the Cypress custom command
registered around lines 75–80 is properly typed and available across the test
suite.
- Around line 71-72: The .click calls are using an unsupported option ({focus:
true}); locate the .click(...) usages (e.g., the call before the assertion on
dt.Classes.TroubleShootPanelTopologyContainer and the duplicate at lines 78-79)
and replace the invalid option with the appropriate Cypress option or remove it
— if you intended to bypass actionability use {force: true}, otherwise call
.click() with no options; update both occurrences so tests run without the
ignored {focus: true} option.
- Line 5: The import for the utility module is broken: locate the import
statement that brings in helperfuncs (the import of the utils module in
troubeshoot-commands.ts) and either correct its relative path to the actual
utils file location or remove the import if unused; ensure the file imports the
exact exported symbols the file uses (referenced as helperfuncs) and verify
TypeScript/module resolution compiles after updating the import.
In `@web/cypress/support/e2e.ts`:
- Around line 33-36: The current Cypress.on('uncaught:exception', (err) => { ...
return false; }) handler unconditionally suppresses all uncaught exceptions and
overrides the allowlist; remove or replace it so suppression only occurs for
whitelisted errors: reuse the existing allowlist logic in this file (the
allowlist predicate/array defined earlier) inside the uncaught:exception
handler, check err.message/err.stack against that allowlist, return false only
when it matches, and otherwise let the exception propagate (do not return
false).
In `@web/cypress/views/tour.ts`:
- Around line 12-13: The close-button click is using a global selector and can
target the wrong dialog; instead scope the click to the already-located modal
node by finding the '[aria-label="Close"]' within the `[aria-label="Welcome
modal"]` element (use the modal element returned by
$body.find(`[aria-label="Welcome modal"]`) or use cy.get('[aria-label="Welcome
modal"]').find('[aria-label="Close"]') ) and then assert visible and click that
scoped element.
In `@web/scripts/run-e2e.sh`:
- Around line 148-149: The two export lines (CYPRESS_BASE_URL and
CYPRESS_OPENSHIFT_VERSION) should not use inline command substitution in the
export (which masks failures) and the jsonpath expressions must be quoted;
instead assign the command results to temporary variables first (e.g.,
base_url_host via `oc get route console -n openshift-console -o
jsonpath='{.spec.host}'` and oc_version via `oc version -o json | jq -r
'.openshiftVersion'`), check their exit status, then export
CYPRESS_BASE_URL="https://$base_url_host" and CYPRESS_OPENSHIFT_VERSION="$(echo
"$oc_version" | cut -f1,2 -d.)"; ensure the jsonpath braces are quoted
('{.spec.host}') and fail fast or log a clear error if any command fails before
exporting.
- Around line 110-127: The enable_troubleshoot_plugin function currently exits
with error if the observability-operator is not running and also unconditionally
attempts to apply a UIPlugin which may already exist; change it to first check
for the existence of the UIPlugin named "troubleshooting-panel" (using oc get
uiplugin troubleshooting-panel) and only apply the manifest if it does not
exist, and if the operator is not running avoid a hard exit—emit a warning and
skip creating the plugin (or verify existence and succeed) instead of calling
exit 1; update logic inside enable_troubleshoot_plugin to reference the function
name and the resource "troubleshooting-panel" when performing these conditional
checks.
- Line 2: Replace the lone "set +x" at the top of run-e2e.sh with strict-shell
settings so failures stop the script: enable "set -euo pipefail" (and keep or
add "set +x" / "set -x" only if you want to control xtrace separately).
Specifically, update the script start where "set +x" appears to enable errexit,
nounset and pipefail to ensure any failing command (cluster/setup etc.) aborts
execution rather than being ignored.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
Makefileweb/cypress.config.tsweb/cypress/README.mdweb/cypress/e2e/acceptance.cy.tsweb/cypress/fixtures/data-test.tsweb/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/env.tsweb/cypress/support/commands/selector-commands.tsweb/cypress/support/commands/troubeshoot-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/e2e.tsweb/cypress/tsconfig.jsonweb/cypress/views/nav.tsweb/cypress/views/tour.tsweb/cypress/views/utils.tsweb/integration-tests/.eslintrcweb/integration-tests/cypress.config.jsweb/integration-tests/fixtures/example.jsonweb/integration-tests/plugins/index.tsweb/integration-tests/reporter-config.jsonweb/integration-tests/tsconfig.jsonweb/package.jsonweb/scripts/run-e2e.sh
💤 Files with no reviewable changes (6)
- web/integration-tests/fixtures/example.json
- web/integration-tests/.eslintrc
- web/integration-tests/plugins/index.ts
- web/integration-tests/tsconfig.json
- web/integration-tests/cypress.config.js
- web/integration-tests/reporter-config.json
🚧 Files skipped from review as they are similar to previous changes (7)
- web/cypress/support/commands/env.ts
- web/cypress/support/commands/utility-commands.ts
- web/package.json
- web/cypress/tsconfig.json
- web/cypress/e2e/acceptance.cy.ts
- web/cypress/support/commands/selector-commands.ts
- Makefile
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
web/cypress/fixtures/data-test.ts (1)
216-216:⚠️ Potential issue | 🟠 Major
LogDetailselector still misses class prefix on descendant.Line 216 uses
lv-plugin__table__detailswithout.. This targets an element tag, not the class, and can breakcy.get(Classes.LogDetail).🐛 Proposed fix
- LogDetail: '.pf-v5-c-table__td lv-plugin__table__details', + LogDetail: '.pf-v5-c-table__td .lv-plugin__table__details',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/fixtures/data-test.ts` at line 216, The selector constant LogDetail currently has a missing class prefix for the descendant (it uses "lv-plugin__table__details" as a tag); update the string assigned to LogDetail so the descendant uses the class selector (prepend a '.'), e.g. change the fragment "lv-plugin__table__details" to ".lv-plugin__table__details" so cy.get(Classes.LogDetail) targets the intended element class; verify the change in the fixture where LogDetail is defined.web/cypress/views/tour.ts (1)
12-15:⚠️ Potential issue | 🟠 MajorScope the close button lookup to the Welcome modal container.
Lines 14 and 37 use a global close selector, which can click the wrong button when multiple dialogs are present.
🔧 Proposed fix
- else if ($body.find(`[aria-label="Welcome modal"]`).length > 0) { + else if ($body.find(`[aria-label="Welcome modal"]`).length > 0) { cy.log('Kubevirt modal detected, attempting to close...'); - cy.get('[aria-label="Close"]').should('be.visible').click(); + cy.get('[aria-label="Welcome modal"]').within(() => { + cy.get('[aria-label="Close"]').should('be.visible').click(); + }); } @@ - if ($body.find(`[aria-label="Welcome modal"]`).length > 0) { - cy.get('[aria-label="Close"]').should('be.visible').click(); + if ($body.find(`[aria-label="Welcome modal"]`).length > 0) { + cy.get('[aria-label="Welcome modal"]').within(() => { + cy.get('[aria-label="Close"]').should('be.visible').click(); + }); }Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/tour.ts` around lines 12 - 15, Scope the Close button lookup to the Welcome modal container instead of using a global selector: when detecting the Welcome modal (the element matched by `[aria-label="Welcome modal"]` in web/cypress/views/tour.ts) find the close button within that container (e.g., use the Welcome modal element's .find('[aria-label="Close"]') or cy.get(`[aria-label="Welcome modal"]`).find('[aria-label="Close"]')) and then .should('be.visible').click(); apply the same change to the other occurrence around lines 36-37 to avoid clicking a Close button from a different dialog.web/cypress/README.md (1)
1-1:⚠️ Potential issue | 🟡 MinorFix malformed heading and wording.
Line 1 is still malformed markdown (
#The) and contains spelling/grammar issues, which makes the new README look unfinished.✏️ Proposed fix
-#The following file are copyed from https://github.com/openshift/logging-view-plugin/tree/main/web. These files will be updated Ad hoc +# The following files are copied from https://github.com/openshift/logging-view-plugin/tree/main/web. These files will be updated ad hoc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/README.md` at line 1, Replace the malformed heading "#The following file are copyed from https://github.com/openshift/logging-view-plugin/tree/main/web. These files will be updated Ad hoc" with a properly formatted markdown heading and corrected grammar: change to "# The following files are copied from https://github.com/openshift/logging-view-plugin/tree/main/web" and follow it with a separate sentence "These files will be updated ad hoc." Ensure the heading includes the leading "# " space and fix typos ("file" → "files", "copyed" → "copied", "Ad" → "ad") and punctuation/capitalization as shown.web/scripts/run-e2e.sh (1)
67-68:⚠️ Potential issue | 🟠 MajorDo not print generated credentials to logs.
Line 68 logs raw
UI_USERS(username:passwordpairs). That leaks secrets into CI logs.🔒 Proposed fix
- echo $UI_USERS >$uiusers_file - echo "Users are store in ${UI_USERS}" + umask 077 + printf '%s\n' "$UI_USERS" > "$uiusers_file" + echo "Users are stored in ${uiusers_file}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 67 - 68, The script currently writes sensitive credentials from the UI_USERS env var to the log via the echo "Users are store in ${UI_USERS}" line; remove or change that log so raw credentials are never printed. Update the run-e2e.sh usage of UI_USERS and uiusers_file so you still write UI_USERS to $uiusers_file (keep the existing echo $UI_USERS >$uiusers_file) but replace the logging line that references ${UI_USERS} with a non-sensitive message such as confirming the file path or a masked/summary placeholder; ensure no other prints or debug echoes expose UI_USERS or its contents.web/cypress/support/commands/auth-commands.ts (2)
114-115:⚠️ Potential issue | 🔴 CriticalSyntax error: Missing default value for
oauthurlparameter.The parameter declaration
oauthurl: string =is incomplete—no value follows the assignment operator. This will cause a parse error and prevent the file from compiling.🐛 Proposed fix
provider: string = Cypress.env('LOGIN_IDP'), username: string = Cypress.env('LOGIN_USERNAME'), password: string = Cypress.env('LOGIN_PASSWORD'), - oauthurl: string = + oauthurl: string = getOauthUrl(), ) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 114 - 115, The parameter declaration for oauthurl currently reads "oauthurl: string =" and causes a parse error; fix the function signature that declares the oauthurl parameter by either providing a default value (e.g., oauthurl: string = ''), or make it optional (oauthurl?: string) and update any call sites accordingly; ensure the change is applied to the function that contains the oauthurl parameter so the TypeScript compiles.
310-315:⚠️ Potential issue | 🔴 Critical
cy.exit()is not a valid Cypress command and will throw a runtime error.
cy.exit()does not exist in the Cypress API. To stop the test run programmatically, useCypress.stop(). To skip the current test, usecy.then(() => this.skip()). Alternatively, throw an error to fail the command.This applies to lines 314, 328, and 344.
🐛 Proposed fix
if( username != "" && userpassword != "" ){ cy.exec(`oc login -u ${username} -p ${userpassword} --kubeconfig=${Env.normal_kubeconfig}`); }else{ cy.log('no user can be found.'); - cy.exit(); + throw new Error('No valid user credentials found for CLI login'); } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 310 - 315, Replace the invalid cy.exit() calls in web/cypress/support/commands/auth-commands.ts with a supported Cypress API: locate each occurrence of cy.exit() (the three spots in the login/credential-check blocks shown in the diff) and either call Cypress.stop() to halt the entire test run or, if the intent was to skip only the current test, replace with cy.then(() => this.skip()); ensure the replacement preserves the surrounding logging (cy.log) and flows that follow the username/password checks.
🧹 Nitpick comments (5)
web/cypress/support/commands/auth-commands.ts (1)
98-105: Hardcodedcy.wait()calls reduce test reliability.Using fixed waits (
cy.wait(2000),cy.wait(10000)) makes tests flaky and slow. Consider using Cypress's built-in retry-ability with assertions orcy.intercept()to wait for specific conditions.♻️ Suggested improvement
Cypress.Commands.add('validateLogin', () => { cy.log('validateLogin'); cy.visit('/'); - cy.wait(2000); cy.byTestID("username", { timeout: 120000 }).should('be.visible'); - cy.wait(10000); guidedTour.close(); });The
cy.byTestIDwith timeout should handle the wait. If additional page load time is needed, consider waiting for a specific network request or DOM state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 98 - 105, Replace the two hardcoded cy.wait(2000) and cy.wait(10000) inside the Cypress.Commands.add('validateLogin', ...) helper with deterministic waits: remove the fixed waits and instead rely on cy.byTestID("username", { timeout: 120000 }).should('be.visible') (or add an additional assertion for a specific DOM state) and/or set up cy.intercept() for the key network call(s) and wait on that intercept before calling guidedTour.close(); ensure the command waits for the relevant request(s) or element assertions so tests are not flaky.web/cypress/e2e/acceptance.cy.ts (4)
68-68: Typo: "BackGroud" should be "Background".- cy.get(dt.Classes.TroubleShootPanelTopologyNodeBackGroud).should('exist'); //backgroup panel + cy.get(dt.Classes.TroubleShootPanelTopologyNodeBackground).should('exist'); //background panelNote: This may require updating the corresponding constant in
data-test.tsas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` at line 68, The test references a misspelled data-test constant TroubleShootPanelTopologyNodeBackGroud; rename this constant to TroubleShootPanelTopologyNodeBackground in data-test.ts and update all usages (e.g., in web/cypress/e2e/acceptance.cy.ts where cy.get(dt.Classes.TroubleShootPanelTopologyNodeBackGroud) is used) so the import symbol and tests match the corrected name; ensure you update both the declaration in data-test.ts and every place that references dt.Classes.TroubleShootPanelTopologyNodeBackGroud to the new TroubleShootPanelTopologyNodeBackground.
9-10: EmptybeforeEachhook can be removed.The
beforeEachhook is empty and serves no purpose. Remove it to reduce noise, or add test isolation logic if needed later.♻️ Suggested fix
- beforeEach( function() { - }); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` around lines 9 - 10, Remove the empty beforeEach hook by deleting the beforeEach( function() { }); block in the acceptance test file (the no-op beforeEach is unused); if you actually need test setup later, replace it with the required initialization logic inside beforeEach instead of leaving an empty stub.
30-32: Typos in comments: "troubelShoot" should be "troubleshoot".Minor typo in multiple comment lines.
✏️ Fix typos
- //troubelShoot Panel query-container + //troubleshoot Panel query-container //<div pf-v5-c-expandable-section pf-m-expanded.pf-m-detached pf-m-indented tp-plugin__panel-query-container> - //troubelShoot Panel query-container -- control div + //troubleshoot Panel query-container -- control div- //troubelShoot Panel query-container -- topology-container + //troubleshoot Panel query-container -- topology-containerAlso applies to: 60-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` around lines 30 - 32, Replace the misspelled comment occurrences of "troubelShoot" with "troubleshoot" (e.g., update comment lines containing "troubelShoot Panel query-container" and "troubelShoot Panel query-container -- control div") and apply the same fix to the other matching comment occurrences mentioned (also applies to the second pair of comment lines). Keep the rest of each comment text unchanged.
17-98: Consider splitting this monolithic test into smaller, focused tests.This single test case validates multiple distinct concerns: panel title bar, query controls, advanced form fields, topology container, graph controls, and specific node labels. Splitting into separate
itblocks would improve:
- Debuggability: Easier to identify which specific validation failed
- Parallelization: Smaller tests can run concurrently
- Maintenance: Changes to one area won't affect unrelated assertions
Example structure
it('should display troubleshoot panel title bar', () => { /* ... */ }); it('should display query control container', () => { /* ... */ }); it('should display advanced form fields', () => { /* ... */ }); it('should display topology graph with controls', () => { /* ... */ }); it('should show expected K8s nodes in graph', () => { /* ... */ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` around lines 17 - 98, Split the large "Essential elements validation" spec into multiple focused tests: extract the shared setup (cy.clickNavLink(['Home','Projects']) and cy.openTroubleshootPanel()) into a beforeEach, then create separate it blocks for the title bar (assertions using dt.Classes.TroubleShootPanelPopoverTitleBar and TroubleShootPanelPopoverClose), the query controls (dt.Classes.TroubleShootPanelQueryControlContainer and cy.focusTroubleshootPanel / cy.clickTroubleshootPanelAdvance), the advanced form (dt.Classes.TroubleShootPanelQueryAdvancedContainer and TroubleShootPanelQueryNumerInput plus textarea#query-input), the topology and graph controls (dt.Classes.TroubleShootPanelTopologyContainer, 'div[data-test-id="topology"]', g[data-id="korrel8r_graph"], and TroubleShootPanelTopologyGraphControlBar), and the node label checks (find g[data-id="k8s:..."] and their text assertions); keep the existing selector symbols and assertions but move them into the respective new it blocks so failures are isolated and setup is reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 270-274: The adminCLI helper currently interpolates the
user-supplied command directly into cy.exec (function adminCLI), which enables
shell command injection; fix it by replacing free-form interpolation with a safe
approach: validate the incoming command string against a strict whitelist of
allowed commands/flags (or parse and validate tokens), or require callers to
pass a predefined command key that maps to a known safe command; then build the
final exec invocation from those validated pieces and pass the kubeconfig only
as a separate, validated argument to cy.exec (avoid direct string concatenation
into cy.exec). Ensure validation logic lives alongside adminCLI and reject or
throw for any unrecognized/unsafe input.
- Line 343: The cy.log calls currently use single-quoted strings with ${index}
(and similar occurrences), which prevents interpolation; update these cy.log
invocations to use template literals (backticks) so the variable values
interpolate correctly—for example replace the literal 'Can not find the ${index}
user' and the other two similar log strings with backtick-wrapped template
strings that interpolate index (and any other variables) inside ${...} in the
auth-commands.ts commands where cy.log is called.
In `@web/cypress/support/commands/env.ts`:
- Around line 1-4: The module references kubeconfigPath before it's declared,
causing a ReferenceError; fix by first reading the env into a local const (e.g.,
declare kubeconfigPath = Cypress.env('KUBECONFIG_PATH')) then perform the
null-check (if (!kubeconfigPath) throw ...) and finally export the value (export
const admin_kubeconfig = kubeconfigPath) so admin_kubeconfig is defined from the
validated variable; update usages to reference admin_kubeconfig or
kubeconfigPath as appropriate.
In `@web/cypress/support/commands/selector-commands.ts`:
- Around line 43-91: Several custom Cypress command callbacks (byTestID,
byLegacyTestID, byButtonText, byDataID, byTestSelector, byTestDropDownMenu,
byTestOperatorRow, byTestSectionHeading, byTestOperandLink) use braced function
bodies but do not return the cy.get(...) Chainable, breaking the Chainable<T>
contract; update each of these command callbacks to explicitly return the result
of the cy.get(...) (or cy.get(...).contains(...) for byButtonText) so callers
receive the Chainable and TypeScript typing is preserved.
In `@web/scripts/run-e2e.sh`:
- Around line 38-45: The if/else is unreachable because set -euo pipefail causes
the script to exit if oc login fails; move the oc login invocation into the if
conditional so the shell tests its exit status instead of exiting early. Update
the run-e2e.sh block that currently runs oc login then checks `$?` to use: if oc
login --username=... --password=... --kubeconfig=/tmp/normal_kubeconfig
>/dev/null 2>&1; then ... else ... exit 1 fi (preserving the success and error
messages), and keep other script flags like set -euo pipefail unchanged.
- Around line 135-147: The emptiness checks in run-e2e.sh use plain variable
expansions which break under set -u; update all conditional tests that reference
KUBECONFIG, CYPRESS_LOGIN_IDP, CYPRESS_LOGIN_USERS, CYPRESS_SPEC, and
CYPRESS_TAG to use default-empty expansions (e.g., ${KUBECONFIG:-},
${CYPRESS_LOGIN_IDP:-}, etc.) and prefer -z/-n form (e.g., if [[ -z
"${KUBECONFIG:-}" ]] or if [[ -n "${CYPRESS_LOGIN_USERS:-}" ]]) so the script
won’t exit when those env vars are unset; apply this change to the checks around
the enable_troubleshoot_plugin / enable_idp_htpasswd logic and the later
CYPRESS_SPEC/CYPRESS_TAG checks.
---
Duplicate comments:
In `@web/cypress/fixtures/data-test.ts`:
- Line 216: The selector constant LogDetail currently has a missing class prefix
for the descendant (it uses "lv-plugin__table__details" as a tag); update the
string assigned to LogDetail so the descendant uses the class selector (prepend
a '.'), e.g. change the fragment "lv-plugin__table__details" to
".lv-plugin__table__details" so cy.get(Classes.LogDetail) targets the intended
element class; verify the change in the fixture where LogDetail is defined.
In `@web/cypress/README.md`:
- Line 1: Replace the malformed heading "#The following file are copyed from
https://github.com/openshift/logging-view-plugin/tree/main/web. These files will
be updated Ad hoc" with a properly formatted markdown heading and corrected
grammar: change to "# The following files are copied from
https://github.com/openshift/logging-view-plugin/tree/main/web" and follow it
with a separate sentence "These files will be updated ad hoc." Ensure the
heading includes the leading "# " space and fix typos ("file" → "files",
"copyed" → "copied", "Ad" → "ad") and punctuation/capitalization as shown.
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 114-115: The parameter declaration for oauthurl currently reads
"oauthurl: string =" and causes a parse error; fix the function signature that
declares the oauthurl parameter by either providing a default value (e.g.,
oauthurl: string = ''), or make it optional (oauthurl?: string) and update any
call sites accordingly; ensure the change is applied to the function that
contains the oauthurl parameter so the TypeScript compiles.
- Around line 310-315: Replace the invalid cy.exit() calls in
web/cypress/support/commands/auth-commands.ts with a supported Cypress API:
locate each occurrence of cy.exit() (the three spots in the
login/credential-check blocks shown in the diff) and either call Cypress.stop()
to halt the entire test run or, if the intent was to skip only the current test,
replace with cy.then(() => this.skip()); ensure the replacement preserves the
surrounding logging (cy.log) and flows that follow the username/password checks.
In `@web/cypress/views/tour.ts`:
- Around line 12-15: Scope the Close button lookup to the Welcome modal
container instead of using a global selector: when detecting the Welcome modal
(the element matched by `[aria-label="Welcome modal"]` in
web/cypress/views/tour.ts) find the close button within that container (e.g.,
use the Welcome modal element's .find('[aria-label="Close"]') or
cy.get(`[aria-label="Welcome modal"]`).find('[aria-label="Close"]')) and then
.should('be.visible').click(); apply the same change to the other occurrence
around lines 36-37 to avoid clicking a Close button from a different dialog.
In `@web/scripts/run-e2e.sh`:
- Around line 67-68: The script currently writes sensitive credentials from the
UI_USERS env var to the log via the echo "Users are store in ${UI_USERS}" line;
remove or change that log so raw credentials are never printed. Update the
run-e2e.sh usage of UI_USERS and uiusers_file so you still write UI_USERS to
$uiusers_file (keep the existing echo $UI_USERS >$uiusers_file) but replace the
logging line that references ${UI_USERS} with a non-sensitive message such as
confirming the file path or a masked/summary placeholder; ensure no other prints
or debug echoes expose UI_USERS or its contents.
---
Nitpick comments:
In `@web/cypress/e2e/acceptance.cy.ts`:
- Line 68: The test references a misspelled data-test constant
TroubleShootPanelTopologyNodeBackGroud; rename this constant to
TroubleShootPanelTopologyNodeBackground in data-test.ts and update all usages
(e.g., in web/cypress/e2e/acceptance.cy.ts where
cy.get(dt.Classes.TroubleShootPanelTopologyNodeBackGroud) is used) so the import
symbol and tests match the corrected name; ensure you update both the
declaration in data-test.ts and every place that references
dt.Classes.TroubleShootPanelTopologyNodeBackGroud to the new
TroubleShootPanelTopologyNodeBackground.
- Around line 9-10: Remove the empty beforeEach hook by deleting the beforeEach(
function() { }); block in the acceptance test file (the no-op beforeEach is
unused); if you actually need test setup later, replace it with the required
initialization logic inside beforeEach instead of leaving an empty stub.
- Around line 30-32: Replace the misspelled comment occurrences of
"troubelShoot" with "troubleshoot" (e.g., update comment lines containing
"troubelShoot Panel query-container" and "troubelShoot Panel query-container --
control div") and apply the same fix to the other matching comment occurrences
mentioned (also applies to the second pair of comment lines). Keep the rest of
each comment text unchanged.
- Around line 17-98: Split the large "Essential elements validation" spec into
multiple focused tests: extract the shared setup
(cy.clickNavLink(['Home','Projects']) and cy.openTroubleshootPanel()) into a
beforeEach, then create separate it blocks for the title bar (assertions using
dt.Classes.TroubleShootPanelPopoverTitleBar and TroubleShootPanelPopoverClose),
the query controls (dt.Classes.TroubleShootPanelQueryControlContainer and
cy.focusTroubleshootPanel / cy.clickTroubleshootPanelAdvance), the advanced form
(dt.Classes.TroubleShootPanelQueryAdvancedContainer and
TroubleShootPanelQueryNumerInput plus textarea#query-input), the topology and
graph controls (dt.Classes.TroubleShootPanelTopologyContainer,
'div[data-test-id="topology"]', g[data-id="korrel8r_graph"], and
TroubleShootPanelTopologyGraphControlBar), and the node label checks (find
g[data-id="k8s:..."] and their text assertions); keep the existing selector
symbols and assertions but move them into the respective new it blocks so
failures are isolated and setup is reused.
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 98-105: Replace the two hardcoded cy.wait(2000) and cy.wait(10000)
inside the Cypress.Commands.add('validateLogin', ...) helper with deterministic
waits: remove the fixed waits and instead rely on cy.byTestID("username", {
timeout: 120000 }).should('be.visible') (or add an additional assertion for a
specific DOM state) and/or set up cy.intercept() for the key network call(s) and
wait on that intercept before calling guidedTour.close(); ensure the command
waits for the relevant request(s) or element assertions so tests are not flaky.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
Makefileweb/cypress.config.tsweb/cypress/README.mdweb/cypress/e2e/acceptance.cy.tsweb/cypress/fixtures/data-test.tsweb/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/env.tsweb/cypress/support/commands/selector-commands.tsweb/cypress/support/commands/troubeshoot-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/e2e.tsweb/cypress/tsconfig.jsonweb/cypress/views/nav.tsweb/cypress/views/tour.tsweb/cypress/views/utils.tsweb/integration-tests/.eslintrcweb/integration-tests/cypress.config.jsweb/integration-tests/fixtures/example.jsonweb/integration-tests/plugins/index.tsweb/integration-tests/reporter-config.jsonweb/integration-tests/tsconfig.jsonweb/package.jsonweb/scripts/run-e2e.sh
💤 Files with no reviewable changes (6)
- web/integration-tests/reporter-config.json
- web/integration-tests/cypress.config.js
- web/integration-tests/plugins/index.ts
- web/integration-tests/fixtures/example.json
- web/integration-tests/tsconfig.json
- web/integration-tests/.eslintrc
🚧 Files skipped from review as they are similar to previous changes (7)
- web/cypress/tsconfig.json
- web/cypress/views/nav.ts
- web/package.json
- web/cypress/views/utils.ts
- web/cypress/support/e2e.ts
- web/cypress/support/commands/utility-commands.ts
- Makefile
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
web/cypress/support/commands/auth-commands.ts (2)
342-345:⚠️ Potential issue | 🟡 MinorTemplate literal syntax error and invalid
cy.exit()call.Line 343 uses single quotes
'Can not find the ${index} user'which won't interpolate the variable. Line 344 uses non-existentcy.exit(). The template literal issue was flagged in a previous review.,
🐛 Proposed fix
}else{ - cy.log('Can not find the ${index} user'); - cy.exit(); + throw new Error(`Cannot find LOGIN_USERS entry for index "${index}"`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 342 - 345, Fix the template literal and remove the invalid Cypress call: change cy.log('Can not find the ${index} user') to use backticks so the index interpolates (cy.log(`Can not find the ${index} user`)) and replace the non-existent cy.exit() with a proper test stop such as throwing an error (throw new Error(`Can not find the ${index} user`)) or otherwise returning/ending the command; update the code around the existing cy.log and cy.exit() calls accordingly.
114-115:⚠️ Potential issue | 🔴 CriticalCritical syntax error: Missing default value for
oauthurlparameter.Line 114 has an incomplete assignment
oauthurl: string =with no value before the closing parenthesis on line 115. This prevents the module from loading.🐛 Proposed fix
provider: string = Cypress.env('LOGIN_IDP'), username: string = Cypress.env('LOGIN_USERNAME'), password: string = Cypress.env('LOGIN_PASSWORD'), - oauthurl: string = + oauthurl: string = getOauthUrl(), ) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 114 - 115, The function signature in web/cypress/support/commands/auth-commands.ts has a broken parameter declaration "oauthurl: string =" with no default value; fix the syntax by either removing the "=" to make oauthurl a required parameter (e.g., "oauthurl: string") or provide a sensible default (e.g., "oauthurl: string = ''") in the same signature where oauthurl is declared so the module can load correctly.
🧹 Nitpick comments (2)
web/cypress/support/commands/auth-commands.ts (2)
98-105: Consider replacing fixed waits with assertions.
cy.wait(2000)andcy.wait(10000)are Cypress anti-patterns that slow tests and create flakiness. Consider using assertions that wait for specific conditions instead, such as waiting for network idle or specific elements to stabilize.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 98 - 105, The validateLogin command uses fixed waits (cy.wait(2000) and cy.wait(10000)) which cause flakiness; replace them with assertions that wait for specific conditions: after cy.visit('/') assert the app has loaded by waiting for a stable element (e.g., cy.byTestID("username").should('be.visible') or a dashboard/root container) and optionally wait for any network calls to finish (use cy.intercept + cy.wait for relevant routes) before calling guidedTour.close(); update the validateLogin implementation to remove the fixed waits and rely on these element/network assertions to ensure stability.
137-149: Fixed waits inswitchPerspective.Similar to
validateLogin, lines 140 and 147 use fixedcy.wait()delays that could be replaced with condition-based waits for more reliable tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 137 - 149, The command switchPerspective uses fixed cy.wait(2000) and cy.wait(3000); replace these with condition-based waits: after detecting and clicking '#nav-toggle' when body.find('.pf-m-collapsed') > 0, wait for the collapse class to be gone (e.g., assert that '.pf-m-collapsed' does not exist or '#nav-toggle' has the expected state) instead of cy.wait(2000); after calling nav.sidenav.switcher.changePerspectiveTo(...perspectives) wait for a deterministic signal that the perspective loaded (such as a URL change, a specific element that appears for the target perspective, or a request alias via cy.intercept) rather than cy.wait(3000); keep guidedTour.close() at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 326-329: The branch that logs "no user can be found." uses an
invalid Cypress command cy.exit(); replace that call with throwing a runtime
error (e.g., throw new Error('no user can be found.')) so the test fails
properly; update the code in the custom command implementation (the auth-related
command in web/cypress/support/commands/auth-commands.ts—same pattern used in
cliLoginAsUser) to log the message then throw the Error instead of calling
cy.exit().
- Around line 310-315: The else branch uses the invalid cy.exit() and
interpolates unescaped credentials; update the three places (the cy.exec block
in this file plus uiLoginAsUser and uiLoginAsClusterAdminForUser) by removing
the unreachable else branch (since getLoginUserByRank() already validates) or,
if you keep a guard, replace cy.exit() with Cypress.stop() or throw new
Error('no user can be found') to abort cleanly; also stop interpolating raw
username/userpassword into the shell command—escape them (e.g., wrap in single
quotes and escape any internal single quotes) before passing to cy.exec or use a
safer exec API to avoid shell injection, and ensure changes are applied to the
functions getLoginUserByRank, uiLoginAsUser, and uiLoginAsClusterAdminForUser.
In `@web/cypress/support/commands/env.ts`:
- Around line 1-18: The file has broken syntax: kubeconfigPath is used before
declaration, admin_kubeconfig is exported twice, and an if block is missing a
closing brace; fix by declaring a single const kubeconfigPath =
Cypress.env('KUBECONFIG_PATH') before any usage, validate it with a properly
closed if (kubeconfigPath) { ... } or throw when falsy, remove the duplicate
admin_kubeconfig export so only export const admin_kubeconfig: string =
kubeconfigPath is present, and keep the other exports (normal_kubeconfig and
Rank) outside the if block; ensure identifiers referenced are admin_kubeconfig,
kubeconfigPath, normal_kubeconfig, and Rank.
---
Duplicate comments:
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 342-345: Fix the template literal and remove the invalid Cypress
call: change cy.log('Can not find the ${index} user') to use backticks so the
index interpolates (cy.log(`Can not find the ${index} user`)) and replace the
non-existent cy.exit() with a proper test stop such as throwing an error (throw
new Error(`Can not find the ${index} user`)) or otherwise returning/ending the
command; update the code around the existing cy.log and cy.exit() calls
accordingly.
- Around line 114-115: The function signature in
web/cypress/support/commands/auth-commands.ts has a broken parameter declaration
"oauthurl: string =" with no default value; fix the syntax by either removing
the "=" to make oauthurl a required parameter (e.g., "oauthurl: string") or
provide a sensible default (e.g., "oauthurl: string = ''") in the same signature
where oauthurl is declared so the module can load correctly.
---
Nitpick comments:
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 98-105: The validateLogin command uses fixed waits (cy.wait(2000)
and cy.wait(10000)) which cause flakiness; replace them with assertions that
wait for specific conditions: after cy.visit('/') assert the app has loaded by
waiting for a stable element (e.g., cy.byTestID("username").should('be.visible')
or a dashboard/root container) and optionally wait for any network calls to
finish (use cy.intercept + cy.wait for relevant routes) before calling
guidedTour.close(); update the validateLogin implementation to remove the fixed
waits and rely on these element/network assertions to ensure stability.
- Around line 137-149: The command switchPerspective uses fixed cy.wait(2000)
and cy.wait(3000); replace these with condition-based waits: after detecting and
clicking '#nav-toggle' when body.find('.pf-m-collapsed') > 0, wait for the
collapse class to be gone (e.g., assert that '.pf-m-collapsed' does not exist or
'#nav-toggle' has the expected state) instead of cy.wait(2000); after calling
nav.sidenav.switcher.changePerspectiveTo(...perspectives) wait for a
deterministic signal that the perspective loaded (such as a URL change, a
specific element that appears for the target perspective, or a request alias via
cy.intercept) rather than cy.wait(3000); keep guidedTour.close() at the end.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
web/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/env.tsweb/cypress/views/tour.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web/cypress/views/tour.ts
anpingli
force-pushed
the
web-test
branch
2 times, most recently
from
60c8f97 to
81e8d2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (8)
web/scripts/run-e2e.sh (3)
135-150:⚠️ Potential issue | 🟠 MajorGuard environment variable checks for
set -ucompatibility.With
set -uenabled, accessing unset variables like$KUBECONFIG,$CYPRESS_LOGIN_IDP,$CYPRESS_LOGIN_USERSwill cause the script to abort before the emptiness check is evaluated. Use default expansion syntax${VAR:-}.🐛 Proposed fix
-if [[ $KUBECONFIG == "" ]]; then +if [[ -z "${KUBECONFIG:-}" ]]; then echo "Exit, you must expose the Environment KUBECONFIG" exit 1 fi enable_troubleshoot_plugin -if [[ "$CYPRESS_LOGIN_IDP" == "" || "$CYPRESS_LOGIN_USERS" == "" ]];then +if [[ -z "${CYPRESS_LOGIN_IDP:-}" || -z "${CYPRESS_LOGIN_USERS:-}" ]]; then enable_idp_htpasswd export CYPRESS_LOGIN_IDP=uiauto-htpasswd-idp export CYPRESS_LOGIN_USERS=$UI_USERS fi -if [[ $CYPRESS_LOGIN_USERS == "" ]];then +if [[ -z "${CYPRESS_LOGIN_USERS:-}" ]]; then echo "Please set correct Env CYPRESS_LOGIN_USERS and CYPRESS_LOGIN_IDP or leave these two Env unset" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 135 - 150, The script currently checks variables like KUBECONFIG, CYPRESS_LOGIN_IDP and CYPRESS_LOGIN_USERS directly which breaks when run with set -u; update all emptiness checks to use parameter expansion defaults (e.g. use "${KUBECONFIG:-}" instead of "$KUBECONFIG") and similarly "${CYPRESS_LOGIN_IDP:-}" and "${CYPRESS_LOGIN_USERS:-}" so the if conditions don't reference unset variables, while leaving calls to enable_troubleshoot_plugin and enable_idp_htpasswd unchanged and preserving the export assignments for CYPRESS_LOGIN_IDP and CYPRESS_LOGIN_USERS when enable_idp_htpasswd is invoked.
170-177:⚠️ Potential issue | 🟠 MajorGuard
CYPRESS_SPECandCYPRESS_TAGchecks forset -ucompatibility.Same issue as above - these variables need default expansion to prevent script abort when unset.
🐛 Proposed fix
-if [[ "$CYPRESS_SPEC" == "" ]];then +if [[ -z "${CYPRESS_SPEC:-}" ]]; then cypress_args=" --spec $(ls cypress/e2e/*.ts|paste -sd ',' -)" else cypress_args=" --spec ${CYPRESS_SPEC}" fi -if [[ "$CYPRESS_TAG" != "" ]]; then +if [[ -n "${CYPRESS_TAG:-}" ]]; then cypress_args="$cypress_args --env grep=${CYPRESS_TAG// /}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 170 - 177, Guard the unset-variable failures by using bash parameter expansion defaults: replace occurrences of "$CYPRESS_SPEC" and "$CYPRESS_TAG" in the conditional checks and assignments with "${CYPRESS_SPEC:-}" and "${CYPRESS_TAG:-}" (e.g. use if [[ "${CYPRESS_SPEC:-}" == "" ]]; then and cypress_args=" --spec ${CYPRESS_SPEC:-}" and when appending the tag use if [[ "${CYPRESS_TAG:-}" != "" ]]; then cypress_args="$cypress_args --env grep=${CYPRESS_TAG// /}" but ensure the test and expansion both use ${CYPRESS_TAG:-} to avoid set -u aborts).
38-46:⚠️ Potential issue | 🟠 MajorElse branch unreachable with
set -eactive.With
set -euo pipefailon line 2, ifoc loginfails on line 38, the script exits immediately before the$?check on line 39. Move the command into theifcondition.🐛 Proposed fix
- oc login --username=uiauto-test-1 --password=${first_passwd} --kubeconfig=/tmp/normal_kubeconfig >/dev/null 2>&1 - if [[ $? == 0 ]];then + if oc login --username=uiauto-test-1 --password="${first_passwd}" --kubeconfig=/tmp/normal_kubeconfig >/dev/null 2>&1; then echo "Login the idp succesed, the users are in $uiusers_file" echo "Enable IDP uiauto-htpasswd-idp succesfully" return 0 else echo "Can not login the idp, please remove uiauto-htpasswd-idp from oauth/cluster and re-run this script" exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 38 - 46, The current oc login call is executed as a separate command so with set -euo pipefail the script will exit on failure before the subsequent if [[ $? == 0 ]] check; change the logic so the oc login is executed inside the conditional (e.g., use if oc login --username=... --password=... --kubeconfig=/tmp/normal_kubeconfig >/dev/null 2>&1; then ...) and keep the existing success and failure branches (echo success message and return 0, echo failure message and exit 1) so the else branch becomes reachable even with set -e.web/cypress/support/commands/auth-commands.ts (4)
349-354:⚠️ Potential issue | 🟡 MinorTemplate literal syntax errors in log messages.
Lines 349 and 354 use single quotes with
${index}, which won't interpolate the variable.🐛 Proposed fix
Cypress.Commands.add('uiLogoutUser', (index: string) => { - cy.log('Logout the ${index} user'); + cy.log(`Logout the ${index} user`); cy.uiLogout(); }) Cypress.Commands.add('uiLogoutClusterAdminForUser', (index: string) => { - cy.log('Logout the ${index} user and remove the cluster admin roles'); + cy.log(`Logout the ${index} user and remove the cluster admin roles`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 349 - 354, The log messages in the custom Cypress commands use literal '${index}' inside single-quoted strings so the index won't interpolate; update the cy.log calls in the commands where 'uiLogout' and 'uiLogoutClusterAdminForUser' are defined to use template literals (backticks) or string concatenation so ${index} is evaluated (e.g., change 'Logout the ${index} user' to use backticks `Logout the ${index} user` or 'Logout the ' + index + ' user') and do the same for the 'Logout the ${index} user and remove the cluster admin roles' message.
312-315:⚠️ Potential issue | 🟠 Major
cy.exit()is not a valid Cypress command — will cause runtime error.
cy.exit()doesn't exist in the Cypress API. Additionally, this else branch is unreachable sincegetLoginUserByRank()already throws for missing/malformed entries.🐛 Proposed fix — remove unreachable code or use proper error handling
const { username, password: userpassword } = getLoginUserByRank(index); - if( username != "" && userpassword != "" ){ - cy.exec(`oc login -u ${username} -p ${userpassword} --kubeconfig=${Env.tmpKubeconfig}`); - }else{ - cy.log('no user can be found.'); - cy.exit(); - } + // getLoginUserByRank already validates - no need for additional checks + cy.exec(`oc login -u ${username} -p ${userpassword} --kubeconfig=${Env.tmpKubeconfig}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 312 - 315, The else block that calls cy.exit() is invalid and unnecessary—replace/remove it: remove the unreachable else branch that logs 'no user can be found.' and calls cy.exit(), or if you need explicit test failure here use a valid failure mechanism (e.g., throw an Error or call Cypress.fail) instead; locate the branch around getLoginUserByRank() in auth-commands.ts and either delete the entire else block or change cy.exit() to a proper error throw/Cypress.fail with a clear message so the test fails cleanly.
326-329:⚠️ Potential issue | 🟠 Major
cy.exit()is not a valid Cypress command.Same issue as
cliLoginAsUser. Remove unreachable code or usethrow new Error(...).🐛 Proposed fix
const { username, password: userpassword } = getLoginUserByRank(index); const oauth_url=getOauthUrl() - if( username != "" && userpassword != "" && Cypress.env('LOGIN_IDP') != "" ){ - cy.login(Cypress.env('LOGIN_IDP'), username, userpassword, oauth_url); - guidedTour.close() - }else{ - cy.log('no user can be found.'); - cy.exit(); - } + if (!Cypress.env('LOGIN_IDP')) { + throw new Error('LOGIN_IDP environment variable is not set'); + } + cy.login(Cypress.env('LOGIN_IDP'), username, userpassword, oauth_url); + guidedTour.close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 326 - 329, Replace the invalid Cypress command cy.exit() in the auth command else branch with a real error path: remove the unreachable cy.exit() call and either throw a new Error('no user can be found') or return/abort appropriately so the test fails fast; specifically update the else block that currently logs "no user can be found." (and any similar use in cliLoginAsUser) to throw an Error instead of calling cy.exit(), ensuring the calling command will stop with a clear failure.
343-345:⚠️ Potential issue | 🟠 MajorTemplate literal syntax error and invalid
cy.exit().Line 343 uses single quotes with
${index}, which outputs the literal text instead of the variable value. Alsocy.exit()is invalid.🐛 Proposed fix
- if( username != "" && userpassword != "" && Cypress.env('LOGIN_IDP') != "" ){ - cy.adminCLI(`oc adm policy add-cluster-role-to-user cluster-admin ${username}`); - cy.login(Cypress.env('LOGIN_IDP'), username, userpassword, oauth_url); - guidedTour.close() - }else{ - cy.log('Can not find the ${index} user'); - cy.exit(); - } + if (!Cypress.env('LOGIN_IDP')) { + throw new Error(`Cannot find LOGIN_IDP for the ${index} user`); + } + cy.adminCLI(`oc adm policy add-cluster-role-to-user cluster-admin ${username}`); + cy.login(Cypress.env('LOGIN_IDP'), username, userpassword, oauth_url); + guidedTour.close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 343 - 345, The log call currently uses single quotes so `${index}` is not interpolated and it calls an invalid cy.exit(); change the log to use a template literal (backticks) so the index variable is interpolated in the message (update the cy.log call), and remove the invalid cy.exit() — if the intent is to stop the test, replace the exit with a thrown error (e.g., throw new Error(...)) or return to abort further steps; specifically update the cy.log(...) and remove/replace cy.exit() where the index-not-found branch is handled in the auth command function.web/cypress/support/commands/selector-commands.ts (1)
43-91:⚠️ Potential issue | 🟠 MajorMissing
returnstatements break the Chainable contract.Several commands with braced callback bodies (
byTestID,byLegacyTestID,byButtonText,byDataID,byTestSelector,byTestDropDownMenu,byTestOperatorRow,byTestSectionHeading,byTestOperandLink) callcy.get(...)without returning it, breaking theChainablecontract declared in the TypeScript interface (lines 10–38). This prevents method chaining likecy.byTestID('foo').click().🐛 Proposed fix (apply pattern across all affected commands)
Cypress.Commands.add( 'byTestID', (selector: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>) => { - cy.get(`[data-test="${selector}"]`, options); + return cy.get(`[data-test="${selector}"]`, options); }, ); // Deprecated! new IDs should use 'data-test', ie. `cy.byTestID(...)` Cypress.Commands.add( 'byLegacyTestID', (selector: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>) => { - cy.get(`[data-test-id="${selector}"]`, options); + return cy.get(`[data-test-id="${selector}"]`, options); }, ); Cypress.Commands.add('byButtonText', (selector: string) => { - cy.get('button[type="button"]').contains(`${selector}`); + return cy.get('button[type="button"]').contains(`${selector}`); }); Cypress.Commands.add('byDataID', (selector: string) => { - cy.get(`[data-id="${selector}"]`); + return cy.get(`[data-id="${selector}"]`); }); Cypress.Commands.add( 'byTestSelector', (selector: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>) => { - cy.get(`[data-test-selector="${selector}"]`, options); + return cy.get(`[data-test-selector="${selector}"]`, options); }, ); Cypress.Commands.add('byTestDropDownMenu', (selector: string) => { - cy.get(`[data-test-dropdown-menu="${selector}"]`); + return cy.get(`[data-test-dropdown-menu="${selector}"]`); }); Cypress.Commands.add('byTestOperatorRow', (selector: string, options?: object) => { - cy.get(`[data-test-operator-row="${selector}"]`, options); + return cy.get(`[data-test-operator-row="${selector}"]`, options); }); Cypress.Commands.add('byTestSectionHeading', (selector: string) => { - cy.get(`[data-test-section-heading="${selector}"]`); + return cy.get(`[data-test-section-heading="${selector}"]`); }); Cypress.Commands.add('byTestOperandLink', (selector: string) => { - cy.get(`[data-test-operand-link="${selector}"]`); + return cy.get(`[data-test-operand-link="${selector}"]`); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/selector-commands.ts` around lines 43 - 91, Several custom Cypress commands (byTestID, byLegacyTestID, byButtonText, byDataID, byTestSelector, byTestDropDownMenu, byTestOperatorRow, byTestSectionHeading, byTestOperandLink) use braced arrow callbacks but do not return the cy.get(...) call, breaking the Chainable contract and preventing chaining; update each of those command registrations to return the cy.get(...) result (either add an explicit "return cy.get(...)" inside the function bodies or convert the callback to an implicit-return arrow expression) so calls like cy.byTestID('foo').click() work as expected (note: byTestActionID already returns implicitly and needs no change).
🧹 Nitpick comments (7)
web/scripts/run-e2e.sh (1)
18-19: Remove unusedSCRIPT_DIRvariable.
SCRIPT_DIRon line 18 is declared but never used. Note thatscript_diron line 165 is a separate variable. Consider removing the unused declaration or consolidating to use one variable.♻️ Proposed fix
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" UI_USERS=""Or consolidate with line 165:
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" UI_USERS="" ... -script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" cd $script_dir/../🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/scripts/run-e2e.sh` around lines 18 - 19, Remove the unused variable by deleting the declaration SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" (it’s never referenced) or consolidate by replacing it with the existing script_dir usage later (ensure you use the same variable name, e.g., script_dir, throughout). Update any references to match the chosen variable name and remove the duplicate/unused variable to avoid confusion between SCRIPT_DIR and script_dir.web/cypress/views/nav.ts (1)
40-44: Inconsistent indentation inswitchTabclosing braces.The closing braces on lines 42-43 have inconsistent indentation compared to the rest of the file. This doesn't affect functionality but reduces readability.
♻️ Proposed fix for indentation
tabs: { /** * Switch to a tab by name * `@param` tabname - The name of the tab to switch to */ switchTab: (tabname: string) => { cy.get(Classes.HorizontalNav).contains(tabname).should('be.visible').click(); - } -} + } + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/nav.ts` around lines 40 - 44, Adjust the inconsistent indentation for the closing braces of the switchTab function so they align with the surrounding object structure: locate the switchTab method in nav.ts and re-indent the closing "}" braces for the function and the containing object to match the opening indentation of switchTab and other methods (ensure the function's closing brace lines up with "switchTab:" and the outer object closing brace aligns with the surrounding export/object formatting).web/cypress/views/tour.ts (1)
11-15: Consider scoping the close button selector within the Kubevirt modal.The
close()method's Kubevirt modal branch (lines 12-14) uses a global[aria-label="Close"]selector, which could match close buttons outside the modal. ThecloseKubevirtTour()method correctly scopes this with.within(). Consider applying the same pattern here for consistency and reliability.♻️ Proposed fix
//Kubevirt modal - else if ($body.find(`[aria-label="Welcome modal"]`).length > 0) { + else if ($body.find('[aria-label="Welcome modal"]').length > 0) { cy.log('Kubevirt modal detected, attempting to close...'); - cy.get('[aria-label="Close"]').should('be.visible').click(); + cy.wrap($body.find('[aria-label="Welcome modal"]')).within(() => { + cy.get('[aria-label="Close"]').should('be.visible').click(); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/views/tour.ts` around lines 11 - 15, In the close() branch handling the Kubevirt modal, avoid the global '[aria-label="Close"]' selector and scope the close-button lookup to the modal element (the same approach used in closeKubevirtTour()); locate the element matching '[aria-label="Welcome modal"]' and call .within() (or use .find()) to get its '[aria-label="Close"]' button and then assert visibility and click it so you only target the modal's close button.web/cypress/e2e/acceptance.cy.ts (3)
9-10: Remove emptybeforeEachhook.The empty
beforeEachblock serves no purpose and adds noise.♻️ Proposed fix
- beforeEach( function() { - }); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` around lines 9 - 10, Remove the no-op test setup by deleting the empty beforeEach hook; locate the beforeEach( function() { }); declaration in the test file (the beforeEach block in acceptance.cy.ts) and remove it so there is no empty lifecycle hook left in the test suite.
30-31: Typos in comments: "troubelShoot" and "backgroup".Minor typos: "troubelShoot" should be "troubleShoot" (lines 30, 32, 60) and "backgroup" should be "background" (line 68).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` around lines 30 - 31, Update the misspelled words in the comments in web/cypress/e2e/acceptance.cy.ts: change every occurrence of "troubelShoot" to "troubleShoot" and "backgroup" to "background" (e.g., within the commented panel marker and any related comment blocks such as the line containing "//troubelShoot Panel query-container" and the commented HTML fragment with tp-plugin__panel-query-container) so comments read correctly.
39-39: Fragile selector: using.eq(2)for refresh button.Using positional index
.eq(2)to find the refresh button is brittle—if buttons are reordered, this will select the wrong element. Consider using a more specific selector likearia-labelor adding adata-testattribute to the refresh button.♻️ Suggested improvement
- cy.get('button').eq(2).should('be.visible'); + // Prefer a more specific selector, e.g.: + cy.get('button[aria-label="Refresh"]').should('be.visible'); + // Or add data-test="refresh-button" to the component🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/e2e/acceptance.cy.ts` at line 39, The test currently selects the refresh button with a brittle positional selector cy.get('button').eq(2); replace this with a stable selector: update the UI to add a data-test attribute (e.g., data-test="refresh-button") or ensure an accessible attribute (aria-label="Refresh") exists, then change the test to query that attribute (e.g., cy.get('button[data-test="refresh-button"]') or cy.get('button[aria-label="Refresh"]')); locate the selector use in acceptance.cy.ts (the cy.get('button').eq(2) call) and update it to the chosen stable selector.web/cypress/support/commands/auth-commands.ts (1)
98-105: Hardcodedcy.wait()delays are flaky.Lines 101 and 103 use fixed
cy.wait(2000)andcy.wait(10000)delays. These can cause flakiness if the UI loads faster or slower than expected.♻️ Consider using Cypress's built-in retry/assertion patterns
The
cy.byTestID("username", { timeout: 120000 }).should('be.visible')already waits. The additionalcy.wait()calls may be unnecessary or could be replaced with more specific assertions that Cypress will retry automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/cypress/support/commands/auth-commands.ts` around lines 98 - 105, The test uses hardcoded cy.wait(2000) and cy.wait(10000) inside the validateLogin command which causes flakiness; remove those fixed waits and instead rely on Cypress retryable assertions—keep cy.byTestID("username", { timeout: 120000 }).should('be.visible') to wait for the login UI, and replace the second arbitrary wait with a specific check before calling guidedTour.close (e.g., assert the tour trigger or overlay is visible/exists and then assert it is gone after close, or wait for a known post-login element to appear). Update the validateLogin command to remove cy.wait calls and use targeted assertions around guidedTour.close and any post-login element to ensure deterministic timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/cypress.config.ts`:
- Around line 71-74: setupNodeEvents currently calls registerCypressGrep(config)
but doesn't return the possibly-modified config from the `@cypress/grep` plugin
and the import path is outdated; update the import to use "import { plugin as
cypressGrepPlugin } from '@cypress/grep/plugin';" and change setupNodeEvents to
call registerCodeCoverage(on, config) and then return cypressGrepPlugin(config)
(i.e., replace registerCypressGrep(config) with returning
cypressGrepPlugin(config)) so the plugin receives and returns the config as
required.
In `@web/cypress/fixtures/data-test.ts`:
- Line 172: Rename the misspelled identifier AppicationLauncher to
ApplicationLauncher in the exported fixture object and update any references to
it across the codebase (tests, imports, and usages) to match the corrected key;
specifically change the property name "AppicationLauncher" to
"ApplicationLauncher" in the data-test fixture and run a global search for
"AppicationLauncher" to replace remaining occurrences so all lookups use the new
symbol.
- Line 216: The LogDetail selector string is missing a dot before the second
class name, causing it to be interpreted as a descendant tag; update the
LogDetail value to use a class selector for lv-plugin__table__details (i.e., add
the '.' and a separating space) so it reads as a descendant class selector like
'.pf-v5-c-table__td .lv-plugin__table__details' in the LogDetail constant.
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 114-115: The function signature contains an incomplete default
parameter "oauthurl: string = " which causes a syntax error; fix the arrow
function parameter by either supplying a valid default (e.g., change the
parameter to oauthurl: string = ''), removing the '=' to make it a required
parameter (oauthurl: string), or make it optional (oauthurl?: string) so the
signature is syntactically valid—update the arrow function declaration that
currently includes "oauthurl" accordingly.
In `@web/cypress/tsconfig.json`:
- Around line 1-9: The tsconfig.json has a duplicate "target" key in
compilerOptions; remove the obsolete "target": "es5" entry so only "target":
"es2018" remains in the compilerOptions object (i.e., edit the compilerOptions
block that currently contains both "target" entries and delete the "es5" line to
produce valid JSON).
In `@web/cypress/views/tour.ts`:
- Around line 41-45: Remove the stray unmatched closing brace that was left
inside the tour callback so the block containing the tour logic remains
balanced; specifically, locate the extra '}' immediately before the comment "//
Prevents navigating away from the page before the tour is closed" (around the
callback that ends with cy.wait(2000)) and delete that brace so the anonymous
function / test block (the closure wrapping the tour steps and cy.wait(2000))
has correct opening/closing braces.
---
Duplicate comments:
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 349-354: The log messages in the custom Cypress commands use
literal '${index}' inside single-quoted strings so the index won't interpolate;
update the cy.log calls in the commands where 'uiLogout' and
'uiLogoutClusterAdminForUser' are defined to use template literals (backticks)
or string concatenation so ${index} is evaluated (e.g., change 'Logout the
${index} user' to use backticks `Logout the ${index} user` or 'Logout the ' +
index + ' user') and do the same for the 'Logout the ${index} user and remove
the cluster admin roles' message.
- Around line 312-315: The else block that calls cy.exit() is invalid and
unnecessary—replace/remove it: remove the unreachable else branch that logs 'no
user can be found.' and calls cy.exit(), or if you need explicit test failure
here use a valid failure mechanism (e.g., throw an Error or call Cypress.fail)
instead; locate the branch around getLoginUserByRank() in auth-commands.ts and
either delete the entire else block or change cy.exit() to a proper error
throw/Cypress.fail with a clear message so the test fails cleanly.
- Around line 326-329: Replace the invalid Cypress command cy.exit() in the auth
command else branch with a real error path: remove the unreachable cy.exit()
call and either throw a new Error('no user can be found') or return/abort
appropriately so the test fails fast; specifically update the else block that
currently logs "no user can be found." (and any similar use in cliLoginAsUser)
to throw an Error instead of calling cy.exit(), ensuring the calling command
will stop with a clear failure.
- Around line 343-345: The log call currently uses single quotes so `${index}`
is not interpolated and it calls an invalid cy.exit(); change the log to use a
template literal (backticks) so the index variable is interpolated in the
message (update the cy.log call), and remove the invalid cy.exit() — if the
intent is to stop the test, replace the exit with a thrown error (e.g., throw
new Error(...)) or return to abort further steps; specifically update the
cy.log(...) and remove/replace cy.exit() where the index-not-found branch is
handled in the auth command function.
In `@web/cypress/support/commands/selector-commands.ts`:
- Around line 43-91: Several custom Cypress commands (byTestID, byLegacyTestID,
byButtonText, byDataID, byTestSelector, byTestDropDownMenu, byTestOperatorRow,
byTestSectionHeading, byTestOperandLink) use braced arrow callbacks but do not
return the cy.get(...) call, breaking the Chainable contract and preventing
chaining; update each of those command registrations to return the cy.get(...)
result (either add an explicit "return cy.get(...)" inside the function bodies
or convert the callback to an implicit-return arrow expression) so calls like
cy.byTestID('foo').click() work as expected (note: byTestActionID already
returns implicitly and needs no change).
In `@web/scripts/run-e2e.sh`:
- Around line 135-150: The script currently checks variables like KUBECONFIG,
CYPRESS_LOGIN_IDP and CYPRESS_LOGIN_USERS directly which breaks when run with
set -u; update all emptiness checks to use parameter expansion defaults (e.g.
use "${KUBECONFIG:-}" instead of "$KUBECONFIG") and similarly
"${CYPRESS_LOGIN_IDP:-}" and "${CYPRESS_LOGIN_USERS:-}" so the if conditions
don't reference unset variables, while leaving calls to
enable_troubleshoot_plugin and enable_idp_htpasswd unchanged and preserving the
export assignments for CYPRESS_LOGIN_IDP and CYPRESS_LOGIN_USERS when
enable_idp_htpasswd is invoked.
- Around line 170-177: Guard the unset-variable failures by using bash parameter
expansion defaults: replace occurrences of "$CYPRESS_SPEC" and "$CYPRESS_TAG" in
the conditional checks and assignments with "${CYPRESS_SPEC:-}" and
"${CYPRESS_TAG:-}" (e.g. use if [[ "${CYPRESS_SPEC:-}" == "" ]]; then and
cypress_args=" --spec ${CYPRESS_SPEC:-}" and when appending the tag use if [[
"${CYPRESS_TAG:-}" != "" ]]; then cypress_args="$cypress_args --env
grep=${CYPRESS_TAG// /}" but ensure the test and expansion both use
${CYPRESS_TAG:-} to avoid set -u aborts).
- Around line 38-46: The current oc login call is executed as a separate command
so with set -euo pipefail the script will exit on failure before the subsequent
if [[ $? == 0 ]] check; change the logic so the oc login is executed inside the
conditional (e.g., use if oc login --username=... --password=...
--kubeconfig=/tmp/normal_kubeconfig >/dev/null 2>&1; then ...) and keep the
existing success and failure branches (echo success message and return 0, echo
failure message and exit 1) so the else branch becomes reachable even with set
-e.
---
Nitpick comments:
In `@web/cypress/e2e/acceptance.cy.ts`:
- Around line 9-10: Remove the no-op test setup by deleting the empty beforeEach
hook; locate the beforeEach( function() { }); declaration in the test file (the
beforeEach block in acceptance.cy.ts) and remove it so there is no empty
lifecycle hook left in the test suite.
- Around line 30-31: Update the misspelled words in the comments in
web/cypress/e2e/acceptance.cy.ts: change every occurrence of "troubelShoot" to
"troubleShoot" and "backgroup" to "background" (e.g., within the commented panel
marker and any related comment blocks such as the line containing
"//troubelShoot Panel query-container" and the commented HTML fragment with
tp-plugin__panel-query-container) so comments read correctly.
- Line 39: The test currently selects the refresh button with a brittle
positional selector cy.get('button').eq(2); replace this with a stable selector:
update the UI to add a data-test attribute (e.g., data-test="refresh-button") or
ensure an accessible attribute (aria-label="Refresh") exists, then change the
test to query that attribute (e.g., cy.get('button[data-test="refresh-button"]')
or cy.get('button[aria-label="Refresh"]')); locate the selector use in
acceptance.cy.ts (the cy.get('button').eq(2) call) and update it to the chosen
stable selector.
In `@web/cypress/support/commands/auth-commands.ts`:
- Around line 98-105: The test uses hardcoded cy.wait(2000) and cy.wait(10000)
inside the validateLogin command which causes flakiness; remove those fixed
waits and instead rely on Cypress retryable assertions—keep
cy.byTestID("username", { timeout: 120000 }).should('be.visible') to wait for
the login UI, and replace the second arbitrary wait with a specific check before
calling guidedTour.close (e.g., assert the tour trigger or overlay is
visible/exists and then assert it is gone after close, or wait for a known
post-login element to appear). Update the validateLogin command to remove
cy.wait calls and use targeted assertions around guidedTour.close and any
post-login element to ensure deterministic timing.
In `@web/cypress/views/nav.ts`:
- Around line 40-44: Adjust the inconsistent indentation for the closing braces
of the switchTab function so they align with the surrounding object structure:
locate the switchTab method in nav.ts and re-indent the closing "}" braces for
the function and the containing object to match the opening indentation of
switchTab and other methods (ensure the function's closing brace lines up with
"switchTab:" and the outer object closing brace aligns with the surrounding
export/object formatting).
In `@web/cypress/views/tour.ts`:
- Around line 11-15: In the close() branch handling the Kubevirt modal, avoid
the global '[aria-label="Close"]' selector and scope the close-button lookup to
the modal element (the same approach used in closeKubevirtTour()); locate the
element matching '[aria-label="Welcome modal"]' and call .within() (or use
.find()) to get its '[aria-label="Close"]' button and then assert visibility and
click it so you only target the modal's close button.
In `@web/scripts/run-e2e.sh`:
- Around line 18-19: Remove the unused variable by deleting the declaration
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" (it’s never
referenced) or consolidate by replacing it with the existing script_dir usage
later (ensure you use the same variable name, e.g., script_dir, throughout).
Update any references to match the chosen variable name and remove the
duplicate/unused variable to avoid confusion between SCRIPT_DIR and script_dir.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
Makefileweb/cypress.config.tsweb/cypress/e2e/acceptance.cy.tsweb/cypress/fixtures/data-test.tsweb/cypress/support/commands/auth-commands.tsweb/cypress/support/commands/env.tsweb/cypress/support/commands/selector-commands.tsweb/cypress/support/commands/troubeshoot-commands.tsweb/cypress/support/commands/utility-commands.tsweb/cypress/support/e2e.tsweb/cypress/tsconfig.jsonweb/cypress/views/nav.tsweb/cypress/views/tour.tsweb/cypress/views/utils.tsweb/integration-tests/.eslintrcweb/integration-tests/cypress.config.jsweb/integration-tests/fixtures/example.jsonweb/integration-tests/plugins/index.tsweb/integration-tests/reporter-config.jsonweb/integration-tests/tsconfig.jsonweb/package.jsonweb/scripts/run-e2e.sh
💤 Files with no reviewable changes (6)
- web/integration-tests/tsconfig.json
- web/integration-tests/plugins/index.ts
- web/integration-tests/fixtures/example.json
- web/integration-tests/reporter-config.json
- web/integration-tests/cypress.config.js
- web/integration-tests/.eslintrc
🚧 Files skipped from review as they are similar to previous changes (4)
- web/cypress/support/e2e.ts
- web/cypress/views/utils.ts
- web/cypress/support/commands/troubeshoot-commands.ts
- web/cypress/support/commands/utility-commands.ts
anpingli
force-pushed
the
web-test
branch
5 times, most recently
from
74379f1 to
48a54b3
Compare
|
@anpingli: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
Contributor
Author
|
@jgbernalp @etmurasaki can you review the PR again? |
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anpingli, jgbernalp 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-ci
bot
added
the
approved
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
4 participants
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.
The first cypress automation case.
Prerequisite: The Troubleshoot panel plugin must be enabled.
Execution：
make test-e2e
Summary by CodeRabbit
Tests
Chores