[WIP] CNTRLPLANE-3846: Bring UI E2E infra into Hypershift repo#9065
[WIP] CNTRLPLANE-3846: Bring UI E2E infra into Hypershift repo#9065georgelipceanu wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@georgelipceanu: This pull request references CNTRLPLANE-3846 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 task 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. |
|
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:
📝 WalkthroughWalkthroughAdded a Playwright-based UI E2E test project under Sequence Diagram(s)sequenceDiagram
participant Playwright
participant auth.setup
participant OpenShiftConsole
participant AdminStorageState
participant UIProjects
Playwright->>auth.setup: Run setup project
auth.setup->>OpenShiftConsole: Log in with configured credentials
OpenShiftConsole-->>auth.setup: Expose authenticated UI state
auth.setup->>AdminStorageState: Save admin.json storage state
UIProjects->>AdminStorageState: Load shared storage state
UIProjects->>OpenShiftConsole: Run common or agent tests
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: georgelipceanu 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 |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (2)
Makefile (1)
714-719: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the checked-in lockfile for UI installs.
Line [717] runs
npm installeven thoughtest/ui/package-lock.jsonexists. This can re-resolve dependency ranges and rewrite the lockfile, making local and future CI runs non-reproducible. Usenpm ciinstead.🤖 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 `@Makefile` around lines 714 - 719, Update the test-ui-install target to use npm ci instead of npm install when installing dependencies in test/ui, so the checked-in package-lock.json is honored. Leave the Playwright browser installation step unchanged.test/ui/src/tests/common/smoke.spec.ts (1)
27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the shared PatternFly selectors.
test/ui/src/constants/index.ts:7-8already exports these selectors, but this test redeclares them. ImportPF_SPINNERandPF_SKELETONfrom@constantsto prevent selector drift.🤖 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 `@test/ui/src/tests/common/smoke.spec.ts` around lines 27 - 28, Remove the local PF_SPINNER and PF_SKELETON declarations in the smoke test and import both shared selectors from `@constants` instead, preserving their existing usages.
🤖 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 `@Makefile`:
- Line 643: Update the codespell command in the Makefile to add
./test/ui/test-results/* to its --skip list, alongside the existing Playwright
report and node_modules exclusions, while preserving all other skip entries.
- Around line 736-739: Update the test-ui-agent target to handle the currently
empty agent test suite by passing Playwright’s --pass-with-no-tests option or
otherwise deferring the target until agent specs exist. Preserve the existing
installation, status message, and agent project selection.
In `@test/ui/playwright.config.ts`:
- Around line 22-26: Replace direct process.env.CI checks in the Playwright
configuration with the parsed config.ci flag for forbidOnly, retries, and
workers. Preserve the existing CI-specific values and local defaults while
ensuring the string value "false" is treated as disabled.
In `@test/ui/README.md`:
- Around line 136-156: Update the fenced directory-tree block in the README to
declare the text language, changing the opening fence before the test/ui tree to
use text while preserving the tree contents.
- Around line 58-62: Update the Scope description in the test project table to
use the compound adjective “Agent-platform HyperShift tests,” preserving the
rest of the table entry unchanged.
- Around line 34-37: Normalize the CI flag handling in the Playwright
configuration so the string value "false" does not enable CI behavior: treat CI
as enabled only when process.env.CI equals "true". Update the CI entry in
test/ui/README.md to document the same exact semantics and default.
- Around line 160-163: Update the “Writing New Tests” instructions to direct
authors to the configured test directories, replacing src/tests/hypershift/ with
src/tests/common/ while retaining src/tests/agent/. Do not add a Playwright
project; align the documentation with the existing test collection
configuration.
In `@test/ui/src/auth.setup.ts`:
- Around line 17-23: Unify authentication with the shared configuration
contract: update test/ui/src/auth.setup.ts lines 17-23 to use config.consoleUrl
and the related shared authentication values instead of directly requiring
process.env.CONSOLE_URL; update test/ui/.env.example lines 4-10 to document
CONSOLE_URL as an optional override, unless the direct lookup is intentionally
retained and made required.
In `@test/ui/src/config/index.ts`:
- Around line 17-27: Update deriveConsoleUrl to parse hubUrl with new URL and
validate the trust-boundary input before deriving the console host: require
HTTPS, a hostname beginning with api., and reject username/password, non-root
paths, queries, and fragments. Use an anchored allow-list validation with
appropriate hostname normalization, then construct the console URL from the
validated hostname; preserve the explicit CONSOLE_URL override.
In `@test/ui/src/services/OcCliService.ts`:
- Around line 11-13: Update OcCliService.run to invoke execPromise with a finite
timeout option, and explicitly surface timeout failures while preserving the
existing command output and error behavior. Because deleteNamespace uses run,
ensure its oc invocation is covered by the same timeout.
- Around line 11-13: Update OcCliService.run and the applyYaml, deleteYaml, and
deleteNamespace callers to avoid shell interpolation by invoking oc through
execFile with an argument array. Validate YAML paths and namespace values before
execution, then pass only validated values as separate arguments to preserve the
existing CLI operations.
In `@test/ui/src/tests/common/smoke.spec.ts`:
- Around line 12-13: Update the heading assertion to use page.getByRole with
role "heading" and name /OpenShift/i instead of page.getByText, while preserving
the first-match selection and existing visibility timeout.
- Around line 21-25: Update the optional 404 handling in the smoke test so it
waits for either the 404 message or the normal page-ready condition before
branching; do not rely on the immediate snapshot from pageNotFound.count().
Preserve the existing early return and MCE-path behavior once the awaited
outcome is known.
In `@test/ui/src/utils/kube-helper.ts`:
- Around line 11-13: Update generateSafeName to normalize the prefix, require a
non-empty DNS-label-safe value using an allow-list, and limit it to 57
characters before appending the five-character suffix. Reject invalid or empty
prefixes rather than generating names that violate Kubernetes’ 63-character
naming limit.
---
Nitpick comments:
In `@Makefile`:
- Around line 714-719: Update the test-ui-install target to use npm ci instead
of npm install when installing dependencies in test/ui, so the checked-in
package-lock.json is honored. Leave the Playwright browser installation step
unchanged.
In `@test/ui/src/tests/common/smoke.spec.ts`:
- Around line 27-28: Remove the local PF_SPINNER and PF_SKELETON declarations in
the smoke test and import both shared selectors from `@constants` instead,
preserving their existing usages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9b661793-902b-427d-a19f-80065c1219ab
⛔ Files ignored due to path filters (1)
test/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
.github/dependabot.ymlMakefiletest/ui/.env.exampletest/ui/.gitignoretest/ui/.prettierignoretest/ui/.prettierrctest/ui/README.mdtest/ui/eslint.config.mjstest/ui/package.jsontest/ui/playwright.config.tstest/ui/src/auth.setup.tstest/ui/src/config/index.tstest/ui/src/constants/index.tstest/ui/src/global-setup.tstest/ui/src/services/OcCliService.tstest/ui/src/services/index.tstest/ui/src/tests/common/smoke.spec.tstest/ui/src/utils/index.tstest/ui/src/utils/kube-helper.tstest/ui/tsconfig.json
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9065 +/- ##
=======================================
Coverage 44.50% 44.50%
=======================================
Files 774 774
Lines 96980 96980
=======================================
Hits 43164 43164
Misses 50828 50828
Partials 2988 2988
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
c2b2479 to
b758577
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/ui/src/fixtures/hypershift-test.ts (1)
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed 2s delay is a heuristic, not a guarantee.
Waiting a flat 2000ms for HostedCluster finalizers before deleting namespaces may be too short under load; consider polling for HostedCluster deletion completion (or increasing/backing off) instead of a fixed sleep. Low priority since failures here are caught and only logged.
🤖 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 `@test/ui/src/fixtures/hypershift-test.ts` around lines 62 - 64, Replace the fixed 2000ms delay in the hostedClusters cleanup flow with polling that waits until the HostedCluster resources are deleted, using a timeout and retry/backoff to avoid indefinite waiting. Preserve the existing behavior when no HostedClusters exist and retain the current cleanup error handling.
🤖 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 `@test/ui/README.md`:
- Around line 40-42: Keep the CI note in README as a single contiguous
blockquote by removing the blank line between its lines or adding a blockquote
marker to that line, ensuring Markdownlint MD028 passes.
- Around line 23-28: Update the environment variable table in the README to add
the optional CONSOLE_URL entry, documenting that it overrides the console URL
and defaults to a value derived from HUB_URL. Keep the existing table formatting
and descriptions unchanged.
In `@test/ui/src/services/OcCliService.ts`:
- Around line 52-61: Update OcCliService.run to pass an appropriate timeout
option to execFilePromise when invoking oc, ensuring hung commands terminate
instead of blocking the Playwright worker indefinitely. Preserve the existing
stdout trimming, redacted failure logging, and error propagation behavior.
- Around line 57-60: Update the error handling around the oc command execution
in OcCliService so console.error does not receive the raw execFile error object.
Log only a sanitized, non-command-specific message while preserving the existing
rethrow behavior.
---
Nitpick comments:
In `@test/ui/src/fixtures/hypershift-test.ts`:
- Around line 62-64: Replace the fixed 2000ms delay in the hostedClusters
cleanup flow with polling that waits until the HostedCluster resources are
deleted, using a timeout and retry/backoff to avoid indefinite waiting. Preserve
the existing behavior when no HostedClusters exist and retain the current
cleanup error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 63944333-6914-41ac-a8da-04cacf5dfed5
⛔ Files ignored due to path filters (1)
test/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.github/dependabot.ymlMakefiletest/ui/.env.exampletest/ui/.gitignoretest/ui/.prettierignoretest/ui/.prettierrctest/ui/README.mdtest/ui/eslint.config.mjstest/ui/package.jsontest/ui/playwright.config.tstest/ui/src/auth.setup.tstest/ui/src/config/index.tstest/ui/src/constants/index.tstest/ui/src/fixtures/hypershift-test.tstest/ui/src/global-setup.tstest/ui/src/services/OcCliService.tstest/ui/src/services/index.tstest/ui/src/tests/common/smoke.spec.tstest/ui/src/utils/index.tstest/ui/src/utils/kube-helper.tstest/ui/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (14)
- test/ui/src/constants/index.ts
- test/ui/package.json
- test/ui/src/utils/index.ts
- test/ui/eslint.config.mjs
- test/ui/.gitignore
- test/ui/.prettierrc
- test/ui/src/services/index.ts
- test/ui/src/global-setup.ts
- test/ui/.prettierignore
- test/ui/playwright.config.ts
- test/ui/src/config/index.ts
- test/ui/src/tests/common/smoke.spec.ts
- test/ui/src/utils/kube-helper.ts
- .github/dependabot.yml
b758577 to
27d4da0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/ui/README.md`:
- Around line 17-29: Separate the Markdown tables in the README by adding blank
lines after the Hub cluster and console authentication table and before the
Playwright Projects heading, while preserving the existing table content and
pipe formatting.
In `@test/ui/src/config/index.ts`:
- Around line 21-23: Update the explicitConsoleUrl branch in the configuration
URL resolver to parse CONSOLE_URL and validate it before returning: require
HTTPS, reject username/password userinfo, and allow only the configured console
host/origin allow-list. Preserve the existing fallback behavior when the
override is absent or invalid, and reuse the project’s established URL
validation symbols where available.
In `@test/ui/src/services/OcCliService.ts`:
- Around line 36-40: The YAML path validation around normalized must inspect the
raw yamlPath segments before normalization, rejecting only exact ".." segments
so names such as "version..yaml" remain valid. Preserve traversal protection
and, if workspace confinement is required by this flow, verify the resolved path
remains within the workspace after resolution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fea281d3-6c31-4658-8712-12c707d471ed
⛔ Files ignored due to path filters (1)
test/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.github/dependabot.ymlMakefiletest/ui/.env.exampletest/ui/.gitignoretest/ui/.prettierignoretest/ui/.prettierrctest/ui/README.mdtest/ui/eslint.config.mjstest/ui/package.jsontest/ui/playwright.config.tstest/ui/src/auth.setup.tstest/ui/src/config/index.tstest/ui/src/constants/index.tstest/ui/src/fixtures/hypershift-test.tstest/ui/src/global-setup.tstest/ui/src/services/OcCliService.tstest/ui/src/services/index.tstest/ui/src/tests/common/smoke.spec.tstest/ui/src/utils/index.tstest/ui/src/utils/kube-helper.tstest/ui/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (15)
- test/ui/.prettierignore
- test/ui/src/constants/index.ts
- test/ui/src/utils/index.ts
- test/ui/eslint.config.mjs
- test/ui/src/services/index.ts
- test/ui/package.json
- test/ui/tsconfig.json
- test/ui/.prettierrc
- test/ui/playwright.config.ts
- test/ui/.gitignore
- test/ui/src/global-setup.ts
- test/ui/src/utils/kube-helper.ts
- test/ui/src/auth.setup.ts
- test/ui/src/fixtures/hypershift-test.ts
- test/ui/src/tests/common/smoke.spec.ts
27d4da0 to
212c656
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/ui/README.md (1)
1-151: 📐 Maintainability & Code Quality | 🔵 TrivialRun
make verify-codespellfor this Markdown file.🤖 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 `@test/ui/README.md` around lines 1 - 151, Run make verify-codespell against test/ui/README.md and correct any reported spelling issues in the documentation, preserving the existing content and formatting.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 `@test/ui/README.md`:
- Around line 49-50: Add a blank line between the “Playwright Projects” heading
and its following projects table in the README, leaving the table content
unchanged.
---
Nitpick comments:
In `@test/ui/README.md`:
- Around line 1-151: Run make verify-codespell against test/ui/README.md and
correct any reported spelling issues in the documentation, preserving the
existing content and formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 26426ca6-bfb9-40f9-932d-bc016eb2a6b6
⛔ Files ignored due to path filters (1)
test/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.github/dependabot.ymlMakefiletest/ui/.env.exampletest/ui/.gitignoretest/ui/.prettierignoretest/ui/.prettierrctest/ui/README.mdtest/ui/eslint.config.mjstest/ui/package.jsontest/ui/playwright.config.tstest/ui/src/auth.setup.tstest/ui/src/config/index.tstest/ui/src/constants/index.tstest/ui/src/fixtures/hypershift-test.tstest/ui/src/global-setup.tstest/ui/src/services/OcCliService.tstest/ui/src/services/index.tstest/ui/src/tests/common/smoke.spec.tstest/ui/src/utils/index.tstest/ui/src/utils/kube-helper.tstest/ui/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (16)
- test/ui/src/services/index.ts
- test/ui/.prettierrc
- test/ui/package.json
- test/ui/src/constants/index.ts
- test/ui/src/global-setup.ts
- test/ui/src/utils/index.ts
- test/ui/tsconfig.json
- test/ui/eslint.config.mjs
- test/ui/src/tests/common/smoke.spec.ts
- test/ui/src/auth.setup.ts
- test/ui/playwright.config.ts
- test/ui/.prettierignore
- .github/dependabot.yml
- test/ui/src/fixtures/hypershift-test.ts
- test/ui/src/config/index.ts
- test/ui/src/utils/kube-helper.ts
212c656 to
6b5be41
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/ui/src/services/OcCliService.ts`:
- Around line 54-56: Increase the default timeout in OcCliService.run so it
exceeds the 60-second cleanup timeout passed to oc, preventing execFilePromise
from terminating the command at the same deadline. Keep explicit timeoutMs
overrides working unchanged.
- Around line 78-89: Update OcCliService.getConsoleUrl to validate that the host
returned by the console route query is non-empty before constructing the URL.
Throw a descriptive error when the result is missing, while preserving the
existing https URL construction for valid hosts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: b00778f5-9a0e-4423-b4ff-d738cb868281
⛔ Files ignored due to path filters (1)
test/ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.github/dependabot.ymlMakefiletest/ui/.env.exampletest/ui/.gitignoretest/ui/.prettierignoretest/ui/.prettierrctest/ui/README.mdtest/ui/eslint.config.mjstest/ui/package.jsontest/ui/playwright.config.tstest/ui/src/auth.setup.tstest/ui/src/config/index.tstest/ui/src/constants/index.tstest/ui/src/fixtures/hypershift-test.tstest/ui/src/global-setup.tstest/ui/src/services/OcCliService.tstest/ui/src/services/index.tstest/ui/src/tests/common/smoke.spec.tstest/ui/src/utils/index.tstest/ui/src/utils/kube-helper.tstest/ui/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (16)
- test/ui/.prettierrc
- test/ui/src/constants/index.ts
- test/ui/.gitignore
- .github/dependabot.yml
- test/ui/src/utils/index.ts
- test/ui/package.json
- test/ui/src/services/index.ts
- test/ui/eslint.config.mjs
- test/ui/playwright.config.ts
- test/ui/src/global-setup.ts
- test/ui/tsconfig.json
- test/ui/src/auth.setup.ts
- test/ui/src/tests/common/smoke.spec.ts
- test/ui/src/utils/kube-helper.ts
- test/ui/src/fixtures/hypershift-test.ts
- test/ui/src/config/index.ts
6b5be41 to
a02ab90
Compare
Setting up Playwright UI E2E infratructure: - StorageState authentication (saves .auth/admin.json) - Basic smoke tests (auth, console loading, MCE detection) - TypeScript with path aliases and console-e2e dependency Signed-off-by: George Lipceanu <glipcean@redhat.com>
a02ab90 to
8aa6286
Compare
What this PR does / why we need it:
This PR sets up UI E2E infrastructure based on current UI E2E standards. This includes:
playwright.config.ts)setup(auth),common(smoke),agent(platform tests - placeholder at the moment for test cases marked for migration)@config,@services,@fixtures,@constants,@utilssmoke.spec.ts)make test-ui-install: Install dependencies and Playwright browsersmake test-ui-common: Run smoke testsWhich issue(s) this PR fixes:
Fixes CNTRLPLANE-3846
Special notes for your reviewer:
test/uidirectory, similar to other test directories.To verify and debug these changes, this was run on an MCE cluster. The commands that were run were as follows:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores