Added tests for pkg/servicenameresolutiondetector #5408

anujagrawal699 · 2024-08-20T17:01:51Z

Description:
This PR introduces tests for servicenameresolutiondetector package covering all the modules in each directory of it.

Additions:*

pkg/servicenameresolutiondetector/coredns/detector_test.go
pkg/servicenameresolutiondetector/store/condition_cache_test.go
pkg/servicenameresolutiondetector/store/node_store_test.go

Test Coverage:
The servicenameresolutiondetector package had a test coverage of 0.00%. The PR enhances its overall test coverage to more than 80%. The test coverage of pkg/servicenameresolutiondetector/coredns is enhanced to 74.3% and of pkg/servicenameresolutiondetector/store to 90.3%. This can be verified using go test ./... -coverprofile=coverage.out in the pkg/servicenameresolutiondetector directory.

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

What this PR does / why we need it:
This PR adds comprehensive tests to the servicenameresolutiondetector package, improving the overall test coverage and ensuring the reliability and maintainability of the codebase.

Which issue(s) this PR fixes:
Fixes a part of #5235

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2024-08-20T23:13:25Z

⚠️ 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 30.59%. Comparing base (6318541) to head (147ede0).
Report is 180 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    #5408      +/-   ##
==========================================
+ Coverage   29.67%   30.59%   +0.91%     
==========================================
  Files         632      632              
  Lines       43936    43936              
==========================================
+ Hits        13037    13441     +404     
+ Misses      29954    29518     -436     
- Partials      945      977      +32

Flag	Coverage Δ
unittests	`30.59% <ø> (+0.91%)`	⬆️

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.

zhzhuang-zju · 2024-08-21T06:43:25Z

/assign

anujagrawal699 · 2024-08-21T07:59:41Z

@XiShanYongYe-Chang Please take a look.

pkg/servicenameresolutiondetector/coredns/detector_test.go

zhzhuang-zju · 2024-08-22T03:30:17Z

pkg/servicenameresolutiondetector/coredns/detector_test.go

