Gitops 9686 UI test app pruning - #1252
Conversation
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>
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>
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a Playwright E2E test that creates a uniquely named guestbook application, deletes it through the Argo CD UI, and verifies removal of the application, deployment, and service. ChangesApplication deletion E2E
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 2
🧹 Nitpick comments (3)
test/ui-e2e/tests/app-deletion.spec.ts (2)
17-39: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass the manifest on stdin instead of through a shell
echopipeline.
execSyncruns the string through/bin/sh, so the YAML passes through shell quoting andecho.echohandling of backslashes and leading dashes is shell-dependent, and any future interpolation intoappYaml(branch name, repo URL, env var) becomes command injection.execFileSyncwith theinputoption avoids both problems.🔒 Proposed fix
-import { execSync } from 'child_process'; +import { execSync, execFileSync } from 'child_process';- execSync(`echo "${appYaml}" | oc apply -f -`, { stdio: 'pipe', timeout: 15000 }); + execFileSync('oc', ['apply', '-f', '-'], { input: appYaml, stdio: 'pipe', 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/tests/app-deletion.spec.ts` around lines 17 - 39, Replace the shell-based execSync echo pipeline in the app deployment test with execFileSync, invoking oc apply directly and passing appYaml through the input option. Preserve the existing timeout and stdio behavior while avoiding shell parsing and interpolation risks.Source: Linters/SAST tools
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared fixtures for login.
test/ui-e2e/src/fixtures.tsalready exports atestwhosepagefixture performs the OpenShift login and throws a clear error whenCLUSTER_PASSWORDis missing. This file re-implements that flow and uses non-null assertions onprocess.env, so a missing password produces the genericLogin required but credentials (user/pass) not providederror fromLoginPage. Importtestfrom../src/fixturesand drop the manual login block.🤖 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/tests/app-deletion.spec.ts` around lines 102 - 108, Replace the manual LoginPage navigation and login flow in the test with the shared test fixture exported by fixtures.ts. Import test from ../src/fixtures, use its authenticated page fixture, and remove the loginPage creation, goto, and loginViaOpenShift calls so missing CLUSTER_PASSWORD uses the fixture’s clear validation error.test/ui-e2e/tests/resource-tree.spec.ts (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
argoVersionis destructured but never used.Playwright instantiates a fixture when the test destructures it. This triggers the
/api/versionrequest and the console log intest/ui-e2e/src/fixtures.tson every run, but the test body (lines 10-26) never reads the value. RemoveargoVersion, or use it to select version-specific behavior inverifyResourceTreeLoaded/clickResourceNode.🤖 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/tests/resource-tree.spec.ts` at line 9, Remove the unused argoVersion fixture from the test callback parameters in the “Navigate to app details, open a Pod, and verify logs stream” test, unless the test is updated to use it for version-specific behavior in verifyResourceTreeLoaded or clickResourceNode.
🤖 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/tests/app-deletion.spec.ts`:
- Around line 143-153: Update the Application deletion checks in
test/ui-e2e/tests/app-deletion.spec.ts at lines 143-153 and 80-90 to call `oc
get ... --ignore-not-found -o name`, treating empty stdout as confirmation that
the resource is deleted; set `backendDeleted` only for that result, and allow
genuine `oc` failures such as timeouts or connectivity errors to propagate.
Apply the same behavior in the `afterAll` cleanup verification at lines 80-90.
- Around line 141-155: The app-deletion test currently verifies only removal of
the Application CR, not its child resources. After the backendDeleted assertion
in the existing polling flow, query deployments and services in openshift-gitops
using the app.kubernetes.io/instance label for appName, and assert that no
matching resources remain; use the existing command-execution conventions or add
the required execFileSync import.
---
Nitpick comments:
In `@test/ui-e2e/tests/app-deletion.spec.ts`:
- Around line 17-39: Replace the shell-based execSync echo pipeline in the app
deployment test with execFileSync, invoking oc apply directly and passing
appYaml through the input option. Preserve the existing timeout and stdio
behavior while avoiding shell parsing and interpolation risks.
- Around line 102-108: Replace the manual LoginPage navigation and login flow in
the test with the shared test fixture exported by fixtures.ts. Import test from
../src/fixtures, use its authenticated page fixture, and remove the loginPage
creation, goto, and loginViaOpenShift calls so missing CLUSTER_PASSWORD uses the
fixture’s clear validation error.
In `@test/ui-e2e/tests/resource-tree.spec.ts`:
- Line 9: Remove the unused argoVersion fixture from the test callback
parameters in the “Navigate to app details, open a Pod, and verify logs stream”
test, unless the test is updated to use it for version-specific behavior in
verifyResourceTreeLoaded or clickResourceNode.
🪄 Autofix
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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 89620ae9-6480-4853-8d2f-12593d256369
📒 Files selected for processing (2)
test/ui-e2e/tests/app-deletion.spec.tstest/ui-e2e/tests/resource-tree.spec.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
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/tests/app-deletion.spec.ts`:
- Around line 192-217: Increase the test timeout configured by test.setTimeout
for the app-deletion test so it covers the UI assertions and the combined
backendDeleted and childrenGone polling budgets, including CLI execution
overhead. Apply the timeout increase before extending the child-resource retry
loop, and ensure any retry-count change remains within the enlarged timeout.
🪄 Autofix
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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4e92d120-96cc-4c8e-8f16-e72a97a0fa08
📒 Files selected for processing (1)
test/ui-e2e/tests/app-deletion.spec.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
argoproj-labs/argocd-operator(manual)
Signed-off-by: Triona Doyle <tekton@example.com>
|
@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. |
1 participant
What type of PR is this?
/kind enhancement
What does this PR do / why we need it:
This PR introduces a new E2E test suite to verify the Argo CD Application Pruning functionality via the UI.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes GITOPS-9686
Test acceptance criteria:
How to test changes / Special notes to the reviewer:
Verification Matrix
This PR has been successfully tested against the following cluster flavors and GitOps Operator versions:
To run the test locally: