add test for pkg/controllers/cluster #5467

xovoxy · 2024-08-30T10:01:41Z

What type of PR is this?
/kind failing-test

What this PR does / why we need it:
add some test for /pkg/controllers/cluster, improve test coverage to 60 above
Which issue(s) this PR fixes:
Parts of ##5235

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2024-08-30T10:13:39Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.33%. Comparing base (136e268) to head (503fb93).
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5467      +/-   ##
==========================================
+ Coverage   31.70%   32.33%   +0.62%     
==========================================
  Files         643      643              
  Lines       44444    44445       +1     
==========================================
+ Hits        14093    14371     +278     
+ Misses      29322    28983     -339     
- Partials     1029     1091      +62

Flag	Coverage Δ
unittests	`32.33% <ø> (+0.62%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anujagrawal699 · 2024-08-30T17:37:04Z

pkg/controllers/cluster/taint_manager_test.go

+			if tt.cluster != nil {
+				if err := tc.Client.Create(context.Background(), tt.cluster, &client.CreateOptions{}); err != nil {
+					t.Fatal(err)
+					return


Can you please add deletion of these resources at the end of the test if possible.

hi, thanks for the review. i cant understand why we need to add the delete operation at the end. my understand is that the obj created by fakeclient should be automatically released after each test case is completed. can you help me understand? thanks~

ohh right, i misunderstood it. the remaining pr looks good to me. Thanks for correction.

anujagrawal699 · 2024-08-31T03:52:00Z

/lgtm

karmada-bot · 2024-08-31T03:52:03Z

@anujagrawal699: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

XiShanYongYe-Chang

Thanks.

XiShanYongYe-Chang · 2024-09-02T03:27:52Z

pkg/controllers/cluster/cluster_controller_test.go

+				}
+			}
+
+			got, err := c.Reconcile(context.Background(), req)


In addition to checking err and result, can you determine whether cluster resources are expected?

Other tests are similar.

xovoxy · 2024-09-04T00:48:02Z

hi @XiShanYongYe-Chang, i have made some changes based on your comment. please check it~

XiShanYongYe-Chang · 2024-09-04T01:20:02Z

Thanks @xovoxy
/assign

XiShanYongYe-Chang · 2024-09-04T01:56:03Z

pkg/controllers/cluster/cluster_controller_test.go

+			wantErr: false,
+		},
+		{
+			name: "cluster with ready condition",


This case run failed on my local side:

=== RUN TestController_monitorClusterHealth === RUN TestController_monitorClusterHealth/cluster_with_ready_condition cluster_controller_test.go:441: Cluster resource get test-cluster, want false --- FAIL: TestController_monitorClusterHealth (0.00s) --- FAIL: TestController_monitorClusterHealth/cluster_with_ready_condition (0.00s) FAIL

hi, i tested this pr locally, and worked fine

and i found an error in the log output and correct it. could you please try again? thanks

ok, let me take a look again.

I update the code to print the detail cluster object:

--- a/pkg/controllers/cluster/cluster_controller_test.go +++ b/pkg/controllers/cluster/cluster_controller_test.go @@ -438,7 +438,7 @@ func TestController_monitorClusterHealth(t *testing.T) { cleanUpCluster(cluster) if !reflect.DeepEqual(cluster, tt.wCluster) { - t.Errorf("Cluster resource get %+v, want %+v", cluster, tt.wCluster) + t.Errorf("Cluster resource get %+v, want %+v", *cluster, *tt.wCluster) return }

Then I run the test, it still failed(No matter how many times I run), the command output shows that the conditions of the cluster are different.

Cluster resource get {TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name:test-cluster GenerateName: Namespace: SelfLink: UID: ResourceVersion: Generation:0 CreationTimestamp:0001-01-01 00:00:00 +0000 UTC DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[] OwnerReferences:[] Finalizers:[karmada.io/cluster-controller] ManagedFields:[]} Spec:{ID: SyncMode:Pull APIEndpoint: SecretRef:<nil> ImpersonatorSecretRef:<nil> InsecureSkipTLSVerification:false ProxyURL: ProxyHeader:map[] Provider: Region: Zone: Zones:[] Taints:[] ResourceModels:[]} Status:{KubernetesVersion: APIEnablements:[] Conditions:[{Type:Ready Status:True ObservedGeneration:0 LastTransitionTime:0001-01-01 00:00:00 +0000 UTC Reason: Message:}] NodeSummary:<nil> ResourceSummary:<nil> RemedyActions:[]}}, want {TypeMeta:{Kind: APIVersion:} ObjectMeta:{Name:test-cluster GenerateName: Namespace: SelfLink: UID: ResourceVersion: Generation:0 CreationTimestamp:0001-01-01 00:00:00 +0000 UTC DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[] OwnerReferences:[] Finalizers:[karmada.io/cluster-controller] ManagedFields:[]} Spec:{ID: SyncMode:Pull APIEndpoint: SecretRef:<nil> ImpersonatorSecretRef:<nil> InsecureSkipTLSVerification:false ProxyURL: ProxyHeader:map[] Provider: Region: Zone: Zones:[] Taints:[] ResourceModels:[]} Status:{KubernetesVersion: APIEnablements:[] Conditions:[{Type:Ready Status:Unknown ObservedGeneration:0 LastTransitionTime:0001-01-01 00:00:00 +0000 UTC Reason:ClusterStatusUnknown Message:Cluster status controller stopped posting cluster status.}] NodeSummary:<nil> ResourceSummary:<nil> RemedyActions:[]}}

alright, i will check it again.

// Step 5: Check whether the probe timestamp has timed out. if metav1.Now().After(clusterHealth.probeTimestamp.Add(gracePeriod)) {

based on the different in the test results, i suspect this section of logic is causing the issue, but I still dont fully understand why. the previous test cases are still passing on my local.

finally, I set the c.ClusterMonitorGracePeriod = 40 * time.Second and modified the test cases. therefor within the timeout period, controller wont change the condition.

please take a look at this, thanks~

Thanks @xovoxy， let me have a try.

XiShanYongYe-Chang · 2024-09-06T09:17:17Z

pkg/controllers/cluster/taint_manager_test.go

+					Namespace: "default",
+				},
+			}
+			if err := tc.syncBindingEviction(key); (err != nil) != tt.wantErr {


It's not enough to judge err.

How about moving the addition of taint_manager_test.go to the next PR?

let me add some detail for this test case

hi, i have added some details about this, could you check it ? if it still not enough, i will consider your suggestion and moving the taint_manager_test.go to the next pr.

Signed-off-by: xovoxy <xilovele@gmail.com>

XiShanYongYe-Chang

Thanks~
/lgtm
/approve

karmada-bot · 2024-09-09T12:21:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: XiShanYongYe-Chang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controllers/OWNERS~~ [XiShanYongYe-Chang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot added the kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. label Aug 30, 2024

karmada-bot requested review from pigletfly and XiShanYongYe-Chang August 30, 2024 10:01

karmada-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 30, 2024

anujagrawal699 reviewed Aug 30, 2024

View reviewed changes

XiShanYongYe-Chang reviewed Sep 2, 2024

View reviewed changes

xovoxy force-pushed the cluster-ut branch from a78400d to 7091f7a Compare September 3, 2024 08:52

karmada-bot assigned XiShanYongYe-Chang Sep 4, 2024

XiShanYongYe-Chang reviewed Sep 4, 2024

View reviewed changes

xovoxy force-pushed the cluster-ut branch 2 times, most recently from 9d6eb1b to 6df9222 Compare September 6, 2024 02:22

XiShanYongYe-Chang reviewed Sep 6, 2024

View reviewed changes

add test for pkg/controllers/cluster

503fb93

Signed-off-by: xovoxy <xilovele@gmail.com>

xovoxy force-pushed the cluster-ut branch from 6df9222 to 503fb93 Compare September 8, 2024 11:56

karmada-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 8, 2024

XiShanYongYe-Chang reviewed Sep 9, 2024

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2024

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2024

karmada-bot merged commit 54b90a2 into karmada-io:master Sep 9, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add test for pkg/controllers/cluster #5467

add test for pkg/controllers/cluster #5467

xovoxy commented Aug 30, 2024

codecov-commenter commented Aug 30, 2024 •

edited

Loading

anujagrawal699 Aug 30, 2024

xovoxy Aug 31, 2024

anujagrawal699 Aug 31, 2024

anujagrawal699 commented Aug 31, 2024

karmada-bot commented Aug 31, 2024

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang Sep 2, 2024

xovoxy commented Sep 4, 2024

XiShanYongYe-Chang commented Sep 4, 2024

XiShanYongYe-Chang Sep 4, 2024

xovoxy Sep 5, 2024

XiShanYongYe-Chang Sep 5, 2024

XiShanYongYe-Chang Sep 5, 2024

xovoxy Sep 5, 2024

xovoxy Sep 6, 2024

XiShanYongYe-Chang Sep 6, 2024

XiShanYongYe-Chang Sep 6, 2024 •

edited

Loading

xovoxy Sep 6, 2024

xovoxy Sep 9, 2024

XiShanYongYe-Chang left a comment

karmada-bot commented Sep 9, 2024

add test for pkg/controllers/cluster #5467

add test for pkg/controllers/cluster #5467

Conversation

xovoxy commented Aug 30, 2024

codecov-commenter commented Aug 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anujagrawal699 commented Aug 31, 2024

karmada-bot commented Aug 31, 2024

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xovoxy commented Sep 4, 2024

XiShanYongYe-Chang commented Sep 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

karmada-bot commented Sep 9, 2024

codecov-commenter commented Aug 30, 2024 •

edited

Loading

XiShanYongYe-Chang Sep 6, 2024 •

edited

Loading