CONSOLE-5241: Migrate knative-ci.feature Cypress tests to Playwright#16658
Open
mvinkler wants to merge 7 commits into
Open
CONSOLE-5241: Migrate knative-ci.feature Cypress tests to Playwright#16658mvinkler wants to merge 7 commits into
mvinkler wants to merge 7 commits 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. |
openshift-ci
Bot
added
component/core
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:
WalkthroughKnative Playwright setup, page objects, smoke tests, Kubernetes manifests, and selector support were added. Existing Knative Cypress integration tests, configuration, fixtures, and execution paths were removed or disabled. ChangesKnative end-to-end coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Playwright
participant OpenShift
participant KubernetesAPI
participant KnativeUI
Playwright->>OpenShift: Apply Serverless and Knative manifests
Playwright->>KubernetesAPI: Poll operator and CR readiness
Playwright->>KnativeUI: Create Serving and Eventing resources
KnativeUI->>KubernetesAPI: Persist resources
Playwright->>KnativeUI: Verify topology and delete resources
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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
June 23, 2026 12:09
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
Author
|
/retest |
1 similar comment
Author
|
/retest |
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
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/e2e/setup/knative.setup.ts (1)
57-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: extract the repeated YAML-apply logic.
The subscription (Lines 61-99), serving (151-172), and eventing (176-202) blocks repeat the same parse → iterate → create → "already exists" swallow pattern, and
KubernetesClientis instantiated identically in both setup steps (Lines 29-36 and 135-142). A sharedapplyDocs(client, yamlPath, handlers)helper plus a client factory would cut the duplication.🤖 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 57 - 202, The YAML apply flow is duplicated across the subscription, KnativeServing, and KnativeEventing setup blocks, and the same KubernetesClient construction is repeated in both setup steps. Extract the common parse → iterate → create → ignore “already exists” behavior into a shared helper such as applyDocs, and centralize the KubernetesClient creation in a small factory/helper so the setup steps only pass the YAML path and resource-specific create logic.
🤖 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.
Nitpick comments:
In `@frontend/e2e/setup/knative.setup.ts`:
- Around line 57-202: The YAML apply flow is duplicated across the subscription,
KnativeServing, and KnativeEventing setup blocks, and the same KubernetesClient
construction is repeated in both setup steps. Extract the common parse → iterate
→ create → ignore “already exists” behavior into a shared helper such as
applyDocs, and centralize the KubernetesClient creation in a small
factory/helper so the setup steps only pass the YAML path and resource-specific
create logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b3f7874b-6040-4c3c-8809-96b181ea295c
📒 Files selected for processing (24)
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/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/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 (4)
- frontend/packages/knative-plugin/integration-tests/package.json
- frontend/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 (10)
- frontend/packages/dev-console/src/components/import/ImportSampleForm.tsx
- frontend/packages/console-shared/src/components/form-utils/FormFooter.tsx
- frontend/public/components/filter-toolbar.tsx
- frontend/packages/topology/src/components/page/TopologyPageToolbar.tsx
- frontend/packages/console-shared/src/components/form-utils/ActionGroupWithIcons.tsx
- frontend/public/components/modals/delete-modal.tsx
- frontend/packages/console-shared/src/components/multi-tab-list/MultiTabListPage.tsx
- frontend/public/components/modals/tags.tsx
- frontend/packages/dev-console/src/components/import/git/GitSection.tsx
- frontend/packages/topology/src/components/dropdowns/ApplicationSelector.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/packages/topology/src/components/modals/EditApplicationModal.tsx
- frontend/public/components/modals/labels-modal.tsx
- frontend/packages/dev-console/src/components/import/app/AppSection.tsx
- frontend/public/components/utils/kebab.tsx
- frontend/playwright.config.ts
- frontend/e2e/tests/knative/serverless/knative-ci.spec.ts
- frontend/e2e/pages/knative/admin-eventing-page.ts
- frontend/e2e/pages/knative/add-flow-page.ts
- frontend/e2e/pages/knative/topology-knative-page.ts
Author
|
/retest |
1 similar comment
Author
|
/retest |
Author
|
/retest ci/prow/e2e-playwright |
Author
|
/retest |
2 similar comments
Author
|
/retest |
Author
|
/retest |
Author
|
/retest ci/prow/e2e-playwright |
Author
|
/retest |
1 similar comment
Author
|
/retest |
Address review feedback: extract repeated inline locators from knative-ci.spec.ts into TopologyKnativePage and AdminEventingPage page object methods. TopologyKnativePage: - confirmModalSubmit() — submit modal and wait for close - verifyConfirmActionVisible() — assert confirm button present - editApplicationGrouping(appName) — full app grouping modal flow - addTrafficTarget(), setTrafficPercent(), selectTrafficTargetRevision(), submitTrafficDistribution() — traffic distribution form interactions AdminEventingPage: - selectPingSourceAndCreate() — PingSource card selection - submitForm() — generic form submission Signed-off-by: Sahil Shah <sshah@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Sahil Shah <sahshah@redhat.com>
Contributor
Contributor
|
Helping get this over the line while Michal is out. Pushed a cleanup commit addressing rhamilto's remaining review feedback (move inline selectors to page objects): mvinkler#1 CI failures are infra timeouts (builds hung, tests never ran) — |
Contributor
|
/retest |
CONSOLE-5241: Move inline selectors from spec to page objects
Contributor
|
/retest backend |
Contributor
|
/test backend |
Contributor
|
/retest |
Author
|
/test backend |
Author
|
/test e2e-playwright |
Author
|
/test backend |
Author
|
/test e2e-playwright |
4 similar comments
Author
|
/test e2e-playwright |
Author
|
/test e2e-playwright |
Author
|
/test e2e-playwright |
Author
|
/test e2e-playwright |
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Michal Vinkler <mvinkler@redhat.com>
Author
|
/test e2e-playwright |
Author
|
/retest |
Author
|
/test e2e-playwright |
3 similar comments
Author
|
/test e2e-playwright |
Author
|
/test e2e-playwright |
Member
|
/test e2e-playwright |
Contributor
|
@mvinkler: 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. |
Author
|
@rhamilto the playwright tests seem to be consistently failing on "Process did not finish before 2h0m0s timeout". This probably affects other PRs as well, is it possible to increase the timeout? |
Member
Contributor
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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
5 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 all knative Cypress integration tests (30 feature files, step definitions, page objects — ~8,900 lines). The remaining knative tests had no active Prow nightly job and will not be migrated.Also fixes OCPBUGS-88296 — perspective-switcher 46% flake rate in knative-ci.feature, resolved by using direct URL navigation.
Changes
Commit 1: Migrate knative-ci.feature to Playwright (16 tests)
Commit 2: Remove all remaining knative Cypress tests
Selector Migration (backward compatible)
OCP 5 compatibility
Test Results
22/22 tests passing on OCP 5 (3.7 min, serial mode)
Screenshots / screen recording
N/A — no visual changes
Test setup
Requires OpenShift cluster with Serverless operator (installed automatically by knative.setup.ts).
Browser conformance
Related
Summary by CodeRabbit
New Features
Tests
Chores