[WIP] CONSOLE-5276: Migrate demo-dynamic-plugin, yaml-editor, and poll-console-updates Cypress tests to Playwright#16664
Open
rhamilto wants to merge 3 commits into
Open
[WIP] CONSOLE-5276: Migrate demo-dynamic-plugin, yaml-editor, and poll-console-updates Cypress tests to Playwright#16664rhamilto wants to merge 3 commits into
rhamilto wants to merge 3 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@rhamilto: This pull request references CONSOLE-5276 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. |
rhamilto
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
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:
WalkthroughThis PR adds Playwright page objects, utilities, UI test selectors, and new console app and CRUD end-to-end specs, while shifting coverage from prior Cypress suites to Playwright-based test flows. ChangesPlaywright E2E migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
component/core
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/public/components/error.tsx (1)
27-50:⚠️ Potential issue | 🟡 MinorMove
data-testafter the prop spread to prevent accidental override.In JSX, props are merged left-to-right, so if a caller passes
data-testin props, it will override the hardcoded selector. Although no current usages do this, movingdata-testafter{...props}defensively ensures the test hook cannot be inadvertently disabled.Suggested fix
<PfErrorState - data-test="error-page" status="none" icon={NotFoundIcon} headingLevel="h1" titleText={t('404: Page Not Found')} @@ customFooter={ <ButtonLink variant="link" href="/"> {t('Return to homepage')} </ButtonLink> } {...props} + data-test="error-page" />🤖 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/public/components/error.tsx` around lines 27 - 50, The data-test prop on the PfErrorState component is positioned before the props spread operator, which means if a caller passes data-test in the props object, it will override the hardcoded value. Move the data-test="error-page" prop to after the {...props} spread to ensure the test selector is always applied defensively and cannot be accidentally overridden by passed-in props.
🧹 Nitpick comments (1)
frontend/public/components/utils/kebab.tsx (1)
73-79: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winKeep the new
data-teststable.Deriving a selector from translated text makes Playwright locators locale-sensitive. Prefer a stable value here, such as the untranslated
labelKeyor another explicit test id, and leavedata-test-actionas-is if it still needs the rendered label.♻️ Proposed fix
- data-test={ - option.labelKey - ? t(option.labelKey, option.labelKind) - : typeof option.label === 'string' - ? option.label - : undefined - } + data-test={ + option.labelKey + ? option.labelKey + : typeof option.label === 'string' + ? option.label + : undefined + }🤖 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/public/components/utils/kebab.tsx` around lines 73 - 79, The data-test attribute in the kebab menu component is currently being set to translated text (via t(option.labelKey, option.labelKind) or option.label), which makes Playwright locators locale-sensitive. Replace this logic to use a stable, untranslated value instead, such as option.labelKey directly, to keep the test selector consistent across different locales. Leave data-test-action unchanged if it still requires the rendered label text.
🤖 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 4-8: The waitForFunction call in the getEditorContent helper
currently requires both model availability AND non-empty, non-whitespace
content, which causes timeouts when the editor is intentionally empty. Modify
the condition to only verify that the Monaco editor model exists and the
getValue method returns a value, then remove the validation that checks for
non-whitespace and non-zero length content, allowing the function to return
empty strings or whitespace-only content as-is.
In `@frontend/e2e/pages/nav-page.ts`:
- Around line 6-14: The clickNavLink method uses exact: false when selecting
sidebar text which can cause ambiguous matches when labels partially overlap,
and it does not validate the path input parameter before accessing path[0] and
path.length, making navigation unreliable. Add input validation at the start of
the clickNavLink method to ensure path is a non-empty array, and change the
exact: false parameter to exact: true in both getByText calls to ensure precise
matching of navigation labels.
In `@frontend/e2e/tests/console/app/demo-dynamic-plugin.spec.ts`:
- Line 58: The find() method used to locate the Deployment resource can return
undefined if no matching resource is found, but the code does not validate this
before using the result in subsequent API calls, which leads to unclear error
messages. Add validation checks after each find() call (including the Deployment
resource lookup and similar calls at the other locations mentioned) to ensure
the resources are found before proceeding, throwing a descriptive error if any
required resource is missing from the manifest.
- Around line 65-75: The containers array rebuilding logic only preserves the
first container (index 0) from the original deployment spec and discards any
additional containers like sidecars. Instead of creating a new containers array
with only the first container modified, map through all containers from the
original deployment spec, override the image property to PLUGIN_PULL_SPEC only
for the first container at index 0, and preserve all remaining containers
unchanged in the rebuilt array.
- Line 17: The SHOULD_DEPLOY_PLUGIN constant uses incorrect logic that prevents
deployment on CI when CYPRESS_PLUGIN_PULL_SPEC is unset. Currently the ternary
expression IS_CI ? !!PLUGIN_PULL_SPEC : !IS_LOCAL_DEV evaluates to false on CI
when PLUGIN_PULL_SPEC is not set, skipping deployment entirely instead of
allowing it to proceed with the manifest's default image. Change the logic so
that deployment is enabled on CI regardless of whether PLUGIN_PULL_SPEC is set,
while still respecting the IS_LOCAL_DEV check for local development scenarios.
- Line 52: Replace the `__dirname` reference in the `manifestPath` variable
assignment with `import.meta.dirname` to ensure ESM compatibility. Since the
frontend/e2e/package.json declares "type": "module" and other e2e fixtures
already use import.meta.dirname for path resolution, this change maintains
consistency and ensures the Playwright test works properly in ESM environments.
In `@frontend/e2e/tests/console/crud/other-routes.spec.ts`:
- Around line 154-223: The test suite in the Perspective query parameters
describe block mutates the cluster-scoped consoles/cluster configuration via
patchClusterCustomObject in the first test (Developer query parameter switches
to Developer perspective) but never restores it, causing inter-test coupling
where the second test (Administrator query parameter switches to Administrator
perspective) implicitly depends on this mutation. Add a test.afterEach hook
within the describe block to restore the original consoles/cluster configuration
after tests run, and refactor the second test to not depend on the state change
from the first test by ensuring it can run independently regardless of test
execution order.
In `@frontend/package.json`:
- Line 234: The `@axe-core/playwright` dependency in the package.json file is
using a caret range version specification (^4.10.2) which allows minor and patch
version updates. To meet supply chain security requirements and ensure
reproducibility, remove the caret prefix and pin this dependency to an exact
version by changing the version string from "^4.10.2" to "4.10.2" on line 234.
---
Outside diff comments:
In `@frontend/public/components/error.tsx`:
- Around line 27-50: The data-test prop on the PfErrorState component is
positioned before the props spread operator, which means if a caller passes
data-test in the props object, it will override the hardcoded value. Move the
data-test="error-page" prop to after the {...props} spread to ensure the test
selector is always applied defensively and cannot be accidentally overridden by
passed-in props.
---
Nitpick comments:
In `@frontend/public/components/utils/kebab.tsx`:
- Around line 73-79: The data-test attribute in the kebab menu component is
currently being set to translated text (via t(option.labelKey, option.labelKind)
or option.label), which makes Playwright locators locale-sensitive. Replace this
logic to use a stable, untranslated value instead, such as option.labelKey
directly, to keep the test selector consistent across different locales. Leave
data-test-action unchanged if it still requires the rendered label text.
🪄 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: 98e8e7fd-4079-427a-b2cc-e79f91074878
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (48)
.claude/migration-context.md.claude/skills/migrate-cypress/SKILL.mdfrontend/e2e/clients/kubernetes-client.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/pages/alertmanager-page.tsfrontend/e2e/pages/base-page.tsfrontend/e2e/pages/console-plugin-page.tsfrontend/e2e/pages/details-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/role-binding-page.tsfrontend/e2e/tests/console/app/demo-dynamic-plugin.spec.tsfrontend/e2e/tests/console/crd-extensions/crd-test-utils.tsfrontend/e2e/tests/console/crud/add-storage-crud.spec.tsfrontend/e2e/tests/console/crud/image-pull-secret.spec.tsfrontend/e2e/tests/console/crud/other-routes.spec.tsfrontend/e2e/tests/console/crud/quotas.spec.tsfrontend/e2e/tests/console/crud/roles-rolebindings.spec.tsfrontend/e2e/utils/a11y.tsfrontend/package.jsonfrontend/packages/console-app/src/components/nav/NavHeader.tsxfrontend/packages/console-app/src/components/tour/tour-context.tsfrontend/packages/console-shared/src/components/actions/menu/ActionMenuContent.tsxfrontend/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsxfrontend/packages/console-shared/src/components/breadcrumbs/Breadcrumbs.tsxfrontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsxfrontend/packages/console-shared/src/constants/common.tsfrontend/packages/integration-tests/cypress-common-config.jsfrontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.tsfrontend/packages/integration-tests/tests/crud/add-storage-crud.cy.tsfrontend/packages/integration-tests/tests/crud/image-pull-secret.cy.tsfrontend/packages/integration-tests/tests/crud/other-routes.cy.tsfrontend/packages/integration-tests/tests/crud/quotas.cy.tsfrontend/packages/integration-tests/tests/crud/roles-rolebindings.cy.tsfrontend/packages/integration-tests/views/rolebindings.tsfrontend/playwright.config.tsfrontend/public/components/api-explorer.tsxfrontend/public/components/cluster-settings/cluster-settings.tsxfrontend/public/components/error.tsxfrontend/public/components/factory/list-page.tsxfrontend/public/components/factory/table.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/search.tsxfrontend/public/components/storage/attach-pvc-storage.tsxfrontend/public/components/utils/actions-menu.tsxfrontend/public/components/utils/kebab.tsxfrontend/public/components/volumes-table.tsx
💤 Files with no reviewable changes (7)
- frontend/packages/integration-tests/tests/crud/image-pull-secret.cy.ts
- frontend/packages/integration-tests/views/rolebindings.ts
- frontend/packages/integration-tests/tests/crud/add-storage-crud.cy.ts
- frontend/packages/integration-tests/tests/crud/other-routes.cy.ts
- frontend/packages/integration-tests/tests/crud/quotas.cy.ts
- frontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.ts
- frontend/packages/integration-tests/tests/crud/roles-rolebindings.cy.ts
Comment on lines
+4
to
+8
| await page.waitForFunction( | ||
| () => { | ||
| const value = (window as any).monaco?.editor?.getModels()?.[0]?.getValue?.(); | ||
| return typeof value === 'string' && value.trim().length > 0; | ||
| }, |
Contributor
There was a problem hiding this comment.
Allow empty Monaco content in getEditorContent.
This helper currently blocks until content is non-whitespace, which will time out when the editor is intentionally empty. It should wait for model readiness, then return the current string as-is.
Proposed fix
export async function getEditorContent(page: Page): Promise<string> {
await page.waitForFunction(
() => {
- const value = (window as any).monaco?.editor?.getModels()?.[0]?.getValue?.();
- return typeof value === 'string' && value.trim().length > 0;
+ const model = (window as any).monaco?.editor?.getModels?.()?.[0];
+ const value = model?.getValue?.();
+ return typeof value === 'string';
},
{ timeout: 30_000 },
);
return page.evaluate(() => {
return (window as any).monaco.editor.getModels()[0].getValue();
});
}🤖 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 4 - 8, The waitForFunction call
in the getEditorContent helper currently requires both model availability AND
non-empty, non-whitespace content, which causes timeouts when the editor is
intentionally empty. Modify the condition to only verify that the Monaco editor
model exists and the getValue method returns a value, then remove the validation
that checks for non-whitespace and non-zero length content, allowing the
function to return empty strings or whitespace-only content as-is.
Comment on lines
+6
to
+14
| async clickNavLink(path: string[]): Promise<void> { | ||
| const navSection = this.sidebar.getByText(path[0], { exact: false }); | ||
| const isExpanded = await navSection.getAttribute('aria-expanded'); | ||
| if (isExpanded !== 'true') { | ||
| await this.robustClick(navSection); | ||
| } | ||
| if (path.length === 2) { | ||
| await this.robustClick(this.sidebar.getByText(path[1], { exact: false })); | ||
| } |
Contributor
There was a problem hiding this comment.
Harden nav selection to avoid ambiguous matches.
exact: false can click the wrong sidebar entry when labels partially overlap, and path[0] is used without validating input. This can make navigation flaky.
Proposed fix
async clickNavLink(path: string[]): Promise<void> {
- const navSection = this.sidebar.getByText(path[0], { exact: false });
+ if (path.length === 0) {
+ throw new Error('clickNavLink requires at least one path segment');
+ }
+ const navSection = this.sidebar.getByText(path[0], { exact: true });
const isExpanded = await navSection.getAttribute('aria-expanded');
if (isExpanded !== 'true') {
await this.robustClick(navSection);
}
if (path.length === 2) {
- await this.robustClick(this.sidebar.getByText(path[1], { exact: false }));
+ await this.robustClick(this.sidebar.getByText(path[1], { exact: true }));
}
}🤖 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/nav-page.ts` around lines 6 - 14, The clickNavLink method
uses exact: false when selecting sidebar text which can cause ambiguous matches
when labels partially overlap, and it does not validate the path input parameter
before accessing path[0] and path.length, making navigation unreliable. Add
input validation at the start of the clickNavLink method to ensure path is a
non-empty array, and change the exact: false parameter to exact: true in both
getByText calls to ensure precise matching of navigation labels.
Comment on lines
+154
to
+223
| test.describe('Perspective query parameters', { tag: ['@admin'] }, () => { | ||
| test('Developer query parameter switches to Developer perspective', async ({ | ||
| page, | ||
| k8sClient, | ||
| }) => { | ||
| await test.step('Ensure Developer perspective is available', async () => { | ||
| await page.goto('/k8s/cluster/projects'); | ||
|
|
||
| const toggle = page.getByTestId('perspective-switcher-toggle'); | ||
| await expect(toggle).toBeVisible(); | ||
|
|
||
| const isSinglePerspective = | ||
| (await toggle.getAttribute('id')) === 'core-platform-perspective'; | ||
| if (isSinglePerspective) { | ||
| await k8sClient.customObjectsApi.patchClusterCustomObject({ | ||
| group: 'operator.openshift.io', | ||
| version: 'v1', | ||
| plural: 'consoles', | ||
| name: 'cluster', | ||
| body: [ | ||
| { | ||
| op: 'add', | ||
| path: '/spec/customization/perspectives', | ||
| value: [{ id: 'dev', visibility: { state: 'Enabled' } }], | ||
| }, | ||
| ], | ||
| }); | ||
| await page.reload(); | ||
| await page.waitForLoadState('domcontentloaded'); | ||
| } | ||
| }); | ||
|
|
||
| await test.step('Navigate with perspective=dev and verify', async () => { | ||
| await page.goto('/topology/all-namespaces?view=graph&perspective=dev'); | ||
| const toggleText = page | ||
| .getByTestId('perspective-switcher-toggle') | ||
| .locator('.pf-v6-c-menu-toggle__text'); | ||
| await expect(toggleText).toContainText('Developer'); | ||
| }); | ||
| }); | ||
|
|
||
| test('Administrator query parameter switches to Administrator perspective', async ({ | ||
| page, | ||
| }) => { | ||
| await test.step('Switch to Developer perspective first', async () => { | ||
| await page.goto('/k8s/cluster/projects'); | ||
|
|
||
| const toggle = page.getByTestId('perspective-switcher-toggle'); | ||
| await toggle.click(); | ||
|
|
||
| const devOption = page | ||
| .getByTestId('perspective-switcher-menu-option') | ||
| .filter({ hasText: 'Developer' }); | ||
| await devOption.click(); | ||
|
|
||
| const toggleText = page | ||
| .getByTestId('perspective-switcher-toggle') | ||
| .locator('.pf-v6-c-menu-toggle__text'); | ||
| await expect(toggleText).toContainText('Developer'); | ||
| }); | ||
|
|
||
| await test.step('Navigate with perspective=admin and verify', async () => { | ||
| await page.goto('/dashboards?perspective=admin'); | ||
| const toggleText = page | ||
| .getByTestId('perspective-switcher-toggle') | ||
| .locator('.pf-v6-c-menu-toggle__text'); | ||
| await expect(toggleText).toContainText('Core platform'); | ||
| }); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Restore perspective configuration and remove inter-test coupling.
Line 168 mutates the cluster-scoped consoles/cluster config to enable Developer perspective, but this describe block never restores that state. The second test (Line 195 onward) also implicitly depends on that mutation having happened earlier. This makes the suite order-dependent and leaks global state to later tests.
🔧 Suggested structure (snapshot + restore + shared ensure helper)
+ let originalPerspectives: unknown;
+ let perspectivePatched = false;
+
+ async function ensureDeveloperPerspective(page, k8sClient) {
+ await page.goto('/k8s/cluster/projects');
+ const toggle = page.getByTestId('perspective-switcher-toggle');
+ await expect(toggle).toBeVisible();
+
+ const isSinglePerspective =
+ (await toggle.getAttribute('id')) === 'core-platform-perspective';
+ if (!isSinglePerspective) return;
+
+ const consoleCR = await k8sClient.customObjectsApi.getClusterCustomObject({
+ group: 'operator.openshift.io',
+ version: 'v1',
+ plural: 'consoles',
+ name: 'cluster',
+ });
+ originalPerspectives = (consoleCR as any).spec?.customization?.perspectives;
+
+ await k8sClient.customObjectsApi.patchClusterCustomObject({
+ group: 'operator.openshift.io',
+ version: 'v1',
+ plural: 'consoles',
+ name: 'cluster',
+ body: [
+ {
+ op: 'add',
+ path: '/spec/customization/perspectives',
+ value: [{ id: 'dev', visibility: { state: 'Enabled' } }],
+ },
+ ],
+ });
+ perspectivePatched = true;
+ await page.reload();
+ }
+
+ test.afterAll(async ({ k8sClient }) => {
+ if (!perspectivePatched) return;
+ await k8sClient.customObjectsApi.patchClusterCustomObject({
+ group: 'operator.openshift.io',
+ version: 'v1',
+ plural: 'consoles',
+ name: 'cluster',
+ body:
+ originalPerspectives === undefined
+ ? [{ op: 'remove', path: '/spec/customization/perspectives' }]
+ : [{ op: 'replace', path: '/spec/customization/perspectives', value: originalPerspectives }],
+ });
+ });Based on learnings, shared suite setup/teardown is valid for expensive shared resources, but cluster-wide mutations still need explicit teardown to preserve isolation.
🤖 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/crud/other-routes.spec.ts` around lines 154 - 223,
The test suite in the Perspective query parameters describe block mutates the
cluster-scoped consoles/cluster configuration via patchClusterCustomObject in
the first test (Developer query parameter switches to Developer perspective) but
never restores it, causing inter-test coupling where the second test
(Administrator query parameter switches to Administrator perspective) implicitly
depends on this mutation. Add a test.afterEach hook within the describe block to
restore the original consoles/cluster configuration after tests run, and
refactor the second test to not depend on the state change from the first test
by ensuring it can run independently regardless of test execution order.
Source: Learnings
| "yup": "^1.7.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@axe-core/playwright": "^4.10.2", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the dependency is pinned exactly (no range operators).
jq -r '.devDependencies["`@axe-core/playwright`"]' frontend/package.json
# Expected output: 4.10.2Repository: openshift/console
Length of output: 69
Pin @axe-core/playwright to an exact version.
Line 234 uses a caret range (^4.10.2), which allows version drift and weakens reproducibility and supply chain security guarantees. Pin this to an exact version per supply chain requirements.
Suggested fix
- "`@axe-core/playwright`": "^4.10.2",
+ "`@axe-core/playwright`": "4.10.2",📝 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.
Suggested change
| "@axe-core/playwright": "^4.10.2", | |
| "`@axe-core/playwright`": "4.10.2", |
🤖 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/package.json` at line 234, The `@axe-core/playwright` dependency in
the package.json file is using a caret range version specification (^4.10.2)
which allows minor and patch version updates. To meet supply chain security
requirements and ensure reproducibility, remove the caret prefix and pin this
dependency to an exact version by changing the version string from "^4.10.2" to
"4.10.2" on line 234.
Source: Path instructions
rhamilto
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
Merged
2 tasks
rhamilto
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/console/app/yaml-editor.spec.ts`:
- Line 45: The regex pattern /\bvs\b/ at lines 45, 126, and 139 in the
yamlEditorPage assertions is ambiguous because it matches the "vs" substring in
both the light-theme class "vs" and the dark-theme class "vs-dark", causing
false-positives in dark mode. Update all three instances of the regex pattern to
use a negative lookahead assertion that ensures "vs" is not followed by a
hyphen, so it only matches the standalone "vs" class and not the "vs-dark"
variant.
🪄 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: 18fce545-38a2-4495-bd81-bd9c37875547
📒 Files selected for processing (5)
frontend/e2e/pages/base-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/tests/console/app/demo-dynamic-plugin.spec.tsfrontend/e2e/tests/console/app/yaml-editor.spec.tsfrontend/packages/integration-tests/tests/app/yaml-editor.cy.ts
💤 Files with no reviewable changes (1)
- frontend/packages/integration-tests/tests/app/yaml-editor.cy.ts
✅ Files skipped from review due to trivial changes (1)
- frontend/e2e/pages/yaml-editor-page.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/e2e/pages/base-page.ts
- frontend/e2e/tests/console/app/demo-dynamic-plugin.spec.ts
| test('should toggle theme to Light mode', async () => { | ||
| await yamlEditorPage.openSettingsModal(); | ||
| await yamlEditorPage.selectTheme('Light'); | ||
| await expect(yamlEditorPage.getMonacoEditor()).toHaveClass(/\bvs\b/); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Light-theme checks are currently ambiguous and can pass in Dark mode.
On Line 45, Line 126, and Line 139, /\bvs\b/ also matches the vs part of vs-dark, so these assertions can false-pass when the editor is still dark-themed.
Suggested assertion hardening
- await expect(yamlEditorPage.getMonacoEditor()).toHaveClass(/\bvs\b/);
+ await expect(yamlEditorPage.getMonacoEditor()).toHaveClass(/(?:^|\s)vs(?:\s|$)/);
+ await expect(yamlEditorPage.getMonacoEditor()).not.toHaveClass(/(?:^|\s)vs-dark(?:\s|$)/);Apply the same pattern at Lines 126 and 139.
Also applies to: 126-126, 139-139
🤖 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/yaml-editor.spec.ts` at line 45, The regex
pattern /\bvs\b/ at lines 45, 126, and 139 in the yamlEditorPage assertions is
ambiguous because it matches the "vs" substring in both the light-theme class
"vs" and the dark-theme class "vs-dark", causing false-positives in dark mode.
Update all three instances of the regex pattern to use a negative lookahead
assertion that ensures "vs" is not followed by a hyphen, so it only matches the
standalone "vs" class and not the "vs-dark" variant.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
frontend/e2e/clients/kubernetes-client.ts (1)
628-628: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the deployment image instead of using a floating
:latesttag.Line 628 currently depends on a mutable image tag, which makes E2E behavior drift over time and can cause non-deterministic failures.
♻️ Proposed change
- image: 'registry.access.redhat.com/ubi9/ubi-minimal:latest', + image: + process.env.E2E_DEPLOYMENT_IMAGE || + 'registry.access.redhat.com/ubi9/ubi-minimal@sha256:<pinned-digest>',🤖 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/clients/kubernetes-client.ts` at line 628, In the kubernetes-client.ts file, the image specification for 'registry.access.redhat.com/ubi9/ubi-minimal:latest' uses a mutable floating tag which causes non-deterministic E2E test behavior. Replace the `:latest` tag with a specific pinned version number (e.g., a specific digest or version tag like `:9.1` or a full SHA256 digest) to ensure consistent and reproducible deployments across test runs.
🤖 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/navigation.ts`:
- Around line 6-10: The navigateViaNav() method unconditionally clicks the
section button without verifying its current state, which causes flaky tests
when the section is already expanded. Before clicking the sectionButton on line
9, check if the section is already expanded by verifying the visibility or
expanded state of the button, and only click it if it is not already expanded.
This ensures the link will be visible and clickable when you attempt to click it
in the subsequent step.
In `@frontend/e2e/tests/console/crud/annotations.spec.ts`:
- Around line 153-154: The second delete-button click on line 154 uses an
unscoped multi-match locator that can interact with stale DOM elements after the
first deletion re-renders the page. Make the second delete-button targeting
deterministic by using a specific selector method like .first() or .nth() to
explicitly target which delete button should be clicked, ensuring the click
occurs on a valid element reference after the DOM has updated.
- Around line 21-23: The getRow function currently selects pairs-list-row
elements from the entire page instead of scoping to the open modal, which could
cause tests to target incorrect rows if other pairs-list-row elements exist on
the underlying page. Modify the getRow function to scope the selection to the
modal's dialog container by using page.getByRole('dialog') pattern before
selecting the pairs-list-row element, ensuring assertions and edits only target
rows within the open modal dialog.
In `@frontend/e2e/tests/console/crud/customresourcedefinition.spec.ts`:
- Around line 54-56: The test tracks the namespace for cleanup but does not
track the CustomResourceDefinition resource, which is cluster-scoped and will
leak if the test fails before deletion. Add a call to
cleanup.trackClusterCustomResource() after the cleanup.trackNamespace(namespace)
line, passing the name of the CRD that was created in the test, along with the
API group 'apiextensions.k8s.io', version 'v1', and plural
'customresourcedefinitions' to ensure the cluster-scoped CRD resource is
properly cleaned up on test completion or failure.
In `@frontend/e2e/tests/console/crud/namespace-crud.spec.ts`:
- Around line 76-84: The issue is that when textContent() returns null,
podNamespace becomes an empty string, and the toContainText('') assertion will
always pass regardless of actual dropdown content, masking real namespace
preservation failures. Add a validation check after capturing podNamespace to
ensure it is not empty. If podNamespace is empty after the replace and trim
operations, either throw an error with a descriptive message or use a test
assertion to fail explicitly before attempting the toContainText check in the
subsequent test step.
In `@frontend/e2e/utils/i18n.ts`:
- Around line 32-67: The pseudoContext is created at the beginning but only
closed at the very end, which means if any error occurs during the goto call,
the expect assertion, or the element iteration loops, the context will not be
closed and resources will leak. Refactor the code by wrapping all the operations
after pseudoContext creation (from the pseudoPage.addInitScript call through all
the loops that call isPseudoLocalized) in a try-finally block, with
pseudoContext.close() moved into the finally block to ensure cleanup happens
regardless of whether an error is thrown.
In `@frontend/e2e/utils/test-name.ts`:
- Around line 2-5: The test name generation function currently limits the random
suffix to 5 characters via the substring(0, 5) call, which creates insufficient
entropy and risks collisions in parallel or retried test runs. Increase the
substring length parameter from 5 to a larger value (such as 8 or 10) to
generate a longer random suffix with greater entropy and lower collision
probability.
---
Nitpick comments:
In `@frontend/e2e/clients/kubernetes-client.ts`:
- Line 628: In the kubernetes-client.ts file, the image specification for
'registry.access.redhat.com/ubi9/ubi-minimal:latest' uses a mutable floating tag
which causes non-deterministic E2E test behavior. Replace the `:latest` tag with
a specific pinned version number (e.g., a specific digest or version tag like
`:9.1` or a full SHA256 digest) to ensure consistent and reproducible
deployments across test runs.
🪄 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: 732935b5-0df1-480c-9bd1-26dff6e589e4
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (53)
frontend/e2e/clients/kubernetes-client.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/pages/base-page.tsfrontend/e2e/pages/console-plugin-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/nav-page.tsfrontend/e2e/pages/navigation.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/tests/console/app/demo-dynamic-plugin.spec.tsfrontend/e2e/tests/console/app/yaml-editor.spec.tsfrontend/e2e/tests/console/crud/annotations.spec.tsfrontend/e2e/tests/console/crud/bulk-create-resources.spec.tsfrontend/e2e/tests/console/crud/customresourcedefinition.spec.tsfrontend/e2e/tests/console/crud/environment.spec.tsfrontend/e2e/tests/console/crud/labels.spec.tsfrontend/e2e/tests/console/crud/namespace-crud.spec.tsfrontend/e2e/tests/console/crud/resource-crud.spec.tsfrontend/e2e/utils/i18n.tsfrontend/e2e/utils/test-name.tsfrontend/package.jsonfrontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsxfrontend/packages/console-shared/src/components/modals/CreateNamespaceModal.tsxfrontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.tsfrontend/packages/integration-tests/tests/app/yaml-editor.cy.tsfrontend/packages/integration-tests/tests/crud/annotations.cy.tsfrontend/packages/integration-tests/tests/crud/bulk-create-resources.cy.tsfrontend/packages/integration-tests/tests/crud/customresourcedefinition.cy.tsfrontend/packages/integration-tests/tests/crud/environment.cy.tsfrontend/packages/integration-tests/tests/crud/labels.cy.tsfrontend/packages/integration-tests/tests/crud/namespace-crud.cy.tsfrontend/packages/integration-tests/tests/crud/resource-crud.cy.tsfrontend/public/components/modals/add-secret-to-workload.tsxfrontend/public/components/modals/alert-routing-modal.tsxfrontend/public/components/modals/cluster-channel-modal.tsxfrontend/public/components/modals/column-management-modal.tsxfrontend/public/components/modals/configure-ns-pull-secret-modal.tsxfrontend/public/components/modals/configure-update-strategy-modal.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/modals/delete-namespace-modal.tsxfrontend/public/components/modals/delete-pvc-modal.tsxfrontend/public/components/modals/expand-pvc-modal.tsxfrontend/public/components/modals/labels-modal.tsxfrontend/public/components/modals/managed-resource-save-modal.tsxfrontend/public/components/modals/remove-idp-modal.tsxfrontend/public/components/modals/remove-volume-modal.tsxfrontend/public/components/modals/rollback-modal.tsxfrontend/public/components/modals/tags.tsxfrontend/public/components/modals/taints-modal.tsxfrontend/public/components/modals/tolerations-modal.tsxfrontend/public/components/utils/name-value-editor.tsxfrontend/public/components/utils/resource-icon.tsxfrontend/public/components/utils/value-from-pair.tsx
💤 Files with no reviewable changes (9)
- frontend/packages/integration-tests/tests/crud/bulk-create-resources.cy.ts
- frontend/packages/integration-tests/tests/app/yaml-editor.cy.ts
- frontend/packages/integration-tests/tests/crud/environment.cy.ts
- frontend/packages/integration-tests/tests/crud/namespace-crud.cy.ts
- frontend/packages/integration-tests/tests/crud/resource-crud.cy.ts
- frontend/packages/integration-tests/tests/crud/annotations.cy.ts
- frontend/packages/integration-tests/tests/crud/labels.cy.ts
- frontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.ts
- frontend/packages/integration-tests/tests/crud/customresourcedefinition.cy.ts
✅ Files skipped from review due to trivial changes (19)
- frontend/e2e/pages/nav-page.ts
- frontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsx
- frontend/public/components/modals/delete-namespace-modal.tsx
- frontend/public/components/modals/expand-pvc-modal.tsx
- frontend/public/components/modals/managed-resource-save-modal.tsx
- frontend/public/components/utils/value-from-pair.tsx
- frontend/packages/console-shared/src/components/modals/CreateNamespaceModal.tsx
- frontend/public/components/modals/configure-ns-pull-secret-modal.tsx
- frontend/public/components/modals/delete-pvc-modal.tsx
- frontend/public/components/modals/alert-routing-modal.tsx
- frontend/public/components/modals/add-secret-to-workload.tsx
- frontend/public/components/modals/labels-modal.tsx
- frontend/public/components/modals/configure-update-strategy-modal.tsx
- frontend/public/components/utils/name-value-editor.tsx
- frontend/public/components/modals/rollback-modal.tsx
- frontend/public/components/modals/taints-modal.tsx
- frontend/public/components/modals/remove-idp-modal.tsx
- frontend/public/components/modals/tags.tsx
- frontend/public/components/modals/delete-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/e2e/tests/console/app/yaml-editor.spec.ts
- frontend/e2e/fixtures/cleanup-fixture.ts
- frontend/e2e/pages/base-page.ts
- frontend/package.json
- frontend/e2e/pages/console-plugin-page.ts
- frontend/e2e/pages/list-page.ts
Comment on lines
+6
to
+10
| async navigateViaNav(section: string, link: string): Promise<void> { | ||
| await this.page.goto('/'); | ||
| const sectionButton = this.page.getByRole('button', { name: section }); | ||
| await sectionButton.click(); | ||
| await this.page.getByRole('link', { name: link }).click(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard section toggle state before clicking.
navigateViaNav() always toggles the section button (Line 9). If the section is already expanded, this can collapse it and make the link click flaky.
Suggested fix
async navigateViaNav(section: string, link: string): Promise<void> {
await this.page.goto('/');
const sectionButton = this.page.getByRole('button', { name: section });
- await sectionButton.click();
+ const expanded = await sectionButton.getAttribute('aria-expanded');
+ if (expanded !== 'true') {
+ await sectionButton.click();
+ }
await this.page.getByRole('link', { name: link }).click();
await this.page.waitForLoadState('domcontentloaded');
}🤖 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/navigation.ts` around lines 6 - 10, The navigateViaNav()
method unconditionally clicks the section button without verifying its current
state, which causes flaky tests when the section is already expanded. Before
clicking the sectionButton on line 9, check if the section is already expanded
by verifying the visibility or expanded state of the button, and only click it
if it is not already expanded. This ensures the link will be visible and
clickable when you attempt to click it in the subsequent step.
Comment on lines
+21
to
+23
| function getRow(modal: ModalPage, index: number) { | ||
| return modal.getSubmitButton().page().getByTestId('pairs-list-row').nth(index); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/e2e/tests/console/crud/annotations.spec.ts | head -80Repository: openshift/console
Length of output: 3927
🏁 Script executed:
rg "getRow" frontend/e2e/tests/console/crud/annotations.spec.ts -B 2 -A 2Repository: openshift/console
Length of output: 2957
🏁 Script executed:
rg "modal\.getSubmitButton\(\)\.page\(\)" frontend/e2e --type tsRepository: openshift/console
Length of output: 289
🏁 Script executed:
rg "locator\('xpath=ancestor" frontend/e2e --type ts -cRepository: openshift/console
Length of output: 335
🏁 Script executed:
cat frontend/e2e/pages/modal-page.tsRepository: openshift/console
Length of output: 1155
🏁 Script executed:
rg "locator\(.*ancestor" frontend/e2e --type ts -cRepository: openshift/console
Length of output: 335
🏁 Script executed:
rg "getByTestId.*modal\|dialog" frontend/e2e/tests --type ts | head -20Repository: openshift/console
Length of output: 155
🏁 Script executed:
rg "\.locator\(" frontend/e2e/pages/modal-page.ts -A 2 | head -30Repository: openshift/console
Length of output: 155
🏁 Script executed:
rg "role.*dialog\|role.*modal" frontend/e2e --type ts --type html -i | head -20Repository: openshift/console
Length of output: 155
🏁 Script executed:
cat frontend/e2e/pages/base-page.ts | head -50Repository: openshift/console
Length of output: 1781
🏁 Script executed:
# Check if modal content is wrapped in a specific container/selector
rg "modal-content\|modal-body" frontend --type ts --type html | head -10Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Check if there's a standard way to reference the modal's dialog element in Playwright tests
rg "role.*dialog\|locator.*dialog" frontend/e2e/pages --type ts -A 3Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Check if BasePage or ModalPage has any dialog-related helpers
rg "dialog\|Dialog" frontend/e2e/pages/modal-page.ts frontend/e2e/pages/base-page.tsRepository: openshift/console
Length of output: 155
🏁 Script executed:
# Search for other Playwright test patterns that scope within modals
rg "modal.*getByTestId\|\.locator\(" frontend/e2e/tests --type ts -B 2 -A 2 | head -40Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Check Playwright Locator API and verify if the proposed fix syntax is correct
# Let's look at how other tests scope within containers
rg "getByTestId.*\)\.locator\(" frontend/e2e --type ts -B 1 -A 1 | head -20Repository: openshift/console
Length of output: 1960
🏁 Script executed:
# Check if there's a getByRole('dialog') pattern anywhere
rg "getByRole.*dialog" frontend --type ts --type htmlRepository: openshift/console
Length of output: 965
🏁 Script executed:
# Look for how other modals/dialogs scope their content
rg "ModalPage\|DialogPage" frontend/e2e/tests --type ts -l | head -5Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Check one of those modal tests to see the pattern
head -100 frontend/e2e/tests/console/crud/network-policies.spec.ts 2>/dev/null || echo "File not found"Repository: openshift/console
Length of output: 170
🏁 Script executed:
# Look at how getByRole('dialog') is used in those other tests
rg "getByRole.*dialog" frontend/e2e/tests -B 2 -A 5 -m 2Repository: openshift/console
Length of output: 4051
🏁 Script executed:
# Check the actual modal styling/structure in the UI component
rg "role.*dialog\|role=\"dialog\"" frontend/packages --type ts --type tsx | head -10Repository: openshift/console
Length of output: 183
🏁 Script executed:
# Verify if the proposed XPath syntax is correct for Playwright
# Check if any tests use 'xpath=' syntax
rg "locator.*xpath=" frontend/e2e --type tsRepository: openshift/console
Length of output: 785
🏁 Script executed:
# Check if ModalPage has or should have a getDialog method
cat frontend/e2e/pages/modal-page.ts | grep -A 50 "class ModalPage"Repository: openshift/console
Length of output: 1025
🏁 Script executed:
# Verify the actual HTML structure - does the modal have role="dialog"?
rg "role=\"dialog\"" frontend/packages/console-internal --type tsx --type ts | head -5Repository: openshift/console
Length of output: 183
🏁 Script executed:
rg "role=\"dialog\"" frontend/packages --type ts | head -10Repository: openshift/console
Length of output: 1665
🏁 Script executed:
# Check how the annotations modal is structured in the component
rg "modal-title\|modal-cancel-action\|confirm-action" frontend/packages --type ts -B 5 | head -40Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Verify if getByRole('dialog').getByTestId is the idiomatic pattern
rg "getByRole.*dialog.*\.getByTestId" frontend/e2e/tests --type tsRepository: openshift/console
Length of output: 155
Scope row selection to the open modal container.
getRow currently resolves from the whole page, not the modal. If another pairs-list-row exists in the underlying page, assertions and edits can target the wrong row set. Other tests in the codebase use page.getByRole('dialog') to scope modal content; use the same pattern here.
Proposed fix
function getRow(modal: ModalPage, index: number) {
- return modal.getSubmitButton().page().getByTestId('pairs-list-row').nth(index);
+ return modal.getSubmitButton().page().getByRole('dialog').getByTestId('pairs-list-row').nth(index);
}🤖 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/crud/annotations.spec.ts` around lines 21 - 23,
The getRow function currently selects pairs-list-row elements from the entire
page instead of scoping to the open modal, which could cause tests to target
incorrect rows if other pairs-list-row elements exist on the underlying page.
Modify the getRow function to scope the selection to the modal's dialog
container by using page.getByRole('dialog') pattern before selecting the
pairs-list-row element, ensuring assertions and edits only target rows within
the open modal dialog.
Comment on lines
+153
to
+154
| await page.getByTestId('delete-button').first().click(); | ||
| await page.getByTestId('delete-button').click(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the test file
find . -type f -name "annotations.spec.ts" | head -5Repository: openshift/console
Length of output: 209
🏁 Script executed:
# Look at the specific test file and lines around 153-154
cat -n frontend/e2e/tests/console/crud/annotations.spec.ts | sed -n '140,170p'Repository: openshift/console
Length of output: 1628
🏁 Script executed:
# Get more context - check if this is Playwright
head -20 frontend/e2e/tests/console/crud/annotations.spec.tsRepository: openshift/console
Length of output: 898
🏁 Script executed:
# Check if there are multiple delete buttons in the annotations list
# Let's look for the component that renders the delete buttons
rg "delete-button" frontend/e2e/tests/console/crud/annotations.spec.ts -A 2 -B 2Repository: openshift/console
Length of output: 619
🏁 Script executed:
# Let's check the pairs-list component or annotation editor to understand the structure
fd -e ts -e tsx | xargs rg "pairs-list" -l | head -5Repository: openshift/console
Length of output: 561
🏁 Script executed:
# Check Playwright configuration for strict mode
cat -n frontend/e2e/playwright.config.ts | head -50Repository: openshift/console
Length of output: 221
🏁 Script executed:
# Find the correct playwright config location
find frontend/e2e -name "playwright.config.*" -o -name "*.config.ts" | head -10Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Also check for any test configuration at the root
find . -maxdepth 2 -name "playwright.config.*"Repository: openshift/console
Length of output: 187
🏁 Script executed:
# Look at more context around the delete step - understand the test structure
cat -n frontend/e2e/tests/console/crud/annotations.spec.ts | sed -n '110,160p'Repository: openshift/console
Length of output: 2807
🏁 Script executed:
# Check if there are multiple delete buttons being rendered - look at how many annotations are set up
rg "annotations\s*=" frontend/e2e/tests/console/crud/annotations.spec.ts -A 5Repository: openshift/console
Length of output: 323
🏁 Script executed:
# Check Playwright config for strict mode settings
cat -n frontend/playwright.config.ts | grep -A 10 -B 5 "strict"Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Let's check the test setup and understand the modal/pairs-list structure better
cat -n frontend/e2e/tests/console/crud/annotations.spec.ts | sed -n '1,50p'Repository: openshift/console
Length of output: 2375
🏁 Script executed:
# Look at the exact test again - how many delete buttons should exist after line 136 deletion
# The test adds 3 annotations, deletes 1 (leaving 2), then tries to delete both
cat -n frontend/e2e/tests/console/crud/annotations.spec.ts | sed -n '95,158p'Repository: openshift/console
Length of output: 3565
🏁 Script executed:
# Check playwright config - broader search
find frontend -name "playwright.config.ts" -exec cat -n {} \;Repository: openshift/console
Length of output: 5162
🏁 Script executed:
# Look for strict mode in the config file
grep -r "strict" frontend/playwright.config.ts 2>/dev/null || echo "Not found with grep"Repository: openshift/console
Length of output: 175
🏁 Script executed:
# Check if the test actually fails without scoping - search for test history or comments
rg "delete.*button.*strict\|strict.*delete" frontend/e2e/tests/console/crud/Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Let's check if there are similar patterns elsewhere in the test file
rg "\.click\(\)" frontend/e2e/tests/console/crud/annotations.spec.ts -B 1 -A 1Repository: openshift/console
Length of output: 1060
🏁 Script executed:
# Check if there are any recent commits or issues related to this test being flaky
git log --oneline frontend/e2e/tests/console/crud/annotations.spec.ts | head -10Repository: openshift/console
Length of output: 227
🏁 Script executed:
# Check if strict mode might be set at project level or elsewhere
rg "strict\s*:" frontend/playwright.config.ts -A 2 -B 2Repository: openshift/console
Length of output: 155
🏁 Script executed:
# Verify the actual behavior - in Playwright without strict mode,
# multiple matches result in the first being used. Let's check the ModalPage implementation
fd -e ts -e tsx "modal-page" frontend/e2e/Repository: openshift/console
Length of output: 188
🏁 Script executed:
# Look for ModalPage implementation to understand the scope
cat frontend/e2e/pages/modal-page.ts | head -80Repository: openshift/console
Length of output: 1155
Use deterministic delete-button targeting in the "delete all" step.
Line 154 calls click() on an unscoped multi-match locator. After the first deletion, the DOM re-renders to reflect the removal. If the second click() executes during this transition, it can interact with stale element references or an outdated DOM state, causing intermittent failures.
Proposed fix
await test.step('Delete all annotations', async () => {
- await page.getByTestId('delete-button').first().click();
- await page.getByTestId('delete-button').click();
+ const deleteButtons = page.getByTestId('delete-button');
+ while (await deleteButtons.count()) {
+ await deleteButtons.first().click();
+ }
await modal.submit();
await modal.waitForClosed();
await expect(page.getByTestId('edit-annotations')).toContainText('0 annotations');
});🤖 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/crud/annotations.spec.ts` around lines 153 - 154,
The second delete-button click on line 154 uses an unscoped multi-match locator
that can interact with stale DOM elements after the first deletion re-renders
the page. Make the second delete-button targeting deterministic by using a
specific selector method like .first() or .nth() to explicitly target which
delete button should be clicked, ensuring the click occurs on a valid element
reference after the DOM has updated.
Comment on lines
+54
to
+56
| await k8sClient.createNamespace(namespace); | ||
| await k8sClient.waitForNamespaceReady(namespace); | ||
| cleanup.trackNamespace(namespace); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect cleanup fixture capabilities:"
fd cleanup-fixture.ts frontend/e2e -x sed -n '1,260p' {}
echo
echo "Find cluster/custom-resource tracking APIs:"
rg -n "track.*(Cluster|Custom|CRD|Resource)|delete.*(Cluster|Custom|CRD|Resource)" frontend/e2e
echo
echo "Show this spec's cleanup usage:"
sed -n '1,240p' frontend/e2e/tests/console/crud/customresourcedefinition.spec.ts | rg -n "cleanup|track|createNamespace|customresourcedefinition|crd"Repository: openshift/console
Length of output: 10887
🏁 Script executed:
sed -n '150,170p' frontend/e2e/tests/console/crud/customresourcedefinition.spec.tsRepository: openshift/console
Length of output: 849
🏁 Script executed:
sed -n '35,60p' frontend/e2e/tests/console/crud/customresourcedefinition.spec.tsRepository: openshift/console
Length of output: 1075
🏁 Script executed:
rg -A 10 "trackClusterCustomResource" frontend/e2e/tests --context=3 | head -80Repository: openshift/console
Length of output: 155
🏁 Script executed:
rg -n "cleanup\.(track|shouldSkip)" frontend/e2e/tests --context=2 | head -100Repository: openshift/console
Length of output: 6735
🏁 Script executed:
sed -n '110,140p' frontend/e2e/tests/console/crud/customresourcedefinition.spec.tsRepository: openshift/console
Length of output: 1477
🏁 Script executed:
sed -n '37,100p' frontend/e2e/tests/console/crud/customresourcedefinition.spec.ts | cat -nRepository: openshift/console
Length of output: 2397
🏁 Script executed:
sed -n '140,175p' frontend/e2e/tests/console/crud/customresourcedefinition.spec.ts | cat -nRepository: openshift/console
Length of output: 1387
Track the cluster-scoped CRD in cleanup fixture to avoid cluster resource leaks on early failure.
The test creates a CustomResourceDefinition (CRD) but only tracks the namespace. CustomResourceDefinitions are cluster-scoped resources; if execution fails before the delete step, the CRD is left behind. Use cleanup.trackClusterCustomResource() with the CRD name, API group 'apiextensions.k8s.io', version 'v1', and plural 'customresourcedefinitions' to ensure cleanup.
🤖 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/crud/customresourcedefinition.spec.ts` around
lines 54 - 56, The test tracks the namespace for cleanup but does not track the
CustomResourceDefinition resource, which is cluster-scoped and will leak if the
test fails before deletion. Add a call to cleanup.trackClusterCustomResource()
after the cleanup.trackNamespace(namespace) line, passing the name of the CRD
that was created in the test, along with the API group 'apiextensions.k8s.io',
version 'v1', and plural 'customresourcedefinitions' to ensure the
cluster-scoped CRD resource is properly cleaned up on test completion or
failure.
Source: Learnings
Comment on lines
+76
to
+84
| const podNamespace = await listPage | ||
| .getNamespaceDropdown() | ||
| .textContent() | ||
| .then((text) => text?.replace('Project:', '').trim() ?? ''); | ||
|
|
||
| await test.step('Nav link back to list preserves namespace', async () => { | ||
| await nav.clickNavLink('Workloads', 'Pods'); | ||
| await expect(listPage.getNamespaceDropdown()).toContainText(podNamespace); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent vacuous namespace assertions when dropdown text is missing.
If textContent() is null, podNamespace becomes '', and toContainText('') will always pass. This can hide real regressions in namespace preservation checks.
Suggested fix
- const podNamespace = await listPage
- .getNamespaceDropdown()
- .textContent()
- .then((text) => text?.replace('Project:', '').trim() ?? '');
+ const podNamespaceText = await listPage.getNamespaceDropdown().textContent();
+ const podNamespace = podNamespaceText?.replace('Project:', '').trim() ?? '';
+ await expect(
+ podNamespace,
+ 'Expected to resolve a non-empty namespace from the dropdown',
+ ).not.toEqual('');Also applies to: 86-90
🤖 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/crud/namespace-crud.spec.ts` around lines 76 - 84,
The issue is that when textContent() returns null, podNamespace becomes an empty
string, and the toContainText('') assertion will always pass regardless of
actual dropdown content, masking real namespace preservation failures. Add a
validation check after capturing podNamespace to ensure it is not empty. If
podNamespace is empty after the replace and trim operations, either throw an
error with a descriptive message or use a test assertion to fail explicitly
before attempting the toContainText check in the subsequent test step.
Comment on lines
+32
to
+67
| const pseudoContext = await browser.newContext({ | ||
| storageState: await page.context().storageState(), | ||
| ignoreHTTPSErrors: true, | ||
| }); | ||
| const pseudoPage = await pseudoContext.newPage(); | ||
| await pseudoPage.addInitScript(() => { | ||
| Object.defineProperty(navigator, 'language', { value: 'en', configurable: true }); | ||
| }); | ||
| await pseudoPage.goto(pseudoUrl.toString(), { waitUntil: 'load' }); | ||
| await expect(pseudoPage.getByTestId('page-heading')).toBeVisible({ timeout: 30_000 }); | ||
|
|
||
| for (const testId of testIDs) { | ||
| const elements = pseudoPage.getByTestId(testId); | ||
| const count = await elements.count(); | ||
| for (let i = 0; i < count; i++) { | ||
| const text = (await elements.nth(i).textContent()) ?? ''; | ||
| await isPseudoLocalized(text, `[data-test="${testId}"]`); | ||
| } | ||
| } | ||
|
|
||
| for (const selector of selectors) { | ||
| const elements = pseudoPage.locator(selector); | ||
| const count = await elements.count(); | ||
| for (let i = 0; i < count; i++) { | ||
| const el = elements.nth(i); | ||
| const notTranslated = await el.getAttribute('i18n-not-translated'); | ||
| if (notTranslated !== null) { | ||
| continue; | ||
| } | ||
| const text = (await el.textContent()) ?? ''; | ||
| await isPseudoLocalized(text, selector); | ||
| } | ||
| } | ||
|
|
||
| await pseudoContext.close(); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify context lifecycle is guarded by try/finally in i18n helper.
rg -n "newContext|try|finally|pseudoContext\\.close" frontend/e2e/utils/i18n.ts
# Expected: pseudoContext.close appears in a finally block.Repository: openshift/console
Length of output: 243
Always close pseudoContext in a finally block.
If any goto, expect, or element assertion fails before line 66, the temporary context stays open and can accumulate across tests, degrading suite stability.
🐛 Proposed fix
- const pseudoContext = await browser.newContext({
- storageState: await page.context().storageState(),
- ignoreHTTPSErrors: true,
- });
- const pseudoPage = await pseudoContext.newPage();
- await pseudoPage.addInitScript(() => {
- Object.defineProperty(navigator, 'language', { value: 'en', configurable: true });
- });
- await pseudoPage.goto(pseudoUrl.toString(), { waitUntil: 'load' });
- await expect(pseudoPage.getByTestId('page-heading')).toBeVisible({ timeout: 30_000 });
-
- for (const testId of testIDs) {
- const elements = pseudoPage.getByTestId(testId);
- const count = await elements.count();
- for (let i = 0; i < count; i++) {
- const text = (await elements.nth(i).textContent()) ?? '';
- await isPseudoLocalized(text, `[data-test="${testId}"]`);
- }
- }
-
- for (const selector of selectors) {
- const elements = pseudoPage.locator(selector);
- const count = await elements.count();
- for (let i = 0; i < count; i++) {
- const el = elements.nth(i);
- const notTranslated = await el.getAttribute('i18n-not-translated');
- if (notTranslated !== null) {
- continue;
- }
- const text = (await el.textContent()) ?? '';
- await isPseudoLocalized(text, selector);
- }
- }
-
- await pseudoContext.close();
+ const pseudoContext = await browser.newContext({
+ storageState: await page.context().storageState(),
+ ignoreHTTPSErrors: true,
+ });
+ try {
+ const pseudoPage = await pseudoContext.newPage();
+ await pseudoPage.addInitScript(() => {
+ Object.defineProperty(navigator, 'language', { value: 'en', configurable: true });
+ });
+ await pseudoPage.goto(pseudoUrl.toString(), { waitUntil: 'load' });
+ await expect(pseudoPage.getByTestId('page-heading')).toBeVisible({ timeout: 30_000 });
+
+ for (const testId of testIDs) {
+ const elements = pseudoPage.getByTestId(testId);
+ const count = await elements.count();
+ for (let i = 0; i < count; i++) {
+ const text = (await elements.nth(i).textContent()) ?? '';
+ await isPseudoLocalized(text, `[data-test="${testId}"]`);
+ }
+ }
+
+ for (const selector of selectors) {
+ const elements = pseudoPage.locator(selector);
+ const count = await elements.count();
+ for (let i = 0; i < count; i++) {
+ const el = elements.nth(i);
+ const notTranslated = await el.getAttribute('i18n-not-translated');
+ if (notTranslated !== null) {
+ continue;
+ }
+ const text = (await el.textContent()) ?? '';
+ await isPseudoLocalized(text, selector);
+ }
+ }
+ } finally {
+ await pseudoContext.close();
+ }🤖 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/utils/i18n.ts` around lines 32 - 67, The pseudoContext is
created at the beginning but only closed at the very end, which means if any
error occurs during the goto call, the expect assertion, or the element
iteration loops, the context will not be closed and resources will leak.
Refactor the code by wrapping all the operations after pseudoContext creation
(from the pseudoPage.addInitScript call through all the loops that call
isPseudoLocalized) in a try-finally block, with pseudoContext.close() moved into
the finally block to ensure cleanup happens regardless of whether an error is
thrown.
Comment on lines
+2
to
+5
| return `test${Math.random() | ||
| .toString(36) | ||
| .replace(/[^a-z]+/g, '') | ||
| .substring(0, 5)}`; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Increase generated-name entropy to reduce collision risk.
A 5-character random suffix can collide across parallel/retried runs; collisions can reuse old resources and make tests flaky.
💡 Proposed change
export function generateTestName(): string {
- return `test${Math.random()
- .toString(36)
- .replace(/[^a-z]+/g, '')
- .substring(0, 5)}`;
+ const rand = Math.random().toString(36).replace(/[^a-z0-9]+/g, '').substring(0, 8);
+ return `test${Date.now().toString(36)}${rand}`;
}🤖 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/utils/test-name.ts` around lines 2 - 5, The test name generation
function currently limits the random suffix to 5 characters via the substring(0,
5) call, which creates insufficient entropy and risks collisions in parallel or
retried test runs. Increase the substring length parameter from 5 to a larger
value (such as 8 or 10) to generate a longer random suffix with greater entropy
and lower collision probability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/console/app/demo-dynamic-plugin.spec.ts`:
- Around line 105-114: The test submits a delete modal around line 331 but
immediately ends without waiting for the UI to confirm the resource is deleted,
which races with the afterAll hook that also calls deleteClusterCustomResource
for the same ConsolePlugin. After submitting the delete modal (near line 331),
add a wait for the UI to verify that the ConsolePlugin resource has been removed
from the displayed list. Then, in the test.afterAll block, update the condition
so that the deleteClusterCustomResource call for the ConsolePlugin is skipped
since the UI deletion has already been verified, while keeping the
deleteNamespace call for namespace cleanup.
🪄 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: c99be2d1-e577-42f9-a08e-ef0d00c94527
📒 Files selected for processing (10)
frontend/e2e/pages/base-page.tsfrontend/e2e/pages/console-plugin-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/tests/console/app/demo-dynamic-plugin.spec.tsfrontend/e2e/tests/console/app/yaml-editor.spec.tsfrontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsxfrontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.tsfrontend/packages/integration-tests/tests/app/yaml-editor.cy.tsfrontend/public/components/modals/delete-modal.tsx
💤 Files with no reviewable changes (2)
- frontend/packages/integration-tests/tests/app/yaml-editor.cy.ts
- frontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/public/components/modals/delete-modal.tsx
- frontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/e2e/pages/base-page.ts
- frontend/e2e/pages/modal-page.ts
- frontend/e2e/pages/yaml-editor-page.ts
- frontend/e2e/tests/console/app/yaml-editor.spec.ts
Comment on lines
+105
to
+114
| test.afterAll(async ({ k8sClient }) => { | ||
| if (SHOULD_DEPLOY_PLUGIN) { | ||
| await k8sClient.deleteClusterCustomResource( | ||
| 'console.openshift.io', | ||
| 'v1', | ||
| 'consoleplugins', | ||
| PLUGIN_NAME, | ||
| ); | ||
| await k8sClient.deleteNamespace(PLUGIN_NAME); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for the UI delete before API teardown.
Line 331 submits the delete modal and immediately ends the test, while afterAll also deletes the same ConsolePlugin. This can race with the UI delete, or let API cleanup mask a failed UI deletion. Wait for the modal/resource to disappear and skip the teardown CR delete once the UI path has verified deletion.
Suggested fix
let detailsPage: DetailsPage;
let listPage: ListPage;
let modalPage: ModalPage;
+ let consolePluginDeleted = false;
test.beforeAll(async ({ k8sClient }) => {
if (SHOULD_DEPLOY_PLUGIN) {
@@
test.afterAll(async ({ k8sClient }) => {
if (SHOULD_DEPLOY_PLUGIN) {
- await k8sClient.deleteClusterCustomResource(
- 'console.openshift.io',
- 'v1',
- 'consoleplugins',
- PLUGIN_NAME,
- );
+ if (!consolePluginDeleted) {
+ await k8sClient.deleteClusterCustomResource(
+ 'console.openshift.io',
+ 'v1',
+ 'consoleplugins',
+ PLUGIN_NAME,
+ );
+ }
await k8sClient.deleteNamespace(PLUGIN_NAME);
}
});
@@
await detailsPage.clickPageAction('Delete ConsolePlugin');
await modalPage.waitForOpen();
await modalPage.submit();
+ await modalPage.waitForClosed();
+ await consolePluginPage.navigateToConsolePlugins();
+ await expect(consolePluginPage.getPluginNameCell(PLUGIN_NAME)).toBeHidden({
+ timeout: 60_000,
+ });
+ consolePluginDeleted = true;
});
});Also applies to: 326-331
🤖 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/demo-dynamic-plugin.spec.ts` around lines 105
- 114, The test submits a delete modal around line 331 but immediately ends
without waiting for the UI to confirm the resource is deleted, which races with
the afterAll hook that also calls deleteClusterCustomResource for the same
ConsolePlugin. After submitting the delete modal (near line 331), add a wait for
the UI to verify that the ConsolePlugin resource has been removed from the
displayed list. Then, in the test.afterAll block, update the condition so that
the deleteClusterCustomResource call for the ConsolePlugin is skipped since the
UI deletion has already been verified, while keeping the deleteNamespace call
for namespace cleanup.
Migrate frontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.ts to Playwright following the Console e2e layered architecture. - Create ConsolePluginPage, ModalPage, and NavPage page objects - Add data-test attributes alongside data-test-id on ConsolePluginModal and delete-modal for Playwright's getByTestId() - Deploy plugin from manifest's default image when CYPRESS_PLUGIN_PULL_SPEC is not set (shared cluster support) - Skip manifest tab tests when ConsolePlugin model is unavailable - Delete original Cypress test file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhamilto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rhamilto
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
rhamilto
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
frontend/e2e/tests/console/crud/customresourcedefinition.spec.ts (1)
54-57: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTrack the created CRD in cleanup to prevent cluster-scoped leaks on early failure.
Only the namespace is tracked, but the CRD is cluster-scoped. If the test fails before the delete step, the CRD can remain and affect later runs.
Suggested fix
await k8sClient.createNamespace(namespace); await k8sClient.waitForNamespaceReady(namespace); cleanup.trackNamespace(namespace); + cleanup.trackClusterCustomResource( + 'apiextensions.k8s.io', + 'v1', + 'customresourcedefinitions', + crdName, + );🤖 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/crud/customresourcedefinition.spec.ts` around lines 54 - 57, The test creates a CRD which is cluster-scoped, but only tracks the namespace for cleanup. If the test fails before the delete step, the CRD will leak into the cluster and affect subsequent test runs. After the cleanup.trackNamespace call, add a corresponding cleanup.trackCRD call (or equivalent method) to track the created CRD for cleanup, ensuring both the namespace and the cluster-scoped CRD are properly cleaned up if the test fails early.frontend/e2e/tests/console/crud/namespace-crud.spec.ts (1)
76-84: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPrevent vacuous namespace-preservation assertions.
Line 79 can resolve
podNamespaceto''; then Lines 83 and 89 pass trivially withtoContainText(''), masking real regressions.Suggested fix
- const podNamespace = await listPage - .getNamespaceDropdown() - .textContent() - .then((text) => text?.replace('Project:', '').trim() ?? ''); + const podNamespaceText = await listPage.getNamespaceDropdown().textContent(); + const podNamespace = podNamespaceText?.replace('Project:', '').trim() ?? ''; + await expect( + podNamespace, + 'Expected to resolve a non-empty namespace from the namespace dropdown', + ).not.toBe('');Also applies to: 86-90
🤖 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/crud/namespace-crud.spec.ts` around lines 76 - 84, The podNamespace variable extracted via textContent().replace().trim() can resolve to an empty string, which makes subsequent assertions with toContainText(podNamespace) on lines 83 and in the similar test block (lines 86-90) pass trivially since any text contains an empty string. Add an assertion to validate that podNamespace is not empty after extraction, ensuring the test fails if the namespace dropdown doesn't contain the expected non-empty text value. This will prevent masked regressions where the namespace is not actually being preserved.frontend/e2e/tests/console/app/demo-dynamic-plugin.spec.ts (1)
105-114: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid race between UI delete and API teardown for
ConsolePlugin.Line 331 submits the delete modal, but teardown (Lines 107-112) still deletes the same CR unconditionally. This can race and mask whether UI deletion actually succeeded.
Suggested fix
let consolePluginPage: ConsolePluginPage; let detailsPage: DetailsPage; let listPage: ListPage; let modalPage: ModalPage; + let consolePluginDeleted = false; @@ test.afterAll(async ({ k8sClient }) => { if (SHOULD_DEPLOY_PLUGIN) { - await k8sClient.deleteClusterCustomResource( - 'console.openshift.io', - 'v1', - 'consoleplugins', - PLUGIN_NAME, - ); + if (!consolePluginDeleted) { + await k8sClient.deleteClusterCustomResource( + 'console.openshift.io', + 'v1', + 'consoleplugins', + PLUGIN_NAME, + ); + } await k8sClient.deleteNamespace(PLUGIN_NAME); } }); @@ await test.step('Delete the ConsolePlugin', async () => { await consolePluginPage.getPluginNameCell(PLUGIN_NAME).locator('a').click(); await expect(detailsPage.getPageHeading()).toContainText(PLUGIN_NAME); await detailsPage.clickPageAction('Delete ConsolePlugin'); await modalPage.waitForOpen(); await modalPage.submit(); + await modalPage.waitForClosed(); + await consolePluginPage.navigateToConsolePlugins(); + await expect(consolePluginPage.getPluginNameCell(PLUGIN_NAME)).not.toBeAttached({ + timeout: 60_000, + }); + consolePluginDeleted = true; });Also applies to: 326-332
🤖 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/demo-dynamic-plugin.spec.ts` around lines 105 - 114, The afterAll hook unconditionally deletes the ConsolePlugin CR and namespace, but the UI test at line 331 also submits a delete for the same resource, creating a race condition where the API cleanup may attempt to delete a resource already deleted by the UI. To fix this, wrap the k8sClient.deleteClusterCustomResource call for the ConsolePlugin in the afterAll hook with a try-catch block to gracefully handle the case where the resource no longer exists, or add an existence check before attempting deletion. This ensures the teardown doesn't mask whether the UI deletion actually succeeded and prevents competing delete operations.
🤖 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.
Duplicate comments:
In `@frontend/e2e/tests/console/app/demo-dynamic-plugin.spec.ts`:
- Around line 105-114: The afterAll hook unconditionally deletes the
ConsolePlugin CR and namespace, but the UI test at line 331 also submits a
delete for the same resource, creating a race condition where the API cleanup
may attempt to delete a resource already deleted by the UI. To fix this, wrap
the k8sClient.deleteClusterCustomResource call for the ConsolePlugin in the
afterAll hook with a try-catch block to gracefully handle the case where the
resource no longer exists, or add an existence check before attempting deletion.
This ensures the teardown doesn't mask whether the UI deletion actually
succeeded and prevents competing delete operations.
In `@frontend/e2e/tests/console/crud/customresourcedefinition.spec.ts`:
- Around line 54-57: The test creates a CRD which is cluster-scoped, but only
tracks the namespace for cleanup. If the test fails before the delete step, the
CRD will leak into the cluster and affect subsequent test runs. After the
cleanup.trackNamespace call, add a corresponding cleanup.trackCRD call (or
equivalent method) to track the created CRD for cleanup, ensuring both the
namespace and the cluster-scoped CRD are properly cleaned up if the test fails
early.
In `@frontend/e2e/tests/console/crud/namespace-crud.spec.ts`:
- Around line 76-84: The podNamespace variable extracted via
textContent().replace().trim() can resolve to an empty string, which makes
subsequent assertions with toContainText(podNamespace) on lines 83 and in the
similar test block (lines 86-90) pass trivially since any text contains an empty
string. Add an assertion to validate that podNamespace is not empty after
extraction, ensuring the test fails if the namespace dropdown doesn't contain
the expected non-empty text value. This will prevent masked regressions where
the namespace is not actually being preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9d56786d-4ff5-4aea-b453-63f4235b6d69
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (54)
frontend/e2e/clients/kubernetes-client.tsfrontend/e2e/fixtures/cleanup-fixture.tsfrontend/e2e/pages/base-page.tsfrontend/e2e/pages/console-plugin-page.tsfrontend/e2e/pages/list-page.tsfrontend/e2e/pages/modal-page.tsfrontend/e2e/pages/navigation.tsfrontend/e2e/pages/yaml-editor-page.tsfrontend/e2e/tests/console/app/demo-dynamic-plugin.spec.tsfrontend/e2e/tests/console/app/poll-console-updates.spec.tsfrontend/e2e/tests/console/app/yaml-editor.spec.tsfrontend/e2e/tests/console/crud/annotations.spec.tsfrontend/e2e/tests/console/crud/bulk-create-resources.spec.tsfrontend/e2e/tests/console/crud/customresourcedefinition.spec.tsfrontend/e2e/tests/console/crud/environment.spec.tsfrontend/e2e/tests/console/crud/labels.spec.tsfrontend/e2e/tests/console/crud/namespace-crud.spec.tsfrontend/e2e/tests/console/crud/resource-crud.spec.tsfrontend/e2e/utils/i18n.tsfrontend/e2e/utils/test-name.tsfrontend/package.jsonfrontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsxfrontend/packages/console-shared/src/components/modals/CreateNamespaceModal.tsxfrontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.tsfrontend/packages/integration-tests/tests/app/poll-console-updates.cy.tsfrontend/packages/integration-tests/tests/app/yaml-editor.cy.tsfrontend/packages/integration-tests/tests/crud/annotations.cy.tsfrontend/packages/integration-tests/tests/crud/bulk-create-resources.cy.tsfrontend/packages/integration-tests/tests/crud/customresourcedefinition.cy.tsfrontend/packages/integration-tests/tests/crud/environment.cy.tsfrontend/packages/integration-tests/tests/crud/labels.cy.tsfrontend/packages/integration-tests/tests/crud/namespace-crud.cy.tsfrontend/packages/integration-tests/tests/crud/resource-crud.cy.tsfrontend/public/components/modals/add-secret-to-workload.tsxfrontend/public/components/modals/alert-routing-modal.tsxfrontend/public/components/modals/cluster-channel-modal.tsxfrontend/public/components/modals/column-management-modal.tsxfrontend/public/components/modals/configure-ns-pull-secret-modal.tsxfrontend/public/components/modals/configure-update-strategy-modal.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/modals/delete-namespace-modal.tsxfrontend/public/components/modals/delete-pvc-modal.tsxfrontend/public/components/modals/expand-pvc-modal.tsxfrontend/public/components/modals/labels-modal.tsxfrontend/public/components/modals/managed-resource-save-modal.tsxfrontend/public/components/modals/remove-idp-modal.tsxfrontend/public/components/modals/remove-volume-modal.tsxfrontend/public/components/modals/rollback-modal.tsxfrontend/public/components/modals/tags.tsxfrontend/public/components/modals/taints-modal.tsxfrontend/public/components/modals/tolerations-modal.tsxfrontend/public/components/utils/name-value-editor.tsxfrontend/public/components/utils/resource-icon.tsxfrontend/public/components/utils/value-from-pair.tsx
💤 Files with no reviewable changes (10)
- frontend/packages/integration-tests/tests/crud/customresourcedefinition.cy.ts
- frontend/packages/integration-tests/tests/crud/environment.cy.ts
- frontend/packages/integration-tests/tests/app/demo-dynamic-plugin.cy.ts
- frontend/packages/integration-tests/tests/app/poll-console-updates.cy.ts
- frontend/packages/integration-tests/tests/crud/resource-crud.cy.ts
- frontend/packages/integration-tests/tests/crud/namespace-crud.cy.ts
- frontend/packages/integration-tests/tests/app/yaml-editor.cy.ts
- frontend/packages/integration-tests/tests/crud/bulk-create-resources.cy.ts
- frontend/packages/integration-tests/tests/crud/labels.cy.ts
- frontend/packages/integration-tests/tests/crud/annotations.cy.ts
✅ Files skipped from review due to trivial changes (20)
- frontend/public/components/utils/resource-icon.tsx
- frontend/public/components/modals/expand-pvc-modal.tsx
- frontend/public/components/modals/tags.tsx
- frontend/public/components/modals/rollback-modal.tsx
- frontend/public/components/utils/value-from-pair.tsx
- frontend/packages/console-shared/src/components/modals/ConsolePluginModal.tsx
- frontend/packages/console-shared/src/components/modals/CreateNamespaceModal.tsx
- frontend/public/components/modals/alert-routing-modal.tsx
- frontend/public/components/modals/column-management-modal.tsx
- frontend/public/components/modals/configure-ns-pull-secret-modal.tsx
- frontend/public/components/modals/cluster-channel-modal.tsx
- frontend/public/components/modals/delete-namespace-modal.tsx
- frontend/public/components/modals/tolerations-modal.tsx
- frontend/public/components/modals/configure-update-strategy-modal.tsx
- frontend/public/components/modals/remove-idp-modal.tsx
- frontend/public/components/utils/name-value-editor.tsx
- frontend/public/components/modals/delete-pvc-modal.tsx
- frontend/public/components/modals/delete-modal.tsx
- frontend/public/components/modals/add-secret-to-workload.tsx
- frontend/public/components/modals/labels-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (15)
- frontend/public/components/modals/taints-modal.tsx
- frontend/public/components/modals/remove-volume-modal.tsx
- frontend/e2e/utils/test-name.ts
- frontend/e2e/pages/navigation.ts
- frontend/e2e/tests/console/crud/bulk-create-resources.spec.ts
- frontend/e2e/utils/i18n.ts
- frontend/public/components/modals/managed-resource-save-modal.tsx
- frontend/e2e/tests/console/crud/environment.spec.ts
- frontend/e2e/pages/modal-page.ts
- frontend/e2e/tests/console/crud/annotations.spec.ts
- frontend/e2e/pages/yaml-editor-page.ts
- frontend/e2e/pages/list-page.ts
- frontend/e2e/fixtures/cleanup-fixture.ts
- frontend/e2e/pages/console-plugin-page.ts
- frontend/e2e/tests/console/app/yaml-editor.spec.ts
Migrate yaml-editor.cy.ts (11 tests, 22 assertions) to Playwright with full feature parity. Add YamlEditorPage page object for settings modal, theme, font size, and sidebar interactions. Move waitForEditorReady() to BasePage to avoid duplication across page objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Migrates the PollConsoleUpdates test from Cypress to Playwright. All 5 tests were permanently skipped (xdescribe) in Cypress due to console reload issues — they now pass reliably using Playwright's page.route() for API response mocking. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
/retest |
Contributor
|
@rhamilto: The following test failed, say
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. |
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
2 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.
Important
This PR depends on openshift/release#80878
which adds the
console-plugin-demoimage dependency to thee2e-playwrightCI job.Demo plugin tests will skip in CI until that PR merges.
Analysis / Root cause:
Migrate the demo dynamic plugin, YAML editor, and poll-console-updates Cypress e2e tests to Playwright as part of the ongoing Cypress → Playwright migration (CONSOLE-5276).
Solution description:
ConsolePluginPage,ModalPage, andYamlEditorPagepage objectswaitForEditorReady()intoBasePagefor reuse across editor testsdata-testattributes alongsidedata-test-idonConsolePluginModalanddelete-modalfor Playwright'sgetByTestId()PLUGIN_PULL_SPECisnot set (shared cluster support)
CYPRESS_PLUGIN_PULL_SPEC→PLUGIN_PULL_SPEC(theCYPRESS_prefix wasspecific to Cypress.expose; Playwright reads
process.envdirectly)SHOULD_DEPLOY_PLUGINso tests skip cleanly when theplugin image dependency is not available
xdescribe) Cypresstests to 5 passing Playwright tests using
page.route()for API response mockingScreenshots / screen recording:
N/A — e2e test migration, no visual changes
Test setup:
Run with the demo plugin enabled locally or on a shared cluster:
Test cases:
Demo dynamic plugin (9 tests pass, 4 correctly skip):
companion openshift/release PR merges
YAML editor (11 tests, 22 assertions):
Poll console updates (5 tests):
Browser conformance:
Additional info:
The
e2e-playwrightCI job currently lacks theconsole-plugin-demoimage dependency,so demo plugin tests will skip in CI until the companion openshift/release PR merges.
The poll-console-updates tests were permanently skipped in Cypress (
xdescribe) and arenow fully functional in Playwright using
page.route()to mock the/api/check-updatesand plugin manifest endpoints.
🤖 Generated with Claude Code