CONSOLE-5233: Playwright-test-migration-for-console/app - #16449
Closed
Cragsmann wants to merge 10 commits into
Closed
CONSOLE-5233: Playwright-test-migration-for-console/app#16449Cragsmann wants to merge 10 commits into
Cragsmann wants to merge 10 commits into
Conversation
Contributor
|
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:
WalkthroughRewrites Playwright E2E page objects ( ChangesPlaywright E2E page objects and test specs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 5❌ Failed checks (5 warnings)
✅ Passed checks (10 passed)
✨ 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}
Contributor
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!
Member
|
Waiting for openshift/release#78021 |
openshift-ci
Bot
added
needs-rebase
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@Cragsmann: This pull request references CONSOLE-5233 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
needs-rebase
Cragsmann
force-pushed
the
CONSOLE-5233--Playwright-test-migration-for-console/app
branch
from
June 1, 2026 13:27
8c11000 to
ae277e9
Compare
openshift-ci
Bot
removed
the
needs-rebase
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/e2e/pages/base-page.ts (1)
90-98: 💤 Low valueOptional: short-circuit the happy path to avoid a fixed 5s wait per call.
When the "Model does not exist" message is absent (the common case),
waitFor({ state: 'visible', timeout: 5_000 })blocks the full 5s before the catch returns. SincereloadIfModelNotFound()runs on everyDetailsPage.waitForPageLoad()andListPage.dvRowsShouldBeLoaded(), this adds ~5s to most loads. A quickisVisible()pre-check avoids the timeout cost.♻️ Proposed change
protected async reloadIfModelNotFound(maxRetries = 3): Promise<void> { for (let attempt = 0; attempt < maxRetries; attempt++) { - try { - await this.page - .getByText('Model does not exist') - .waitFor({ state: 'visible', timeout: 5_000 }); - } catch { - return; - } + if (!(await this.page.getByText('Model does not exist').isVisible().catch(() => false))) { + return; + }🤖 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/base-page.ts` around lines 90 - 98, The reloadIfModelNotFound method currently always calls waitFor with a 5s timeout which adds latency when the "Model does not exist" text is absent; modify reloadIfModelNotFound to first perform a fast presence check (e.g., call this.page.getByText('Model does not exist').isVisible() or an equivalent quick check) and return immediately if not visible, otherwise proceed to the existing waitFor/retry logic; update callers like DetailsPage.waitForPageLoad and ListPage.dvRowsShouldBeLoaded only if they rely on the old blocking behavior.frontend/e2e/pages/details-page.ts (1)
45-48: ⚡ Quick winUse
locator.or().first().waitForinstead ofPromise.racefor this 30s visibility wait
Promise.racewon’t produce an unhandled rejection (it consumes rejections), but it still runs bothwaitFors in parallel and can burn the full timeout unnecessarily. Prefer a single active wait by unioning the locators:♻️ Use
locator.or()instead ofPromise.race- await Promise.race([ - this.resourceTitle.waitFor({ state: 'visible', timeout: 30_000 }), - this.pageHeading.waitFor({ state: 'visible', timeout: 30_000 }), - ]); + await this.resourceTitle + .or(this.pageHeading) + .first() + .waitFor({ state: 'visible', timeout: 30_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/details-page.ts` around lines 45 - 48, Replace the parallel Promise.race waits for resourceTitle and pageHeading with a single locator union so only one active wait runs: use the locator.or(...) union of this.resourceTitle and this.pageHeading, take .first(), and call .waitFor({ state: 'visible', timeout: 30_000 }) on that union; update the code around the Promise.race block to use this locator.or(...).first().waitFor instead of running both waits in parallel.
🤖 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/base-page.ts`:
- Around line 99-101: The current reload uses page.evaluate(() =>
window.location.reload()) followed by page.waitForLoadState('load'), which can
race and cause flakiness; replace the JS-initiated reload with Playwright's
deterministic API by calling page.reload({ waitUntil: 'load', timeout: 30000 })
and then await this.waitForLoadingComplete(10_000); if you cannot change the
app-triggered reload, instead synchronize the trigger and navigation by awaiting
both the load event and the evaluate call together (e.g., Promise.all([
page.waitForEvent('load'), this.page.evaluate(...) ])) so the subsequent
page.waitForLoadState / waitForLoadingComplete observe the new navigation
reliably—look for usages around page.evaluate, page.waitForLoadState, and
waitForLoadingComplete in base-page.ts to update.
---
Nitpick comments:
In `@frontend/e2e/pages/base-page.ts`:
- Around line 90-98: The reloadIfModelNotFound method currently always calls
waitFor with a 5s timeout which adds latency when the "Model does not exist"
text is absent; modify reloadIfModelNotFound to first perform a fast presence
check (e.g., call this.page.getByText('Model does not exist').isVisible() or an
equivalent quick check) and return immediately if not visible, otherwise proceed
to the existing waitFor/retry logic; update callers like
DetailsPage.waitForPageLoad and ListPage.dvRowsShouldBeLoaded only if they rely
on the old blocking behavior.
In `@frontend/e2e/pages/details-page.ts`:
- Around line 45-48: Replace the parallel Promise.race waits for resourceTitle
and pageHeading with a single locator union so only one active wait runs: use
the locator.or(...) union of this.resourceTitle and this.pageHeading, take
.first(), and call .waitFor({ state: 'visible', timeout: 30_000 }) on that
union; update the code around the Promise.race block to use this
locator.or(...).first().waitFor instead of running both waits in parallel.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 375e6f0d-d6ce-49e7-baab-02ab2b64a8f4
📒 Files selected for processing (4)
frontend/e2e/pages/base-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts
- frontend/e2e/pages/list-page.ts
- Align rowsShouldExist/rowsShouldNotExist to use consistent data-test-id selector - Use safe attribute selector [id=""] instead of CSS ID selector # - Rename misleading clickCreateYAMLButton getter to createYAMLButton - Handle single-level paths in clickNavLink to prevent no-op - Add null check for Monaco editor model in setEditorContent - Remove environment-specific conditional in perspective switching test - Use deterministic pod selection by name for IP isolation check - Use expect.poll for robust debug pod cleanup verification - Define MachineConfig interface to replace any type - Add fail-fast assertion when kebab retry loop is exhausted Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The console's API model discovery can race with page rendering in fresh browser contexts, causing "Model does not exist" errors. Add a reloadIfModelNotFound() helper to BasePage that detects the error and triggers a page reload (matching the "Try again" button behavior). Integrate the retry into DetailsPage.isLoaded() and ListPage.dvRowsShouldBeLoaded(), and add isLoaded()/dvRowsShouldBeLoaded() calls in cronjob tests that were missing page-ready checks. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tandards
- Remove redundant .eslintrc.json (superseded by .eslintrc.cjs on main)
- Replace waitFor() calls with expect assertions where applicable
- Add eslint-disable comments for intentional waitFor probes (try/catch)
- Add eslint-disable for expect-expect false positives (assertions in
test.step blocks and page object methods)
- Remove redundant waitFor before clickDebugContainerLink (robustClick
already auto-waits)
- Fix data-test-id selector to use getByTestId in login-page
- Add { tag: ['@admin'] } to all test.describe blocks
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rhamilto
force-pushed
the
CONSOLE-5233--Playwright-test-migration-for-console/app
branch
from
June 29, 2026 18:02
ce00b8d to
87b9881
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/e2e/pages/modal-page.ts (1)
10-12: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winScope the submit button to the modal to avoid strict-mode ambiguity.
button[type=submit]matches any submit button on the page, not just the modal. If an underlying page (e.g., a create/edit form behind the modal) also renders a submit button, Playwright strict mode will throw on.click(). ThecancelButtonis already scoped via its test id; consider scoping the submit button to the dialog as well.♻️ Proposed scoping
private get submitButton() { - return this.page.locator('button[type=submit]'); + return this.page.locator('[role="dialog"] button[type=submit]'); }Please confirm the modal container's role/selector in the rendered DOM so the scoping locator matches.
🤖 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/modal-page.ts` around lines 10 - 12, The submitButton locator in ModalPage is too broad and can hit submit buttons outside the modal, causing strict-mode ambiguity. Update the submitButton getter to scope its selector to the modal container/dialog used by this page object, similar to how cancelButton is already scoped with a test id, and make sure the locator targets the rendered modal role/selector rather than the generic button[type=submit].
🤖 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 108-111: The absence check in dvRowsShouldNotExist only verifies
dvCell is hidden, which can pass when the DataView cell is never forwarded to
the DOM. Update dvRowsShouldNotExist in ListPage to mirror dvRowsShouldExist by
checking both the dvCell and the generic dvRow locator for the same
resourceName, and require the row itself to be hidden so delete/uninstall flows
don’t falsely pass when only the cell is missing.
---
Nitpick comments:
In `@frontend/e2e/pages/modal-page.ts`:
- Around line 10-12: The submitButton locator in ModalPage is too broad and can
hit submit buttons outside the modal, causing strict-mode ambiguity. Update the
submitButton getter to scope its selector to the modal container/dialog used by
this page object, similar to how cancelButton is already scoped with a test id,
and make sure the locator targets the rendered modal role/selector rather than
the generic button[type=submit].
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3cf0997-e680-4726-87bb-267b4a9ff9d9
📒 Files selected for processing (18)
.gitignoreAGENTS.mdfrontend/.eslintignorefrontend/e2e/pages/base-page.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
✅ Files skipped from review due to trivial changes (2)
- frontend/.eslintignore
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (11)
- frontend/e2e/tests/console/app/deployments.spec.ts
- frontend/e2e/tests/console/app/machine-config.spec.ts
- frontend/e2e/pages/login-page.ts
- frontend/e2e/pages/nav-page.ts
- frontend/e2e/pages/machine-config-page.ts
- AGENTS.md
- frontend/e2e/tests/console/app/admission-webhook-warning-notifications.spec.ts
- frontend/e2e/tests/console/app/debug-pod.spec.ts
- frontend/e2e/tests/console/app/start-job-from-cronjob.spec.ts
- frontend/e2e/tests/console/app/auth-multiuser-login.spec.ts
- frontend/e2e/pages/base-page.ts
- YamlEditorPage.setEditorContent now delegates to BasePage which includes a waitForFunction readiness guard for the Monaco model - YamlEditorPage.isImportLoaded uses expect assertion instead of waitFor - filterByStatus falls through to dvFilterBy for DataView filter pages - dvRowsShouldNotExist checks both dvCell and dvRow locators Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove YamlEditorPage.setEditorContent pass-through (inherits from BasePage) - Extract sectionHeading locator in DetailsPage, reuse in sectionHeaderShouldExist - MachineConfigPage extends DetailsPage instead of BasePage, removing duplicate sectionHeading method - Simplify machine-config.spec.ts to use single MachineConfigPage instance Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…in details-page Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
|
/test backend |
Member
|
/retest |
1 similar comment
Member
|
/retest |
Member
|
/test images |
Member
|
/retest |
1 similar comment
Member
|
/retest |
…ir APIs Restore upstream page objects (details-page, list-page, masthead-page, modal-page, yaml-editor-page, base-page) to their main branch versions to avoid breaking existing tests. Update all new test files to consume the upstream APIs directly, using inline page.locator()/page.getByTestId() for test-specific elements. Add retry-model-error utility to handle the transient "Model does not exist" error without modifying shared page objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- retryOnModelNotFound now throws a descriptive error after exhausting retries instead of silently continuing - Restore .artifacts/ gitignore entry and trailing newline - Use Playwright baseURL in LoginPage instead of hardcoded localhost - Simplify redundant provider button visibility check - Remove unnecessary kubeadmin password guard from htpasswd login test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rhamilto
added
the
tide/merge-method-squash
Member
|
/retest |
1 similar comment
Member
|
/retest |
Member
|
/test e2e-gcp-console |
Contributor
|
@Cragsmann: 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. |
Member
|
/approved |
openshift-ci
Bot
added
docs-approved
Member
|
/verified by CI |
openshift-ci-robot
added
the
verified
Contributor
|
@rhamilto: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
logonoff
approved these changes
Aug 10, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Cragsmann, logonoff The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-rebase
Contributor
|
PR needs rebase. 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. |
logonoff
reviewed
Aug 10, 2026
Member
There was a problem hiding this comment.
.eslintignore no longer exists
Contributor
|
As agreed with @rhamilto I took advantage of this migration task that needed rebase to test the new skills for the agents ( #16986 ). So I have created a new PR ready to be reviewed (#17015). The description lists the improvements brought by the new skill on the first try, resulting in a clean migration from an already rebased branch. /cc @logonoff |
3 tasks
Member
|
Replaced by #17015 |
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
6 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.
Summary
datagrid-test-utils.tswith reusable cleanup/wait helpers for Data Grid operator resources (cluster-scoped Operator CRs, CSVs, subscriptions)DetailsPage,ModalPage,ListPage,NavPage,YamlEditorPagepage objects and OLM-specific page objects (InstalledOperatorsPage,OperatorDetailsPage,OperatorHubPage)k8sClientAPI polling throughout to verify resource lifecycle (CSV removal, operand deletion) instead of relying on UI-only assertions with fixed timeoutsgetByRoleinstead of ambiguoush1locator)Tests migrated (4 Cypress → 4 Playwright)
Source component changes
None — all changes are in the test infrastructure (
frontend/e2e/)Screenshots / Screen recordings
Automated test migration — no visual changes
Test plan
--workers=1)Summary by CodeRabbit