Add unit tests for pkg/agent/util/net #4312

qiyueyao · 2022-10-18T17:49:30Z

Signed-off-by: Qiyue Yao yaoq@vmware.com

codecov · 2022-10-18T18:56:36Z

Codecov Report

Merging #4312 (05e9060) into main (8eb5c8d) will increase coverage by 1.43%.
The diff coverage is 95.53%.

@@            Coverage Diff             @@
##             main    #4312      +/-   ##
==========================================
+ Coverage   69.79%   71.23%   +1.43%     
==========================================
  Files         402      402              
  Lines       59570    59568       -2     
==========================================
+ Hits        41579    42432     +853     
+ Misses      15189    14293     -896     
- Partials     2802     2843      +41

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.40% <ø> (+0.02%)`	⬆️	Carriedforward from 8eb5c8d
integration-tests	`34.41% <36.84%> (+0.08%)`	⬆️
kind-e2e-tests	`47.88% <24.56%> (+0.72%)`	⬆️
unit-tests	`61.33% <95.53%> (+1.56%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/util/net_windows.go	`83.55% <90.90%> (+76.48%)`	⬆️
pkg/agent/util/net.go	`82.95% <100.00%> (+24.62%)`	⬆️
pkg/agent/util/net_linux.go	`87.05% <100.00%> (+54.07%)`	⬆️
...nt/apiserver/handlers/serviceexternalip/handler.go	`37.03% <0.00%> (-14.82%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`70.27% <0.00%> (-6.76%)`	⬇️
pkg/apiserver/certificate/certificate.go	`70.37% <0.00%> (-6.49%)`	⬇️
pkg/controller/networkpolicy/tier.go	`53.84% <0.00%> (-4.62%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	`75.75% <0.00%> (-4.33%)`	⬇️
...g/agent/controller/serviceexternalip/controller.go	`79.18% <0.00%> (-2.39%)`	⬇️
pkg/agent/ipassigner/local_ip_detector_linux.go	`75.75% <0.00%> (-2.03%)`	⬇️
... and 25 more

qiyueyao · 2023-01-12T01:56:50Z

@wenyingd Got a question for this WindowsHyperVEnabled function, why it returns true instead of false when cmd returns an error?

wenyingd · 2023-01-12T02:24:18Z

@wenyingd Got a question for this WindowsHyperVEnabled function, why it returns true instead of false when cmd returns an error?

It should be a mistake, false is expected.

luolanzone · 2023-02-01T05:51:11Z

Hi @qiyueyao could you resolve the code conflicts? is this ready for review?

qiyueyao · 2023-02-02T22:42:35Z

Hi @qiyueyao could you resolve the code conflicts? is this ready for review?

Done. This is ready for review.

wenyingd · 2023-02-23T10:28:51Z

pkg/agent/util/net_linux_test.go

+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			getNS = func(nspath string) (ns.NetNS, error) {


Could you follow this format for all the mock functions?

func mockGetNS(testNS ns.NetNS, err error) func() { originalGetNS := getNS getNS = func(nspath string) (ns.NetNS, error) { return testNS, err } return func() { getNS = originalGetNS } }

And use defer functions like this in the test cases,

... defer mockGetNS(&mockNetNS{}, tc.getNSErr)() ...

pkg/agent/util/net_windows.go

pkg/agent/util/net_windows_test.go

wenyingd · 2023-02-23T11:11:30Z

pkg/agent/util/net_windows_test.go

+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			runCommand = func(cmd string) (string, error) {


I would prefer to have comparasion on the final command ( the parameter cmd ) with an expected comand string to run with powershell when mocking the funciont "runCommand" in this file. In this way, it is easier to catch the command modification in the future changes.

Added the comparison, and since runCommand is usually called multiple times in a test function, assert.Contains is used instead of assert.Equal.

pkg/agent/util/net_linux_test.go

pkg/agent/util/net_test.go

pkg/agent/util/net_windows_test.go

luolanzone · 2023-03-02T12:39:29Z

@qiyueyao any update for this PR?

pkg/agent/util/net.go

pkg/agent/util/net_linux.go

pkg/agent/util/net_linux_test.go

pkg/agent/util/net_test.go

pkg/agent/util/net_windows_test.go

hongliangl · 2023-03-09T09:13:28Z

pkg/agent/util/net_linux_test.go

+// Copyright 2023 Antrea Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build linux
+// +build linux


Suggested change

// Copyright 2023 Antrea Authors

//

// Licensed under the Apache License, Version 2.0 (the "License");

// you may not use this file except in compliance with the License.

// You may obtain a copy of the License at

//

// http://www.apache.org/licenses/LICENSE-2.0

//

// Unless required by applicable law or agreed to in writing, software

// distributed under the License is distributed on an "AS IS" BASIS,

// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

// See the License for the specific language governing permissions and

// limitations under the License.

//go:build linux

// +build linux

//go:build linux

// +build linux

// Copyright 2023 Antrea Authors

//

// Licensed under the Apache License, Version 2.0 (the "License");

// you may not use this file except in compliance with the License.

// You may obtain a copy of the License at

//

// http://www.apache.org/licenses/LICENSE-2.0

//

// Unless required by applicable law or agreed to in writing, software

// distributed under the License is distributed on an "AS IS" BASIS,

// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

// See the License for the specific language governing permissions and

// limitations under the License.

@qiyueyao This is not addressed.

Oops I changed the one in pkg/agent/util/net_linux.go instead, but I guess that should also be addressed.

hongliangl · 2023-03-14T07:38:24Z

pkg/agent/util/net_linux_test.go

+			gotMac, gotIndex, gotErr := SetLinkUp("antrea-en0")
+			assert.Equal(t, tc.wantMac, gotMac)
+			assert.Equal(t, tc.wantIndex, gotIndex)
+			assert.Equal(t, tc.wantErr, gotErr)


Use assert.EqualErrorf. So do others.

May I ask the purpose of using assert.EqualErrorf instead of assert.Equal? Because according to the definition"EqualErrorf asserts that a function returned an error (i.e. not nil) and that it is equal to the provided error.", then in the case of nil error, this introduces more case check.

Kept most cases, but changed required.NoError inspired by other PR.

wenyingd

Overall LGTM, one minor comment is left.

pkg/agent/util/net_windows_test.go

wenyingd

LGTM

luolanzone

LGTM overall, one nit. @tnqn do you have time to take a quick look? thanks.

luolanzone · 2023-03-16T09:44:05Z

pkg/agent/util/net_linux_test.go

+// Copyright 2023 Antrea Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build linux
+// +build linux


@qiyueyao This is not addressed.

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

luolanzone

LGTM

qiyueyao · 2023-03-17T04:29:41Z

/test-all
/test-windows-all
/test-ipv6-all
/test-ipv6-only-all

tnqn

LGTM, thanks for the effort.

qiyueyao · 2023-03-17T07:45:30Z

Thanks @tnqn, I actually have a suggestion, to add this UT pattern to the code review comments/UT section for developers to reference. It would be of great help.

tnqn · 2023-03-17T10:26:06Z

Thanks @tnqn, I actually have a suggestion, to add this UT pattern to the code review comments/UT section for developers to reference. It would be of great help.

@qiyueyao please feel free to open a PR to that doc with any suggestions that you think helpful.

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

qiyueyao changed the title ~~Add UT to net.go~~ Add unit tests for pkg/agent/util/net Oct 27, 2022

luolanzone mentioned this pull request Oct 28, 2022

Improve unit test coverage #4142

Open

37 tasks

qiyueyao force-pushed the net-ut branch 3 times, most recently from d4cc01c to 89e0b43 Compare November 2, 2022 03:14

qiyueyao force-pushed the net-ut branch from 89e0b43 to c7b2e43 Compare November 9, 2022 01:32

qiyueyao force-pushed the net-ut branch from c7b2e43 to df817bf Compare November 16, 2022 23:16

qiyueyao force-pushed the net-ut branch 5 times, most recently from 1c14185 to c81dfed Compare December 22, 2022 15:35

qiyueyao force-pushed the net-ut branch 5 times, most recently from 7548847 to 33e64d8 Compare January 10, 2023 05:42

qiyueyao marked this pull request as ready for review January 10, 2023 17:40

qiyueyao requested review from mengdie-song, Dyanngg, tnqn and wenyingd January 10, 2023 17:46

qiyueyao force-pushed the net-ut branch from 33e64d8 to a4fccb1 Compare January 12, 2023 01:54

qiyueyao force-pushed the net-ut branch 2 times, most recently from 44e03bc to 41e610e Compare February 2, 2023 18:07

qiyueyao force-pushed the net-ut branch from e6dbf07 to 05e9060 Compare February 22, 2023 04:37

wenyingd reviewed Feb 23, 2023

View reviewed changes

luolanzone reviewed Feb 28, 2023

View reviewed changes

luolanzone added this to the Antrea v1.11 release milestone Mar 2, 2023

qiyueyao force-pushed the net-ut branch 3 times, most recently from 866355a to ceae3fc Compare March 8, 2023 07:51

qiyueyao requested review from wenyingd and luolanzone March 8, 2023 18:08

wenyingd reviewed Mar 10, 2023

View reviewed changes

hongliangl reviewed Mar 14, 2023

View reviewed changes

qiyueyao force-pushed the net-ut branch 2 times, most recently from a23489c to 4d039ff Compare March 14, 2023 12:59

wenyingd reviewed Mar 15, 2023

View reviewed changes

pkg/agent/util/net_windows_test.go Show resolved Hide resolved

qiyueyao force-pushed the net-ut branch from 4d039ff to 11c2e2b Compare March 15, 2023 05:46

wenyingd previously approved these changes Mar 15, 2023

View reviewed changes

luolanzone previously approved these changes Mar 16, 2023

View reviewed changes

Add unit tests for pkg/agent/util/net*.

c528f46

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

qiyueyao dismissed stale reviews from luolanzone and wenyingd via c528f46 March 16, 2023 11:23

qiyueyao force-pushed the net-ut branch from 11c2e2b to c528f46 Compare March 16, 2023 11:23

luolanzone approved these changes Mar 17, 2023

View reviewed changes

tnqn approved these changes Mar 17, 2023

View reviewed changes

tnqn merged commit c8ed472 into antrea-io:main Mar 17, 2023

qiyueyao deleted the net-ut branch March 17, 2023 07:45

jainpulkit22 pushed a commit to urharshitha/antrea that referenced this pull request Apr 28, 2023

Add unit tests for pkg/agent/util/net (antrea-io#4312)

9272252

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for pkg/agent/util/net #4312

Add unit tests for pkg/agent/util/net #4312

qiyueyao commented Oct 18, 2022

codecov bot commented Oct 18, 2022 •

edited

Loading

qiyueyao commented Jan 12, 2023

wenyingd commented Jan 12, 2023

luolanzone commented Feb 1, 2023

qiyueyao commented Feb 2, 2023

wenyingd Feb 23, 2023

wenyingd Feb 23, 2023

qiyueyao Mar 8, 2023

luolanzone commented Mar 2, 2023

hongliangl Mar 9, 2023

luolanzone Mar 16, 2023

qiyueyao Mar 16, 2023

hongliangl Mar 14, 2023

qiyueyao Mar 14, 2023

qiyueyao Mar 14, 2023

wenyingd left a comment

wenyingd left a comment

luolanzone left a comment

luolanzone Mar 16, 2023

luolanzone left a comment

qiyueyao commented Mar 17, 2023 •

edited

Loading

tnqn left a comment

qiyueyao commented Mar 17, 2023

tnqn commented Mar 17, 2023

Add unit tests for pkg/agent/util/net #4312

Add unit tests for pkg/agent/util/net #4312

Conversation

qiyueyao commented Oct 18, 2022

codecov bot commented Oct 18, 2022 • edited Loading

Codecov Report

qiyueyao commented Jan 12, 2023

wenyingd commented Jan 12, 2023

luolanzone commented Feb 1, 2023

qiyueyao commented Feb 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Mar 2, 2023

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

wenyingd left a comment

Choose a reason for hiding this comment

wenyingd 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

luolanzone left a comment

Choose a reason for hiding this comment

qiyueyao commented Mar 17, 2023 • edited Loading

tnqn left a comment

Choose a reason for hiding this comment

qiyueyao commented Mar 17, 2023

tnqn commented Mar 17, 2023

codecov bot commented Oct 18, 2022 •

edited

Loading

qiyueyao commented Mar 17, 2023 •

edited

Loading