-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat: Ginkgo tests for Leave workflow Hub agent #108
Conversation
Codecov upload limit reached
|
pkg/controllers/membercluster/membercluster_controller_integration_test.go
Outdated
Show resolved
Hide resolved
219b9d4
to
def99ee
Compare
result, err := r.Reconcile(ctx, ctrl.Request{ | ||
NamespacedName: memberClusterNamespacedName, | ||
}) | ||
Expect(result).Should(Equal(ctrl.Result{})) | ||
Expect(err).Should(Not(HaveOccurred())) | ||
|
||
By("simulate member agent updating internal member cluster status") | ||
var imc fleetv1alpha1.InternalMemberCluster | ||
Expect(k8sClient.Get(ctx, types.NamespacedName{Name: memberClusterName, Namespace: namespaceName}, &imc)).Should(Succeed()) | ||
|
||
imc.Status.Capacity = utils.NewResourceList() | ||
imc.Status.Allocatable = utils.NewResourceList() | ||
joinedCondition := metav1.Condition{ | ||
Type: fleetv1alpha1.ConditionTypeInternalMemberClusterJoin, | ||
Status: metav1.ConditionTrue, | ||
Reason: reasonMemberClusterJoined, | ||
ObservedGeneration: imc.GetGeneration(), | ||
} | ||
heartBeatReceivedCondition := metav1.Condition{ | ||
Type: fleetv1alpha1.ConditionTypeInternalMemberClusterHeartbeat, | ||
Status: metav1.ConditionTrue, | ||
Reason: "InternalMemberClusterHeartbeatReceived", | ||
ObservedGeneration: imc.GetGeneration(), | ||
} | ||
imc.SetConditions(joinedCondition, heartBeatReceivedCondition) | ||
Expect(k8sClient.Status().Update(ctx, &imc)).Should(Succeed()) | ||
|
||
By("trigger reconcile again to update member cluster status") | ||
result, err = r.Reconcile(ctx, ctrl.Request{ | ||
NamespacedName: memberClusterNamespacedName, | ||
}) | ||
Expect(result).Should(Equal(ctrl.Result{})) | ||
Expect(err).Should(Not(HaveOccurred())) | ||
|
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.
IIRC, this is the join flow, right? Is there any way to move this to a beforeEach block
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.
Move to beforeEach
Expect(result).Should(Equal(ctrl.Result{})) | ||
Expect(err).Should(Not(HaveOccurred())) | ||
|
||
Expect(k8sClient.Get(ctx, memberClusterNamespacedName, &mc)).Should(Succeed()) |
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.
what about the namespace/role/imc?
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.
Since we delete the namespace to remove them and gingko's test env doesn't delete it they will all remain kubernetes-sigs/controller-runtime#880 I saw no point in checking if they don't get deleted
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 see. So it's not possible to test this in the integration test, right? Can we make sure it's tested in the E2E?
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 role and imc being deleted is not being checked in the e2e test will add a PR to check that
Expect(result).Should(Equal(ctrl.Result{})) | ||
Expect(err).Should(Not(HaveOccurred())) | ||
|
||
Expect(k8sClient.Get(ctx, memberClusterNamespacedName, &mc)).Should(Succeed()) |
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 see. So it's not possible to test this in the integration test, right? Can we make sure it's tested in the E2E?
Description of your changes
Fixes #
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer