test: add UI E2E tests for Argo CD Resource Tree and Pod logs#1190
test: add UI E2E tests for Argo CD Resource Tree and Pod logs#1190trdoyle81 wants to merge 1 commit into
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 |
Signed-off-by: Triona Doyle <tekton@example.com>
trdoyle81
force-pushed
the
GITOPS-9685-Resource-Tree
branch
from
5d99606 to
9ac9377
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new ChangesUI E2E Test Suite Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 6
🤖 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/README.md`:
- Line 71: The unescaped pipe character in the inline flag `--env=<ci|pipeline>`
on line 71 of the table is being interpreted as a Markdown table column
separator, causing the table structure to break (MD056 error). Escape the pipe
character by adding a backslash before it in the flag value so that Markdown
treats it as literal text rather than a column delimiter, maintaining the
intended 2-column table layout.
In `@test/ui-e2e/run-ui-tests.sh`:
- Around line 70-82: In the CI/pipeline automation block where ENV is "ci" or
"pipeline", the npx playwright test command on line 81 passes TEST_ARGS verbatim
without enforcing headless mode. If TEST_ARGS contains a --headed flag, it will
override the intended automation behavior and can fail in headless-only
environments. Either filter out any --headed flags from TEST_ARGS before passing
them to the playwright test command, or explicitly add a --headed=false flag to
the npx playwright test invocation within the CI/pipeline conditional block to
ensure headless execution is enforced regardless of user-provided arguments.
- Around line 15-24: The .env file is being sourced before the script changes to
its own directory, causing the check for .env and the source command to look in
the caller's current working directory instead of the test/ui-e2e directory.
Move the cd "$(dirname "$0")" || exit 1 command to execute before the if [ -f
.env ] block so that the .env file is correctly loaded from the script's own
directory, not from wherever the script was invoked.
In `@test/ui-e2e/src/pages/ApplicationDetailsPage.ts`:
- Line 26: The assertion on the "Healthy" text is currently scoped to the entire
page via this.page.getByText, which can match unrelated UI elements and cause
false positives. Instead of using this.page.getByText('Healthy', { exact: true
}).first(), first locate the resource tree container element, then apply the
getByText selector to that specific container element rather than the whole page
to ensure you're only checking the health status within the correct UI region.
In `@test/ui-e2e/src/pages/ApplicationsPage.ts`:
- Around line 107-119: Scope the selectors in the sync panel operations to
prevent matching unintended elements. Instead of using page-wide selectors
(`this.page.getByRole()` and `this.page.getByText()`), scope the allLink
selector at line 107, the expectedResource text selector at line 115, and the
synchronize button selector at line 118 to the sync panel context that was
opened earlier with `appContainer.getByText('Sync')`. Store the sync panel
locator and use it to scope all three subsequent selectors to that specific
panel rather than querying the entire page.
- Around line 140-143: The appLink selector in the openApplication method uses
non-exact matching on both the filter and getByRole calls, which can incorrectly
match similarly-named applications (e.g., petclinic vs petclinic-dev). Add
exact: true to both the hasText filter and the getByRole link selector to ensure
precise matching. This pattern is already demonstrated elsewhere in the file at
line 13 for the Create button selector.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 18061886-be38-48c8-bd67-bf81bbece892
📒 Files selected for processing (8)
test/ui-e2e/.auth/setup.tstest/ui-e2e/README.mdtest/ui-e2e/run-ui-tests.shtest/ui-e2e/src/pages/ApplicationDetailsPage.tstest/ui-e2e/src/pages/ApplicationsPage.tstest/ui-e2e/src/pages/LoginPage.tstest/ui-e2e/tests/admin-login.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)
| | `--headed` | Launches the visible Chromium browser UI. Excellent for local debugging. | | ||
| | `--trace on` | Records a granular execution trace (DOM snapshots, network calls, actions) for visual triage. | | ||
| | `--reporter=list` | Switches stdout to a clean line-by-line format, ideal for monitoring real-time execution steps. | | ||
| | `--env=<ci|pipeline>` | Overrides the local setup to simulate automation. It forces headless execution, performs a clean `npm ci`, and installs required browser binaries dynamically. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape | in the table cell to prevent broken rendering
Line 71 currently creates an extra Markdown table column (MD056). Escape the pipe in the inline flag so the table stays 2-column.
Suggested fix
-| `--env=<ci|pipeline>` | Overrides the local setup to simulate automation. It forces headless execution, performs a clean `npm ci`, and installs required browser binaries dynamically. |
+| `--env=<ci\|pipeline>` | Overrides the local setup to simulate automation. It forces headless execution, performs a clean `npm ci`, and installs required browser binaries dynamically. |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `--env=<ci|pipeline>` | Overrides the local setup to simulate automation. It forces headless execution, performs a clean `npm ci`, and installs required browser binaries dynamically. | | |
| | `--env=<ci\|pipeline>` | Overrides the local setup to simulate automation. It forces headless execution, performs a clean `npm ci`, and installs required browser binaries dynamically. | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 71-71: Table column count
Expected: 2; Actual: 3; Too many cells, extra data will be missing
(MD056, table-column-count)
🤖 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/README.md` at line 71, The unescaped pipe character in the inline
flag `--env=<ci|pipeline>` on line 71 of the table is being interpreted as a
Markdown table column separator, causing the table structure to break (MD056
error). Escape the pipe character by adding a backslash before it in the flag
value so that Markdown treats it as literal text rather than a column delimiter,
maintaining the intended 2-column table layout.
Source: Linters/SAST tools
| if [ -f .env ]; then | ||
| echo "Loading variables from .env file..." | ||
| set -a #export all variables | ||
| source .env | ||
| set +a # stop automatically exporting | ||
| set +a #stop auto export | ||
| fi | ||
|
|
||
| #making sure we are in the correct dir | ||
| cd "$(dirname "$0")" || exit 1 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Load .env after switching to the script directory
source .env runs before cd "$(dirname "$0")", so Line 15 checks the caller’s CWD, not test/ui-e2e. That breaks the documented setup when the script is launched from repo root or CI wrappers.
Suggested fix
- if [ -f .env ]; then
- echo "Loading variables from .env file..."
- set -a `#export` all variables
- source .env
- set +a `#stop` auto export
- fi
-
- `#making` sure we are in the correct dir
- cd "$(dirname "$0")" || exit 1
+ `#making` sure we are in the correct dir
+ cd "$(dirname "$0")" || exit 1
+
+ if [ -f .env ]; then
+ echo "Loading variables from .env file..."
+ set -a `#export` all variables
+ source .env
+ set +a `#stop` auto export
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ -f .env ]; then | |
| echo "Loading variables from .env file..." | |
| set -a #export all variables | |
| source .env | |
| set +a # stop automatically exporting | |
| set +a #stop auto export | |
| fi | |
| #making sure we are in the correct dir | |
| cd "$(dirname "$0")" || exit 1 | |
| `#making` sure we are in the correct dir | |
| cd "$(dirname "$0")" || exit 1 | |
| if [ -f .env ]; then | |
| echo "Loading variables from .env file..." | |
| set -a `#export` all variables | |
| source .env | |
| set +a `#stop` auto export | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 18-18: Not following: .env was not specified as input (see shellcheck -x).
(SC1091)
🤖 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/run-ui-tests.sh` around lines 15 - 24, The .env file is being
sourced before the script changes to its own directory, causing the check for
.env and the source command to look in the caller's current working directory
instead of the test/ui-e2e directory. Move the cd "$(dirname "$0")" || exit 1
command to execute before the if [ -f .env ] block so that the .env file is
correctly loaded from the script's own directory, not from wherever the script
was invoked.
| if [[ "$ENV" == "ci" ]] || [[ "$ENV" == "pipeline" ]]; then | ||
| echo "Running headlessly in automation ($ENV)..." | ||
| npm ci | ||
|
|
||
| # Prevent sudo jump-scares for local Mac users simulating CI | ||
| if [[ "$(uname -s)" == "Darwin" ]]; then | ||
| npx playwright install chromium | ||
| else | ||
| npx playwright install chromium --with-deps | ||
| fi | ||
|
|
||
| npx playwright test "${TEST_ARGS[@]}" --reporter=list | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
--env=ci|pipeline does not actually force headless
In CI/pipeline mode, Line 81 still passes user args verbatim. If --headed is provided, Playwright can run headed, which conflicts with the intended automation behavior and can fail in headless-only environments.
Suggested fix
if [[ "$ENV" == "ci" ]] || [[ "$ENV" == "pipeline" ]]; then
echo "Running headlessly in automation ($ENV)..."
npm ci
@@
- npx playwright test "${TEST_ARGS[@]}" --reporter=list
+ FILTERED_ARGS=()
+ for arg in "${TEST_ARGS[@]}"; do
+ [[ "$arg" == "--headed" ]] && continue
+ FILTERED_ARGS+=("$arg")
+ done
+ npx playwright test "${FILTERED_ARGS[@]}" --reporter=list📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ "$ENV" == "ci" ]] || [[ "$ENV" == "pipeline" ]]; then | |
| echo "Running headlessly in automation ($ENV)..." | |
| npm ci | |
| # Prevent sudo jump-scares for local Mac users simulating CI | |
| if [[ "$(uname -s)" == "Darwin" ]]; then | |
| npx playwright install chromium | |
| else | |
| npx playwright install chromium --with-deps | |
| fi | |
| npx playwright test "${TEST_ARGS[@]}" --reporter=list | |
| if [[ "$ENV" == "ci" ]] || [[ "$ENV" == "pipeline" ]]; then | |
| echo "Running headlessly in automation ($ENV)..." | |
| npm ci | |
| # Prevent sudo jump-scares for local Mac users simulating CI | |
| if [[ "$(uname -s)" == "Darwin" ]]; then | |
| npx playwright install chromium | |
| else | |
| npx playwright install chromium --with-deps | |
| fi | |
| FILTERED_ARGS=() | |
| for arg in "${TEST_ARGS[@]}"; do | |
| [[ "$arg" == "--headed" ]] && continue | |
| FILTERED_ARGS+=("$arg") | |
| done | |
| npx playwright test "${FILTERED_ARGS[@]}" --reporter=list |
🤖 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/run-ui-tests.sh` around lines 70 - 82, In the CI/pipeline
automation block where ENV is "ci" or "pipeline", the npx playwright test
command on line 81 passes TEST_ARGS verbatim without enforcing headless mode. If
TEST_ARGS contains a --headed flag, it will override the intended automation
behavior and can fail in headless-only environments. Either filter out any
--headed flags from TEST_ARGS before passing them to the playwright test
command, or explicitly add a --headed=false flag to the npx playwright test
invocation within the CI/pipeline conditional block to ensure headless execution
is enforced regardless of user-provided arguments.
| //wait tree to be visible | ||
| await expect(this.resourceTreeContainer).toBeVisible({ timeout: 20000 }); | ||
| //wait for healthy status | ||
| await expect(this.page.getByText('Healthy', { exact: true }).first()).toBeVisible({ timeout: 30000 }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Scope the “Healthy” assertion to the resource tree container to avoid false positives.
this.page.getByText('Healthy') can match unrelated UI regions, so the helper may pass even when the tree state is wrong.
Suggested fix
- await expect(this.page.getByText('Healthy', { exact: true }).first()).toBeVisible({ timeout: 30000 });
+ await expect(
+ this.resourceTreeContainer.getByText('Healthy', { exact: true }).first()
+ ).toBeVisible({ timeout: 30000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await expect(this.page.getByText('Healthy', { exact: true }).first()).toBeVisible({ timeout: 30000 }); | |
| await expect( | |
| this.resourceTreeContainer.getByText('Healthy', { exact: true }).first() | |
| ).toBeVisible({ timeout: 30000 }); |
🤖 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/src/pages/ApplicationDetailsPage.ts` at line 26, The assertion on
the "Healthy" text is currently scoped to the entire page via
this.page.getByText, which can match unrelated UI elements and cause false
positives. Instead of using this.page.getByText('Healthy', { exact: true
}).first(), first locate the resource tree container element, then apply the
getByText selector to that specific container element rather than the whole page
to ensure you're only checking the health status within the correct UI region.
| const allLink = this.page.getByRole('link', { name: 'all', exact: true }); | ||
| try { | ||
| await allLink.waitFor({ state: 'visible', timeout: 3000 }); | ||
| await allLink.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await allLink.click(); | ||
| } catch (error) { | ||
| //all link didn't appear within 3 sec | ||
| // all link didn't appear within 5 sec | ||
| } | ||
|
|
||
| //wait for the manifests to render on the panel | ||
| await expect(this.page.getByText(expectedResource).first()).toBeVisible({ timeout: 30000 }); | ||
|
|
||
| //click the main sync button | ||
| await this.page.getByRole('button', { name: /^synchronize$/i }).first().click(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, explore the file structure and locate it
find . -type f -name "ApplicationsPage.ts" 2>/dev/null
# Get an outline of the file to understand its structure
ast-grep outline test/ui-e2e/src/pages/ApplicationsPage.ts --view expanded | head -100Repository: redhat-developer/gitops-operator
Length of output: 1011
🏁 Script executed:
#!/bin/bash
# Read the specific section in context (lines 95-130 to see surrounding code)
sed -n '95,130p' test/ui-e2e/src/pages/ApplicationsPage.tsRepository: redhat-developer/gitops-operator
Length of output: 1716
🏁 Script executed:
#!/bin/bash
# Search for "synchronize panel" or similar references to understand the panel structure
rg -n "panel|modal|dialog|synchronize" test/ui-e2e/src/pages/ApplicationsPage.ts -A 2 -B 2Repository: redhat-developer/gitops-operator
Length of output: 1141
🏁 Script executed:
#!/bin/bash
# Look for how other methods scope their selectors in this file
rg -n "getByRole|locator|within|filter" test/ui-e2e/src/pages/ApplicationsPage.ts | head -30Repository: redhat-developer/gitops-operator
Length of output: 2152
Scope selectors for the 'all' link, expectedResource text, and synchronize button to the sync panel.
Lines 107-119 use page-wide selectors (this.page.getByRole(), this.page.getByText()) that can match unintended elements in background table content. This contradicts the scoping pattern established earlier in the method (line 100-104, where appContainer is used). After opening the sync panel with appContainer.getByText('Sync'), the subsequent operations should be scoped to that panel context rather than querying the entire page, which can introduce flaky behavior and false positives.
🤖 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/src/pages/ApplicationsPage.ts` around lines 107 - 119, Scope the
selectors in the sync panel operations to prevent matching unintended elements.
Instead of using page-wide selectors (`this.page.getByRole()` and
`this.page.getByText()`), scope the allLink selector at line 107, the
expectedResource text selector at line 115, and the synchronize button selector
at line 118 to the sync panel context that was opened earlier with
`appContainer.getByText('Sync')`. Store the sync panel locator and use it to
scope all three subsequent selectors to that specific panel rather than querying
the entire page.
| const appLink = this.page.locator('.white-box, .argo-table-list__row') | ||
| .filter({ hasText: appName }) | ||
| .getByRole('link', { name: appName }); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the file in question
find . -type f -name "ApplicationsPage.ts" 2>/dev/null | head -20Repository: redhat-developer/gitops-operator
Length of output: 214
🏁 Script executed:
#!/bin/bash
# If found, examine the file structure and content around lines 140-143
if [ -f "test/ui-e2e/src/pages/ApplicationsPage.ts" ]; then
echo "=== File exists, examining structure ==="
wc -l test/ui-e2e/src/pages/ApplicationsPage.ts
echo ""
echo "=== AST outline to understand context ==="
ast-grep outline test/ui-e2e/src/pages/ApplicationsPage.ts --view expanded | head -100
fiRepository: redhat-developer/gitops-operator
Length of output: 1097
🏁 Script executed:
#!/bin/bash
# Read the specific lines with context
if [ -f "test/ui-e2e/src/pages/ApplicationsPage.ts" ]; then
echo "=== Lines 130-160 with context ==="
sed -n '130,160p' test/ui-e2e/src/pages/ApplicationsPage.ts | cat -n
fiRepository: redhat-developer/gitops-operator
Length of output: 1301
🏁 Script executed:
#!/bin/bash
# Check for similar patterns with getByRole in test files
rg "getByRole.*name:" test/ui-e2e --type typescript -A 1 -B 1 | head -40Repository: redhat-developer/gitops-operator
Length of output: 205
🏁 Script executed:
#!/bin/bash
# Check if there are test fixtures with similarly-named apps
rg "petclinic|app.*name|createApp" test/ui-e2e --type typescript | grep -E "(test|fixture|data|const)" | head -20Repository: redhat-developer/gitops-operator
Length of output: 205
🏁 Script executed:
#!/bin/bash
# Check for similar patterns with getByRole in test files (using ts type)
rg "getByRole.*name:" test/ui-e2e -A 1 -B 1 | head -40Repository: redhat-developer/gitops-operator
Length of output: 2988
🏁 Script executed:
#!/bin/bash
# Check test data and app creation patterns
rg "createApp|spring-petclinic|appName" test/ui-e2e/src -A 2 -B 1 | head -50Repository: redhat-developer/gitops-operator
Length of output: 3873
🏁 Script executed:
#!/bin/bash
# Check if syncApplication and verifyStatus have similar patterns
sed -n '96,130p' test/ui-e2e/src/pages/ApplicationsPage.ts | cat -nRepository: redhat-developer/gitops-operator
Length of output: 1960
Use exact app-link matching in openApplication.
hasText: appName combined with non-exact getByRole('link', { name: appName }) can match similarly-named apps (e.g., petclinic vs petclinic-dev), causing the wrong application to be opened. Resolve by using exact: true on both the link selector and the row filter—this pattern is already used elsewhere in the file (e.g., line 13 for the Create button).
Suggested fix
- const appLink = this.page.locator('.white-box, .argo-table-list__row')
- .filter({ hasText: appName })
- .getByRole('link', { name: appName });
+ const appLink = this.page.locator('.white-box, .argo-table-list__row')
+ .filter({ has: this.page.getByRole('link', { name: appName, exact: true }) })
+ .getByRole('link', { name: appName, exact: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const appLink = this.page.locator('.white-box, .argo-table-list__row') | |
| .filter({ hasText: appName }) | |
| .getByRole('link', { name: appName }); | |
| const appLink = this.page.locator('.white-box, .argo-table-list__row') | |
| .filter({ has: this.page.getByRole('link', { name: appName, exact: true }) }) | |
| .getByRole('link', { name: appName, exact: true }); |
🤖 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/src/pages/ApplicationsPage.ts` around lines 140 - 143, The
appLink selector in the openApplication method uses non-exact matching on both
the filter and getByRole calls, which can incorrectly match similarly-named
applications (e.g., petclinic vs petclinic-dev). Add exact: true to both the
hasText filter and the getByRole link selector to ensure precise matching. This
pattern is already demonstrated elsewhere in the file at line 13 for the Create
button selector.
|
@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 adds the UI E2E test suite for the Argo CD Application Details page. It verifies that the Resource Tree renders correctly and that Pod log streams are accessible. It also includes cross-version stability updates to handle OpenShift popups and Argo CD loading banners.
Have you updated the necessary documentation?
Which issue(s) this PR fixes:
Fixes GITOPS-9685
Test acceptance criteria:
[ ] Unit Test
[x] E2E Test
How to test changes / Special notes to the reviewer:
Run the new test locally against any provisioned cluster:
./run-ui-tests.sh tests/resource-tree.spec.ts --headed --project=chromium