HYPERFLEET-859 - docs: design E2E test cases for Update and Delete lifecycle#71
HYPERFLEET-859 - docs: design E2E test cases for Update and Delete lifecycle#71tzhou5 wants to merge 2 commits intoopenshift-hyperfleet:mainfrom
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 |
WalkthroughUpdated test specifications expand cluster and nodepool end-to-end lifecycle tests. Cluster tests now validate create → PATCH update → DELETE flows: assert Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-design/testcases/cluster.md`:
- Around line 121-123: Replace the incorrect `Reconciled` condition references
in the three bullet lines with `Ready` so the documentation matches the actual
tests: change "Cluster `Reconciled` condition transitions..." to "Cluster
`Ready` condition transitions...", the final condition line to reference
`{"type": "Ready"}` alongside `{"type": "Available"}`, and the
observedGeneration validation to check Ready and Available instead of
Reconciled; this should align the markdown with the test implementation (see
creation.go) and other references to `Ready` and `Available`.
🪄 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: Pro
Run ID: 8f9566e1-3f94-42c7-ada3-7d21eb0b1e2d
📒 Files selected for processing (2)
test-design/testcases/cluster.mdtest-design/testcases/nodepool.md
| - Cluster `Reconciled` condition transitions from `status: False` to `status: True` | ||
| - Final cluster conditions have `status: True` for both condition `{"type": "Reconciled"}` and `{"type": "Available"}` | ||
| - Validate that the observedGeneration for the Reconciled and Available conditions is 1 for a new creation request |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for condition name usage patterns across the codebase
echo "=== Checking API/model definitions for condition types ==="
rg -n --type=go -C3 '\b(Ready|Available|Reconciled)\b.*[Cc]ondition'
echo -e "\n=== Checking test implementations for condition assertions ==="
rg -n --type=go -C3 'condition.*type.*==.*"(Ready|Available|Reconciled)"'
echo -e "\n=== Checking OpenAPI/proto specs for condition definitions ==="
fd -e yaml -e proto -e json | xargs rg -l 'condition' | xargs rg -n -C3 '"(Ready|Available|Reconciled)"'Repository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 50391
Change lines 121-123 to reference Ready condition instead of Reconciled.
Lines 121-123 check for a Reconciled condition that does not exist in the API. The actual implementation (e2e/cluster/creation.go) validates Ready and Available conditions only. This is also inconsistent with earlier lines in the same file (79-80, 104, 115-118) which correctly reference Ready and Available. Update to match the actual system behavior:
- Cluster `Ready` condition transitions from `status: False` to `status: True`
- Final cluster conditions have `status: True` for both condition `{"type": "Ready"}` and `{"type": "Available"}`
- Validate that the observedGeneration for the Ready and Available conditions is 1 for a new creation request
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-design/testcases/cluster.md` around lines 121 - 123, Replace the
incorrect `Reconciled` condition references in the three bullet lines with
`Ready` so the documentation matches the actual tests: change "Cluster
`Reconciled` condition transitions..." to "Cluster `Ready` condition
transitions...", the final condition line to reference `{"type": "Ready"}`
alongside `{"type": "Available"}`, and the observedGeneration validation to
check Ready and Available instead of Reconciled; this should align the markdown
with the test implementation (see creation.go) and other references to `Ready`
and `Available`.
There was a problem hiding this comment.
The Reconciled condition in Steps 4-8 is intentional. This test case was extended to cover the full Create → Update → Delete lifecycle (Post-MVP scope), which is based on the HYPERFLEET-560 DDR where Ready is replaced by Reconciled. Steps 1-3 retain the original Ready references as they reflect the current automated implementation. The new steps use Reconciled to align with the DDR design — the actual condition name will be confirmed when the implementation lands (HYPERFLEET-853).
There was a problem hiding this comment.
@tzhou5, understood — thank you for the context. The intentional split makes sense: Steps 1–3 document the current Ready/Available behavior, while Steps 4–8 use Reconciled to align with the Post-MVP DDR design from HYPERFLEET-560, pending confirmation in HYPERFLEET-853. I'll disregard this comment.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-design/testcases/cluster.md`:
- Around line 193-196: The AfterEach cleanup text only mentions deleting the
namespace, which misses other artifacts and diverges from the implemented
multi-stage cleanup in CleanupTestCluster; update the AfterEach sections to
describe and invoke the same multi-stage cleanup contract used by
CleanupTestCluster (see pkg/helper/helper.go and its usage in
e2e/cluster/creation.go) so that helper-level cleanup removes Maestro resource
bundles and other non-namespace artifacts first and only falls back to `kubectl
delete namespace {cluster_id} --ignore-not-found` as the last-resort step.
- Around line 1436-1437: The Concurrent PATCH test's acceptance criteria are
inconsistent (description allows "succeed or reasonable conflict error" but
later requires all PATCH calls to succeed); update the test text and assertions
so they match: change acceptance to permit 2xx or 409 responses (no 5xx), and
replace the "all succeed" assertion with a convergence check that verifies the
final cluster state matches the latest generation/observed_generation; update
the narrative and the assertions in the Concurrent PATCH test block (references:
the Concurrent PATCH test description and the assertions currently forcing all
PATCH calls to succeed) so both the allowed response set and the final-state
verification are consistent.
🪄 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: Pro
Run ID: 19adff51-81cb-41ed-9455-f7074e2ddf82
📒 Files selected for processing (1)
test-design/testcases/cluster.md
| - If the test fails before hard-delete completes, clean up remaining resources: | ||
| ```bash | ||
| kubectl delete namespace {cluster_id} --ignore-not-found | ||
| ``` |
There was a problem hiding this comment.
Fallback cleanup is under-specified and risks leaked resources
Line 195/326/1223 currently documents namespace-only cleanup. That misses non-namespace artifacts (for example Maestro resource bundles) and diverges from the implemented cleanup flow (pkg/helper/helper.go:59-90, used in e2e/cluster/creation.go:387-396). Please update these AfterEach sections to use the same multi-stage cleanup contract as CleanupTestCluster, with namespace deletion as a last-resort fallback only.
As per coding guidelines, "**: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 324-327, 1221-1224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-design/testcases/cluster.md` around lines 193 - 196, The AfterEach
cleanup text only mentions deleting the namespace, which misses other artifacts
and diverges from the implemented multi-stage cleanup in CleanupTestCluster;
update the AfterEach sections to describe and invoke the same multi-stage
cleanup contract used by CleanupTestCluster (see pkg/helper/helper.go and its
usage in e2e/cluster/creation.go) so that helper-level cleanup removes Maestro
resource bundles and other non-namespace artifacts first and only falls back to
`kubectl delete namespace {cluster_id} --ignore-not-found` as the last-resort
step.
| This test validates that the system handles concurrent PATCH requests to the same cluster without errors or inconsistent state. Multiple simultaneous updates should each succeed or return a reasonable conflict error, and the final state should be consistent with the latest generation. | ||
|
|
There was a problem hiding this comment.
Concurrent PATCH acceptance criteria are contradictory
The description allows “succeed or reasonable conflict error,” but Line 1469 requires all PATCH calls to succeed. Please make expected results consistent (e.g., accept 2xx/409 with no 5xx, and assert final state convergence by generation/observed_generation).
As per coding guidelines, "**: -Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 1469-1471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-design/testcases/cluster.md` around lines 1436 - 1437, The Concurrent
PATCH test's acceptance criteria are inconsistent (description allows "succeed
or reasonable conflict error" but later requires all PATCH calls to succeed);
update the test text and assertions so they match: change acceptance to permit
2xx or 409 responses (no 5xx), and replace the "all succeed" assertion with a
convergence check that verifies the final cluster state matches the latest
generation/observed_generation; update the narrative and the assertions in the
Concurrent PATCH test block (references: the Concurrent PATCH test description
and the assertions currently forcing all PATCH calls to succeed) so both the
allowed response set and the final-state verification are consistent.
1 participant
Summary
Extend cluster TC1 (Basic Workflow) to full lifecycle: Create → Update → Delete
Extend cluster TC2 (K8s Resources Check) with deletion cleanup verification
Add 13 new cluster test cases for Update/Delete workflows (Tier0: 4, Tier1: 9)
Add 2 nodepool smoke tests for UPDATE and DELETE happy paths
Test cases based on HYPERFLEET-560 DDR (adapter deletion flow design)
HYPERFLEET-859
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)Summary by CodeRabbit