Add unit test for pkg/agent/route #4295

tnqn · 2022-10-13T18:30:22Z

Signed-off-by: Quan Tian qtian@vmware.com

pkg/agent/util/net.go

codecov · 2022-10-13T19:08:20Z

Codecov Report

Merging #4295 (311478b) into main (0307eff) will increase coverage by 1.15%.
The diff coverage is 45.57%.

❗ Current head 311478b differs from pull request most recent head 3ad73fd. Consider uploading reports for the commit 3ad73fd to get more accurate results

@@            Coverage Diff             @@
##             main    #4295      +/-   ##
==========================================
+ Coverage   60.60%   61.75%   +1.15%     
==========================================
  Files         378      389      +11     
  Lines       54850    55654     +804     
==========================================
+ Hits        33240    34371    +1131     
+ Misses      19095    18673     -422     
- Partials     2515     2610      +95

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`39.48% <21.84%> (?)`
integration-tests	`34.35% <45.63%> (-0.14%)`	⬇️	Carriedforward from 6b51f29
kind-e2e-tests	`42.51% <31.73%> (-5.97%)`	⬇️	Carriedforward from 6b51f29
unit-tests	`45.48% <0.00%> (-0.51%)`	⬇️	Carriedforward from 6b51f29

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

Impacted Files	Coverage Δ
pkg/agent/nodeportlocal/rules/iptable_rule.go	`55.78% <0.00%> (ø)`
pkg/agent/util/net_linux.go	`33.77% <ø> (+1.51%)`	⬆️
pkg/agent/route/route_linux.go	`40.37% <33.33%> (-10.10%)`	⬇️
pkg/agent/util/netlink/netlink_linux.go	`41.66% <41.66%> (ø)`
pkg/agent/util/iptables/iptables.go	`58.96% <73.33%> (+13.36%)`	⬆️
pkg/agent/util/ipset/ipset.go	`79.66% <85.00%> (+10.43%)`	⬆️
pkg/agent/proxy/endpointslicecache.go	`0.00% <0.00%> (-83.60%)`	⬇️
...kg/apiserver/registry/stats/multicastgroup/rest.go	`11.59% <0.00%> (-79.32%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`22.29% <0.00%> (-50.68%)`	⬇️
...g/agent/controller/serviceexternalip/controller.go	`35.83% <0.00%> (-43.35%)`	⬇️
... and 110 more

tnqn · 2022-10-18T09:41:08Z

/test-all

antoninbas

overall lgtm

I assume that changes outside of route_linux_test.go are cosmetic changes to allow for writing unit tests

It seems that code coverage has some issues since it reports that coverage will decrease when we merge this PR? Is it addressed by #4226 or it some other issue?

antoninbas · 2022-10-18T20:08:17Z

pkg/agent/util/iptables/iptables.go

+	Restore(data string, flush bool, useIPv6 bool) error
+
+	Save() ([]byte, error)


can you explain why Save returns a []byte but Restore takes a string as parameter? Intuitively the 2 functions should be "symmetric".

For Restore, if we use []byte as the argument type, it's difficult to know what's the actual argument the method was called with when the test fails. The error message would show two long slices of bytes which are not readable, and developer needs to change the argument type like I do in this patch to get what's wrong.
For Save, we could also change it to return string, but the above readability issue doesn't exist as we could convert the returned []byte to string in test code. Since exec.Commandreturns []byte and the only consumer dumpIPTables uses []byte, I didn't make the change to avoid two conversions.

Let me know if you think making them consistent is necessary.

I think it's fine

antoninbas · 2022-10-18T20:10:47Z

pkg/agent/route/route_linux_test.go

+)
+
+func getIPNet(cidr string) *net.IPNet {
+	_, ipNet, _ := net.ParseCIDR(cidr)


nit: maybe make this function getIPNetOrDie and panic if there is a parsing error

Just found there is already a util func MustParseCIDR doing what you described, changed to use that function.

tnqn · 2022-10-19T06:15:00Z

I assume that changes outside of route_linux_test.go are cosmetic changes to allow for writing unit tests

It seems that code coverage has some issues since it reports that coverage will decrease when we merge this PR? Is it addressed by #4226 or it some other issue?

Yes, all changes in this patch are for unit tests.
It should improve the coverage about 1.8%, there is some issue in codecov that it reported wrong improvement in comments, but the absolute number xx.xx% in checks like below are correct. I don't know the root cause of wrong report yet, @wenqiq is working on it, #4226 probably won't fix it.

codecov/project/antrea-unit-tests Successful in 2s — xx.xx% (+yy.yy%) compared to ...

wenyingd · 2022-10-19T06:37:48Z

pkg/agent/util/netlink/netlink_linux.go

@@ -0,0 +1,95 @@
+//go:build !windows


I remember that library netlink is only suitable on Linux, and there are some limitations in darwin.

With the linux suffix in the file name, this file will only be compiled on linux, I have removed the build tag.

wenyingd · 2022-10-19T06:46:53Z

pkg/agent/util/netlink/netlink_linux.go

+	return netlink.NeighDel(neigh)
+}
+
+func (c *Client) LinkByName(name string) (netlink.Link, error) {


There are some other exising usage from netlink package in net_linux.go, are they planned to be converted to functions provided in this interface?

Besides, some other packages may use functions like netlink.LinkByName directly ( not calling net.LinkByName()), so is it expected to give netlink.Interface as parameter to those callers, e.g., pkg/agent/cniserver/interface_configuration_linux.go

Yes, when we write tests for them in the future, they will have to use the interface.

Signed-off-by: Quan Tian <qtian@vmware.com>

hongliangl · 2022-10-19T03:40:22Z

pkg/agent/route/route_linux.go

@@ -94,7 +95,9 @@ type Client struct {
 	nodeConfig    *config.NodeConfig
 	networkConfig *config.NetworkConfig
 	noSNAT        bool
-	ipt           *iptables.Client
+	ipt           iptables.Interface


Maybe name it to iptables

Personally I'm fine with either but the variable was named ipt since the begining and the PR is for unit testing, not naming style adjustment, so I would prefer keeping it as was to avoid unrelated change.

tnqn · 2022-10-19T16:40:21Z

/skip-all

Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn commented Oct 13, 2022

View reviewed changes

pkg/agent/util/net.go Outdated Show resolved Hide resolved

tnqn force-pushed the unit-test-for-ipset branch from 550f54e to 07df504 Compare October 13, 2022 18:35

tnqn marked this pull request as draft October 13, 2022 18:45

tnqn force-pushed the unit-test-for-ipset branch 2 times, most recently from 1c86482 to 989fc96 Compare October 14, 2022 05:07

tnqn mentioned this pull request Oct 14, 2022

Improve unit test coverage #4142

Open

37 tasks

tnqn force-pushed the unit-test-for-ipset branch 4 times, most recently from 95492e8 to 735a773 Compare October 17, 2022 07:01

tnqn marked this pull request as ready for review October 17, 2022 14:30

tnqn mentioned this pull request Oct 17, 2022

Fix nil pointer error when collecting agent supportbundle fails #4306

Merged

tnqn force-pushed the unit-test-for-ipset branch 2 times, most recently from b611ffd to b359f49 Compare October 18, 2022 02:29

tnqn requested a review from antoninbas October 18, 2022 15:10

tnqn force-pushed the unit-test-for-ipset branch 2 times, most recently from 2c6d072 to 276ec69 Compare October 18, 2022 17:33

antoninbas reviewed Oct 18, 2022

View reviewed changes

tnqn force-pushed the unit-test-for-ipset branch from 276ec69 to 891b723 Compare October 19, 2022 05:54

tnqn requested a review from hongliangl October 19, 2022 06:15

wenyingd reviewed Oct 19, 2022

View reviewed changes

Add unit test for pkg/agent/route

3ad73fd

Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn force-pushed the unit-test-for-ipset branch from 891b723 to 3ad73fd Compare October 19, 2022 07:21

hongliangl reviewed Oct 19, 2022

View reviewed changes

antoninbas approved these changes Oct 19, 2022

View reviewed changes

tnqn merged commit 28f0fac into antrea-io:main Oct 19, 2022

tnqn deleted the unit-test-for-ipset branch October 19, 2022 16:40

vicky-liu added this to the Antrea v1.9 release milestone Oct 20, 2022

hongliangl pushed a commit to hongliangl/antrea that referenced this pull request Oct 22, 2022

Add unit test for pkg/agent/route (antrea-io#4295)

e5b21ca

Signed-off-by: Quan Tian <qtian@vmware.com>

heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023

Add unit test for pkg/agent/route (antrea-io#4295)

46e008b

Signed-off-by: Quan Tian <qtian@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test for pkg/agent/route #4295

Add unit test for pkg/agent/route #4295

tnqn commented Oct 13, 2022 •

edited

Loading

codecov bot commented Oct 13, 2022 •

edited

Loading

tnqn commented Oct 18, 2022

antoninbas left a comment

antoninbas Oct 18, 2022

tnqn Oct 19, 2022

tnqn Oct 19, 2022

antoninbas Oct 19, 2022

antoninbas Oct 18, 2022

tnqn Oct 19, 2022

tnqn commented Oct 19, 2022

wenyingd Oct 19, 2022

tnqn Oct 19, 2022

wenyingd Oct 19, 2022

tnqn Oct 19, 2022

hongliangl Oct 19, 2022

tnqn Oct 19, 2022

tnqn commented Oct 19, 2022

		Restore(data string, flush bool, useIPv6 bool) error

		Save() ([]byte, error)

Add unit test for pkg/agent/route #4295

Add unit test for pkg/agent/route #4295

Conversation

tnqn commented Oct 13, 2022 • edited Loading

codecov bot commented Oct 13, 2022 • edited Loading

Codecov Report

tnqn commented Oct 18, 2022

antoninbas 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Oct 19, 2022

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

tnqn commented Oct 19, 2022

tnqn commented Oct 13, 2022 •

edited

Loading

codecov bot commented Oct 13, 2022 •

edited

Loading