Add unit test for pkg/ipfix #4415

NamanAg30 · 2022-11-25T14:50:41Z

For #4142
Signed-off-by: Naman Agarwal naman.agarwal75@gmail.com

codecov · 2022-11-25T15:12:17Z

Codecov Report

Merging #4415 (0da7785) into main (a9fd9d2) will decrease coverage by 1.59%.
The diff coverage is n/a.

❗ Current head 0da7785 differs from pull request most recent head 35bc252. Consider uploading reports for the commit 35bc252 to get more accurate results

@@            Coverage Diff             @@
##             main    #4415      +/-   ##
==========================================
- Coverage   65.51%   63.92%   -1.60%     
==========================================
  Files         402      402              
  Lines       57238    57238              
==========================================
- Hits        37502    36590     -912     
- Misses      17013    17945     +932     
+ Partials     2723     2703      -20

Flag	Coverage Δ
integration-tests	`34.78% <0.00%> (+0.05%)`	⬆️
kind-e2e-tests	`40.96% <0.00%> (-5.42%)`	⬇️
unit-tests	`50.27% <0.00%> (-0.77%)`	⬇️

Impacted Files	Coverage Δ
pkg/agent/proxy/endpointslicecache.go	`0.00% <0.00%> (-83.60%)`	⬇️
...gent/nodeportlocal/portcache/port_table_windows.go	`0.00% <0.00%> (-64.43%)`	⬇️
...g/agent/controller/serviceexternalip/controller.go	`35.83% <0.00%> (-32.77%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	`52.81% <0.00%> (-22.95%)`	⬇️
pkg/agent/proxy/proxier.go	`57.25% <0.00%> (-18.83%)`	⬇️
pkg/agent/ipassigner/responder/arp_responder.go	`55.29% <0.00%> (-17.65%)`	⬇️
...pportbundlecollection/support_bundle_controller.go	`47.98% <0.00%> (-17.59%)`	⬇️
pkg/agent/cniserver/ipam/ipam_delegator.go	`54.54% <0.00%> (-13.64%)`	⬇️
pkg/agent/controller/trafficcontrol/controller.go	`70.83% <0.00%> (-11.95%)`	⬇️
pkg/util/k8s/client.go	`35.00% <0.00%> (-11.67%)`	⬇️
... and 43 more

rajnkamr · 2022-11-28T05:56:08Z

pkg/ipfix/ipfix_collector_test.go

+	tcpTransport = "tcp"
+	udpTransport = "udp"
+	hostPortIPv4 = "127.0.0.1:0"
+	hostPortIPv6 = "[::1]:0"


same as above HostIPv6Port

rajnkamr · 2022-11-28T05:56:10Z

pkg/ipfix/ipfix_collector_test.go

+const (
+	tcpTransport = "tcp"
+	udpTransport = "udp"
+	hostPortIPv4 = "127.0.0.1:0"


use name like hostIPv4Port

rajnkamr · 2022-11-28T05:56:12Z

pkg/ipfix/ipfix_collector_test.go

+)
+
+const (
+	tcpTransport = "tcp"


name like protocol type will make more sense

rajnkamr · 2022-11-28T06:55:52Z

pkg/ipfix/ipfix_collector_test.go

+				assert.Equal(t, tt.expectedNum, cp.GetNumConnToCollector())
+			} else {
+				assert.Equal(t, tt.err, err.Error())
+			}


Coverage for GetNumRecordsReceived ?

rajnkamr · 2022-11-28T07:34:34Z

pkg/ipfix/ipfix_intermediate_test.go

+		})
+	}
+}
+


few functions like SetCorrelatedFieldsFilled in ipfix_intermediate.go , yet to be covered !

rajnkamr

Increase the coverage percentage

NamanAg30 · 2022-12-01T11:06:15Z

coverage has been increased and other comments have been addressed @rajnkamr

hongliangl · 2022-12-08T03:24:17Z

pkg/ipfix/ipfix_collector_test.go

@@ -0,0 +1,22 @@
+package ipfix


Add license warranty for every new file like other files.

hongliangl · 2022-12-08T03:25:17Z

pkg/ipfix/ipfix_process_test.go

@@ -0,0 +1,101 @@
+// Copyright 2020 Antrea Authors


Suggested change

// Copyright 2020 Antrea Authors

// Copyright 2022 Antrea Authors

fixed for all files

hongliangl · 2022-12-08T12:36:42Z

pkg/ipfix/ipfix_process_test.go

+			input: exporter.ExporterInput{
+				CollectorAddress: "tcp",
+			},
+			expectedError: "error while initializing IPFIX exporting process: dial: unknown network ",


Suggested change

expectedError: "error while initializing IPFIX exporting process: dial: unknown network ",

expectedError: "error while initializing IPFIX exporting process: dial: unknown network",

hongliangl · 2022-12-08T12:42:30Z

pkg/ipfix/ipfix_process_test.go

+	"github.com/stretchr/testify/assert"
+	"github.com/vmware/go-ipfix/pkg/entities"
+	"github.com/vmware/go-ipfix/pkg/exporter"
+	test "github.com/vmware/go-ipfix/pkg/test"


This alias is unnecessary since it is the same as the suffix of the lib.

tnqn · 2022-12-21T05:05:05Z

go.sum

@@ -100,6 +100,7 @@ github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbt
 github.com/PuerkitoBio/urlesc v0.0.0-20160726150825-5bd2802263f2/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE=
 github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 h1:d+Bc7a5rLufV/sSk/8dngufqelfh6jnri85riMAaF/M=
 github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE=
+github.com/Shopify/sarama v1.27.2 h1:1EyY1dsxNDUQEv0O/4TsjosHI2CgB1uo9H/v56xzTxc=


Are they added automatically? Not sure why there were missing on main branch and suddenly added here.

This test was failing on github 'Go / Check tidy, code generation and manifest (pull_request)'...so on running go mod tidy these lines were added....Should I remove them?

Now the test is failing, I think they shouldn't be added.
https://github.com/antrea-io/antrea/actions/runs/3877253764/jobs/6612065974

tnqn · 2022-12-21T05:29:45Z

pkg/ipfix/ipfix_collector_test.go

+	defer cp.Stop()
+	assert.NotNil(t, cp.GetMsgChan())
+	assert.Equal(t, 0, int(cp.GetNumRecordsReceived()))
+	assert.Equal(t, 0, int(cp.GetNumConnToCollector()))


Sorry, not related to your test code, but actually the whole CollectorInput struct itself is meaningless as it just acts as a proxy class, which is not even needed in Go.
I have created #4505 to completely remove the unneeded structs. Thus, only test code in ipfix_registry_test.go is needed.

So should I remove ipfix_collector_test.go , ipfix_intermediate_test.go and ipfix_process_test.go from the PR or will this PR be merged only after #4505 ?

tnqn · 2023-01-11T04:32:34Z

go.sum

@@ -100,6 +100,7 @@ github.com/PuerkitoBio/purell v1.1.1/go.mod h1:c11w/QuzBsJSee3cPx9rAFu61PvFxuPbt
 github.com/PuerkitoBio/urlesc v0.0.0-20160726150825-5bd2802263f2/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE=
 github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 h1:d+Bc7a5rLufV/sSk/8dngufqelfh6jnri85riMAaF/M=
 github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578/go.mod h1:uGdkoq3SwY9Y+13GIhn11/XLaGBb4BfwItxLd5jeuXE=
+github.com/Shopify/sarama v1.27.2 h1:1EyY1dsxNDUQEv0O/4TsjosHI2CgB1uo9H/v56xzTxc=


Now the test is failing, I think they shouldn't be added.
https://github.com/antrea-io/antrea/actions/runs/3877253764/jobs/6612065974

tnqn · 2023-01-11T04:35:36Z

pkg/ipfix/ipfix_registry_test.go

+			reg.LoadRegistry()
+			element, err := reg.GetInfoElement(tt.name, tt.enterpriseID)
+			if tt.expectedError == "" {
+				assert.Equal(t, tt.expectedElementID, element.ElementId)


It should use require.NoError to ensure no error is returned first, otherwise accessing element.ElementId directly may panic if code doesn't work as expected.

tnqn · 2023-01-11T04:55:07Z

pkg/ipfix/ipfix_registry_test.go

+			if tt.expectedError == "" {
+				assert.Equal(t, tt.expectedElementID, element.ElementId)
+			} else {
+				assert.Equal(t, tt.expectedError, err.Error())


ditto, err.Error() could panic when got error is nil, it could use assert.ErrorContains

Signed-off-by: Naman Agarwal <naman.agarwal75@gmail.com>

tnqn

LGTM

tnqn · 2023-01-16T14:21:16Z

/skip-all

Signed-off-by: Naman Agarwal <naman.agarwal75@gmail.com>

NamanAg30 requested a review from rajnkamr November 25, 2022 14:50

rajnkamr reviewed Nov 28, 2022

View reviewed changes

NamanAg30 force-pushed the ipfix branch from 5744f69 to 1afaf94 Compare December 1, 2022 11:03

NamanAg30 requested a review from rajnkamr December 1, 2022 11:03

NamanAg30 force-pushed the ipfix branch from 1afaf94 to 68af4ba Compare December 7, 2022 06:49

NamanAg30 marked this pull request as ready for review December 7, 2022 08:42

NamanAg30 requested a review from hongliangl December 7, 2022 08:45

hongliangl reviewed Dec 8, 2022

View reviewed changes

NamanAg30 force-pushed the ipfix branch 2 times, most recently from 35bc252 to 6b2b7a6 Compare December 9, 2022 05:30

NamanAg30 requested a review from hongliangl December 9, 2022 05:31

NamanAg30 force-pushed the ipfix branch 3 times, most recently from c9a18c9 to 50e83bf Compare December 14, 2022 07:09

NamanAg30 requested review from tnqn and jianjuns December 14, 2022 12:47

tnqn reviewed Dec 21, 2022

View reviewed changes

NamanAg30 requested a review from tnqn December 22, 2022 11:56

NamanAg30 force-pushed the ipfix branch from 50e83bf to 621769f Compare January 9, 2023 19:56

tnqn reviewed Jan 11, 2023

View reviewed changes

Add unit test for pkg/ipfix

b6a25ee

Signed-off-by: Naman Agarwal <naman.agarwal75@gmail.com>

NamanAg30 force-pushed the ipfix branch from 621769f to b6a25ee Compare January 13, 2023 06:59

NamanAg30 requested a review from tnqn January 16, 2023 03:37

tnqn approved these changes Jan 16, 2023

View reviewed changes

tnqn merged commit 8961420 into antrea-io:main Jan 16, 2023

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

Add unit test for pkg/ipfix (antrea-io#4415)

fe17135

Signed-off-by: Naman Agarwal <naman.agarwal75@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit test for pkg/ipfix #4415

Add unit test for pkg/ipfix #4415

NamanAg30 commented Nov 25, 2022

codecov bot commented Nov 25, 2022 •

edited

Loading

rajnkamr Nov 28, 2022

rajnkamr Nov 28, 2022

rajnkamr Nov 28, 2022

rajnkamr Nov 28, 2022

rajnkamr Nov 28, 2022

rajnkamr left a comment

NamanAg30 commented Dec 1, 2022

hongliangl Dec 8, 2022

NamanAg30 Dec 9, 2022

hongliangl Dec 8, 2022

NamanAg30 Dec 8, 2022

hongliangl Dec 8, 2022

NamanAg30 Dec 9, 2022

hongliangl Dec 8, 2022

NamanAg30 Dec 8, 2022

tnqn Dec 21, 2022

NamanAg30 Dec 22, 2022

tnqn Jan 11, 2023

tnqn Dec 21, 2022

NamanAg30 Dec 22, 2022

tnqn Jan 11, 2023

tnqn Jan 11, 2023

tnqn Jan 11, 2023

tnqn left a comment

tnqn commented Jan 16, 2023

	// Copyright 2020 Antrea Authors
	// Copyright 2022 Antrea Authors

	expectedError: "error while initializing IPFIX exporting process: dial: unknown network ",
	expectedError: "error while initializing IPFIX exporting process: dial: unknown network",

Add unit test for pkg/ipfix #4415

Add unit test for pkg/ipfix #4415

Conversation

NamanAg30 commented Nov 25, 2022

codecov bot commented Nov 25, 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

rajnkamr left a comment

Choose a reason for hiding this comment

NamanAg30 commented Dec 1, 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

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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jan 16, 2023

codecov bot commented Nov 25, 2022 •

edited

Loading