hyperfleet-693 | test: add and automate maestro negative test cases#54
Open
yingzhanredhat wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Open
hyperfleet-693 | test: add and automate maestro negative test cases#54yingzhanredhat wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
yingzhanredhat wants to merge 1 commit 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 |
WalkthroughAdds negative end-to-end Maestro-transport adapter tests and expands test fixtures; replaces suite-level shared namespace/resource identifiers with per-test scoped names; centralizes and changes cleanup to use Helper.CleanupTestCluster which deletes Maestro resource bundles then Kubernetes namespaces by prefix; adds Kubernetes client methods for namespace discovery/deletion and Maestro client methods to list resource bundles by cluster ID or adapter name; extends adapter deployment options to accept extra Helm --set values; and introduces multiple adapter test configurations and task manifests for negative scenarios. Sequence Diagram(s)sequenceDiagram
participant Test as Test Harness
participant Adapter as Adapter (deployed per-test)
participant Maestro as Maestro API
participant K8s as Kubernetes API
participant HyperFleet as HyperFleet API
Test->>Adapter: Deploy with unique release + ADAPTER_NAME env
Test->>Maestro: Create target cluster / consumer
Adapter->>Maestro: Attempt to apply ManifestWork / query consumer
Maestro-->>Adapter: Consumer not found / error
Adapter->>K8s: ManifestWork apply fails (no namespace created)
Adapter->>HyperFleet: POST status (Applied=False, Health=False)
HyperFleet-->>Adapter: Records failure
Test->>Maestro: Query resource bundles by clusterID/adapterName
Maestro-->>Test: Verify no resource bundle created for cluster
Test->>K8s: Verify no namespace/resources exist
sequenceDiagram
participant Test as Test Harness
participant Adapter as Adapter (deployed per-test)
participant Maestro as Maestro API
participant K8s as Kubernetes API
participant HyperFleet as HyperFleet API
Test->>Adapter: Deploy adapter with per-test values
Test->>Maestro: Create ManifestWork (correct name)
Adapter->>K8s: Create ManifestWork (succeeds)
Adapter->>K8s: Discover by wrong name (intentional)
K8s-->>Adapter: Not found
Adapter->>HyperFleet: POST status (Applied=False, Available=False, Health=False)
Test->>K8s: Confirm ManifestWork exists with correct name
Test->>Helper: CleanupTestCluster
Helper->>Maestro: FindAllResourceBundlesByClusterID -> delete bundles
Helper->>K8s: FindNamespacesByPrefix -> DeleteNamespaceAndWait for each
K8s-->>Helper: Confirm namespaces removed
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)
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
🧹 Nitpick comments (1)
pkg/client/kubernetes/client.go (1)
121-136: Consider usingstrings.HasPrefixfor clarity.The prefix matching logic is correct, but using
strings.HasPrefix(ns.Name, prefix)would be more idiomatic Go.♻️ Optional refactor
+import "strings" + func (c *Client) FindNamespacesByPrefix(ctx context.Context, prefix string) ([]string, error) { namespaces, err := c.CoreV1().Namespaces().List(ctx, metav1.ListOptions{}) if err != nil { return nil, fmt.Errorf("failed to list namespaces: %w", err) } var matchingNamespaces []string for _, ns := range namespaces.Items { - if len(ns.Name) >= len(prefix) && ns.Name[:len(prefix)] == prefix { + if strings.HasPrefix(ns.Name, prefix) { matchingNamespaces = append(matchingNamespaces, ns.Name) } } return matchingNamespaces, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/kubernetes/client.go` around lines 121 - 136, Replace the manual prefix check in FindNamespacesByPrefix with strings.HasPrefix for clarity and idiomatic Go; import "strings" if not already present, and change the loop condition from comparing lengths and slicing (ns.Name[:len(prefix)] == prefix) to using strings.HasPrefix(ns.Name, prefix) when building matchingNamespaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/adapter/adapter_with_maestro.go`:
- Around line 563-571: The test is calling
GetMaestroClient().FindResourceBundlesByAdapterName which returns global results
and can pick up bundles from other clusters; instead query Maestro for bundles
in the current cluster (e.g. call a cluster-scoped method such as
FindResourceBundlesByClusterID(ctx, clusterID) or otherwise pass clusterID to
the query) and then filter those results for the adapter label (adapterName)
before asserting empty/specific contents; update the checks around
FindResourceBundlesByAdapterName (and the other occurrences mentioned) to use
the cluster-scoped query and then narrow by adapterName/maestro.io/source-id
prior to the g.Expect assertions.
- Around line 468-493: The current ginkgo.DeferCleanup registrations call
UninstallAdapter first then CleanupTestCluster, causing LIFO cleanup to run
cluster cleanup while the adapter is still running; fix by registering the
cleanup callbacks in the opposite order (register CleanupTestCluster before
UninstallAdapter) or combine into a single DeferCleanup that calls
UninstallAdapter(releaseName, ...) first then CleanupTestCluster(clusterID,
...); update the blocks that reference UninstallAdapter, CleanupTestCluster,
ginkgo.DeferCleanup, releaseName and clusterID (also apply same change to the
other occurrences noted) so the adapter is uninstalled before the cluster is
cleaned up.
In `@pkg/helper/helper.go`:
- Around line 71-83: The Maestro cleanup currently swallows errors from
maestroClient.FindAllResourceBundlesByClusterID and
maestroClient.DeleteResourceBundle inside CleanupTestCluster so stale Maestro
state can be left behind; modify CleanupTestCluster to collect any errors from
FindAllResourceBundlesByClusterID and each DeleteResourceBundle (e.g., append to
a slice or use errors.Join), continue attempting all deletions, and after the
namespace cleanup phase return a combined error if any Maestro-related errors
were collected. Reference the maestroClient.FindAllResourceBundlesByClusterID
and maestroClient.DeleteResourceBundle calls and ensure the final return from
CleanupTestCluster returns the accumulated Maestro error(s) (using errors.Join
or a wrapped fmt.Errorf) instead of nil.
In `@test-design/testcases/adapter-with-maestro-transport.md`:
- Around line 1329-1331: The "Condition validation" header in test Step 7 is
empty; update the "Condition validation" section to enumerate the exact expected
conditions for Step 7 (e.g., expected transport messages, adapter responses,
status codes, and any timeout/retry behavior) and reference the test step by
name ("Step 7") so readers can map conditions to the step; include concrete
assertions such as expected payload fields, message counts, success/failure
flags, and acceptable time windows to make the test pass/fail criteria
unambiguous.
- Around line 935-936: The Helm install command in the diff uses the wrong
values file for this test: update the helm command that invokes helm install
{release_name} {adapter_charts_folder} ... -f
testdata/adapter-configs/cl-maestro-unregistered-consumer/values.yaml to point
to the correct scenario file
testdata/adapter-configs/cl-maestro-wrong-discovery/values.yaml so the "wrong
discovery" test loads the appropriate config; ensure any references to
cl-maestro-unregistered-consumer in the same test block are replaced with
cl-maestro-wrong-discovery.
- Around line 1547-1557: The "Step 9: Cleanup" heading and following fenced code
block are incorrect: close the open code fence after the kubectl delete ns
${CLUSTER_ID}-${ADAPTER_NAME}-namespace line by adding ``` to properly terminate
the bash block, and fix the step numbering gap by renaming "Step 9: Cleanup" to
"Step 7: Cleanup" (or insert explicit placeholder steps for 7 & 8 if intended)
so the sequence follows from Step 6; update the heading text "Step 9: Cleanup"
and ensure the fenced block containing the two commands (the kubectl exec ...
curl -s -X DELETE ... and kubectl delete ns ...) is properly closed before the
note.
---
Nitpick comments:
In `@pkg/client/kubernetes/client.go`:
- Around line 121-136: Replace the manual prefix check in FindNamespacesByPrefix
with strings.HasPrefix for clarity and idiomatic Go; import "strings" if not
already present, and change the loop condition from comparing lengths and
slicing (ns.Name[:len(prefix)] == prefix) to using strings.HasPrefix(ns.Name,
prefix) when building matchingNamespaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f0c40d8-b774-4ea9-8946-ab4882ebd048
📒 Files selected for processing (18)
e2e/adapter/adapter_with_maestro.gopkg/client/kubernetes/client.gopkg/client/maestro/client.gopkg/helper/adapter.gopkg/helper/helper.gotest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-m-bad-api/adapter-config.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/values.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/values.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/values.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/values.yaml
yingzhanredhat
force-pushed
the
hyperfleet-693
branch
from
b85925e to
1243262
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test-design/testcases/adapter-with-maestro-transport.md (3)
935-936:⚠️ Potential issue | 🟡 MinorIncorrect config path reference remains unaddressed.
This test is for "Main discovery fails when ManifestWork name is wrong" but still references the wrong config file. Should be
cl-maestro-wrong-discoverynotcl-maestro-unregistered-consumer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-with-maestro-transport.md` around lines 935 - 936, The helm install command in the test uses the wrong values file: update the -f path in the helm install invocation so it points to testdata/adapter-configs/cl-maestro-wrong-discovery/values.yaml (instead of cl-maestro-unregistered-consumer) to match the "Main discovery fails when ManifestWork name is wrong" test; ensure the rest of the command (release_name, adapter_charts_folder, namespace_name) remains unchanged so the test uses the correct configuration set.
1329-1331:⚠️ Potential issue | 🟡 MinorIncomplete condition validation section remains unaddressed.
The "Condition validation" header for Step 7 is still empty with no actual conditions listed, leaving expected results unclear.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-with-maestro-transport.md` around lines 1329 - 1331, Step 7's "Condition validation" header in test-design/testcases/adapter-with-maestro-transport.md is empty; update the Step 7 section (look for the "Step 7" block and the "Condition validation" header) to list the specific preconditions and expected postconditions for the step (e.g., required inputs, transport state, expected adapter responses, error cases and pass/fail criteria). Ensure each condition is concise and testable (use bullet points or numbered items) and clearly ties to the step actions and expected results already described in the Step 7 "Actions" and "Expected results" sections.
1547-1557:⚠️ Potential issue | 🟡 MinorStep numbering gap and unclosed code block remain unaddressed.
Step numbering jumps from "Step 6" to "Step 9" (missing Steps 7 & 8), and the code block starting at line 1549 is not properly closed before the
> **Note:**block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-design/testcases/adapter-with-maestro-transport.md` around lines 1547 - 1557, Rename or renumber the steps so the sequence is continuous (fix "Step 9: Cleanup" to follow the previous step numbering, e.g., rename to "Step 7" or insert missing "Step 7" and "Step 8" content), and close the open fenced code block that begins with the kubectl exec example (the block starting at the kubectl exec line under "Step 9: Cleanup") by adding the missing trailing ``` before the `> **Note:**` paragraph; ensure the heading text "Step 9: Cleanup" and the fenced code block are updated together so the markdown renders correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test-design/testcases/adapter-with-maestro-transport.md`:
- Around line 935-936: The helm install command in the test uses the wrong
values file: update the -f path in the helm install invocation so it points to
testdata/adapter-configs/cl-maestro-wrong-discovery/values.yaml (instead of
cl-maestro-unregistered-consumer) to match the "Main discovery fails when
ManifestWork name is wrong" test; ensure the rest of the command (release_name,
adapter_charts_folder, namespace_name) remains unchanged so the test uses the
correct configuration set.
- Around line 1329-1331: Step 7's "Condition validation" header in
test-design/testcases/adapter-with-maestro-transport.md is empty; update the
Step 7 section (look for the "Step 7" block and the "Condition validation"
header) to list the specific preconditions and expected postconditions for the
step (e.g., required inputs, transport state, expected adapter responses, error
cases and pass/fail criteria). Ensure each condition is concise and testable
(use bullet points or numbered items) and clearly ties to the step actions and
expected results already described in the Step 7 "Actions" and "Expected
results" sections.
- Around line 1547-1557: Rename or renumber the steps so the sequence is
continuous (fix "Step 9: Cleanup" to follow the previous step numbering, e.g.,
rename to "Step 7" or insert missing "Step 7" and "Step 8" content), and close
the open fenced code block that begins with the kubectl exec example (the block
starting at the kubectl exec line under "Step 9: Cleanup") by adding the missing
trailing ``` before the `> **Note:**` paragraph; ensure the heading text "Step
9: Cleanup" and the fenced code block are updated together so the markdown
renders correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f075af3-4ed9-4a81-a118-b718d6e99271
📒 Files selected for processing (18)
e2e/adapter/adapter_with_maestro.gopkg/client/kubernetes/client.gopkg/client/maestro/client.gopkg/helper/adapter.gopkg/helper/helper.gotest-design/testcases/adapter-with-maestro-transport.mdtestdata/adapter-configs/cl-m-bad-api/adapter-config.yamltestdata/adapter-configs/cl-m-bad-api/adapter-task-config.yamltestdata/adapter-configs/cl-m-bad-api/values.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yamltestdata/adapter-configs/cl-m-unreg-consumer/values.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-ds/values.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-config.yamltestdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yamltestdata/adapter-configs/cl-m-wrong-nest/values.yaml
✅ Files skipped from review due to trivial changes (11)
- testdata/adapter-configs/cl-m-wrong-nest/adapter-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/values.yaml
- testdata/adapter-configs/cl-m-bad-api/values.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/values.yaml
- testdata/adapter-configs/cl-m-wrong-nest/values.yaml
- testdata/adapter-configs/cl-m-bad-api/adapter-config.yaml
- testdata/adapter-configs/cl-m-wrong-nest/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-unreg-consumer/adapter-task-config.yaml
- testdata/adapter-configs/cl-m-wrong-ds/adapter-task-config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/client/kubernetes/client.go
- pkg/client/maestro/client.go
tzhou5
reviewed
Mar 24, 2026
| 2. Adapter is deployed in Maestro transport mode (`transport.client: "maestro"`) | ||
| 3. Adapter task config is configured to target a consumer named "unregistered-consumer" which does NOT exist in Maestro | ||
| 4. At least one valid Maestro consumer exists for comparison (e.g., `cluster1`) | ||
| 5. **Option 1**: Use the pre-configured adapter config: `testdata/adapter-configs/cl-maestro-unregistered-consumer/` |
Contributor
There was a problem hiding this comment.
The test design doc references config directory names that don't match the actual directories in this PR, this would be cl-m-unreg-consumer
tzhou5
reviewed
Mar 24, 2026
| - name: "placementClusterName" | ||
| expression: "\"unregistered-consumer\"" # Points to non-existent consumer to test apply failure | ||
| # Use helm install cmd to deploy | ||
| helm install {release_name} {adapter_charts_folder} --namespace {namespace_name} --create-namespace -f testdata/adapter-configs/cl-maestro-wrong-discovery/values.yaml |
Contributor
There was a problem hiding this comment.
in the line 736: export ADAPTER_NAME='cl-maestro-unregistered-consumer', while it using the cl-maestro-wrong-discovery here. these are mismatched
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
2 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.
Summary by CodeRabbit
New Features
Tests