Add STP, STD, and tests for CNV-72112: VM IP address filtering#41
Open
guyoron1 wants to merge 1 commit intoRedHatQE:mainfrom
Open
Add STP, STD, and tests for CNV-72112: VM IP address filtering#41guyoron1 wants to merge 1 commit intoRedHatQE:mainfrom
guyoron1 wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
Add complete test artifacts for CNV-72112 (VM IP Address Filtering in OCP Console Across All Projects) under sig-ui: - STP: stps/sig-ui/vm-ip-filter-stp.md - STD stubs: stds/sig-ui/ (Go + Python design review stubs) - Tests: tests/sig-ui/ (12 Tier 1 Go/Ginkgo + 3 Tier 2 Python/pytest) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
WalkthroughThis pull request introduces comprehensive test infrastructure for VM IP address filtering functionality. It adds test stubs, end-to-end test implementations, test fixtures, and a detailed test plan across both Python and Go, covering cross-namespace VM discovery, VM lifecycle verification, and bulk operations filtered by IP address. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stds/sig-ui/vm_ip_filter_stubs_test.go`:
- Around line 45-235: The test stubs use the invalid Ginkgo v2 identifier
PendingIt; replace every occurrence of PendingIt(...) with PIt(...) (e.g., the
pending specs named "[test_id:TS-CNV-72112-001]" through
"[test_id:TS-CNV-72112-012]") so the pending tests use the correct Ginkgo v2
syntax; update each PendingIt invocation to PIt keeping the same arguments and
Skip(...) body intact.
In `@stps/sig-ui/vm-ip-filter-stp.md`:
- Line 82: Update the "Automation Testing" table entries that currently state
"Playwright-based UI tests. Existing test at
`playwright/tests/tier1/vm-actions.spec.ts`..." to accurately reflect this PR's
executable coverage: replace with a short description like "API-level automated
tests using Go/Ginkgo and Python/pytest; see relevant Ginkgo test(s) and pytest
modules covering the VM IP filter scenarios" and update any test file references
to the actual Go/Ginkgo and Python/pytest test files in this PR; apply the same
replacement for both occurrences currently mentioning Playwright (the row at
line ~82 and the similar row at ~114). Ensure the text clearly indicates
API-level coverage and points to the concrete test suites used in this PR.
In `@tests/sig-ui/conftest.py`:
- Around line 83-87: get_vmis_matching_ip is using substring matching
(any(target_ip in addr ...)) which can produce false positives; change the check
to exact equality against each address (e.g., compare target_ip == addr for addr
in ip_addresses), and ensure you handle None/empty entries consistently
(strip/normalize if needed) when iterating over iface.get("ipAddresses", []) in
the loop that looks up interfaces and appends matched vmi.
In `@tests/sig-ui/test_vm_ip_filter_e2e.py`:
- Around line 109-111: The LOGGER.info call is using an unnecessary f-string
(Ruff F541); change the LOGGER.info invocation that currently has f"Verifying
decoy VMs are not returned in IP filter results" to a plain string literal by
removing the leading "f" so it becomes "Verifying decoy VMs are not returned in
IP filter results" (locate the LOGGER.info(...) statement in
test_vm_ip_filter_e2e.py).
- Around line 217-223: The current immediate post-delete check uses
get_vmis_matching_ip and sets vm_still_present by matching vmi.name == vm_name
OR vmi.namespace == namespace.name which is race-prone and can match unrelated
VMIs; change this to poll with a timeout/retry (e.g., exponential backoff or
test helper wait_until) after deletion and consider a VMI present only if both
vmi.name == vm_name AND vmi.namespace == namespace.name, i.e., replace the OR
with AND and loop until get_vmis_matching_ip returns no VMI that matches both
vm_name and namespace.name or until timeout, failing the test only after the
timeout elapses.
In `@tests/sig-ui/vm_ip_filter_test.go`:
- Around line 45-56: filterVMIsByIP can append the same VMI multiple times
because breaking from the inner loop only exits the IP loop; update the logic in
filterVMIsByIP to ensure each vmi is appended at most once by using a found flag
(e.g., matchedFlag) when you detect a match in either iface.IP or iface.IPs,
append vmi once and then break out of the outer loop over vmi.Status.Interfaces
(or return/continue to next vmi) so subsequent interfaces cannot append the same
vmi again.
- Around line 389-399: The current error handling for
virtClient.VirtualMachineInstance(...).List and
virtClient.VirtualMachine(...).List treats every error as "namespace missing"
and returns, hiding real failures; add the import alias apierrors
"k8s.io/apimachinery/pkg/api/errors" and change each err check to explicitly
test apierrors.IsNotFound(err) and return only in that case, otherwise call
Expect(err).ToNot(HaveOccurred()): update the error branches around
VirtualMachineInstance(...).List and VirtualMachine(...).List to use
apierrors.IsNotFound(err) before returning so RBAC/transport/API errors surface
via Expect.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
stds/sig-ui/test_vm_ip_filter_e2e_stubs.pystds/sig-ui/vm_ip_filter_stubs_test.gostps/sig-ui/vm-ip-filter-stp.mdtests/sig-ui/conftest.pytests/sig-ui/test_vm_ip_filter_e2e.pytests/sig-ui/vm_ip_filter_test.go
Comment on lines
+45
to
+235
| PendingIt("[test_id:TS-CNV-72112-001] should return only the VM matching the IP address filter", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - At least one VM running in the cluster | ||
| - IP address 192.0.2.99 not assigned to any VM | ||
|
|
||
| Steps: | ||
| 1. List all VMIs across namespaces | ||
| 2. Filter by non-existent IP address (192.0.2.99) | ||
| 3. Verify no VMIs are matched | ||
|
|
||
| Expected: | ||
| - Filter with non-existent IP returns zero results | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-002] should display empty state when filtering by non-existent IP", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - Multiple VMs with IPs sharing a common prefix (e.g., 10.128.x.x) | ||
|
|
||
| Steps: | ||
| 1. List all VMIs across namespaces | ||
| 2. Filter by partial IP prefix substring | ||
| 3. Verify matching VMs are shown | ||
|
|
||
| Expected: | ||
| - Partial IP filter returns all VMs with matching IP substring | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-003] should return VMs matching partial IP address substring", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
| }) | ||
|
|
||
| Context("Regression: existing filters after fix", func() { | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - VMs with unique names across namespaces | ||
|
|
||
| Steps: | ||
| 1. List VMs with field selector for target name | ||
| 2. Verify only matching VM is returned | ||
|
|
||
| Expected: | ||
| - Name filter returns only VMs matching the entered name | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-004] should return the correct VM when filtering by name", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - VMs with distinct labels (e.g., env=production, env=staging) | ||
|
|
||
| Steps: | ||
| 1. List VMs with label selector for env=production | ||
| 2. Verify only production VMs are shown | ||
|
|
||
| Expected: | ||
| - Label filter returns only VMs with the matching label | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-005] should return VMs matching the selected label", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
| }) | ||
|
|
||
| Context("Fallback path: proxy pod not active", func() { | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - Proxy pod is not running (isProxyPodAlive === false) | ||
| - VMs running with assigned IP addresses | ||
|
|
||
| Steps: | ||
| 1. List VMIs in a single namespace (non-proxy path) | ||
| 2. Filter by target IP | ||
| 3. Verify correct VM is shown | ||
|
|
||
| Expected: | ||
| - IP filter works when proxy pod is not active | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-006] should use frontend-filtered data when proxy pod is not active", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
| }) | ||
|
|
||
| Context("Pagination and selection with filtered data", func() { | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - 5+ VMs across different namespaces | ||
|
|
||
| Steps: | ||
| 1. List all VMIs and record total count | ||
| 2. Filter by target IP | ||
| 3. Verify filtered count is less than total | ||
|
|
||
| Expected: | ||
| - Pagination item count matches the number of filtered VMs | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-007] should update pagination item count after filtering", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - Multiple VMs with varied IPs across namespaces | ||
|
|
||
| Steps: | ||
| 1. Filter VMIs by target IP | ||
| 2. Build selection set from filtered results | ||
| 3. Verify selection scope matches filtered count | ||
|
|
||
| Expected: | ||
| - Select-all selects only VMs currently displayed in the filtered list | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-008] should select only the filtered VMs when select-all is clicked", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - VMs selected via select-all on filtered list | ||
|
|
||
| Steps: | ||
| 1. Clear selection set | ||
| 2. Verify no VMs are selected | ||
|
|
||
| Expected: | ||
| - Deselect-all removes all VM selections | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-009] should clear all VM selections when deselect-all is clicked", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
| }) | ||
|
|
||
| Context("Empty state and summary component", func() { | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - No VMs running in any accessible namespace | ||
|
|
||
| Steps: | ||
| 1. List VMs in an empty namespace | ||
| 2. Verify list is empty | ||
|
|
||
| Expected: | ||
| - Empty state component is rendered with actionable content | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-010] should display the empty state component when no VMs exist", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - Exactly 3 VMs created across 2 namespaces | ||
|
|
||
| Steps: | ||
| 1. List all VMs across namespaces | ||
| 2. Verify total count matches expected | ||
|
|
||
| Expected: | ||
| - Summary component shows the correct total VM count | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-011] should display the correct VM count in the summary component", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) | ||
| }) | ||
|
|
||
| Context("Single-project view", func() { | ||
|
|
||
| /* | ||
| Preconditions: | ||
| - At least 2 VMs in a single namespace with different IPs | ||
|
|
||
| Steps: | ||
| 1. List VMIs in a single namespace | ||
| 2. Filter by target IP | ||
| 3. Verify correct VM shown | ||
|
|
||
| Expected: | ||
| - IP filter returns the correct VM within a single project | ||
| */ | ||
| PendingIt("[test_id:TS-CNV-72112-012] should return the correct VM when filtering by IP within a single project", func() { | ||
| Skip("Phase 1: Design only - awaiting implementation") | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for PendingIt usage, local definitions, and Ginkgo version
echo "=== Searching for PendingIt usage ==="
rg -n '\bPendingIt\s*\(' --type go | head -20
echo ""
echo "=== Searching for local PendingIt definition ==="
rg -n '^\s*func\s+PendingIt\s*\(' --type go
echo ""
echo "=== Checking Ginkgo version in go.mod ==="
rg -n 'github.com/onsi/ginkgo' go.modRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 2117
🏁 Script executed:
#!/bin/bash
# Check repository structure and file context
echo "=== Repository structure ==="
find . -type f -name "*.go" | head -10
echo ""
echo "=== Checking for go.mod or go.sum anywhere ==="
fd -e mod -e sum | head -10
echo ""
echo "=== Checking file header and imports for vm_ip_filter_stubs_test.go ==="
head -50 stds/sig-ui/vm_ip_filter_stubs_test.go
echo ""
echo "=== Checking README or docs ==="
ls -la | grep -i readme
cat README.md 2>/dev/null | head -30Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 2939
Replace PendingIt with PIt to reflect valid Ginkgo v2 syntax in this design stub.
While this repository is design documentation that won't be compiled, the file imports Ginkgo v2 and should demonstrate correct syntax. PendingIt is not valid in Ginkgo v2; use PIt(...) instead for pending tests.
Suggested fix pattern (apply to all 12 occurrences)
- PendingIt("[test_id:TS-CNV-72112-001] should return only the VM matching the IP address filter", func() {
- Skip("Phase 1: Design only - awaiting implementation")
- })
+ PIt("[test_id:TS-CNV-72112-001] should return only the VM matching the IP address filter")📝 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.
Suggested change
| PendingIt("[test_id:TS-CNV-72112-001] should return only the VM matching the IP address filter", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| /* | |
| Preconditions: | |
| - At least one VM running in the cluster | |
| - IP address 192.0.2.99 not assigned to any VM | |
| Steps: | |
| 1. List all VMIs across namespaces | |
| 2. Filter by non-existent IP address (192.0.2.99) | |
| 3. Verify no VMIs are matched | |
| Expected: | |
| - Filter with non-existent IP returns zero results | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-002] should display empty state when filtering by non-existent IP", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| /* | |
| Preconditions: | |
| - Multiple VMs with IPs sharing a common prefix (e.g., 10.128.x.x) | |
| Steps: | |
| 1. List all VMIs across namespaces | |
| 2. Filter by partial IP prefix substring | |
| 3. Verify matching VMs are shown | |
| Expected: | |
| - Partial IP filter returns all VMs with matching IP substring | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-003] should return VMs matching partial IP address substring", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| }) | |
| Context("Regression: existing filters after fix", func() { | |
| /* | |
| Preconditions: | |
| - VMs with unique names across namespaces | |
| Steps: | |
| 1. List VMs with field selector for target name | |
| 2. Verify only matching VM is returned | |
| Expected: | |
| - Name filter returns only VMs matching the entered name | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-004] should return the correct VM when filtering by name", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| /* | |
| Preconditions: | |
| - VMs with distinct labels (e.g., env=production, env=staging) | |
| Steps: | |
| 1. List VMs with label selector for env=production | |
| 2. Verify only production VMs are shown | |
| Expected: | |
| - Label filter returns only VMs with the matching label | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-005] should return VMs matching the selected label", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| }) | |
| Context("Fallback path: proxy pod not active", func() { | |
| /* | |
| Preconditions: | |
| - Proxy pod is not running (isProxyPodAlive === false) | |
| - VMs running with assigned IP addresses | |
| Steps: | |
| 1. List VMIs in a single namespace (non-proxy path) | |
| 2. Filter by target IP | |
| 3. Verify correct VM is shown | |
| Expected: | |
| - IP filter works when proxy pod is not active | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-006] should use frontend-filtered data when proxy pod is not active", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| }) | |
| Context("Pagination and selection with filtered data", func() { | |
| /* | |
| Preconditions: | |
| - 5+ VMs across different namespaces | |
| Steps: | |
| 1. List all VMIs and record total count | |
| 2. Filter by target IP | |
| 3. Verify filtered count is less than total | |
| Expected: | |
| - Pagination item count matches the number of filtered VMs | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-007] should update pagination item count after filtering", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| /* | |
| Preconditions: | |
| - Multiple VMs with varied IPs across namespaces | |
| Steps: | |
| 1. Filter VMIs by target IP | |
| 2. Build selection set from filtered results | |
| 3. Verify selection scope matches filtered count | |
| Expected: | |
| - Select-all selects only VMs currently displayed in the filtered list | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-008] should select only the filtered VMs when select-all is clicked", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| /* | |
| Preconditions: | |
| - VMs selected via select-all on filtered list | |
| Steps: | |
| 1. Clear selection set | |
| 2. Verify no VMs are selected | |
| Expected: | |
| - Deselect-all removes all VM selections | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-009] should clear all VM selections when deselect-all is clicked", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| }) | |
| Context("Empty state and summary component", func() { | |
| /* | |
| Preconditions: | |
| - No VMs running in any accessible namespace | |
| Steps: | |
| 1. List VMs in an empty namespace | |
| 2. Verify list is empty | |
| Expected: | |
| - Empty state component is rendered with actionable content | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-010] should display the empty state component when no VMs exist", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| /* | |
| Preconditions: | |
| - Exactly 3 VMs created across 2 namespaces | |
| Steps: | |
| 1. List all VMs across namespaces | |
| 2. Verify total count matches expected | |
| Expected: | |
| - Summary component shows the correct total VM count | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-011] should display the correct VM count in the summary component", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| }) | |
| Context("Single-project view", func() { | |
| /* | |
| Preconditions: | |
| - At least 2 VMs in a single namespace with different IPs | |
| Steps: | |
| 1. List VMIs in a single namespace | |
| 2. Filter by target IP | |
| 3. Verify correct VM shown | |
| Expected: | |
| - IP filter returns the correct VM within a single project | |
| */ | |
| PendingIt("[test_id:TS-CNV-72112-012] should return the correct VM when filtering by IP within a single project", func() { | |
| Skip("Phase 1: Design only - awaiting implementation") | |
| }) | |
| PIt("[test_id:TS-CNV-72112-001] should return only the VM matching the IP address filter") | |
| PIt("[test_id:TS-CNV-72112-002] should display empty state when filtering by non-existent IP") | |
| PIt("[test_id:TS-CNV-72112-003] should return VMs matching partial IP address substring") | |
| }) | |
| Context("Regression: existing filters after fix", func() { | |
| PIt("[test_id:TS-CNV-72112-004] should return the correct VM when filtering by name") | |
| PIt("[test_id:TS-CNV-72112-005] should return VMs matching the selected label") | |
| }) | |
| Context("Fallback path: proxy pod not active", func() { | |
| PIt("[test_id:TS-CNV-72112-006] should use frontend-filtered data when proxy pod is not active") | |
| }) | |
| Context("Pagination and selection with filtered data", func() { | |
| PIt("[test_id:TS-CNV-72112-007] should update pagination item count after filtering") | |
| PIt("[test_id:TS-CNV-72112-008] should select only the filtered VMs when select-all is clicked") | |
| PIt("[test_id:TS-CNV-72112-009] should clear all VM selections when deselect-all is clicked") | |
| }) | |
| Context("Empty state and summary component", func() { | |
| PIt("[test_id:TS-CNV-72112-010] should display the empty state component when no VMs exist") | |
| PIt("[test_id:TS-CNV-72112-011] should display the correct VM count in the summary component") | |
| }) | |
| Context("Single-project view", func() { | |
| PIt("[test_id:TS-CNV-72112-012] should return the correct VM when filtering by IP within a single project") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stds/sig-ui/vm_ip_filter_stubs_test.go` around lines 45 - 235, The test stubs
use the invalid Ginkgo v2 identifier PendingIt; replace every occurrence of
PendingIt(...) with PIt(...) (e.g., the pending specs named
"[test_id:TS-CNV-72112-001]" through "[test_id:TS-CNV-72112-012]") so the
pending tests use the correct Ginkgo v2 syntax; update each PendingIt invocation
to PIt keeping the same arguments and Skip(...) body intact.
| | Item | Description | Applicable (Y/N or N/A) | Comments | | ||
| |:-----|:------------|:------------------------|:---------| | ||
| | Functional Testing | Validates that the feature works according to specified requirements and user stories | Y | Core testing of IP address filtering across all projects with proxy pod active. Verify correct VMs are returned and no false matches occur. | | ||
| | Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | Playwright-based UI tests. Existing test at `playwright/tests/tier1/vm-actions.spec.ts` line 98 covers this scenario. | |
There was a problem hiding this comment.
Automation strategy should reflect the actual test implementations in this PR.
Line 82 and Line 114 currently describe Playwright UI automation only, while this PR’s executable coverage is Go/Ginkgo + Python/pytest at API level. Please align this section to avoid traceability drift.
📝 Suggested doc update
-| Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | Playwright-based UI tests. Existing test at `playwright/tests/tier1/vm-actions.spec.ts` line 98 covers this scenario. |
+| Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | API-level automated tests in Go/Ginkgo (Tier 1) and Python/pytest (Tier 2) validate console filtering semantics for this fix scope. |
-| **Test Framework** | Playwright (UI tests, Tier 1), pytest (Tier 2 / Python) |
+| **Test Framework** | Go/Ginkgo (Tier 1 API-level), pytest (Tier 2 API-level) |Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-ui/vm-ip-filter-stp.md` at line 82, Update the "Automation Testing"
table entries that currently state "Playwright-based UI tests. Existing test at
`playwright/tests/tier1/vm-actions.spec.ts`..." to accurately reflect this PR's
executable coverage: replace with a short description like "API-level automated
tests using Go/Ginkgo and Python/pytest; see relevant Ginkgo test(s) and pytest
modules covering the VM IP filter scenarios" and update any test file references
to the actual Go/Ginkgo and Python/pytest test files in this PR; apply the same
replacement for both occurrences currently mentioning Playwright (the row at
line ~82 and the similar row at ~114). Ensure the text clearly indicates
API-level coverage and points to the concrete test suites used in this PR.
Comment on lines
+83
to
+87
| interfaces = vmi.instance.status.interfaces or [] | ||
| for iface in interfaces: | ||
| ip_addresses = iface.get("ipAddresses", []) | ||
| if any(target_ip in addr for addr in ip_addresses): | ||
| matched.append(vmi) |
There was a problem hiding this comment.
Use exact IP equality in get_vmis_matching_ip to avoid false positives.
Line 86 currently does substring matching. That can over-match and make exact-IP tests flaky or incorrect.
🔧 Proposed fix
for vmi in all_vmis:
interfaces = vmi.instance.status.interfaces or []
for iface in interfaces:
ip_addresses = iface.get("ipAddresses", [])
- if any(target_ip in addr for addr in ip_addresses):
+ primary_ip = iface.get("ip")
+ if primary_ip == target_ip or any(addr == target_ip for addr in ip_addresses):
matched.append(vmi)
break📝 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.
Suggested change
| interfaces = vmi.instance.status.interfaces or [] | |
| for iface in interfaces: | |
| ip_addresses = iface.get("ipAddresses", []) | |
| if any(target_ip in addr for addr in ip_addresses): | |
| matched.append(vmi) | |
| interfaces = vmi.instance.status.interfaces or [] | |
| for iface in interfaces: | |
| ip_addresses = iface.get("ipAddresses", []) | |
| primary_ip = iface.get("ip") | |
| if primary_ip == target_ip or any(addr == target_ip for addr in ip_addresses): | |
| matched.append(vmi) | |
| break |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sig-ui/conftest.py` around lines 83 - 87, get_vmis_matching_ip is using
substring matching (any(target_ip in addr ...)) which can produce false
positives; change the check to exact equality against each address (e.g.,
compare target_ip == addr for addr in ip_addresses), and ensure you handle
None/empty entries consistently (strip/normalize if needed) when iterating over
iface.get("ipAddresses", []) in the loop that looks up interfaces and appends
matched vmi.
Comment on lines
+109
to
+111
| LOGGER.info( | ||
| f"Verifying decoy VMs are not returned in IP filter results" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the file exists and read lines 109-111
if [ -f "tests/sig-ui/test_vm_ip_filter_e2e.py" ]; then
echo "=== File exists ==="
echo "=== Lines 109-111 ==="
sed -n '109,111p' tests/sig-ui/test_vm_ip_filter_e2e.py | cat -n
else
echo "File not found"
fiRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Run Ruff check to verify F541 finding
if [ -f "tests/sig-ui/test_vm_ip_filter_e2e.py" ]; then
echo "=== Ruff F541 check ==="
ruff check tests/sig-ui/test_vm_ip_filter_e2e.py --select F541 2>&1
else
echo "File not found for Ruff check"
fiRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 762
Remove the unnecessary f prefix to satisfy Ruff F541.
Line 110 is an f-string without interpolation and should have the f prefix removed.
🔧 Proposed fix
- LOGGER.info(
- f"Verifying decoy VMs are not returned in IP filter results"
- )
+ LOGGER.info(
+ "Verifying decoy VMs are not returned in IP filter results"
+ )📝 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.
Suggested change
| LOGGER.info( | |
| f"Verifying decoy VMs are not returned in IP filter results" | |
| ) | |
| LOGGER.info( | |
| "Verifying decoy VMs are not returned in IP filter results" | |
| ) |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 110-110: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sig-ui/test_vm_ip_filter_e2e.py` around lines 109 - 111, The
LOGGER.info call is using an unnecessary f-string (Ruff F541); change the
LOGGER.info invocation that currently has f"Verifying decoy VMs are not returned
in IP filter results" to a plain string literal by removing the leading "f" so
it becomes "Verifying decoy VMs are not returned in IP filter results" (locate
the LOGGER.info(...) statement in test_vm_ip_filter_e2e.py).
Comment on lines
+217
to
+223
| matched_after_delete = get_vmis_matching_ip( | ||
| admin_client=admin_client, target_ip=vm_ip_str | ||
| ) | ||
| vm_still_present = any( | ||
| vmi.name == vm_name or vmi.namespace == namespace.name | ||
| for vmi in matched_after_delete | ||
| ) |
There was a problem hiding this comment.
Post-delete assertion is race-prone and can false-fail.
The check runs immediately after deletion and uses name OR namespace, which can flag unrelated VMIs as stale results.
🔧 Proposed fix
+import time
@@
- matched_after_delete = get_vmis_matching_ip(
- admin_client=admin_client, target_ip=vm_ip_str
- )
- vm_still_present = any(
- vmi.name == vm_name or vmi.namespace == namespace.name
- for vmi in matched_after_delete
- )
+ vm_still_present = True
+ for _ in range(30):
+ matched_after_delete = get_vmis_matching_ip(
+ admin_client=admin_client, target_ip=vm_ip_str
+ )
+ vm_still_present = any(
+ vmi.name == vm_name and vmi.namespace == namespace.name
+ for vmi in matched_after_delete
+ )
+ if not vm_still_present:
+ break
+ time.sleep(2)
assert not vm_still_present, (
f"Deleted VM {vm_name} still appears in IP filter results "
f"for {vm_ip_str}. Stale data detected."
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sig-ui/test_vm_ip_filter_e2e.py` around lines 217 - 223, The current
immediate post-delete check uses get_vmis_matching_ip and sets vm_still_present
by matching vmi.name == vm_name OR vmi.namespace == namespace.name which is
race-prone and can match unrelated VMIs; change this to poll with a
timeout/retry (e.g., exponential backoff or test helper wait_until) after
deletion and consider a VMI present only if both vmi.name == vm_name AND
vmi.namespace == namespace.name, i.e., replace the OR with AND and loop until
get_vmis_matching_ip returns no VMI that matches both vm_name and namespace.name
or until timeout, failing the test only after the timeout elapses.
Comment on lines
+45
to
+56
| for _, iface := range vmi.Status.Interfaces { | ||
| if iface.IP == targetIP { | ||
| matched = append(matched, vmi) | ||
| break | ||
| } | ||
| for _, ip := range iface.IPs { | ||
| if ip == targetIP { | ||
| matched = append(matched, vmi) | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
filterVMIsByIP can append the same VMI more than once.
When a match is found in iface.IPs, the break only exits the IP loop. The next interface can append the same VMI again.
🔧 Proposed fix
func filterVMIsByIP(vmis []v1.VirtualMachineInstance, targetIP string) []v1.VirtualMachineInstance {
var matched []v1.VirtualMachineInstance
for _, vmi := range vmis {
+ found := false
for _, iface := range vmi.Status.Interfaces {
if iface.IP == targetIP {
- matched = append(matched, vmi)
- break
+ found = true
+ break
}
for _, ip := range iface.IPs {
if ip == targetIP {
- matched = append(matched, vmi)
+ found = true
break
}
}
+ if found {
+ break
+ }
+ }
+ if found {
+ matched = append(matched, vmi)
}
}
return matched
}📝 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.
Suggested change
| for _, iface := range vmi.Status.Interfaces { | |
| if iface.IP == targetIP { | |
| matched = append(matched, vmi) | |
| break | |
| } | |
| for _, ip := range iface.IPs { | |
| if ip == targetIP { | |
| matched = append(matched, vmi) | |
| break | |
| } | |
| } | |
| } | |
| func filterVMIsByIP(vmis []v1.VirtualMachineInstance, targetIP string) []v1.VirtualMachineInstance { | |
| var matched []v1.VirtualMachineInstance | |
| for _, vmi := range vmis { | |
| found := false | |
| for _, iface := range vmi.Status.Interfaces { | |
| if iface.IP == targetIP { | |
| found = true | |
| break | |
| } | |
| for _, ip := range iface.IPs { | |
| if ip == targetIP { | |
| found = true | |
| break | |
| } | |
| } | |
| if found { | |
| break | |
| } | |
| } | |
| if found { | |
| matched = append(matched, vmi) | |
| } | |
| } | |
| return matched | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sig-ui/vm_ip_filter_test.go` around lines 45 - 56, filterVMIsByIP can
append the same VMI multiple times because breaking from the inner loop only
exits the IP loop; update the logic in filterVMIsByIP to ensure each vmi is
appended at most once by using a found flag (e.g., matchedFlag) when you detect
a match in either iface.IP or iface.IPs, append vmi once and then break out of
the outer loop over vmi.Status.Interfaces (or return/continue to next vmi) so
subsequent interfaces cannot append the same vmi again.
Comment on lines
+389
to
+399
| if err != nil { | ||
| // Namespace may not exist, which effectively means empty state | ||
| By("Namespace does not exist, which is equivalent to empty VM list") | ||
| return | ||
| } | ||
|
|
||
| vmList, err := virtClient.VirtualMachine(emptyNamespace).List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| By("Namespace does not exist for VM list, empty state applies") | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/sig-ui/vm_ip_filter_test.go | sed -n '380,410p'Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 1614
🏁 Script executed:
head -20 tests/sig-ui/vm_ip_filter_test.goRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 531
🏁 Script executed:
head -35 tests/sig-ui/vm_ip_filter_test.goRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 1000
🏁 Script executed:
grep -n "apierrors\|IsNotFound" tests/sig-ui/vm_ip_filter_test.goRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 77
🏁 Script executed:
grep -n "GetTestNamespace\|emptyNamespace\|namespace.Create\|namespace.Install" tests/sig-ui/vm_ip_filter_test.go | head -20Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 420
🏁 Script executed:
rg "apierrors\." tests/ -l | head -10Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 77
🏁 Script executed:
rg "IsNotFound|IsNotPermitted|StatusError" tests/sig-ui/ -n | head -20Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 77
🏁 Script executed:
rg "apierrors\." . --type go -l | head -10Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 77
🏁 Script executed:
rg "if err != nil" tests/sig-ui/vm_ip_filter_test.go -B2 -A2 | head -50Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 612
🏁 Script executed:
wc -l tests/sig-ui/vm_ip_filter_test.goRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 132
🏁 Script executed:
cat -n tests/sig-ui/vm_ip_filter_test.go | tail -50Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 2322
🏁 Script executed:
python3 << 'EOF'
# Check if the namespace is expected to exist
import re
with open('tests/sig-ui/vm_ip_filter_test.go', 'r') as f:
content = f.read()
# Look for namespace creation or setup
setup_section = content[content.find('Context("Empty state'):content.find('It("[test_id:TS-CNV-72112-010]') + 100]
print("=== Context Setup ===")
print(setup_section)
# Check if there's any cleanup or deletion
cleanup = re.findall(r'DeferCleanup|BeforeEach|AfterEach', content)
print(f"\n=== Cleanup markers found: {cleanup}")
EOFRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 355
🏁 Script executed:
rg "\.List\(" tests/sig-ui/vm_ip_filter_test.go -B2 -A3Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 5291
Distinguish namespace-not-found errors from other API failures.
Lines 388-392 and 395-399 treat all errors identically by returning early, which masks real regressions (RBAC, transport, API failures) as passing empty-state tests. Every other .List() call in this file uses Expect(err).ToNot(HaveOccurred()).
Add the import:
apierrors "k8s.io/apimachinery/pkg/api/errors"Then check for NotFound explicitly before returning:
vmiList, err := virtClient.VirtualMachineInstance(emptyNamespace).List(ctx, metav1.ListOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
By("Namespace does not exist, which is equivalent to empty VM list")
return
}
Expect(err).ToNot(HaveOccurred())
}
vmList, err := virtClient.VirtualMachine(emptyNamespace).List(ctx, metav1.ListOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
By("Namespace does not exist for VM list, empty state applies")
return
}
Expect(err).ToNot(HaveOccurred())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/sig-ui/vm_ip_filter_test.go` around lines 389 - 399, The current error
handling for virtClient.VirtualMachineInstance(...).List and
virtClient.VirtualMachine(...).List treats every error as "namespace missing"
and returns, hiding real failures; add the import alias apierrors
"k8s.io/apimachinery/pkg/api/errors" and change each err check to explicitly
test apierrors.IsNotFound(err) and return only in that case, otherwise call
Expect(err).ToNot(HaveOccurred()): update the error branches around
VirtualMachineInstance(...).List and VirtualMachine(...).List to use
apierrors.IsNotFound(err) before returning so RBAC/transport/API errors surface
via Expect.
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
3 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.
STP Metadata
Jira issue: CNV-72112
PR: kubevirt-ui/kubevirt-plugin#3164
SIG: sig-ui
What this PR does
Adds complete test planning and implementation artifacts for CNV-72112 (VM IP Address Filtering in OCP Console Across All Projects):
STP (
stps/sig-ui/vm-ip-filter-stp.md):STD Stubs (
stds/sig-ui/):Working Tests (
tests/sig-ui/):vm_ip_filter_test.go- 12 Tier 1 Go/Ginkgo tests using kubevirt/kubevirt patterns (Ordered context, BeforeAll/AfterAll, cross-namespace VM setup)test_vm_ip_filter_e2e.py+conftest.py- 3 Tier 2 Python/pytest tests using openshift-python-wrapper patterns (VirtualMachineForTests, context managers, cross-namespace IP search)Test Coverage
Special notes for your reviewer
matchedVMSto filter fromfilteredDatainstead of rawvmsinVirtualMachinesList.tsxSummary by CodeRabbit