CONSOLE-5233: Playwright-test-migration-for-console/app#16449
CONSOLE-5233: Playwright-test-migration-for-console/app#16449Cragsmann wants to merge 3 commits into
Conversation
…right-test-migration-for-console/app
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR completes a foundational migration of OpenShift Console's e2e testing framework from Cypress to Playwright. It introduces Playwright configuration, a global setup with multi-state login detection, eight page objects encapsulating UI interactions across auth, navigation, resource lists, resource details, and specialized pages like MachineConfig, and six test suites covering admission webhook warnings, multi-user authentication, debug pod terminal access, deployment autoscaling, machine configuration inspection, and CronJob job creation workflows. Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 18
🧹 Nitpick comments (9)
frontend/e2e/tests/console/app/debug-pod.spec.ts (1)
27-40: ⚡ Quick winReplace
anytypes withKubernetesClientandk8s.V1PodinpollForPodCrashState.The
k8sClientparameter andpvariable are typed asany, which hides the shape contract of the client API and pod structure.KubernetesClientis already available through the test fixtures (and properly typed throughout the codebase), andgetPods()returnsPromise<k8s.V1Pod[]>. Applying these types prevents shape regressions in critical polling logic.async function pollForPodCrashState( k8sClient: KubernetesClient, namespace: string, podName: string, timeoutMs: number, ): Promise<{ ready: boolean; reason: string }> { // ... const pod = pods.find((p: k8s.V1Pod) => p.metadata?.name === podName);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/console/app/debug-pod.spec.ts` around lines 27 - 40, In pollForPodCrashState, replace the loose any types with the proper Kubernetes types: change the k8sClient parameter type to KubernetesClient and ensure getPods() is treated as returning Promise<k8s.V1Pod[]> so the pods array is typed; update the find callback to use (p: k8s.V1Pod) (or inferred V1Pod) when locating pod.metadata?.name === podName; this ensures type-safe access to pod.metadata and prevents shape regressions in pollForPodCrashState.frontend/e2e/pages/login-page.ts (2)
19-19: ⚖️ Poor tradeoffConsider proper typing for SERVER_FLAGS.
The
window.SERVER_FLAGScast toanycould use a proper interface. Consider defining a type or interface forwindow.SERVER_FLAGSin a shared types file to improve type safety across the e2e codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/pages/login-page.ts` at line 19, Replace the any cast for window.SERVER_FLAGS by introducing a proper type (e.g., interface ServerFlags { authDisabled?: boolean; /* other flags */ }) in a shared types file and augment the global Window interface (declare global { interface Window { SERVER_FLAGS?: ServerFlags } }) so e2e code can use (window.SERVER_FLAGS?.authDisabled) without casting; update frontend/e2e/pages/login-page.ts to import the shared type module (or rely on global augmentation) and remove (window as any).SERVER_FLAGS cast, referencing SERVER_FLAGS and the evaluate call that currently accesses authDisabled.
26-36: ⚡ Quick winDuplicated login detection logic.
Lines 26-36 duplicate the multi-state login detection from global.setup.ts:45-58. Consider extracting this into a shared helper to maintain a single source of truth for the login flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/pages/login-page.ts` around lines 26 - 36, The block that waits for either loginButton, usernameInput, or provider button duplicates multi-state login detection; extract this into a shared helper (e.g., waitForLoginFlow or waitForLoginElements) placed in your e2e test helpers and replace the duplicated code in login-page.ts and global.setup.ts with a call to that helper; the helper should accept the page or page-locators and internally use the same logic (use providerButton(provider), this.loginButton, this.usernameInput) to wait for the first visible element and then, if the provider button is present and visible, click it and wait for usernameInput to be visible.frontend/e2e/pages/list-page.ts (2)
45-64: ⚡ Quick winSilent error suppression in filter visibility checks.
Using
.catch(() => false)at lines 47 and 58 silently swallows all errors, including legitimate failures like network timeouts or selector typos. If both filter UIs are absent due to an upstream bug, this method will silently succeed without applying any filter.Prefer explicit timeout handling or
isVisible({ timeout: N })without catch, and let real errors propagate.♻️ Proposed change to make errors visible
async filterByStatus(status: string): Promise<void> { const filterToggle = this.page.locator('[data-ouia-component-id="DataViewCheckboxFilter"]'); - if (await filterToggle.isVisible().catch(() => false)) { + const isCheckboxFilterVisible = await filterToggle.isVisible({ timeout: 3_000 }).catch(() => false); + if (isCheckboxFilterVisible) { await this.robustClick(filterToggle); const filterItem = this.page.locator( `[data-ouia-component-id="DataViewCheckboxFilter-filter-item-${status}"]`, ); await this.robustClick(filterItem); await this.robustClick(filterToggle); } else { const filterDropdownToggle = this.page.locator( '[data-test-id="filter-dropdown-toggle"] button', ); - if (await filterDropdownToggle.isVisible().catch(() => false)) { + const isDropdownFilterVisible = await filterDropdownToggle.isVisible({ timeout: 3_000 }).catch(() => false); + if (isDropdownFilterVisible) { await this.robustClick(filterDropdownToggle); await this.page.locator(`#${status}`).click(); await this.robustClick(filterDropdownToggle); + } else { + throw new Error(`No filter UI found for status: ${status}`); } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/pages/list-page.ts` around lines 45 - 64, The filterByStatus function silently swallows errors by using .catch(() => false) on visibility checks for filterToggle and filterDropdownToggle; replace those calls with explicit visibility checks (e.g., isVisible({ timeout: SOME_MS })) so timeouts/selector errors propagate, and if neither UI is present throw a clear error indicating both filter controls are missing; update references to filterByStatus, filterToggle, and filterDropdownToggle accordingly so legitimate errors aren't suppressed and failures are visible.
87-87: ⚖️ Poor tradeoffXPath usage for ancestor traversal.
Using
xpath=ancestor::trworks but is generally discouraged in Playwright in favor of chaining locators. If PatternFly doesn't expose a direct way to get the row from a cell, this is acceptable, but consider requestingdata-testattributes on the row element upstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/pages/list-page.ts` at line 87, The locator uses an XPath ancestor traversal via cell.locator('xpath=ancestor::tr'); replace this with a chained Playwright locator or a test attribute to avoid XPath: e.g., from the variable cell, chain parent locators (cell.locator('..').locator('[data-test="row"]') or cell.locator('..').locator('role=row')) or ask for a data-test attribute on the row element upstream so you can use cell.locator('[data-test="row"]') instead of xpath; update all occurrences referencing cell.locator('xpath=ancestor::tr') accordingly.frontend/e2e/pages/machine-config-page.ts (2)
7-8: ⚡ Quick winFragile selectors risk test flakiness.
- Line 7: The
-0suffix hardcodes the first config file. If files are reordered or filtered, this selector breaks.- Line 8: Class-based selector
.co-copy-to-clipboard__textcouples tests to implementation details and breaks when CSS refactoring occurs.Consider using
data-testattributes for both locators to improve stability.♻️ Recommended improvements
- readonly configFilePath = this.page.getByTestId('config-file-path-0'); - readonly copyToClipboard = this.page.locator('.co-copy-to-clipboard__text'); + readonly configFilePath = this.page.getByTestId('machine-config-file-path').first(); + readonly copyToClipboard = this.page.getByTestId('copy-to-clipboard');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/pages/machine-config-page.ts` around lines 7 - 8, Replace fragile DOM selectors used by configFilePath and copyToClipboard: stop using the hardcoded test id suffix 'config-file-path-0' and the implementation-class '.co-copy-to-clipboard__text'. Instead update the page to locate elements by stable data-test attributes (e.g., data-test="config-file-path" and data-test="copy-to-clipboard") and change the locators in the test to use page.getByTestId or page.locator with those attributes (update the references to configFilePath and copyToClipboard to use the new data-test selectors), so ordering or CSS refactors won't break the e2e tests.
14-16: ⚡ Quick winMethod name doesn't match implementation; text matching breaks i18n.
The method name
errorHeadingimplies error-specific behavior, but the implementation is a generic text locator. Text-based selectors are fragile when internationalization changes strings.If this is truly for errors, use a
data-testattribute or role-based selector. If it's for generic text, rename to reflect that.♻️ Suggested alternatives
Option 1: If truly error-specific:
- errorHeading(text: string): Locator { - return this.page.getByText(text); + errorHeading(): Locator { + return this.page.getByTestId('machine-config-error'); }Option 2: If generic text locator:
- errorHeading(text: string): Locator { + textHeading(text: string): Locator { return this.page.getByText(text); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/pages/machine-config-page.ts` around lines 14 - 16, The method errorHeading currently returns a generic text locator via this.page.getByText(text) which is fragile for i18n and misnamed; either (A) make it truly error-specific by changing the selector to a stable attribute or role (e.g., use a data-test attribute like locator('[data-test="error-heading"]') or a semantic role/alert/heading-based locator) and keep the method name errorHeading, or (B) if it should be a generic text finder, rename the method to something like textLocator or findByText and document it; update all callers to match the chosen approach and prefer data-test or role selectors over getByText for error-specific elements.frontend/e2e/tests/console/app/auth-multiuser-login.spec.ts (2)
23-23: ⚡ Quick winRemove unreachable fallback in passwd assignment.
The fallback
|| 'test'on line 23 is dead code—htpasswdPasswordis verified to be truthy at line 16, so it can never be falsy here. This reduces code clarity.♻️ Proposed fix to remove dead code
- const passwd = htpasswdPassword || 'test'; + const passwd = htpasswdPassword;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/console/app/auth-multiuser-login.spec.ts` at line 23, The assignment to passwd contains a dead fallback; remove the unreachable `|| 'test'` and assign the verified value directly (i.e., set passwd = htpasswdPassword). Update the use of the passwd variable in the surrounding test (auth-multiuser-login.spec) to rely on the single source of truth htpasswdPassword and ensure no other code expects a default string.
13-14: ⚡ Quick winRemove unnecessary kubeadminPassword dependency from htpasswd test.
The htpasswd test checks for
kubeadminPasswordon line 13 and skips if missing on line 16, but never uses it. This creates an unnecessary environment dependency—the htpasswd test will skip even when htpasswd credentials are available but kubeadmin credentials are not.♻️ Proposed fix to remove kubeadminPassword check
- const kubeadminPassword = process.env.BRIDGE_KUBEADMIN_PASSWORD; const htpasswdPassword = process.env.BRIDGE_HTPASSWD_PASSWORD; - if (!kubeadminPassword || !htpasswdPassword) { + if (!htpasswdPassword) { test.skip(); return; }Also applies to: 16-16
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/e2e/tests/console/app/auth-multiuser-login.spec.ts` around lines 13 - 14, The test unnecessarily reads and checks kubeadminPassword even though it's unused; update the test to remove the kubeadminPassword dependency by deleting the kubeadminPassword variable and changing the skip condition to only verify htpasswdPassword (i.e., reference htpasswdPassword instead of kubeadminPassword in the conditional that calls test.skip), ensuring any imports/usages of kubeadminPassword are removed and only htpasswdPassword controls test execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/pages/list-page.ts`:
- Around line 92-105: The dvRowsShouldExist method contains fragile
triple-nested try/catch with a full page.reload that masks selector/timing bugs;
replace the nested retries with a single explicit wait pattern (use a single
expect/waitFor with a clear timeout and one fallback) and remove the automatic
full-page reload unless you document why it’s required; investigate and fix the
root cause for missing data-test attributes (ensure PatternFly DataView forwards
them) rather than retrying, and when keeping a reload keep a short comment in
dvRowsShouldExist referencing dvCell and dvRow explaining the exact condition
that warrants reload and the observable you expect after reload.
- Around line 158-160: The getter clickCreateYAMLButton is misleading because it
returns a Locator instead of performing a click; either rename it to
createYAMLButton or getCreateYAMLButton, or change it into an async method that
performs the click (e.g., async clickCreateYAMLButton() { await
this.page.getByTestId('item-create').click(); }). Update all call sites to match
the chosen change and keep the unique symbol clickCreateYAMLButton (if
converting) or use the new getter name where referenced.
- Around line 15-25: The two helpers rowsShouldExist and rowsShouldNotExist use
different selectors which can cause false results; update rowsShouldNotExist to
use the same locator strategy as rowsShouldExist (i.e.,
this.page.locator('[data-test-rows="resource-row"]').filter({ hasText:
resourceName })) and assert it is hidden/not visible with the same timeout, or
alternatively change both to use data-test-id consistently—pick one strategy and
make both functions use that same locator (refer to rowsShouldExist and
rowsShouldNotExist to locate where to change).
- Line 60: The selector built with `#${status}` assumes `status` is a valid CSS
identifier and will break for values with spaces or special chars; replace it
with an attribute selector that matches the id value (e.g., use
this.page.locator(`[id="${status}"]`).click()) or otherwise escape `status`
before interpolation; update the occurrence of
`this.page.locator(`#${status}`).click();` to use the safe attribute form (or a
proper escaping helper) so `status` values containing special characters are
handled correctly.
- Line 154: The test constructs a RegExp from unchecked user input
(`checkboxLabel`) which enables ReDoS; update the assertion in the method using
this.page and toHaveURL so the input is escaped or avoided: either call
toHaveURL with a plain string/substring match (e.g., assert the URL contains
`=${checkboxLabel}`) or sanitize the value by passing `checkboxLabel` through a
safe escape routine (e.g., escapeRegExp) before building new
RegExp(`=${escapedLabel}`); ensure the escape helper is used wherever
`checkboxLabel` is interpolated into a RegExp.
In `@frontend/e2e/pages/login-page.ts`:
- Line 4: The locator for loginButton uses the wrong attribute (data-test-id)
causing it to miss selectors configured to use data-test; update the loginButton
definition (symbol: loginButton) to use the same test-id strategy as the rest of
the tests — either switch the manual locator to target data-test="login" or
replace it with page.getByTestId('login') so it aligns with global setup and the
getByTestId('user-dropdown-toggle') usage.
In `@frontend/e2e/pages/machine-config-page.ts`:
- Around line 18-29: The checkConfigFileDetails method uses fragile selectors
and magic values; update it to click the Info button and scope subsequent
lookups to the same info panel (avoid .first()) by targeting dedicated test
attributes (e.g., data-test="config-info-btn" for the button and
data-test="config-info-panel" for the panel) so locators like description list
and code block are looked up within that panel; replace the brittle class
selector '[class*="description-list"]' with a data-test attribute (e.g.,
data-test="config-description-list") and find displayed values for mode and
overwrite using a tolerant matcher that accepts expected renderings for booleans
(e.g., map overwrite to ["true","false","Yes","No"] or allow substring/regex
matches) instead of String(overwrite); eliminate the magic .slice(0, 30) by
either deriving the expected preview length from a named constant or by
asserting that the decoded content startsWith the expected snippet (or
parameterize the expected preview length) and scope the code block lookup to the
info panel (e.g., data-test="config-code") so locator('code').first() is no
longer used.
In `@frontend/e2e/pages/modal-page.ts`:
- Line 7: The locator in modal-page.ts uses the wrong attribute name: update the
return statement that calls this.page.locator(...) for the modal cancel button
so it targets the configured data-test attribute instead of data-test-id (or,
alternatively, use Playwright's getByTestId('modal-cancel-action')). Change the
selector that references "modal-cancel-action" to match the project's configured
test-id attribute (data-test) so the locator resolves correctly.
In `@frontend/e2e/pages/nav-page.ts`:
- Around line 68-77: The clickNavLink function may skip clicking the actual
target when given a single-level path because it only clicks the second element
when path.length === 2; update clickNavLink to always click the intended target
after ensuring the parent is expanded by calling robustClick on the final
segment (use path[path.length - 1]) — e.g., ensure you call
this.robustClick(this.sidebar.getByText(...)) for the target regardless of
whether path.length is 1 or 2 while still using the existing aria-expanded check
on navItem; reference function clickNavLink, method robustClick, and
this.sidebar.getByText.
In `@frontend/e2e/pages/yaml-editor-page.ts`:
- Around line 11-15: The setEditorContent method uses a (window as any) cast and
assumes a model exists; replace the any cast with proper optional chaining and a
null-check: inside setEditorContent's page.evaluate callback reference
window.monaco?.editor?.getModels()?.[0], verify the model is defined (and
models.length > 0) before calling setValue, and avoid the any cast by relying on
Monaco's global types or adding a minimal type declaration for window.monaco in
the project; keep the existing isImportLoaded readiness assumption but add this
defensive null-check around model access in setEditorContent.
In
`@frontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.ts`:
- Around line 106-139: The test 'Create a pod and display Admission Webhook
warning notification' duplicates the end-to-end flow and should be converted to
a table-driven (scenario) test: extract the shared setup/act flow (creating
YamlEditorPage, DetailsPage, navigation to `/k8s/ns/${testNs}/import`,
yamlEditor.isImportLoaded(), yamlEditor.setEditorContent(),
yamlEditor.clickSaveCreateButton(), and
detailsPage.sectionHeaderShouldExist('Pod details')) into a reusable function
and run it for each scenario defined in a scenarios array that includes
per-scenario values like input payload (pod1ReqObj, bulkResourcesReqObj), route
mocks (the page.route handler(s) for POST to
`**/api/kubernetes/api/v1/namespaces/${testNs}/pods` and any deployments route),
and assertions (expectations against detailsPage.admissionWarning(WARNING_ID),
admissionWarning(LEARN_MORE_ID), and text checks using WARNING_FOO, POD_NAME,
and any multi-warning filters); parameterize the route setup and assertion
callbacks in the scenario entries so the test loop composes the correct mock
headers and checks for single vs. multi-warning behavior.
In `@frontend/e2e/tests/console/app/auth-multiuser-login.spec.ts`:
- Around line 87-95: Test contains environment-specific branching around the
perspective switcher causing inconsistency with earlier unconditional behavior;
remove the conditional and always call nav.changePerspectiveTo('Core platform')
followed by nav.perspectiveSwitcherShouldHaveText('Core platform') (the
changePerspectiveTo and perspectiveSwitcherShouldHaveText helpers already handle
no-op and retries), or if you intentionally need special handling for
non-localhost, replace the if-block with a concise inline comment explaining the
exact environment-specific reason and link to any issue/commit that justifies it
so future readers understand why branching remains.
In `@frontend/e2e/tests/console/app/debug-pod.spec.ts`:
- Around line 184-188: The current check only looks under a Running filter and
can miss non-running leftover debug pods; change the cleanup assertion to verify
the debug pod is truly deleted regardless of phase by (1) locating any pod with
metadata.name !== POD_NAME from k8sClient.getPods (keep use of pods and
debugPod), (2) if found call k8sClient.getPod(debugPod.metadata.name) or
re-fetch getPods and assert the API returns no pod with that name (i.e., 404 or
absent from the array) instead of relying on listPage.dvRowsShouldNotExist under
the Running filter, and (3) optionally still call listPage.dvRowsShouldNotExist
for UI-level check but make the authoritative assertion against the k8sClient
API.
- Around line 164-168: The pod IP comparison is flaky because it uses the
unstable list order from k8sClient.getPods; make selection deterministic by
sorting or selecting pods by a stable key (e.g., metadata.name or metadata.uid)
before picking two to compare. Update the test that calls
k8sClient.getPods(testNs) to sort the returned pods (referencing the pods array
and pod objects' metadata.name/metadata.uid) and then set
ipAddressOne/ipAddressTwo from the first two entries of the sorted list so the
IP isolation check is stable.
In `@frontend/e2e/tests/console/app/deployments.spec.ts`:
- Around line 51-66: The two tests 'Enable deployment autoscale button should
exist' and 'Enable deployment autoscale button should not exist' run against the
same deployment state (the HPA created in beforeAll), causing a contradiction;
update the second test to remove the HPA before asserting the "Enable autoscale"
button is visible/hidden (or rename/assertions to match intent). Specifically,
in the second test that references DetailsPage and enableAutoscaleButton, call
the teardown step to delete the HPA (the same resource created in beforeAll) or
otherwise ensure no HPA is attached to the deployment before navigating to
/k8s/ns/${testNs}/deployments/${workloadName}, then assert the
enableAutoscaleButton visibility accordingly. Ensure the test names reflect the
verified state.
In `@frontend/e2e/tests/console/app/machine-config.spec.ts`:
- Around line 23-38: Replace the use of `any` for `mcResource` with a strong
MachineConfig type matching the OpenShift schema (e.g., define MachineConfig and
MachineConfigFile interfaces that include
spec.config.storage.files[*].contents.source, mode, overwrite), cast or type the
result of `k8sClient.customObjectsApi.getClusterCustomObject` to
`MachineConfig`, and update the code around `mcResource`, `fileEntry`, and the
destructuring before calling `mcPage.checkConfigFileDetails` to use typed
properties and optional chaining/null guards so the compiler enforces the shape
and avoids runtime errors if fields are missing (keep the call to
`mcPage.checkConfigFileDetails(mode, overwrite, source)` unchanged).
In `@frontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts`:
- Line 44: The test uses a root-absolute Playwright navigation
(page.goto(`/k8s/ns/${testNs}/import`)) which breaks when the Console is served
under a base path; change it to use a base-path-safe route by removing the
leading slash or calling the shared URL helper used across tests (e.g., use
page.goto(`k8s/ns/${testNs}/import`) or wrap the path with the repository's
base-path prefix helper) and apply the same fix to the other page.goto calls
referenced (lines using page.goto with `/k8s/...`); keep references to page.goto
and testNs to locate the spots to update.
- Around line 71-83: The retry loop that hovers/clicks the kebab (variables
kebab, action and the deadline loop) can exit by timeout but still proceeds to
await action.click(), producing a confusing later failure; after the while loop,
add an explicit check/assert that the action menu is visible (e.g., verify
action.waitFor({ state: 'visible' }) or throw a clear error if the loop expired)
and only then call action.click(), so failures are fast and deterministic.
---
Nitpick comments:
In `@frontend/e2e/pages/list-page.ts`:
- Around line 45-64: The filterByStatus function silently swallows errors by
using .catch(() => false) on visibility checks for filterToggle and
filterDropdownToggle; replace those calls with explicit visibility checks (e.g.,
isVisible({ timeout: SOME_MS })) so timeouts/selector errors propagate, and if
neither UI is present throw a clear error indicating both filter controls are
missing; update references to filterByStatus, filterToggle, and
filterDropdownToggle accordingly so legitimate errors aren't suppressed and
failures are visible.
- Line 87: The locator uses an XPath ancestor traversal via
cell.locator('xpath=ancestor::tr'); replace this with a chained Playwright
locator or a test attribute to avoid XPath: e.g., from the variable cell, chain
parent locators (cell.locator('..').locator('[data-test="row"]') or
cell.locator('..').locator('role=row')) or ask for a data-test attribute on the
row element upstream so you can use cell.locator('[data-test="row"]') instead of
xpath; update all occurrences referencing cell.locator('xpath=ancestor::tr')
accordingly.
In `@frontend/e2e/pages/login-page.ts`:
- Line 19: Replace the any cast for window.SERVER_FLAGS by introducing a proper
type (e.g., interface ServerFlags { authDisabled?: boolean; /* other flags */ })
in a shared types file and augment the global Window interface (declare global {
interface Window { SERVER_FLAGS?: ServerFlags } }) so e2e code can use
(window.SERVER_FLAGS?.authDisabled) without casting; update
frontend/e2e/pages/login-page.ts to import the shared type module (or rely on
global augmentation) and remove (window as any).SERVER_FLAGS cast, referencing
SERVER_FLAGS and the evaluate call that currently accesses authDisabled.
- Around line 26-36: The block that waits for either loginButton, usernameInput,
or provider button duplicates multi-state login detection; extract this into a
shared helper (e.g., waitForLoginFlow or waitForLoginElements) placed in your
e2e test helpers and replace the duplicated code in login-page.ts and
global.setup.ts with a call to that helper; the helper should accept the page or
page-locators and internally use the same logic (use providerButton(provider),
this.loginButton, this.usernameInput) to wait for the first visible element and
then, if the provider button is present and visible, click it and wait for
usernameInput to be visible.
In `@frontend/e2e/pages/machine-config-page.ts`:
- Around line 7-8: Replace fragile DOM selectors used by configFilePath and
copyToClipboard: stop using the hardcoded test id suffix 'config-file-path-0'
and the implementation-class '.co-copy-to-clipboard__text'. Instead update the
page to locate elements by stable data-test attributes (e.g.,
data-test="config-file-path" and data-test="copy-to-clipboard") and change the
locators in the test to use page.getByTestId or page.locator with those
attributes (update the references to configFilePath and copyToClipboard to use
the new data-test selectors), so ordering or CSS refactors won't break the e2e
tests.
- Around line 14-16: The method errorHeading currently returns a generic text
locator via this.page.getByText(text) which is fragile for i18n and misnamed;
either (A) make it truly error-specific by changing the selector to a stable
attribute or role (e.g., use a data-test attribute like
locator('[data-test="error-heading"]') or a semantic role/alert/heading-based
locator) and keep the method name errorHeading, or (B) if it should be a generic
text finder, rename the method to something like textLocator or findByText and
document it; update all callers to match the chosen approach and prefer
data-test or role selectors over getByText for error-specific elements.
In `@frontend/e2e/tests/console/app/auth-multiuser-login.spec.ts`:
- Line 23: The assignment to passwd contains a dead fallback; remove the
unreachable `|| 'test'` and assign the verified value directly (i.e., set passwd
= htpasswdPassword). Update the use of the passwd variable in the surrounding
test (auth-multiuser-login.spec) to rely on the single source of truth
htpasswdPassword and ensure no other code expects a default string.
- Around line 13-14: The test unnecessarily reads and checks kubeadminPassword
even though it's unused; update the test to remove the kubeadminPassword
dependency by deleting the kubeadminPassword variable and changing the skip
condition to only verify htpasswdPassword (i.e., reference htpasswdPassword
instead of kubeadminPassword in the conditional that calls test.skip), ensuring
any imports/usages of kubeadminPassword are removed and only htpasswdPassword
controls test execution.
In `@frontend/e2e/tests/console/app/debug-pod.spec.ts`:
- Around line 27-40: In pollForPodCrashState, replace the loose any types with
the proper Kubernetes types: change the k8sClient parameter type to
KubernetesClient and ensure getPods() is treated as returning
Promise<k8s.V1Pod[]> so the pods array is typed; update the find callback to use
(p: k8s.V1Pod) (or inferred V1Pod) when locating pod.metadata?.name === podName;
this ensures type-safe access to pod.metadata and prevents shape regressions in
pollForPodCrashState.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 270ee980-abec-4a42-966a-29bd9ada12cd
📒 Files selected for processing (19)
.gitignoreAGENTS.mdfrontend/.eslintignorefrontend/e2e/.eslintrc.jsonfrontend/e2e/global.setup.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/login-page.tsfrontend/e2e/pages/machine-config-page.tsfrontend/e2e/pages/masthead-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*
📄 CodeRabbit inference engine (STYLEGUIDE.md)
Use lowercase dash-separated names for all files (to avoid git issues with case-insensitive file systems)
Files:
frontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/pages/login-page.tsAGENTS.mdfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/masthead-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/global.setup.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/pages/machine-config-page.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx,js,jsx}: New code MUST be written in TypeScript, not JavaScript
Run the linter and follow all rules defined in .eslintrc
Never use absolute paths in code; the app should be able to run behind a proxy under an arbitrary path
Use PascalCase for component file names, kebab-case for utility files, and*.spec.ts(x)for test filesUse camelCase for variable names in TypeScript and JavaScript files
**/*.{ts,tsx,js,jsx}: Any usage of i18next'sTFunction(rather than react-i18next'sTFunction) must be performed inside a function or component.
Don't use backticks inside of aTFunction. Our code parser will not automatically pick up the keys that contain backticks. Use single or double quotes instead.
Specify possible static values in comments for dynamic i18next keys that can't be interpolated by i18next-parser, such ast(key),t('key' + id), ort(key${id}).
Files:
frontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/pages/login-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/masthead-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/global.setup.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/pages/machine-config-page.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
**/*.{ts,tsx}: Prefer functional programming patterns and immutable data structures in TypeScript/JavaScript
Use React functional components with hooks instead of class components in TypeScript
Use React hooks and Context API for state management (migrating away from legacy Redux/Immutable.js)
Use existing hooks fromconsole-sharedwhen possible (useK8sWatchResource,useUserSettings, etc.)
Use k8s resource hooks for data fetching andconsoleFetchJSONfor HTTP requests in TypeScript
Place plugin routes in plugin-specific route files
Check existing types inconsole-sharedbefore creating new types
Use SCSS modules co-located with components, PatternFly design system components, and avoid any SCSS/CSS if possible
Follow WCAG 2.1 AA standards for accessibility; use semantic HTML, ARIA labels where needed, ensure keyboard navigation, and test with screen readers
UseuseTranslation('namespace')hook withkeyformat for translation keys in TypeScript
Use ErrorBoundary components and graceful degradation patterns for error handling in TypeScript
UseuseCallbackto memoize callbacks and prevent unnecessary re-renders in React
UseuseMemofor expensive filtering and computations to prevent re-computation on every render
UseReact.lazy()to lazy load heavy components
Avoid using theanytype in TypeScript; suggest proper type definitions instead
Check that null/undefined are properly handled in TypeScript (e.g.,string | undefined)
Verify exported types for reusable components in TypeScript
Reuse types from existing components rather than duplicating type definitions in component props
UseusePluginInfohook for plugin data in TypeScript
Avoid deprecated components; check for JSDoc@deprecatedtags, import paths containing/deprecated, andDEPRECATED_file name prefixes
Use direct imports to specific files instead of barrel exports (index.ts) to avoid circular dependency cycles and improve build performance
Useimport typefor importing type...
Files:
frontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/pages/login-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/masthead-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/global.setup.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/pages/machine-config-page.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (STYLEGUIDE.md)
TypeScript tests should follow a similar 'test tables' convention as used in Go where applicable
Files:
frontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts
frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (README.md)
Follow internationalization guidelines as documented in INTERNATIONALIZATION.md for all frontend code
Never import from package index files (barrel imports) in new code - import from specific file paths instead to avoid circular dependencies and slow builds
Never use absolute URLs or paths - the console runs behind a proxy under an arbitrary path
Files:
frontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/pages/login-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/masthead-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/global.setup.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/pages/machine-config-page.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.ts
**/*.{ts,tsx,jsx}
📄 CodeRabbit inference engine (INTERNATIONALIZATION.md)
**/*.{ts,tsx,jsx}: Thearia-label,aria-placeholder,aria-roledescription, andaria-valuetextattributes should be internationalized.
The optionali18nKeyproperty on the react-i18next Trans component should only be used as a last resort when the parser incorrectly generates keys containing HTML tags.
Files:
frontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/pages/login-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/masthead-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/global.setup.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/pages/machine-config-page.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.ts
frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import from deprecated packages or use code with the
@deprecatedTSdoc tag in new codeThe i18n parser cannot extract keys from template literals - use single or double quotes in t() calls
Files:
frontend/e2e/tests/console/app/deployments.spec.tsfrontend/e2e/tests/console/app/machine-config.spec.tsfrontend/e2e/tests/console/app/auth-multiuser-login.spec.tsfrontend/e2e/pages/login-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/masthead-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/global.setup.tsfrontend/e2e/tests/console/app/debug-pod.spec.tsfrontend/e2e/pages/machine-config-page.tsfrontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.ts
AGENTS.md
📄 CodeRabbit inference engine (CLAUDE.md)
AGENTS.md file should be created and maintained as documentation for agent configuration and behavior guidelines
Files:
AGENTS.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Run yarn i18n after adding translatable strings and commit updated keys alongside any code changes that affect i18n
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Separate backend dependency updates into their own commit to isolate core logic changes
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Bug fixes should be prefixed with bug number or Jira key (e.g., OCPBUGS-1234: Fix ...) in commit messages
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Commit subject line should answer 'what changed'; body should answer 'why'
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: When opening a PR, fill out the PR template located in docs/pull_request_template.md with all required sections
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Always link to the relevant JIRA issue in the PR title and description
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Use feature branch naming convention CONSOLE-#### for feature work (Jira story number) and OCPBUGS-#### for bug fixes
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Before starting ANY changes to the dynamic plugin SDK, ensure your changes do not impact the public API by checking internal-*.ts files to avoid breakage
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Always consider impact on external plugin developers when modifying the dynamic plugin SDK
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Maintain backward compatibility in the dynamic plugin SDK as it's a public API
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Provide comprehensive documentation for all public APIs in the dynamic plugin SDK
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Ensure changes to extension schemas in the dynamic plugin SDK have migration paths
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Migrate Cypress e2e tests to Playwright using the migration context in .claude/migration-context.md
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Consult TESTING.md before writing or modifying tests for frameworks, patterns, and best practices
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Consult STYLEGUIDE.md when writing new code or reviewing style questions for TypeScript, Go, and SCSS standards
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Consult INTERNATIONALIZATION.md when adding or modifying user-facing strings for i18n patterns and useTranslation usage
Learnt from: CR
Repo: openshift/console
Timestamp: 2026-05-15T08:09:27.734Z
Learning: Consult the Dynamic Plugin SDK documentation before modifying SDK code for architecture, design principles, and development guidelines
🪛 ast-grep (0.42.2)
frontend/e2e/pages/list-page.ts
[warning] 153-153: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(=${checkboxLabel})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (21)
.gitignore (1)
43-43: LGTM!AGENTS.md (1)
127-129: LGTM!frontend/.eslintignore (1)
15-15: LGTM!frontend/e2e/.eslintrc.json (1)
2-3: LGTM!frontend/e2e/global.setup.ts (2)
5-5: LGTM!Also applies to: 9-9
45-58: LGTM!frontend/e2e/pages/masthead-page.ts (1)
1-13: LGTM!frontend/e2e/pages/details-page.ts (2)
6-67: LGTM!
42-42: ⚡ Quick winNo action needed—
toBeEmpty()is a standard Playwright assertion.The
.not.toBeEmpty()assertion at line 42 is a valid, documented Playwright API that checks if an element has non-empty text content. It's already in use elsewhere in the codebase (e.g.,smoke-test.spec.ts) and is an auto-retrying assertion, making it the appropriate choice for this check.> Likely an incorrect or invalid review comment.frontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.ts (1)
1-104: LGTM!frontend/e2e/pages/machine-config-page.ts (1)
10-12: LGTM!frontend/e2e/tests/console/app/deployments.spec.ts (1)
8-45: LGTM!frontend/e2e/tests/console/app/machine-config.spec.ts (2)
5-8: LGTM!
41-52: LGTM!frontend/e2e/tests/console/app/auth-multiuser-login.spec.ts (7)
1-4: LGTM!
6-7: LGTM!
9-10: LGTM!
29-54: LGTM!
57-77: LGTM!
79-85: LGTM!
97-102: LGTM!
| async rowsShouldExist(resourceName: string): Promise<void> { | ||
| await expect( | ||
| this.page.locator('[data-test-rows="resource-row"]').filter({ hasText: resourceName }), | ||
| ).toBeVisible({ timeout: 60_000 }); | ||
| } | ||
|
|
||
| async rowsShouldNotExist(resourceName: string): Promise<void> { | ||
| await expect(this.page.locator(`[data-test-id="${resourceName}"]`)).toBeHidden({ | ||
| timeout: 90_000, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Inconsistent selector strategy between existence checks.
rowsShouldExist filters by data-test-rows="resource-row" + text match, while rowsShouldNotExist uses data-test-id="${resourceName}". This mismatch means you're checking different DOM elements for presence vs. absence, which could produce false positives if the row exists but the data-test-id attribute is missing or named differently.
Align both methods to use the same locator strategy. If data-test-id is more reliable, use it for both; otherwise use the data-test-rows + text filter for both.
🔧 Proposed fix to align selector strategies
async rowsShouldExist(resourceName: string): Promise<void> {
await expect(
- this.page.locator('[data-test-rows="resource-row"]').filter({ hasText: resourceName }),
+ this.page.locator(`[data-test-id="${resourceName}"]`),
).toBeVisible({ timeout: 60_000 });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async rowsShouldExist(resourceName: string): Promise<void> { | |
| await expect( | |
| this.page.locator('[data-test-rows="resource-row"]').filter({ hasText: resourceName }), | |
| ).toBeVisible({ timeout: 60_000 }); | |
| } | |
| async rowsShouldNotExist(resourceName: string): Promise<void> { | |
| await expect(this.page.locator(`[data-test-id="${resourceName}"]`)).toBeHidden({ | |
| timeout: 90_000, | |
| }); | |
| } | |
| async rowsShouldExist(resourceName: string): Promise<void> { | |
| await expect( | |
| this.page.locator(`[data-test-id="${resourceName}"]`), | |
| ).toBeVisible({ timeout: 60_000 }); | |
| } | |
| async rowsShouldNotExist(resourceName: string): Promise<void> { | |
| await expect(this.page.locator(`[data-test-id="${resourceName}"]`)).toBeHidden({ | |
| timeout: 90_000, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/pages/list-page.ts` around lines 15 - 25, The two helpers
rowsShouldExist and rowsShouldNotExist use different selectors which can cause
false results; update rowsShouldNotExist to use the same locator strategy as
rowsShouldExist (i.e.,
this.page.locator('[data-test-rows="resource-row"]').filter({ hasText:
resourceName })) and assert it is hidden/not visible with the same timeout, or
alternatively change both to use data-test-id consistently—pick one strategy and
make both functions use that same locator (refer to rowsShouldExist and
rowsShouldNotExist to locate where to change).
| ); | ||
| if (await filterDropdownToggle.isVisible().catch(() => false)) { | ||
| await this.robustClick(filterDropdownToggle); | ||
| await this.page.locator(`#${status}`).click(); |
There was a problem hiding this comment.
Unvalidated CSS ID selector construction.
Constructing a CSS ID selector with #${status} assumes status is a valid CSS identifier. If status contains special characters (spaces, colons, etc.), this selector will fail silently.
🛡️ Proposed fix using attribute selector
- await this.page.locator(`#${status}`).click();
+ await this.page.locator(`[id="${status}"]`).click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.page.locator(`#${status}`).click(); | |
| await this.page.locator(`[id="${status}"]`).click(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/pages/list-page.ts` at line 60, The selector built with
`#${status}` assumes `status` is a valid CSS identifier and will break for
values with spaces or special chars; replace it with an attribute selector that
matches the id value (e.g., use this.page.locator(`[id="${status}"]`).click())
or otherwise escape `status` before interpolation; update the occurrence of
`this.page.locator(`#${status}`).click();` to use the safe attribute form (or a
proper escaping helper) so `status` values containing special characters are
handled correctly.
| async dvRowsShouldExist(resourceName: string, cellName = 'name'): Promise<void> { | ||
| const cell = this.dvCell(resourceName, cellName); | ||
| const row = this.dvRow(resourceName); | ||
| try { | ||
| await expect(cell).toBeVisible({ timeout: 30_000 }); | ||
| } catch { | ||
| await this.page.reload({ waitUntil: 'domcontentloaded' }); | ||
| try { | ||
| await expect(cell).toBeVisible({ timeout: 30_000 }); | ||
| } catch { | ||
| await expect(row).toBeVisible({ timeout: 30_000 }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Excessive retry logic with page reload masks underlying issues.
Triple-nested try-catch with a full page reload is a strong signal that the underlying selectors or timing assumptions are fragile. This pattern will make test failures non-deterministic and harder to diagnose.
Consider:
- Root-cause why
data-testattributes are sometimes missing and fix at the source (ensure PatternFly DataView forwards them). - Use explicit
waitForwith a single fallback rather than nested try-catch. - If reload is truly necessary, add a comment explaining why and under what conditions it occurs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/pages/list-page.ts` around lines 92 - 105, The dvRowsShouldExist
method contains fragile triple-nested try/catch with a full page.reload that
masks selector/timing bugs; replace the nested retries with a single explicit
wait pattern (use a single expect/waitFor with a clear timeout and one fallback)
and remove the automatic full-page reload unless you document why it’s required;
investigate and fix the root cause for missing data-test attributes (ensure
PatternFly DataView forwards them) rather than retrying, and when keeping a
reload keep a short comment in dvRowsShouldExist referencing dvCell and dvRow
explaining the exact condition that warrants reload and the observable you
expect after reload.
| ); | ||
| await expect(filterItem).toBeVisible(); | ||
| await this.robustClick(filterItem); | ||
| await expect(this.page).toHaveURL(new RegExp(`=${checkboxLabel}`), { timeout: 10_000 }); |
There was a problem hiding this comment.
Regular expression constructed from user input enables ReDoS.
Constructing new RegExp(\=${checkboxLabel}`)without escapingcheckboxLabel` allows an attacker-controlled string (or a malicious test fixture) to inject a catastrophic backtracking pattern. While this is test code, malicious fixtures or compromised dependencies could exploit this to hang the test suite.
Escape the input or use a static pattern with string matching.
🔒 Proposed fix to escape regex input
+ // Helper to escape regex special characters
+ private escapeRegex(str: string): string {
+ return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+ }
async dvFilterBy(filterName: string, checkboxLabel: string): Promise<void> {
// ... existing code ...
- await expect(this.page).toHaveURL(new RegExp(`=${checkboxLabel}`), { timeout: 10_000 });
+ await expect(this.page).toHaveURL(new RegExp(`=${this.escapeRegex(checkboxLabel)}`), { timeout: 10_000 });
await this.robustClick(this.page.locator('[data-ouia-component-id="DataViewCheckboxFilter"]'));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect(this.page).toHaveURL(new RegExp(`=${checkboxLabel}`), { timeout: 10_000 }); | |
| private escapeRegex(str: string): string { | |
| return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); | |
| } | |
| async dvFilterBy(filterName: string, checkboxLabel: string): Promise<void> { | |
| // ... existing code ... | |
| await expect(this.page).toHaveURL(new RegExp(`=${this.escapeRegex(checkboxLabel)}`), { timeout: 10_000 }); | |
| await this.robustClick(this.page.locator('[data-ouia-component-id="DataViewCheckboxFilter"]')); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/pages/list-page.ts` at line 154, The test constructs a RegExp
from unchecked user input (`checkboxLabel`) which enables ReDoS; update the
assertion in the method using this.page and toHaveURL so the input is escaped or
avoided: either call toHaveURL with a plain string/substring match (e.g., assert
the URL contains `=${checkboxLabel}`) or sanitize the value by passing
`checkboxLabel` through a safe escape routine (e.g., escapeRegExp) before
building new RegExp(`=${escapedLabel}`); ensure the escape helper is used
wherever `checkboxLabel` is interpolated into a RegExp.
| get clickCreateYAMLButton(): Locator { | ||
| return this.page.getByTestId('item-create'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Misleading getter name implies action but returns locator.
The getter is named clickCreateYAMLButton but returns a Locator rather than performing a click. This violates the principle of least surprise—callers expect click* methods to execute the action.
Rename to createYAMLButton or getCreateYAMLButton, or convert to an async method that performs the click.
♻️ Proposed rename
- get clickCreateYAMLButton(): Locator {
+ get createYAMLButton(): Locator {
return this.page.getByTestId('item-create');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get clickCreateYAMLButton(): Locator { | |
| return this.page.getByTestId('item-create'); | |
| } | |
| get createYAMLButton(): Locator { | |
| return this.page.getByTestId('item-create'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/pages/list-page.ts` around lines 158 - 160, The getter
clickCreateYAMLButton is misleading because it returns a Locator instead of
performing a click; either rename it to createYAMLButton or getCreateYAMLButton,
or change it into an async method that performs the click (e.g., async
clickCreateYAMLButton() { await this.page.getByTestId('item-create').click();
}). Update all call sites to match the chosen change and keep the unique symbol
clickCreateYAMLButton (if converting) or use the new getter name where
referenced.
| const pods = await k8sClient.getPods(testNs); | ||
| const debugPod = pods.find((p) => p.metadata?.name !== POD_NAME); | ||
| if (debugPod?.metadata?.name) { | ||
| await listPage.dvRowsShouldNotExist(debugPod.metadata.name); | ||
| } |
There was a problem hiding this comment.
Debug-pod cleanup assertion can pass without proving deletion.
Line 186–188 only checks row absence under a Running filter and only if a non-main pod is found once. A non-running leftover debug pod would still pass this test.
Suggested fix
- const pods = await k8sClient.getPods(testNs);
- const debugPod = pods.find((p) => p.metadata?.name !== POD_NAME);
- if (debugPod?.metadata?.name) {
- await listPage.dvRowsShouldNotExist(debugPod.metadata.name);
- }
+ await expect
+ .poll(async () => {
+ const pods = await k8sClient.getPods(testNs);
+ return pods.filter((p) => p.metadata?.name !== POD_NAME).length;
+ }, { timeout: 60_000 })
+ .toBe(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pods = await k8sClient.getPods(testNs); | |
| const debugPod = pods.find((p) => p.metadata?.name !== POD_NAME); | |
| if (debugPod?.metadata?.name) { | |
| await listPage.dvRowsShouldNotExist(debugPod.metadata.name); | |
| } | |
| await expect | |
| .poll(async () => { | |
| const pods = await k8sClient.getPods(testNs); | |
| return pods.filter((p) => p.metadata?.name !== POD_NAME).length; | |
| }, { timeout: 60_000 }) | |
| .toBe(0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/app/debug-pod.spec.ts` around lines 184 - 188, The
current check only looks under a Running filter and can miss non-running
leftover debug pods; change the cleanup assertion to verify the debug pod is
truly deleted regardless of phase by (1) locating any pod with metadata.name !==
POD_NAME from k8sClient.getPods (keep use of pods and debugPod), (2) if found
call k8sClient.getPod(debugPod.metadata.name) or re-fetch getPods and assert the
API returns no pod with that name (i.e., 404 or absent from the array) instead
of relying on listPage.dvRowsShouldNotExist under the Running filter, and (3)
optionally still call listPage.dvRowsShouldNotExist for UI-level check but make
the authoritative assertion against the k8sClient API.
| test('Enable deployment autoscale button should exist', async ({ page }) => { | ||
| const detailsPage = new DetailsPage(page); | ||
|
|
||
| await page.goto(`/k8s/ns/${testNs}/deployments/${workloadName}`); | ||
| await detailsPage.isLoaded(); | ||
| await expect(detailsPage.enableAutoscaleButton).toBeVisible(); | ||
| await detailsPage.enableAutoscaleButton.click(); | ||
| }); | ||
|
|
||
| test('Enable deployment autoscale button should not exist', async ({ page }) => { | ||
| const detailsPage = new DetailsPage(page); | ||
|
|
||
| await page.goto(`/k8s/ns/${testNs}/deployments/${workloadName}`); | ||
| await detailsPage.isLoaded(); | ||
| await expect(detailsPage.enableAutoscaleButton).toBeHidden({ timeout: 10_000 }); | ||
| }); |
There was a problem hiding this comment.
Contradictory test logic will cause failures.
Both tests execute serially against the same deployment with the same HPA state:
beforeAll(line 31-44) creates an HPA attached to the deployment- Test 1 (lines 51-58) expects the autoscale button to be visible
- Test 2 (lines 60-66) expects the autoscale button to be hidden
Both tests run against identical state—the HPA persists throughout the suite. One of these tests will fail, or the test names don't match their intent.
Likely intent:
- Test 1: Verify the "Remove autoscale" button appears when HPA exists
- Test 2: Verify the "Enable autoscale" button appears when HPA does not exist
If that's the case, test 2 must delete the HPA first.
🐛 Proposed fix
test('Enable deployment autoscale button should not exist', async ({ page }) => {
const detailsPage = new DetailsPage(page);
+
+ // Delete the HPA to test the "enable autoscale" flow
+ await k8sClient.deleteCustomResource(
+ 'autoscaling',
+ 'v1',
+ testNs,
+ 'horizontalpodautoscalers',
+ workloadName
+ );
await page.goto(`/k8s/ns/${testNs}/deployments/${workloadName}`);
await detailsPage.isLoaded();
await expect(detailsPage.enableAutoscaleButton).toBeHidden({ timeout: 10_000 });
});Or clarify test names and assertions if the intent differs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/app/deployments.spec.ts` around lines 51 - 66, The
two tests 'Enable deployment autoscale button should exist' and 'Enable
deployment autoscale button should not exist' run against the same deployment
state (the HPA created in beforeAll), causing a contradiction; update the second
test to remove the HPA before asserting the "Enable autoscale" button is
visible/hidden (or rename/assertions to match intent). Specifically, in the
second test that references DetailsPage and enableAutoscaleButton, call the
teardown step to delete the HPA (the same resource created in beforeAll) or
otherwise ensure no HPA is attached to the deployment before navigating to
/k8s/ns/${testNs}/deployments/${workloadName}, then assert the
enableAutoscaleButton visibility accordingly. Ensure the test names reflect the
verified state.
| const mcResource: any = await k8sClient.customObjectsApi.getClusterCustomObject({ | ||
| group: 'machineconfiguration.openshift.io', | ||
| version: 'v1', | ||
| plural: 'machineconfigs', | ||
| name: MC_WITH_CONFIG_FILES, | ||
| }); | ||
| const fileEntry = mcResource?.spec?.config?.storage?.files?.[0]; | ||
| expect(fileEntry).toHaveProperty('contents'); | ||
| expect(fileEntry).toHaveProperty('mode'); | ||
| expect(fileEntry).toHaveProperty('overwrite'); | ||
| const { | ||
| contents: { source }, | ||
| mode, | ||
| overwrite, | ||
| } = fileEntry; | ||
| await mcPage.checkConfigFileDetails(mode, overwrite, source); |
There was a problem hiding this comment.
Avoid any type; define MachineConfig interface.
Line 23 uses any, bypassing TypeScript's safety. The nested property access (line 29) and destructuring (lines 33-37) have no type checking, risking runtime errors if the MachineConfig structure changes.
Define a MachineConfig interface matching the OpenShift API schema.
💡 Recommended type definition
Create a type definition (or reuse from @openshift/console-types if available):
interface MachineConfigFile {
path: string;
contents: {
source: string;
};
mode: number;
overwrite: boolean;
}
interface MachineConfig {
apiVersion: string;
kind: string;
metadata: {
name: string;
};
spec: {
config?: {
storage?: {
files?: MachineConfigFile[];
};
};
};
}Then apply:
- const mcResource: any = await k8sClient.customObjectsApi.getClusterCustomObject({
+ const mcResource = await k8sClient.customObjectsApi.getClusterCustomObject({
group: 'machineconfiguration.openshift.io',
version: 'v1',
plural: 'machineconfigs',
name: MC_WITH_CONFIG_FILES,
- });
+ }) as MachineConfig;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/app/machine-config.spec.ts` around lines 23 - 38,
Replace the use of `any` for `mcResource` with a strong MachineConfig type
matching the OpenShift schema (e.g., define MachineConfig and MachineConfigFile
interfaces that include spec.config.storage.files[*].contents.source, mode,
overwrite), cast or type the result of
`k8sClient.customObjectsApi.getClusterCustomObject` to `MachineConfig`, and
update the code around `mcResource`, `fileEntry`, and the destructuring before
calling `mcPage.checkConfigFileDetails` to use typed properties and optional
chaining/null guards so the compiler enforces the shape and avoids runtime
errors if fields are missing (keep the call to
`mcPage.checkConfigFileDetails(mode, overwrite, source)` unchanged).
| const yamlEditor = new YamlEditorPage(page); | ||
| const detailsPage = new DetailsPage(page); | ||
|
|
||
| await page.goto(`/k8s/ns/${testNs}/import`); |
There was a problem hiding this comment.
Avoid root-absolute navigation paths in Playwright routes.
These page.goto('/k8s/...') calls hardcode root-relative URLs and can break when Console is served behind a proxy/base path. Please switch to base-path-safe navigation (no leading slash, or a shared helper that prefixes the active base path).
Suggested update
- await page.goto(`/k8s/ns/${testNs}/import`);
+ await page.goto(`k8s/ns/${testNs}/import`);
- await page.goto(`/k8s/ns/${testNs}/cronjobs`);
+ await page.goto(`k8s/ns/${testNs}/cronjobs`);
- await page.goto(`/k8s/ns/${testNs}/cronjobs`);
+ await page.goto(`k8s/ns/${testNs}/cronjobs`);
- await page.goto(`/k8s/ns/${testNs}/cronjobs/${CRONJOB_NAME}/jobs`);
+ await page.goto(`k8s/ns/${testNs}/cronjobs/${CRONJOB_NAME}/jobs`);
- await page.goto(`/k8s/ns/${testNs}/cronjobs/${CRONJOB_NAME}/events`);
+ await page.goto(`k8s/ns/${testNs}/cronjobs/${CRONJOB_NAME}/events`);As per coding guidelines, "Never use absolute URLs or paths - the console runs behind a proxy under an arbitrary path".
Also applies to: 60-60, 92-92, 94-94, 101-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts` at line 44,
The test uses a root-absolute Playwright navigation
(page.goto(`/k8s/ns/${testNs}/import`)) which breaks when the Console is served
under a base path; change it to use a base-path-safe route by removing the
leading slash or calling the shared URL helper used across tests (e.g., use
page.goto(`k8s/ns/${testNs}/import`) or wrap the path with the repository's
base-path prefix helper) and apply the same fix to the other page.goto calls
referenced (lines using page.goto with `/k8s/...`); keep references to page.goto
and testNs to locate the spots to update.
| const deadline = Date.now() + 30_000; | ||
| while (Date.now() < deadline) { | ||
| await kebab.hover(); | ||
| await kebab.click(); | ||
| try { | ||
| await action.waitFor({ state: 'visible', timeout: 5_000 }); | ||
| break; | ||
| } catch { | ||
| // Menu may have closed due to table re-render; retry | ||
| } | ||
| } | ||
| await action.click(); | ||
|
|
There was a problem hiding this comment.
Fail fast when kebab-action retry is exhausted.
If the loop reaches the deadline, Line 82 still clicks action, which usually fails later with a less clear timeout. Add an explicit post-loop assertion so failures are deterministic and easier to diagnose.
Suggested update
const deadline = Date.now() + 30_000;
+ let found = false;
while (Date.now() < deadline) {
await kebab.hover();
await kebab.click();
try {
await action.waitFor({ state: 'visible', timeout: 5_000 });
+ found = true;
break;
} catch {
// Menu may have closed due to table re-render; retry
}
}
+ expect(found).toBeTruthy();
await action.click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const deadline = Date.now() + 30_000; | |
| while (Date.now() < deadline) { | |
| await kebab.hover(); | |
| await kebab.click(); | |
| try { | |
| await action.waitFor({ state: 'visible', timeout: 5_000 }); | |
| break; | |
| } catch { | |
| // Menu may have closed due to table re-render; retry | |
| } | |
| } | |
| await action.click(); | |
| const deadline = Date.now() + 30_000; | |
| let found = false; | |
| while (Date.now() < deadline) { | |
| await kebab.hover(); | |
| await kebab.click(); | |
| try { | |
| await action.waitFor({ state: 'visible', timeout: 5_000 }); | |
| found = true; | |
| break; | |
| } catch { | |
| // Menu may have closed due to table re-render; retry | |
| } | |
| } | |
| expect(found).toBeTruthy(); | |
| await action.click(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts` around lines
71 - 83, The retry loop that hovers/clicks the kebab (variables kebab, action
and the deadline loop) can exit by timeout but still proceeds to await
action.click(), producing a confusing later failure; after the while loop, add
an explicit check/assert that the action menu is visible (e.g., verify
action.waitFor({ state: 'visible' }) or throw a clear error if the loop expired)
and only then call action.click(), so failures are fast and deterministic.
|
Waiting for openshift/release#78021 |
2 participants
Analysis / Root cause:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit
New Features
Tests
Documentation