Improve the UT coverage for portcache #4284

jainpulkit22 · 2022-10-12T13:22:37Z

Added unit test for pkg/agent/nodeportlocal/portcahce package.

Signed-off-by: Pulkit Jain jainpu@vmware.com

codecov · 2022-10-12T13:37:55Z

Codecov Report

Merging #4284 (2686845) into main (d9b2e37) will increase coverage by 0.84%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #4284      +/-   ##
==========================================
+ Coverage   64.52%   65.36%   +0.84%     
==========================================
  Files         402      402              
  Lines       57238    57238              
==========================================
+ Hits        36932    37416     +484     
+ Misses      17594    17117     -477     
+ Partials     2712     2705       -7

Flag	Coverage Δ
integration-tests	`34.66% <ø> (-0.06%)`	⬇️
kind-e2e-tests	`46.32% <ø> (+5.00%)`	⬆️
unit-tests	`51.04% <ø> (+0.13%)`	⬆️

Impacted Files	Coverage Δ
...g/agent/apiserver/handlers/featuregates/handler.go	`4.54% <0.00%> (-77.28%)`	⬇️
pkg/support/dump_others.go	`0.00% <0.00%> (-44.74%)`	⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go	`5.00% <0.00%> (-35.00%)`	⬇️
pkg/agent/apiserver/handlers/agentinfo/handler.go	`40.00% <0.00%> (-31.12%)`	⬇️
...agent/controller/traceflow/traceflow_controller.go	`53.57% <0.00%> (-19.39%)`	⬇️
pkg/ipfix/ipfix_process.go	`81.25% <0.00%> (-18.75%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`60.59% <0.00%> (-12.32%)`	⬇️
pkg/agent/util/iptables/iptables.go	`39.37% <0.00%> (-10.89%)`	⬇️
pkg/agent/apiserver/handlers/multicast/handler.go	`69.76% <0.00%> (-9.31%)`	⬇️
pkg/agent/apiserver/apiserver.go	`71.27% <0.00%> (-5.32%)`	⬇️
... and 60 more

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

luolanzone · 2022-10-26T03:06:31Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	portTable := newPortTable(mockIPTables, mockPortOpener)
+	podIP := "10.0.0.1"
+
+	portTable.DeleteRule(podIP, 1001, "tcp")


What's the point to call the function without checking anything? The main purpose of unit test is to verify that the function works as we expected. You should check the returned error and comparing it.
I think you can try to call portTable.addPortTableCache() to add a few data on the cache, then test the function with the cache.

luolanzone · 2022-10-26T03:07:05Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	mockIPTables.EXPECT().AddRule(nodePort2, podIP, 1002, "udp")
+
+	syncedCh := make(chan struct{})
+	const timeout = 1 * time.Second


You can define this const on line 32

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

tnqn · 2022-10-27T03:03:57Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	endPort   = 65000
+)
+
+func newPortTable(mockIPTables rules.PodPortRules, mockPortOpener LocalPortOpener) *PortTable {


windows doesn't have IPTable, which may be the reason why the type is named generically. Using mockIPTables as argument name doesn't make sense.

tnqn · 2022-10-27T03:04:33Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+func TestRestoreRules(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+	mockIPTables := rulestesting.NewMockPodPortRules(mockCtrl)


tnqn · 2022-10-27T03:05:27Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	select {
+	case <-syncedCh:
+		break
+	case <-time.After(timeout):
+		// this will not kill the goroutine created by RestoreRules,
+		// which should be acceptable.
+		t.Fatalf("Rule restoration not complete after %v", timeout)
+	}


RestoreRules's windows version has nothing to do with goroutine, this shouldn't be copied.

tnqn · 2022-10-27T03:06:24Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	portTable := newPortTable(mockIPTables, mockPortOpener)
+	podIP := "10.0.0.1"
+
+	portTable.DeleteRule(podIP, 1001, "tcp")


Atish-iaf · 2022-11-28T12:29:27Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+			} else {
+				mockPortRules.EXPECT().AddRule(tc.nodePort, tc.podIP, tc.podPort, tc.protocol)
+				nodePort, err := portTable.AddRule(tc.podIP, tc.podPort, tc.protocol)
+				assert.NoError(t, err)


require.NoError(t, err) is a good practice to ensure that the test will abort earlier.

But in this case there are no follow-up function calls that will panic, so isn't this fine to use assert here?

also the rest of the cases for the current test are skipped if we use require instead of assert.

That is expected, I think there is no point to check the other values when there is an unexpected error.

rajnkamr · 2022-11-28T13:52:59Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	startPort = 61000
+	endPort   = 65000
+)
+


Functions like addRuleforFreePort and addRuleForPort needs to called from test file by mocking the top level function and hence internal functions will be considered for percentage UT coverage, this can substantialy increase the coverage, the UT tool does not consider the coverage if the functions are internal, Suggest to follow this approach to increase UT coverage.

rajnkamr

Functions like addRuleforFreePort and addRuleForPort needs to called from test file by mocking the top level function and hence internal functions will be considered for percentage UT coverage, this can substantialy increase the coverage, the UT tool does not consider the coverage if the functions are internal, Suggest to follow this approach to increase UT coverage.

luolanzone · 2022-11-30T10:06:39Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+const (
+	startPort = 61000
+	endPort   = 65000
+)


Can these variables and newPortTable be shared for linux and windows test? I didn't see any differences.

yes they can be, but we do not have any shared file for test purpose, or you want me to declare these variables in the actual code file.

luolanzone · 2022-11-30T10:15:27Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	podIP := "10.0.0.1"
+	nodePort1 := startPort
+	nodePort2 := startPort + 1


You probably can also define shared test data for these.

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

luolanzone · 2022-11-30T10:21:04Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+			podIP:    "10.0.0.1",
+			podPort:  1001,


I suppose these two fields can be removed if their values are all the same in all cases. If they should change to cover more, then you may need to add more different cases.

luolanzone

LGTM overall, two nits.

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

luolanzone

LGTM

tnqn · 2022-12-07T16:57:36Z

pkg/agent/nodeportlocal/portcache/shared_data.go

@@ -0,0 +1,44 @@
+// Copyright 2022 Antrea Authors


Not having test.go suffix will make the file be compiled into antrea-agent binary, increasing its size unnecessarily. Normally the platform-generic code could be put under foo_test.go, then both foo_linux_test.go and foo_windows_test.go can use it. We should rename the current port_table_test.go to port_table_linux_test.go if it's supposed to run on linux only or port_table_other_test.go.

tnqn · 2022-12-07T17:11:07Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+			if tc.testForPod {
+				err = portTable.DeleteRule(podIP, 1001, "tcp")
+			} else {
+				err = portTable.DeleteRulesForPod(podIP)
+			}


When testForPod is true, it tests DeleteRule, which looks confusing.

It's actually not common to test two different functions in one test, orginazing them as table driven style, which is usually about expected input and output for same function. To share code for multiple tests, extracting common code to a shared function is more common.

However, I think the point of DeleteRulesForPod is it can delete all rules for the specific pod IP, unlike DeleteRule can only delete one specified rule. So an appropriate test for the former is to construct multiple rules first, then make sure a single call can clean up all of them.

tnqn · 2022-12-08T13:40:41Z

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go

+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.expectedErr == "" {
+				mockPortRules.EXPECT().AddRule(tc.nodePort, podIP, podPort, tc.protocol)
+			}
+			nodePort, err := portTable.AddRule(podIP, podPort, tc.protocol)
+			if tc.expectedErr != "" {
+				assert.ErrorContains(t, err, tc.expectedErr)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tc.nodePort, nodePort)
+			}
+		})
+	}


table driven test's cases should be individual, so it's easy to add new cases and adjust existing ones with worrying about the others, and it makes it possible to run specific sub test only.
I assume in this test the 2nd subtest relies on the 1st one.
It should just do it sequentially to make the dependency clear:

mockCtrl := gomock.NewController(t) defer mockCtrl.Finish() mockPortRules := rulestesting.NewMockPodPortRules(mockCtrl) mockPortOpener := portcachetesting.NewMockLocalPortOpener(mockCtrl) portTable := newPortTable(mockPortRules, mockPortOpener) podPort := 1001 // Adding the rule the first time should succeed. mockPortRules.EXPECT().AddRule(tc.nodePort, podIP, podPort, tc.protocol) gotNodePort, err := portTable.AddRule(podIP, podPort, tc.protocol) require.NoError(t, err) assert.Equal(t, startPort, gotNodePort) // Add the same rule the second time should fail. _, err := portTable.AddRule(podIP, podPort, tc.protocol) assert.ErrorContains(t, err, "existing Windows Nodeport entry for")

tnqn · 2022-12-08T13:42:34Z

pkg/agent/nodeportlocal/portcache/shared_data_test.go

@@ -0,0 +1,44 @@
+// Copyright 2022 Antrea Authors


It's not common to have a test file whose name's prefix doesn't map to a file.
Since this is for the test code of port_table.go, it could just be named port_table_test.go.

Signed-off-by: Pulkit Jain <jainpu@vmware.com>

tnqn

LGTM

tnqn · 2022-12-08T15:17:15Z

/skip-all

Signed-off-by: Pulkit Jain <jainpu@vmware.com>

luolanzone mentioned this pull request Oct 13, 2022

Improve unit test coverage #4142

Open

37 tasks

jainpulkit22 marked this pull request as ready for review October 13, 2022 03:49

jainpulkit22 marked this pull request as draft October 13, 2022 04:17

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch from 6e208dd to f98c6b8 Compare October 13, 2022 05:43

luolanzone reviewed Oct 13, 2022

View reviewed changes

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go Outdated Show resolved Hide resolved

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch 2 times, most recently from 40f41ad to 0071fcb Compare October 18, 2022 12:37

jainpulkit22 marked this pull request as ready for review October 18, 2022 12:38

jainpulkit22 requested a review from luolanzone October 18, 2022 12:42

jainpulkit22 changed the title ~~Improve the unit test coverage for portcache~~ Improve the UT coverage for portcache Oct 18, 2022

luolanzone requested a review from wenyingd October 20, 2022 07:47

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch 2 times, most recently from e781b7b to 51039c4 Compare October 21, 2022 04:52

luolanzone reviewed Oct 26, 2022

View reviewed changes

luolanzone requested review from tnqn and XinShuYang October 26, 2022 03:12

tnqn reviewed Oct 27, 2022

View reviewed changes

luolanzone added this to the Antrea v1.10 release milestone Oct 27, 2022

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch 2 times, most recently from 3163eaa to c6b39cc Compare November 23, 2022 14:24

jainpulkit22 marked this pull request as draft November 23, 2022 14:25

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch 8 times, most recently from e39d51d to a1aaa1c Compare November 28, 2022 06:22

jainpulkit22 requested review from Atish-iaf and rajnkamr November 28, 2022 10:45

Atish-iaf reviewed Nov 28, 2022

View reviewed changes

rajnkamr reviewed Nov 28, 2022

View reviewed changes

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch from 222730c to ff7901a Compare November 29, 2022 06:11

jainpulkit22 requested a review from luolanzone November 29, 2022 07:14

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch 3 times, most recently from b8f9a0c to 895204a Compare November 29, 2022 10:06

luolanzone reviewed Nov 30, 2022

View reviewed changes

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch from 895204a to ffa209b Compare December 1, 2022 11:34

luolanzone removed this from the Antrea v1.10 release milestone Dec 5, 2022

jainpulkit22 requested review from luolanzone and tnqn December 5, 2022 03:58

luolanzone reviewed Dec 6, 2022

View reviewed changes

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go Outdated Show resolved Hide resolved

pkg/agent/nodeportlocal/portcache/port_table_windows_test.go Outdated Show resolved Hide resolved

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch from ffa209b to 26643e1 Compare December 6, 2022 08:59

jainpulkit22 requested a review from luolanzone December 6, 2022 09:17

luolanzone previously approved these changes Dec 7, 2022

View reviewed changes

tnqn reviewed Dec 7, 2022

View reviewed changes

jainpulkit22 dismissed luolanzone’s stale review via 502f590 December 8, 2022 05:13

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch from 26643e1 to 502f590 Compare December 8, 2022 05:13

jainpulkit22 requested a review from tnqn December 8, 2022 05:48

tnqn reviewed Dec 8, 2022

View reviewed changes

Improve the unit test coverage for portcache

2686845

Signed-off-by: Pulkit Jain <jainpu@vmware.com>

jainpulkit22 force-pushed the nodeportlocal-unit-test-coverage branch from 502f590 to 2686845 Compare December 8, 2022 14:36

tnqn approved these changes Dec 8, 2022

View reviewed changes

tnqn merged commit a9fd9d2 into antrea-io:main Dec 8, 2022

GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Jan 27, 2023

Improve the unit test coverage for portcache (antrea-io#4284)

4b04de3

Signed-off-by: Pulkit Jain <jainpu@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the UT coverage for portcache #4284

Improve the UT coverage for portcache #4284

jainpulkit22 commented Oct 12, 2022 •

edited

Loading

codecov bot commented Oct 12, 2022 •

edited

Loading

luolanzone Oct 26, 2022

tnqn Oct 27, 2022

luolanzone Oct 26, 2022

tnqn Oct 27, 2022

tnqn Oct 27, 2022

tnqn Oct 27, 2022

tnqn Oct 27, 2022

Atish-iaf Nov 28, 2022

jainpulkit22 Nov 28, 2022

jainpulkit22 Nov 28, 2022

Atish-iaf Nov 29, 2022

rajnkamr Nov 28, 2022

rajnkamr left a comment

luolanzone Nov 30, 2022

jainpulkit22 Nov 30, 2022

luolanzone Nov 30, 2022

luolanzone Nov 30, 2022 •

edited

Loading

luolanzone left a comment

luolanzone left a comment

tnqn Dec 7, 2022

tnqn Dec 7, 2022

tnqn Dec 8, 2022

tnqn Dec 8, 2022

tnqn left a comment

tnqn commented Dec 8, 2022

Improve the UT coverage for portcache #4284

Improve the UT coverage for portcache #4284

Conversation

jainpulkit22 commented Oct 12, 2022 • edited Loading

codecov bot commented Oct 12, 2022 • 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

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

rajnkamr left a 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

luolanzone Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

luolanzone left a 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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Dec 8, 2022

jainpulkit22 commented Oct 12, 2022 •

edited

Loading

codecov bot commented Oct 12, 2022 •

edited

Loading

luolanzone Nov 30, 2022 •

edited

Loading