+			for _, status := range tt.expectedStatus {
+				if condition.Status == status {
+					validStatus = true
+					break


We can add a check: when condition.Status = metav1.ConditionFalse, check whether condition.Reason = serviceDomainNameResolutionFailed.

zhzhuang-zju · 2024-08-22T03:47:52Z

pkg/servicenameresolutiondetector/coredns/detector_test.go

+		name           string
+		alarm          bool
+		expectedStatus metav1.ConditionStatus
+		expectedReason string


Suggested change

expectedReason string

expectedReason string

expectedMessage string

zhzhuang-zju · 2024-08-22T07:54:27Z

pkg/servicenameresolutiondetector/store/node_store_test.go

+			case "ListAll":
+				result, opErr = store.ListAll()


Suggested change

case "ListAll":

result, opErr = store.ListAll()

zhzhuang-zju · 2024-08-22T08:00:19Z

pkg/servicenameresolutiondetector/store/node_store_test.go

+		expectedError  bool
+	}{
+		{
+			name: "Load existing node condition",


Add a case where the node exists, but the conditionType of the node does not match the store.

zhzhuang-zju · 2024-08-22T08:22:24Z

pkg/servicenameresolutiondetector/store/node_store_test.go

+				},
+			},
+			expectedResult: []metav1.Condition{
+				{Type: string(corev1.NodeReady), Status: metav1.ConditionTrue},


listAll function does not guarantee that the returned result will maintain the same order as the input nodes, which can lead to intermittent test failures.

=== RUN TestNodeStore_ListAll/List_with_not_ready_and_stale_nodes node_store_test.go:249: Error Trace: /root/home/workspace/code/karmada/pkg/servicenameresolutiondetector/store/node_store_test.go:249 Error: Not equal: expected: "True" actual : "Unknown" Diff: --- Expected +++ Actual @@ -1,2 +1,2 @@ -(v1.ConditionStatus) (len=4) "True" +(v1.ConditionStatus) (len=7) "Unknown" Test: TestNodeStore_ListAll/List_with_not_ready_and_stale_nodes node_store_test.go:249: Error Trace: /root/home/workspace/code/karmada/pkg/servicenameresolutiondetector/store/node_store_test.go:249 Error: Not equal: expected: "Unknown" actual : "True" Diff: --- Expected +++ Actual @@ -1,2 +1,2 @@ -(v1.ConditionStatus) (len=7) "Unknown" +(v1.ConditionStatus) (len=4) "True" Test: TestNodeStore_ListAll/List_with_not_ready_and_stale_nodes

I can sort the results before comparing.

Looks like you are converting the result array into a map to avoid ordering issues, which makes sense to me

Can we move ahead with it?

yes, please go ahead

@zhzhuang-zju can you trigger a retest?

anujagrawal699 · 2024-08-22T17:32:09Z

@zhzhuang-zju @XiShanYongYe-Chang PTAL

XiShanYongYe-Chang · 2024-08-23T02:26:50Z

Hi @anujagrawal699, can you help squash the commits into one?

anujagrawal699 · 2024-08-23T03:05:34Z

Hi @anujagrawal699, can you help squash the commits into one?

Hey @XiShanYongYe-Chang can you check if the current TestNodeStore_ListAll function is correct as mentioned by @zhzhuang-zju in the review. I'll squash commit after making the necessary changes.

Signed-off-by: Anuj Agrawal <[email protected]> Added tests for pkg/servicenameresolutiondetector Signed-off-by: Anuj Agrawal <[email protected]> Added tests for pkg/servicenameresolutiondetector Signed-off-by: Anuj Agrawal <[email protected]> Added tests for pkg/servicenameresolutiondetector Signed-off-by: Anuj Agrawal <[email protected]>

zhzhuang-zju · 2024-08-23T06:22:36Z

/retest

yashpandey06 · 2024-08-23T09:01:56Z

pkg/servicenameresolutiondetector/store/node_store_test.go

+	}
+}
+
+func TestSetNodeStatusCondition(t *testing.T) {


Hey @anujagrawal699 , do we have tested FindNodeStatusCondition function explicitly ?

No, we haven't. The size of PR was going big as you see (1000+ lines) and i though of another pr on pkg/servicenameresolutiondetector after i complete pending tests on another packages.

@yashpandey06 But still the FindNodeStatusCondition function is being indirectly tested within the TestNodeStore function.

If it's indirectly tested , that would suffice the need for now 😃

XiShanYongYe-Chang · 2024-08-23T09:23:49Z

/assign

XiShanYongYe-Chang

Thanks~
/lgtm
/approve

karmada-bot · 2024-08-24T09:08:26Z

[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:

~~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 20, 2024

karmada-bot requested review from chaunceyjiang and whitewindmills August 20, 2024 17:01

karmada-bot added kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 20, 2024

anujagrawal699 force-pushed the addedTests-pkg/servicenameresolutiondetector branch 2 times, most recently from 8a2571a to 3111a59 Compare August 21, 2024 05:59

karmada-bot assigned zhzhuang-zju Aug 21, 2024

zhzhuang-zju reviewed Aug 22, 2024

View reviewed changes

pkg/servicenameresolutiondetector/coredns/detector_test.go Outdated Show resolved Hide resolved

pkg/servicenameresolutiondetector/coredns/detector_test.go Outdated Show resolved Hide resolved

zhzhuang-zju reviewed Aug 22, 2024

View reviewed changes

anujagrawal699 force-pushed the addedTests-pkg/servicenameresolutiondetector branch from 3111a59 to dd5ba22 Compare August 22, 2024 17:22

karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Aug 22, 2024

anujagrawal699 force-pushed the addedTests-pkg/servicenameresolutiondetector branch from dd5ba22 to ef6fa6b Compare August 22, 2024 17:25

karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Aug 22, 2024

anujagrawal699 force-pushed the addedTests-pkg/servicenameresolutiondetector branch from ef6fa6b to 7ba7812 Compare August 22, 2024 17:30

anujagrawal699 force-pushed the addedTests-pkg/servicenameresolutiondetector branch from 00dc3d6 to 147ede0 Compare August 23, 2024 04:54

yashpandey06 reviewed Aug 23, 2024

View reviewed changes

karmada-bot assigned XiShanYongYe-Chang Aug 23, 2024

XiShanYongYe-Chang reviewed Aug 24, 2024

View reviewed changes

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

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

karmada-bot merged commit aee21b8 into karmada-io:master Aug 24, 2024
12 checks passed

RainbowMango added this to the v1.11 milestone Aug 31, 2024

anujagrawal699 mentioned this pull request Sep 19, 2024

Test Plan: Enhance Karmada controller-manager and schedule testing coverage (LFX 2024 TERM 3) #5470

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for pkg/servicenameresolutiondetector #5408

Added tests for pkg/servicenameresolutiondetector #5408

anujagrawal699 commented Aug 20, 2024

codecov-commenter commented Aug 20, 2024 •

edited

Loading

zhzhuang-zju commented Aug 21, 2024

anujagrawal699 commented Aug 21, 2024

zhzhuang-zju Aug 22, 2024

zhzhuang-zju Aug 22, 2024

zhzhuang-zju Aug 22, 2024

zhzhuang-zju Aug 22, 2024

zhzhuang-zju Aug 22, 2024

anujagrawal699 Aug 22, 2024

zhzhuang-zju Aug 23, 2024

anujagrawal699 Aug 23, 2024

zhzhuang-zju Aug 23, 2024

anujagrawal699 Aug 23, 2024

anujagrawal699 commented Aug 22, 2024

XiShanYongYe-Chang commented Aug 23, 2024

anujagrawal699 commented Aug 23, 2024 •

edited

Loading

zhzhuang-zju commented Aug 23, 2024

yashpandey06 Aug 23, 2024

anujagrawal699 Aug 23, 2024

anujagrawal699 Aug 23, 2024

yashpandey06 Aug 23, 2024

XiShanYongYe-Chang commented Aug 23, 2024

XiShanYongYe-Chang left a comment

karmada-bot commented Aug 24, 2024

	expectedReason string
	expectedReason string
	expectedMessage string

Added tests for pkg/servicenameresolutiondetector #5408

Added tests for pkg/servicenameresolutiondetector #5408

Conversation

anujagrawal699 commented Aug 20, 2024

codecov-commenter commented Aug 20, 2024 • edited Loading

Codecov Report

zhzhuang-zju commented Aug 21, 2024

anujagrawal699 commented Aug 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anujagrawal699 commented Aug 22, 2024

XiShanYongYe-Chang commented Aug 23, 2024

anujagrawal699 commented Aug 23, 2024 • edited Loading

zhzhuang-zju commented Aug 23, 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

XiShanYongYe-Chang commented Aug 23, 2024

XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

karmada-bot commented Aug 24, 2024

codecov-commenter commented Aug 20, 2024 •

edited

Loading

anujagrawal699 commented Aug 23, 2024 •

edited

Loading