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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (28)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (16)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughMigrates Knative CI smoke tests from Cypress/Gherkin to Playwright. Adds ChangesKnative Playwright E2E Migration
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over PlaywrightRunner,K8sAPI: Cluster Setup Phase
PlaywrightRunner->>knative.setup.ts: execute knative-setup project
knative.setup.ts->>K8sAPI: check ClusterServiceVersion<br/>in openshift-serverless
alt CSV not at Succeeded phase
knative.setup.ts->>K8sAPI: apply Serverless subscription YAML
knative.setup.ts->>K8sAPI: poll CSV every 10s for Succeeded<br/>(5 min timeout)
end
knative.setup.ts->>K8sAPI: apply KnativeServing manifest
knative.setup.ts->>K8sAPI: apply KnativeEventing manifest
knative.setup.ts->>K8sAPI: poll KnativeServing<br/>status.conditions for Ready=True
knative.setup.ts->>K8sAPI: poll KnativeEventing<br/>status.conditions for Ready=True
knative.setup.ts-->>PlaywrightRunner: setup complete
end
rect rgba(60, 179, 113, 0.5)
Note over Test,AdminEventingPage: Test Execution Phase
Test->>AddFlowPage: navigateToAddPage(namespace)
AddFlowPage->>AddFlowPage: clickImportFromGitCard()
AddFlowPage->>AddFlowPage: enterGitUrl() + fallback logic
AddFlowPage->>AddFlowPage: selectServerlessDeployment()
AddFlowPage->>AddFlowPage: clickCreate()
AddFlowPage-->>Test: redirected to topology URL
Test->>TopologyKnativePage: navigateToTopology(namespace)
TopologyKnativePage->>TopologyKnativePage: verifyWorkloadVisible(serviceName)
Test->>TopologyKnativePage: rightClickOnKnativeService(name)
TopologyKnativePage->>TopologyKnativePage: selectContextMenuAction(action)
Test->>AdminEventingPage: navigateToEventing(namespace)
AdminEventingPage->>AdminEventingPage: clickTab('Event Sources')
AdminEventingPage-->>Test: eventing tab loaded
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 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
Comment on lines
+101
to
+125
| // Wait for the CSV to reach Succeeded phase | ||
| // eslint-disable-next-line no-console | ||
| console.log('Waiting for Serverless operator CSV to succeed...'); | ||
| const startTime = Date.now(); | ||
| const csvTimeout = 300_000; | ||
| while (Date.now() - startTime < csvTimeout) { | ||
| try { | ||
| const csvList = await k8sClient.customObjectsApi.listNamespacedCustomObject({ | ||
| group: 'operators.coreos.com', | ||
| version: 'v1alpha1', | ||
| namespace: 'openshift-serverless', | ||
| plural: 'clusterserviceversions', | ||
| }); | ||
| const items = (csvList as { items?: Array<{ status?: { phase?: string } }> }).items || []; | ||
| if (items.some((csv) => csv.status?.phase === 'Succeeded')) { | ||
| // eslint-disable-next-line no-console | ||
| console.log('Serverless operator installed successfully'); | ||
| break; | ||
| } | ||
| } catch { | ||
| // CSV not ready yet | ||
| } | ||
| await new Promise((r) => setTimeout(r, 10_000)); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Fail setup when CSV never reaches Succeeded.
The loop at Line 106 stops after csvTimeout, but the setup does not throw when readiness is never reached. That allows the setup test to pass and pushes failure into later specs.
Suggested fix
const startTime = Date.now();
const csvTimeout = 300_000;
+ let csvReady = false;
while (Date.now() - startTime < csvTimeout) {
try {
const csvList = await k8sClient.customObjectsApi.listNamespacedCustomObject({
group: 'operators.coreos.com',
version: 'v1alpha1',
namespace: 'openshift-serverless',
plural: 'clusterserviceversions',
});
const items = (csvList as { items?: Array<{ status?: { phase?: string } }> }).items || [];
if (items.some((csv) => csv.status?.phase === 'Succeeded')) {
// eslint-disable-next-line no-console
console.log('Serverless operator installed successfully');
+ csvReady = true;
break;
}
} catch {
// CSV not ready yet
}
await new Promise((r) => setTimeout(r, 10_000));
}
+ if (!csvReady) {
+ throw new Error(
+ `Serverless operator CSV did not reach Succeeded within ${csvTimeout}ms`,
+ );
+ }🤖 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 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.
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
|
@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. |
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
Release Notes
Tests
Chores