GITOPS-9682 UI E2E Create App test#1162
Open
trdoyle81 wants to merge 6 commits into
Open
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
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:
📝 WalkthroughWalkthroughRefactors Playwright UI E2E: asserts auth readiness and persists storageState after dashboard detection, syncs CLI login in the wrapper, adds LoginPage conditional credential flow, a new ApplicationsPage, fixtures for auto-login/managed apps, new admin/create tests, and an expanded README. ChangesUI E2E Testing Infrastructure Overhaul
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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-e2e/.auth/setup.ts`:
- Around line 58-60: Before saving the storage state, assert that the login
actually succeeded instead of relying on page.waitForLoadState('networkidle');
add an explicit check (e.g., waitForSelector or evaluate a known post-login
indicator) to confirm a logged-in UI element or auth cookie/token is present and
throw or fail the setup if it is missing; then only call
page.context().storageState({ path: authFile }) after that positive assertion so
a failed login isn't cached.
In `@test/ui-e2e/src/fixtures.ts`:
- Around line 44-49: The cleanup currently ignores both 404 and 403 responses,
letting RBAC-denied deletes pass silently; change the logic in
test/ui-e2e/src/fixtures.ts so only 404 is ignored and 403 is treated as a
failure: remove the 403 branch/console.log and ensure the code calls
expect(response.status()).toBeLessThan(400) (or otherwise asserts failure) when
response.status() === 403 so teardown failures for managedApp are reported.
- Around line 13-17: The page fixture currently calls
LoginPage.loginViaOpenShift() with no credentials which fails when storage state
is missing; update the fixture in fixtures.ts to read the exported environment
variables CLUSTER_USER, CLUSTER_PASSWORD, and IDP from process.env and pass them
into LoginPage.loginViaOpenShift(user, password, idp). If loginViaOpenShift does
not already accept these parameters, modify LoginPage.loginViaOpenShift to
accept (user: string, password: string, idp?: string) and use them instead of
relying on stored state; validate presence of required vars and surface a clear
error if any are missing.
In `@test/ui-e2e/src/pages/ApplicationsPage.ts`:
- Around line 87-90: The sync helper currently hard-codes the manifest name
'spring-petclinic' causing false negatives; update the syncApplication() flow
(where resourcesSection is defined and awaited) to use a dynamic resource name
instead—either accept the expected resource name as an argument to
syncApplication(appNameOrResourceName) or derive it from the existing appName
parameter passed to createApp, and replace the hard-coded string in the
expect(resourcesSection).toContainText(...) call with that variable so the
locator/assertion works for any app.
In `@test/ui-e2e/src/pages/LoginPage.ts`:
- Around line 39-41: The login probe currently only queries
this.page.getByLabel(/Username/i) (usernameInput) so needsLogin can be false on
variants; update the selector logic used to determine needsLogin (and the
usernameInput locator) to reuse the broader username selector strategy used in
test/ui-e2e/.auth/setup.ts — e.g., try getByLabel(/Username/i) first, then fall
back to this.page.locator('input[name="username"]') and a placeholder-based
locator like input[placeholder*="Username"] when resolving usernameInput and
computing needsLogin; ensure needsLogin awaits the visible state on the unified
locator so clusters using alternate markup won't skip credential entry.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d0efcc05-687e-4347-a1e3-577b5d469691
📒 Files selected for processing (9)
test/ui-e2e/.auth/setup.tstest/ui-e2e/README.mdtest/ui-e2e/run-ui-tests.shtest/ui-e2e/src/fixtures.tstest/ui-e2e/src/pages/ApplicationsPage.tstest/ui-e2e/src/pages/LoginPage.tstest/ui-e2e/tests/admin-login.spec.tstest/ui-e2e/tests/create-application.spec.tstest/ui-e2e/tests/login.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/ui-e2e/.auth/setup.ts (1)
59-60: ⚡ Quick winConsider URL pattern matching for more robust auth verification.
The navigation element check is a solid improvement over
networkidleand addresses the past review concern. However, checking the URL pattern would provide higher confidence that authentication actually succeeded and the user landed on the console dashboard, rather than relying on a navigation element that could theoretically appear on other pages.Alternative approach using URL pattern
- await expect(page.getByRole('navigation').first()).toBeVisible({ timeout: 15000 }); + await expect(page).toHaveURL(/console|overview|k8s/i, { timeout: 15000 }); await page.context().storageState({ path: authFile });🤖 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-e2e/.auth/setup.ts` around lines 59 - 60, Replace or augment the visibility check on await expect(page.getByRole('navigation').first()).toBeVisible(...) with a URL assertion to ensure the app actually navigated to the console/dashboard; use Playwright's page.waitForURL or expect(page).toHaveURL with a specific path or regex (e.g. matching "/console" or the expected dashboard pattern) before calling page.context().storageState({ path: authFile }) so you only persist storage when the page URL confirms successful auth.
🤖 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 `@test/ui-e2e/.auth/setup.ts`:
- Around line 59-60: Replace or augment the visibility check on await
expect(page.getByRole('navigation').first()).toBeVisible(...) with a URL
assertion to ensure the app actually navigated to the console/dashboard; use
Playwright's page.waitForURL or expect(page).toHaveURL with a specific path or
regex (e.g. matching "/console" or the expected dashboard pattern) before
calling page.context().storageState({ path: authFile }) so you only persist
storage when the page URL confirms successful auth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d9d03ffd-30a7-48dc-a1f5-d9184a35b9e8
📒 Files selected for processing (4)
test/ui-e2e/.auth/setup.tstest/ui-e2e/src/fixtures.tstest/ui-e2e/src/pages/ApplicationsPage.tstest/ui-e2e/src/pages/LoginPage.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ui-e2e/src/pages/ApplicationsPage.ts
- test/ui-e2e/src/pages/LoginPage.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/ui-e2e/.auth/setup.ts (1)
60-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBroaden the post-login URL assertion to avoid false negatives across cluster versions.
Line 60 hardcodes
dashboards, which is not guaranteed as the post-auth landing route in all OpenShift versions/configs. This can fail valid logins and preventstorageStatefrom being saved.Suggested minimal fix
- await expect(page).toHaveURL(/.*dashboards.*/, { timeout: 15000 }); + await expect(page).toHaveURL(/(console|k8s|overview|dashboards)/i, { timeout: 15000 });🤖 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-e2e/.auth/setup.ts` at line 60, Replace the strict post-login URL assertion (await expect(page).toHaveURL(...)) with a broader check so valid logins across OpenShift variants aren't rejected: update the call that uses page.toHaveURL to either match multiple accepted landing paths (e.g. use a regex like /.*(dashboards|console).*/ in place of /.*dashboards.*/) or invert the check to assert the page is not the login route (e.g. await expect(page).not.toHaveURL(/.*login.*/,{ timeout:15000 })); ensure you only change the argument to page.toHaveURL (or switch to page.toHaveURL negation) so storageState saving still occurs after successful auth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/ui-e2e/.auth/setup.ts`:
- Line 60: Replace the strict post-login URL assertion (await
expect(page).toHaveURL(...)) with a broader check so valid logins across
OpenShift variants aren't rejected: update the call that uses page.toHaveURL to
either match multiple accepted landing paths (e.g. use a regex like
/.*(dashboards|console).*/ in place of /.*dashboards.*/) or invert the check to
assert the page is not the login route (e.g. await
expect(page).not.toHaveURL(/.*login.*/,{ timeout:15000 })); ensure you only
change the argument to page.toHaveURL (or switch to page.toHaveURL negation) so
storageState saving still occurs after successful auth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2c9bf28f-ec04-4e1c-9aab-17ff80c5a7c9
📒 Files selected for processing (1)
test/ui-e2e/.auth/setup.ts
added 5 commits
June 4, 2026 12:35
Signed-off-by: Triona Doyle <tekton@example.com>
Signed-off-by: Triona Doyle <tekton@example.com>
Signed-off-by: Triona Doyle <tekton@example.com>
Signed-off-by: Triona Doyle <tekton@example.com>
Signed-off-by: Triona Doyle <tekton@example.com>
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 `@test/ui-e2e/src/pages/ApplicationsPage.ts`:
- Around line 97-101: The current check uses allLink.isVisible() which can
return immediately and cause a race; replace the visibility probe with an
explicit wait before clicking: use await allLink.waitFor({ state: 'visible',
timeout: 5000 }) (or page.waitForSelector with the same role/name) and then
await allLink.click(); keep the existing allLink locator and remove the
isVisible() branch (optionally catch a timeout to silently continue if the link
never appears).
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: e9d423be-2459-4b8a-aab3-394d5605485f
📒 Files selected for processing (1)
test/ui-e2e/src/pages/ApplicationsPage.ts
Signed-off-by: Triona Doyle <tekton@example.com>
trdoyle81
force-pushed
the
GITOPS-9682-Create-App-Test
branch
from
c5b47b0 to
430bac8
Compare
|
@trdoyle81: 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
1 participant
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.
** STACKED PR **
This PR is stacked on top of
#1140(Login as admin).#1140 needs to be merged first. The changes isolated for this PR are in the
GITOPS-9682 UI E2E Create App testcommit.What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
This PR introduces the automated UI End-to-End test for the "Create Application" flow in Argo CD, ensuring cross-version compatibility.
create-application.spec.tstest to deploy application via the UI and verify its health.fixtures.tsto automatically clean up the created application via API at the end of the test..auth/setup.tsandrun-ui-tests.shto handle cross-cluster authentication more reliably and fail loudly if credentials or URLs are missing.test/ui-e2e/README.mdwith instructions on execution commands, and trace viewer debugging.Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes GITOPS-9682
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
To test these changes locally against your active cluster:
.envfile is populated with your cluster credentials or exported locally../run-ui-tests.sh tests/create-application.spec.ts --project=chromium --headed --trace on