-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add v1beta1 E2E test cases (3/) #559
Conversation
It("should place resources on specified clusters", func() { | ||
resourcePlacedActual := workNamespaceAndConfigMapPlacedOnClusterActual(memberCluster1) | ||
Eventually(resourcePlacedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to place resources on specified clusters") | ||
}) | ||
|
||
It("should update CRP status as expected", func() { | ||
crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster1Name}, nil) | ||
Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the check order should be reversed
|
||
// Update the CRP to pick a different cluster. | ||
Expect(hubClient.Get(ctx, types.NamespacedName{Name: crpName}, crp)).To(Succeed(), "Failed to get CRP") | ||
crp.Spec.Policy.ClusterNames = []string{memberCluster2Name} | ||
Expect(hubClient.Update(ctx, crp)).To(Succeed(), "Failed to update CRP") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what logic should be in BeforeAll vs IT? I don't really see a clear pattern. I would get rid of the BeforeAll or only keep it to only the initial setup.
It("should update CRP status as expected", func() { | ||
crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), []string{memberCluster2Name}, nil) | ||
Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected") | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most user will check the CRP status first
ClusterNames: []string{ | ||
memberCluster4Name, | ||
memberCluster5Name, | ||
memberCluster6Name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess some of them are unhealthy cluster, it would be great if their name reflect that to improve the test readability.
Description of your changes
This PR adds more E2E tests, specifically concerning placement using CRPs of the PickFixed placement type.
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
N/A
Special notes for your reviewer
Part of this PR overlaps with #558, which would go away after 558 is merged + this PR is rebased.