CONSOLE-5241: Migrate knative-ci.feature Cypress tests to Playwright#16658
Open
mvinkler wants to merge 1 commit into
Open
CONSOLE-5241: Migrate knative-ci.feature Cypress tests to Playwright#16658mvinkler wants to merge 1 commit into
mvinkler wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@mvinkler: This pull request references CONSOLE-5241 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. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mvinkler 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 |
Contributor
WalkthroughThis PR adds Playwright-based Knative end-to-end setup, page objects, and smoke tests, updates shared UI selectors used by those tests, wires a Knative setup project into Playwright, and removes headless Knative Cypress runner entries. ChangesKnative Playwright coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
component/knative
![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: 1
🧹 Nitpick comments (3)
frontend/e2e/pages/knative/add-flow-page.ts (1)
56-69: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider consolidating rate-limit fallback logic.
The page object's conditional rate-limit fallback (lines 56-69) duplicates similar logic in the test file (knative-ci.spec.ts lines 35-43). The test always forces Builder Image strategy, while the page object only applies it when rate-limited.
Consider centralizing this logic:
- If the fallback is a general UI concern (GitHub rate limits affect any test), keep it only in
enterGitUrl()and remove the duplicate from the test.- If the test needs explicit control, document why both are necessary.
This will reduce maintenance burden and ensure consistent behavior across tests.
🤖 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/knative/add-flow-page.ts` around lines 56 - 69, The rate-limit fallback logic that selects Builder Image strategy and Node.js is duplicated between the enterGitUrl() method in the page object (add-flow-page.ts) and similar logic in the test file (knative-ci.spec.ts). Consolidate this logic by keeping it only in the enterGitUrl() method as a general UI concern for handling GitHub rate limits, then remove the duplicate fallback logic from the test file. This ensures consistent behavior across all tests that use enterGitUrl() and reduces maintenance burden.frontend/e2e/pages/knative/topology-knative-page.ts (1)
167-175: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
robustClickfor sidebar action interactions.Line 171 and Line 175 use direct clicks in a dynamic pane; this is more prone to transient visibility/intercept failures than the rest of this page object.
Suggested update
async selectSidebarAction(actionName: string): Promise<void> { const actionsButton = this.sidePane.locator( '[data-test="actions-menu-button"], [data-test-id="actions-menu-button"]', ); - await actionsButton.first().click({ timeout: 10_000 }); + await this.robustClick(actionsButton.first(), { timeout: 10_000 }); const actionItem = this.page.locator( `[data-test="${actionName}"], [data-test-action="${actionName}"]`, ); - await actionItem.first().click({ timeout: 10_000 }); + await this.robustClick(actionItem.first(), { timeout: 10_000 }); + await this.waitForLoadingComplete(); }As per coding guidelines,
frontend/e2e/pages/**/*.tsshould userobustClick()andwaitForLoadingComplete()in page-object actions.🤖 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/knative/topology-knative-page.ts` around lines 167 - 175, In the selectSidebarAction method, replace the two direct click() calls on the actionsButton and actionItem locators with robustClick() calls instead. The first click() call that selects the actions menu button (using the actionsButton locator) and the second click() call that selects the action item (using the actionItem locator) should both be changed to robustClick() to ensure better reliability when interacting with the dynamic sidebar pane, following the page object coding guidelines.Source: Coding guidelines
frontend/e2e/tests/knative/serverless/knative-ci.spec.ts (1)
29-444: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftRefactor repeated scenario blocks into test-table driven cases.
This spec duplicates the same setup/action/assert scaffolding across many cases. Converting the repeated flows to table entries (scenario metadata + executor) would reduce maintenance churn and align with the repo’s spec-test convention.
As per coding guidelines,
**/*.spec.{ts,tsx}tests should follow a test-tables convention similar to Go.🤖 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/knative/serverless/knative-ci.spec.ts` around lines 29 - 444, The knative-ci.spec.ts file contains many test cases with duplicated scaffolding patterns, such as repeated navigation steps, similar action sequences, and comparable assertion structures. Refactor this into a table-driven test pattern by defining a test data interface that captures the test parameters (test name, test identifier, actions to perform, and expected outcomes), then create an array of test case objects containing these parameters. Implement a single parameterized test function that iterates through this test table and executes the common test flow using the data from each row, similar to Go's table-driven testing convention. This approach will eliminate the repetitive test.step blocks and setup/teardown logic that appear across cases like the KN-02-TC02, KN-02-TC17, KA-01-TC01, and KE-05-TC01 tests, making the test suite more maintainable and easier to extend with new cases.Source: Coding guidelines
🤖 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/setup/knative.setup.ts`:
- Around line 101-125: The while loop that waits for the Serverless operator CSV
to reach the Succeeded phase exits silently after the csvTimeout (300_000
milliseconds) without verifying that the CSV actually succeeded. Add a check
after the while loop completes to throw an error if the CSV never reached the
Succeeded phase. Track whether the CSV succeeded (for example, using a boolean
flag inside the try block when csv.status?.phase === 'Succeeded' is found) and
after the loop, verify this flag was set to true; if not, throw an error with a
descriptive message so that the setup fails immediately rather than allowing the
test to continue and fail later.
---
Nitpick comments:
In `@frontend/e2e/pages/knative/add-flow-page.ts`:
- Around line 56-69: The rate-limit fallback logic that selects Builder Image
strategy and Node.js is duplicated between the enterGitUrl() method in the page
object (add-flow-page.ts) and similar logic in the test file
(knative-ci.spec.ts). Consolidate this logic by keeping it only in the
enterGitUrl() method as a general UI concern for handling GitHub rate limits,
then remove the duplicate fallback logic from the test file. This ensures
consistent behavior across all tests that use enterGitUrl() and reduces
maintenance burden.
In `@frontend/e2e/pages/knative/topology-knative-page.ts`:
- Around line 167-175: In the selectSidebarAction method, replace the two direct
click() calls on the actionsButton and actionItem locators with robustClick()
calls instead. The first click() call that selects the actions menu button
(using the actionsButton locator) and the second click() call that selects the
action item (using the actionItem locator) should both be changed to
robustClick() to ensure better reliability when interacting with the dynamic
sidebar pane, following the page object coding guidelines.
In `@frontend/e2e/tests/knative/serverless/knative-ci.spec.ts`:
- Around line 29-444: The knative-ci.spec.ts file contains many test cases with
duplicated scaffolding patterns, such as repeated navigation steps, similar
action sequences, and comparable assertion structures. Refactor this into a
table-driven test pattern by defining a test data interface that captures the
test parameters (test name, test identifier, actions to perform, and expected
outcomes), then create an array of test case objects containing these
parameters. Implement a single parameterized test function that iterates through
this test table and executes the common test flow using the data from each row,
similar to Go's table-driven testing convention. This approach will eliminate
the repetitive test.step blocks and setup/teardown logic that appear across
cases like the KN-02-TC02, KN-02-TC17, KA-01-TC01, and KE-05-TC01 tests, making
the test suite more maintainable and easier to extend with new cases.
🪄 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: c4d14c8b-4d6f-492c-8fc5-fac234f8fdb3
📒 Files selected for processing (28)
frontend/e2e/pages/knative/add-flow-page.tsfrontend/e2e/pages/knative/admin-eventing-page.tsfrontend/e2e/pages/knative/topology-knative-page.tsfrontend/e2e/setup/knative.setup.tsfrontend/e2e/tests/knative/serverless/knative-ci.spec.tsfrontend/integration-tests/test-cypress.shfrontend/package.jsonfrontend/packages/console-shared/src/components/actions/menu/ActionMenuContent.tsxfrontend/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsxfrontend/packages/console-shared/src/components/form-utils/ActionGroupWithIcons.tsxfrontend/packages/console-shared/src/components/form-utils/FormFooter.tsxfrontend/packages/console-shared/src/components/multi-tab-list/MultiTabListPage.tsxfrontend/packages/dev-console/src/components/import/ImportSampleForm.tsxfrontend/packages/dev-console/src/components/import/app/AppSection.tsxfrontend/packages/dev-console/src/components/import/git/GitSection.tsxfrontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.featurefrontend/packages/knative-plugin/integration-tests/package.jsonfrontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/packages/topology/src/components/page/TopologyPageToolbar.tsxfrontend/playwright.config.tsfrontend/public/components/factory/list-page.tsxfrontend/public/components/filter-toolbar.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/modals/labels-modal.tsxfrontend/public/components/modals/tags.tsxfrontend/public/components/utils/actions-menu.tsxfrontend/public/components/utils/kebab.tsx
💤 Files with no reviewable changes (4)
- frontend/packages/knative-plugin/integration-tests/package.json
- frontend/integration-tests/test-cypress.sh
- frontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.feature
- frontend/package.json
mvinkler
force-pushed
the
CONSOLE-5241
branch
2 times, most recently
from
f39cfac to
dbcc42a
Compare
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/setup/knative.setup.ts`:
- Around line 53-55: The empty catch blocks at lines 53-55, 122-124, and 231-237
in the knative.setup.ts file are suppressing all Kubernetes API errors
indiscriminately, masking authentication, RBAC, and API failures as simple
readiness timeouts. Replace these broad exception handlers by checking the error
type or status code and only suppressing expected "not found" or "not ready yet"
errors (such as 404 Not Found or specific error messages). For unexpected
errors, either re-throw them to fail fast or capture the error details and
include them in the timeout message so that actual API/auth/RBAC failures are
visible instead of being silently swallowed.
🪄 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: 47837dcf-1670-4a78-8d4d-bb9497c7dc0a
📒 Files selected for processing (28)
frontend/e2e/pages/knative/add-flow-page.tsfrontend/e2e/pages/knative/admin-eventing-page.tsfrontend/e2e/pages/knative/topology-knative-page.tsfrontend/e2e/setup/knative.setup.tsfrontend/e2e/tests/knative/serverless/knative-ci.spec.tsfrontend/integration-tests/test-cypress.shfrontend/package.jsonfrontend/packages/console-shared/src/components/actions/menu/ActionMenuContent.tsxfrontend/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsxfrontend/packages/console-shared/src/components/form-utils/ActionGroupWithIcons.tsxfrontend/packages/console-shared/src/components/form-utils/FormFooter.tsxfrontend/packages/console-shared/src/components/multi-tab-list/MultiTabListPage.tsxfrontend/packages/dev-console/src/components/import/ImportSampleForm.tsxfrontend/packages/dev-console/src/components/import/app/AppSection.tsxfrontend/packages/dev-console/src/components/import/git/GitSection.tsxfrontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.featurefrontend/packages/knative-plugin/integration-tests/package.jsonfrontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/packages/topology/src/components/page/TopologyPageToolbar.tsxfrontend/playwright.config.tsfrontend/public/components/factory/list-page.tsxfrontend/public/components/filter-toolbar.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/modals/labels-modal.tsxfrontend/public/components/modals/tags.tsxfrontend/public/components/utils/actions-menu.tsxfrontend/public/components/utils/kebab.tsx
💤 Files with no reviewable changes (4)
- frontend/packages/knative-plugin/integration-tests/package.json
- frontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.feature
- frontend/integration-tests/test-cypress.sh
- frontend/package.json
✅ Files skipped from review due to trivial changes (15)
- frontend/packages/console-shared/src/components/form-utils/FormFooter.tsx
- frontend/public/components/utils/actions-menu.tsx
- frontend/packages/console-shared/src/components/form-utils/ActionGroupWithIcons.tsx
- frontend/public/components/utils/kebab.tsx
- frontend/public/components/filter-toolbar.tsx
- frontend/packages/dev-console/src/components/import/app/AppSection.tsx
- frontend/packages/console-shared/src/components/actions/menu/ActionMenuContent.tsx
- frontend/packages/topology/src/components/page/TopologyPageToolbar.tsx
- frontend/public/components/modals/delete-modal.tsx
- frontend/packages/dev-console/src/components/import/git/GitSection.tsx
- frontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsx
- frontend/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsx
- frontend/public/components/modals/tags.tsx
- frontend/packages/topology/src/components/modals/EditApplicationModal.tsx
- frontend/public/components/modals/labels-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/public/components/factory/list-page.tsx
- frontend/packages/dev-console/src/components/import/ImportSampleForm.tsx
- frontend/playwright.config.ts
- frontend/packages/console-shared/src/components/multi-tab-list/MultiTabListPage.tsx
- frontend/e2e/pages/knative/add-flow-page.ts
- frontend/e2e/pages/knative/topology-knative-page.ts
- frontend/e2e/pages/knative/admin-eventing-page.ts
- frontend/e2e/tests/knative/serverless/knative-ci.spec.ts
Comment on lines
+53
to
+55
| } catch { | ||
| // Namespace may not exist yet — proceed with installation | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t suppress all Kubernetes API errors during probe/poll loops.
Line 53, Line 122, and Line 231 currently swallow every exception, so auth/RBAC/API failures can be misreported as readiness timeouts after several minutes. Only ignore expected “not found / not ready yet” cases and fail fast (or at least include the last non-retryable error in the timeout message).
Suggested fix
+const isExpectedTransientError = (err: unknown): boolean => {
+ const msg = err instanceof Error ? err.message : String(err);
+ return /not[\s-]?found|404/i.test(msg);
+};
@@
- } catch {
+ } catch (err) {
+ if (!isExpectedTransientError(err)) {
+ throw err;
+ }
// Namespace may not exist yet — proceed with installation
}
@@
let csvSucceeded = false;
+ let lastCsvError: string | undefined;
while (Date.now() - startTime < csvTimeout) {
@@
- } catch {
+ } catch (err) {
+ const msg = err instanceof Error ? err.message : String(err);
+ lastCsvError = msg;
+ if (!isExpectedTransientError(err)) {
+ throw err;
+ }
// CSV not ready yet
}
@@
if (!csvSucceeded) {
- throw new Error('Serverless operator CSV did not reach Succeeded phase');
+ throw new Error(
+ `Serverless operator CSV did not reach Succeeded phase within ${csvTimeout}ms${
+ lastCsvError ? `. Last API error: ${lastCsvError}` : ''
+ }`,
+ );
}
@@
const waitForReady = async (
@@
) => {
const start = Date.now();
+ let lastReadyError: string | undefined;
while (Date.now() - start < timeoutMs) {
@@
- } catch {
+ } catch (err) {
+ const msg = err instanceof Error ? err.message : String(err);
+ lastReadyError = msg;
+ if (!isExpectedTransientError(err)) {
+ throw err;
+ }
// Not ready yet
}
@@
- throw new Error(`${name} in ${namespace} not ready after ${timeoutMs}ms`);
+ throw new Error(
+ `${name} in ${namespace} not ready after ${timeoutMs}ms${
+ lastReadyError ? `. Last API error: ${lastReadyError}` : ''
+ }`,
+ );
};Also applies to: 122-124, 231-237
🤖 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/setup/knative.setup.ts` around lines 53 - 55, The empty catch
blocks at lines 53-55, 122-124, and 231-237 in the knative.setup.ts file are
suppressing all Kubernetes API errors indiscriminately, masking authentication,
RBAC, and API failures as simple readiness timeouts. Replace these broad
exception handlers by checking the error type or status code and only
suppressing expected "not found" or "not ready yet" errors (such as 404 Not
Found or specific error messages). For unexpected errors, either re-throw them
to fail fast or capture the error details and include them in the timeout
message so that actual API/auth/RBAC failures are visible instead of being
silently swallowed.
Author
|
/retest |
1 similar comment
Author
|
/retest |
Migrate the knative CI smoke test (knative-ci.feature) from Cypress Gherkin to Playwright and remove the original Cypress file. 16 automatable scenarios migrated using serial test mode. 8 manual/ broken-test scenarios dropped (not migrated). New files: - e2e/tests/knative/serverless/knative-ci.spec.ts (16 serial tests) - e2e/pages/knative/topology-knative-page.ts (topology interactions) - e2e/pages/knative/add-flow-page.ts (git import flow) - e2e/pages/knative/admin-eventing-page.ts (admin eventing page) - e2e/setup/knative.setup.ts (Serverless operator install + CR setup) Removed: - features/e2e/knative-ci.feature (replaced by Playwright spec) - test-cypress-knative-headless script and CI references React components modified to add data-test attributes alongside existing legacy test attributes (data-test-id, data-test-action, data-test-dropdown-menu) for Playwright getByTestId() usage. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Michal Vinkler <mvinkler@redhat.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/e2e/setup/knative.setup.ts (1)
53-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon’t swallow all Kubernetes API errors in readiness loops.
Line 53, Line 122, and Line 231 catch and ignore every exception, so auth/RBAC/API failures can be misreported as “not ready yet” until timeout. Only retry expected transient errors (for example, not-found) and rethrow others immediately.
Suggested fix
+const isTransientK8sError = (err: unknown): boolean => { + const msg = err instanceof Error ? err.message : String(err); + return /not[\s-]?found|404/i.test(msg); +}; @@ - } catch { + } catch (err) { + if (!isTransientK8sError(err)) { + throw err; + } // Namespace may not exist yet — proceed with installation } @@ - } catch { + } catch (err) { + if (!isTransientK8sError(err)) { + throw err; + } // CSV not ready yet } @@ - } catch { + } catch (err) { + if (!isTransientK8sError(err)) { + throw err; + } // Not ready yet }Also applies to: 122-124, 231-233
🤖 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/setup/knative.setup.ts` around lines 53 - 55, The readiness checks are catching every exception and treating all failures as transient, which can hide auth/RBAC/API problems until timeout. Update the catch blocks in the namespace setup and the two readiness polling paths in knative.setup.ts to only ignore expected transient Kubernetes errors such as not-found, and immediately rethrow any other error types so real failures surface right away.
🤖 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/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsx`:
- Around line 68-70: The ActionMenuItem props object contains a duplicate
data-test key, so one assignment is overwritten and the extra entry is dead
code. Update the object in ActionMenuItem to keep only a single data-test
property alongside data-test-action, and remove the redundant duplicate so the
intent is clear and the props remain unambiguous.
In `@frontend/public/components/factory/list-page.tsx`:
- Around line 297-299: The dropdown menu props in list-page.tsx define data-test
twice, and the later value overwrites the Playwright selector set in the menu
item builder. Update the object in the dropdown menu rendering logic to keep the
intended data-test value from the first assignment and remove the duplicate
data-test entry so the selector remains in the dropdown-menu-${item} format. Use
the dropdown menu/item code in the list-page component to locate the duplicate
key.
---
Duplicate comments:
In `@frontend/e2e/setup/knative.setup.ts`:
- Around line 53-55: The readiness checks are catching every exception and
treating all failures as transient, which can hide auth/RBAC/API problems until
timeout. Update the catch blocks in the namespace setup and the two readiness
polling paths in knative.setup.ts to only ignore expected transient Kubernetes
errors such as not-found, and immediately rethrow any other error types so real
failures surface right away.
🪄 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: ec57f155-e96d-47d8-ad5d-379cd20c75c2
📒 Files selected for processing (27)
frontend/e2e/pages/knative/add-flow-page.tsfrontend/e2e/pages/knative/admin-eventing-page.tsfrontend/e2e/pages/knative/topology-knative-page.tsfrontend/e2e/setup/knative.setup.tsfrontend/e2e/tests/knative/serverless/knative-ci.spec.tsfrontend/integration-tests/test-cypress.shfrontend/package.jsonfrontend/packages/console-shared/src/components/actions/menu/ActionMenuContent.tsxfrontend/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsxfrontend/packages/console-shared/src/components/form-utils/ActionGroupWithIcons.tsxfrontend/packages/console-shared/src/components/form-utils/FormFooter.tsxfrontend/packages/console-shared/src/components/multi-tab-list/MultiTabListPage.tsxfrontend/packages/dev-console/src/components/import/ImportSampleForm.tsxfrontend/packages/dev-console/src/components/import/app/AppSection.tsxfrontend/packages/dev-console/src/components/import/git/GitSection.tsxfrontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.featurefrontend/packages/knative-plugin/integration-tests/package.jsonfrontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsxfrontend/packages/topology/src/components/modals/EditApplicationModal.tsxfrontend/packages/topology/src/components/page/TopologyPageToolbar.tsxfrontend/playwright.config.tsfrontend/public/components/factory/list-page.tsxfrontend/public/components/filter-toolbar.tsxfrontend/public/components/modals/delete-modal.tsxfrontend/public/components/modals/labels-modal.tsxfrontend/public/components/modals/tags.tsxfrontend/public/components/utils/kebab.tsx
💤 Files with no reviewable changes (3)
- frontend/packages/knative-plugin/integration-tests/package.json
- frontend/packages/knative-plugin/integration-tests/features/e2e/knative-ci.feature
- frontend/integration-tests/test-cypress.sh
✅ Files skipped from review due to trivial changes (13)
- frontend/packages/console-shared/src/components/form-utils/FormFooter.tsx
- frontend/packages/dev-console/src/components/import/ImportSampleForm.tsx
- frontend/packages/console-shared/src/components/form-utils/ActionGroupWithIcons.tsx
- frontend/packages/dev-console/src/components/import/app/AppSection.tsx
- frontend/packages/console-shared/src/components/multi-tab-list/MultiTabListPage.tsx
- frontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsx
- frontend/packages/topology/src/components/modals/EditApplicationModal.tsx
- frontend/public/components/modals/delete-modal.tsx
- frontend/public/components/modals/tags.tsx
- frontend/public/components/modals/labels-modal.tsx
- frontend/public/components/filter-toolbar.tsx
- frontend/public/components/utils/kebab.tsx
- frontend/packages/topology/src/components/page/TopologyPageToolbar.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- frontend/packages/console-shared/src/components/actions/menu/ActionMenuContent.tsx
- frontend/e2e/pages/knative/add-flow-page.ts
- frontend/playwright.config.ts
- frontend/packages/dev-console/src/components/import/git/GitSection.tsx
- frontend/e2e/pages/knative/admin-eventing-page.ts
- frontend/package.json
- frontend/e2e/pages/knative/topology-knative-page.ts
- frontend/e2e/tests/knative/serverless/knative-ci.spec.ts
Comment on lines
68
to
+70
| 'data-test': label, | ||
| 'data-test-action': label, | ||
| 'data-test': label, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsx | sed -n '60,80p'Repository: openshift/console
Length of output: 766
🏁 Script executed:
cd frontend && rg "no-dupe-keys|duplicate.*key" . --type json --type ts --type js | head -20Repository: openshift/console
Length of output: 636
🏁 Script executed:
cat -n frontend/packages/eslint-plugin-console/lib/config/rules/typescript.js | grep -A 5 -B 5 "no-dupe-keys"Repository: openshift/console
Length of output: 588
🏁 Script executed:
cd frontend && yarn tsc --noEmit packages/console-shared/src/components/actions/menu/ActionMenuItem.tsx 2>&1 | head -30Repository: openshift/console
Length of output: 374
🏁 Script executed:
node --version && tsc --version 2>/dev/null || echo "TypeScript not available"Repository: openshift/console
Length of output: 178
🏁 Script executed:
cd frontend/packages/console-shared && npx tsc --noEmit src/components/actions/menu/ActionMenuItem.tsx 2>&1 | grep -i "dupe\|duplicate\|2378\|1117" || echo "No duplicate key errors detected by TypeScript"Repository: openshift/console
Length of output: 202
🏁 Script executed:
cd frontend && rg "ActionMenuItem" packages/console-shared --type ts -A 3 -B 3 | head -40Repository: openshift/console
Length of output: 3849
Remove the duplicate data-test object key.
Line 68 is overwritten by Line 70, resulting in dead code that should be removed for clarity.
Suggested fix
const props = {
icon,
autoFocus,
isDisabled,
className: classes,
onClick: handleClick,
- 'data-test': label,
'data-test-action': label,
'data-test': label,
translate: 'no' as 'no',
};🧰 Tools
🪛 Biome (2.5.0)
[error] 68-68: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
🤖 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/packages/console-shared/src/components/actions/menu/ActionMenuItem.tsx`
around lines 68 - 70, The ActionMenuItem props object contains a duplicate
data-test key, so one assignment is overwritten and the extra entry is dead
code. Update the object in ActionMenuItem to keep only a single data-test
property alongside data-test-action, and remove the redundant duplicate so the
intent is clear and the props remain unambiguous.
Source: Linters/SAST tools
Comment on lines
297
to
+299
| 'data-test': `dropdown-menu-${item}`, | ||
| 'data-test-dropdown-menu': item, | ||
| 'data-test': item, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
cat -n frontend/public/components/factory/list-page.tsx | sed -n '290,310p'Repository: openshift/console
Length of output: 1027
🏁 Script executed:
rg 'dropdown-menu-' frontend --type ts --type tsx -A 2 -B 2Repository: openshift/console
Length of output: 183
🏁 Script executed:
rg 'dropdown-menu-' frontend -A 2 -B 2Repository: openshift/console
Length of output: 12208
🏁 Script executed:
git log --oneline frontend/public/components/factory/list-page.tsx | head -5Repository: openshift/console
Length of output: 235
🏁 Script executed:
git show af19c7ad23 frontend/public/components/factory/list-page.tsx | head -100Repository: openshift/console
Length of output: 4421
🏁 Script executed:
cat -n frontend/e2e/pages/list-page.ts | sed -n '1,30p'Repository: openshift/console
Length of output: 1671
🏁 Script executed:
git show af19c7ad23 frontend/public/components/factory/list-page.tsx | grep -A 20 -B 5 "dropdown-menu"Repository: openshift/console
Length of output: 2386
🏁 Script executed:
cd frontend && yarn eslint public/components/factory/list-page.tsx 2>&1 | head -50Repository: openshift/console
Length of output: 374
🏁 Script executed:
cat -n frontend/e2e/pages/list-page.ts | sed -n '20,45p'Repository: openshift/console
Length of output: 1077
🏁 Script executed:
grep -r "dropdown-menu-" frontend/e2e --include="*.ts"Repository: openshift/console
Length of output: 257
Remove duplicate data-test key that overwrites Playwright test selector.
Line 299 overwrites the data-test value from line 297, breaking the Playwright e2e test that expects dropdown-menu-${item} format (e.g., dropdown-menu-yaml). Remove the duplicate key at line 299 to preserve the intended selector.
Suggested fix
initialItems={Object.keys(createProps.items).map((item) => ({
value: item,
content: createProps.items[item],
'data-test': `dropdown-menu-${item}`,
'data-test-dropdown-menu': item,
- 'data-test': item,
}))}📝 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
| 'data-test': `dropdown-menu-${item}`, | |
| 'data-test-dropdown-menu': item, | |
| 'data-test': item, | |
| 'data-test': `dropdown-menu-${item}`, | |
| 'data-test-dropdown-menu': item, |
🧰 Tools
🪛 Biome (2.5.0)
[error] 297-297: This property is later overwritten by an object member with the same name.
(lint/suspicious/noDuplicateObjectKeys)
🤖 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/factory/list-page.tsx` around lines 297 - 299, The
dropdown menu props in list-page.tsx define data-test twice, and the later value
overwrites the Playwright selector set in the menu item builder. Update the
object in the dropdown menu rendering logic to keep the intended data-test value
from the first assignment and remove the duplicate data-test entry so the
selector remains in the dropdown-menu-${item} format. Use the dropdown menu/item
code in the list-page component to locate the duplicate key.
Source: Linters/SAST tools
Contributor
|
@mvinkler: The following tests 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.
Summary
Migrates the knative CI smoke test (
knative-ci.feature) from Cypress Gherkin to Playwright and removes the original Cypress file. This is the per-PR CI test that runs viae2e-gcp-console— the remaining 30 knative feature files (nightly only) will follow in separate PRs.Changes
Test Migration (16 tests)
knative-ci.featuretoknative-ci.spec.ts@manual/@broken-testscenarios dropped (not migrated)test.describe.configure({ mode: 'serial' })) — matching the CypresstestIsolation: falsesequential workflowTopologyKnativePage,AddFlowPage,AdminEventingPageknative.setup.ts— installs Serverless operator, creates KnativeServing/KnativeEventing CRsCypress Removal
features/e2e/knative-ci.featuretest-cypress-knative-headlessscript frompackage.jsonandtest-cypress.shtest-cypress-headless-all,test-cypress-knative-nightly) and local dev runner kept for remaining 30 feature filesSelector Migration (backward compatible)
data-testalongside existing legacy attributes (data-test-action,data-test-id,data-test-dropdown-menu) in shared React componentsActionMenuItem,ActionMenuContent,MultiTabListPage,list-page,actions-menu,kebab,AppSection,ImportSampleForm,FormFooter,ActionGroupWithIcons,ApplicationSelector,EditApplicationModal,TopologyPageToolbar,GitSection,filter-toolbar,labels-modal,tags,delete-modalgetByTestId()with fallback to legacy selectorsTest Results
16/16 tests passing (serial mode, 4.6 min)
Screenshots / screen recording
Test setup
Requires OpenShift cluster with Serverless operator (installed automatically by
knative.setup.ts).Browser conformance
Related
Summary by CodeRabbit
Tests
Chores