Adds the unit test for pkg/support #4452

urharshitha · 2022-12-07T06:39:50Z

Adds the unit test for support pkg
for #4142
Signed-off-by: Harshitha U R harshithaur1611@gmail.com

codecov · 2022-12-07T07:52:09Z

Codecov Report

Merging #4452 (e7dc787) into main (7e055c6) will increase coverage by 1.02%.
The diff coverage is n/a.

❗ Current head e7dc787 differs from pull request most recent head e37e346. Consider uploading reports for the commit e37e346 to get more accurate results

@@            Coverage Diff             @@
##             main    #4452      +/-   ##
==========================================
+ Coverage   67.15%   68.18%   +1.02%     
==========================================
  Files         400      404       +4     
  Lines       58151    58202      +51     
==========================================
+ Hits        39052    39684     +632     
+ Misses      16371    15767     -604     
- Partials     2728     2751      +23

Flag	Coverage Δ
integration-tests	`34.80% <0.00%> (+0.17%)`	⬆️
kind-e2e-tests	`48.06% <0.00%> (+1.93%)`	⬆️
unit-tests	`56.78% <0.00%> (-0.18%)`	⬇️

Impacted Files	Coverage Δ
cmd/antrea-agent/options_linux.go	`0.00% <0.00%> (-100.00%)`	⬇️
cmd/antrea-agent/options.go	`23.20% <0.00%> (-13.27%)`	⬇️
pkg/controller/externalippool/controller.go	`86.16% <0.00%> (-6.25%)`	⬇️
...trollers/multicluster/resourceexport_controller.go	`76.20% <0.00%> (-4.28%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`72.97% <0.00%> (-4.06%)`	⬇️
pkg/util/ip/ip.go	`86.99% <0.00%> (-3.26%)`	⬇️
...gent/controller/networkpolicy/status_controller.go	`79.16% <0.00%> (-2.50%)`	⬇️
pkg/controller/grouping/controller.go	`65.13% <0.00%> (-1.98%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`83.24% <0.00%> (-1.53%)`	⬇️
...pportbundlecollection/support_bundle_controller.go	`64.10% <0.00%> (-1.47%)`	⬇️
... and 52 more

jainpulkit22

Try to make the test table driven if a test has two or more cases.

jainpulkit22 · 2022-12-19T10:41:46Z

pkg/support/dump_test.go

@@ -30,6 +34,10 @@ func TestParseLogDate(t *testing.T) {
 	ts, err = parseTimeFromLogLine(data, "2021", "ovs")
 	assert.Nil(t, err)
 	assert.Equal(t, ts.String(), "2021-06-01 09:30:43 +0000 UTC")
+
+	_, err = parseTimeFromLogLine("", "2021", "ovs")
+	assert.Error(t, err)


you should expect the exact error statement.

since log string is empty, it returns back with time and message "log line is empty"

jainpulkit22 · 2022-12-19T10:42:26Z

pkg/support/dump_test.go

+
+	_, err = parseTimeFromLogLine("", "2021", "ovs")
+	assert.Error(t, err)
+	assert.Equal(t, "", "")


if you are expecting error then maybe this is not required, and try to use table driven test here.

assert.Equal usage when both the values to check for equality remain empty , what is the need for this line here

jainpulkit22 · 2022-12-19T10:49:30Z

pkg/support/dump_test.go

+	since := "7s"
+	ts := timestampFilter(since)
+	currentTime := time.Now()
+	assert.Contains(t, ts.String(), currentTime.Format("2006-01-02"))


in expected output you are not using the since option that you passed as the parameter and also try using assert.Equal and match the exact output.

"since" is used only to call the function , and for matching exact output it includes time also so every time it changes due to seconds, minutes ,so I tried matching with the date format

how the time since = 7s is calculated for this test !

It is not calculated ,timestampFilter gives the time when dump have started ,to get that exact time ,the time specified as since is subtracted from current time( time.Now() ) so for testing purpose it is given as "7s"(7 seconds) we can also give "1h", "3m" etc...

I agree that the test is not great as it is. Something like this would make more sense IMO:

ts := timestampFilter("2m") currentTime := time.Now() require.NotNil(t, ts) expectedTime := currentTime.Add(-2*time.Minute) assert.WithinDuration(t, expectedTime, *ts, 10*time.Second)

jainpulkit22 · 2022-12-19T11:06:15Z

pkg/support/dump_test.go

+	data := []byte("antct -oyaml get networkpolicies")
+	err := writeFile(fs, filepath.Join(basedir, resource), resource, data)
+	if err != nil {
+		require.EqualError(t, err, "error when writing networkpolicies to file: open dir1/dir2/networkpolicies: no such file or directory")


here you have to be sure with the test case that whether the error will be generated or not, and you can cover these in two separate test cases. Also when you are checking for no error you should also check the contents of the file written.

jainpulkit22 · 2022-12-19T11:06:26Z

pkg/support/dump_test.go

+	basedir := "dir1/dir2"
+	data := []byte("antct -oyaml get agentinfo")
+	err := writeYAMLFile(fs, filepath.Join(basedir, resource), resource, data)
+	if err != nil {


rajnkamr · 2022-12-20T07:00:58Z

pkg/support/dump_test.go

+	if err != nil {
+		require.EqualError(t, err, "error when writing networkpolicies to file: open dir1\\dir2\\networkpolicies: The system cannot find the path specified.")
+	} else {
+		require.NoError(t, err)


when err is nil it comes to line 66, so try to print additional informative lines for example require.NoError(t, err, "error when opening a stub database connection")

jainpulkit22

when checking for err, we should always check with expected err, instead of err, because there may be cases where your expected error is not nil but error is nil and there may be other such panic situations as well.

pkg/support/dump_test.go

jainpulkit22 · 2023-01-03T10:43:48Z

pkg/support/dump_test.go

+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			gotTS, err := parseTimeFromLogLine(tc.data, tc.year, tc.prefix)


use a significant name instead of gotTS

pkg/support/dump_test.go

jainpulkit22 · 2023-01-03T10:45:19Z

pkg/support/dump_test.go

+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			gotTS, err := parseTimeFromLogLine(tc.data, tc.year, tc.prefix)
+			if err != nil {


instead of checking with err check if expectedErr is not nil then match the error statements else assert for result and assert NoError.

jainpulkit22 · 2023-01-03T10:46:49Z

pkg/support/dump_test.go

@@ -38,3 +100,309 @@ func TestParseFileName(t *testing.T) {
 	assert.Nil(t, err)
 	assert.Equal(t, ts.String(), "2021-08-17 09:47:58 +0000 UTC")
 }
+
+func TestTimestampFilter(t *testing.T) {
+	since := "7s"


no need of creating a variable for this you can directly pass the value to the function.

jainpulkit22 · 2023-01-03T10:49:46Z

pkg/support/dump_test.go

+			defer func() {
+				exe = exec.New()
+			}()
+			if err != nil {


jainpulkit22 · 2023-01-03T10:50:33Z

pkg/support/dump_windows_test.go

+	err := varr.DumpLog(basedir)
+	assert.NoError(t, err)
+
+}


leave a line after this

jainpulkit22 · 2023-01-03T10:50:52Z

pkg/support/dump_windows_test.go

+	assert.NoError(t, err)
+
+}
+func TestDumpHostNetworkInfo(t *testing.T) {


in none of the test cases you are expecting error.

jainpulkit22 · 2023-01-03T10:51:02Z

pkg/support/dump_windows_test.go

+			basedir:   "dir1/dir2",
+			since:     "3s",
+		},
+	}


jainpulkit22 · 2023-01-03T10:51:44Z

pkg/support/dump_windows_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			varr := NewAgentDumper(fs, exe, nil, nil, nil, tc.since, tc.v4Enabled, tc.v6Enabled)
+			err := varr.DumpHostNetworkInfo(tc.basedir)
+			if err != nil {


add cases with expected error and then check with expected error only instead of err.

pkg/support/dump_others_test.go

pkg/support/dump_test.go

antoninbas

make sure you address the golangci-lint errors from CI
you can reproduce locally with make golangci

pkg/support/dump_test.go

pkg/support/dump_others_test.go

pkg/support/dump_test.go

pkg/support/dump_windows_test.go

pkg/support/dump_others_test.go

pkg/support/dump_windows_test.go

antoninbas

one more nit, otherwise LGTM

antoninbas · 2023-06-05T19:27:24Z

pkg/support/dump_others_test.go

+	err := dumper.DumpLog(baseDir)
+	require.NoError(t, err)
+
+	x, err := afero.Exists(fs, filepath.Join(baseDir, "logs", "agent", "antrea-agent.log"))


rename the x variables to ok (there is another test like this as well), which is more in line with Go conventions

…ws.go Signed-off-by: Harshitha U R <harshithaur1611@gmail.com>

antoninbas

LGTM

antoninbas · 2023-06-05T19:46:08Z

/test-all

antoninbas · 2023-06-06T18:14:25Z

/test-all

NamanAg30 · 2023-06-06T18:27:51Z

@antoninbas Can this PR be merged now?

antoninbas · 2023-06-06T18:29:00Z

@antoninbas Can this PR be merged now?

I'm waiting for tests to pass (test-e2e) and the testbeds seem to be broken. cc @XinShuYang

XinShuYang · 2023-06-07T07:57:11Z

/test-e2e

XinShuYang · 2023-06-07T10:41:51Z

TestAntreaPolicy/TestGroupNoK8sNP/Case=ACNPFQDNPolicy failed, but I think it is not related to this PR? @antoninbas

NamanAg30 · 2023-06-07T10:48:34Z

@XinShuYang This is a UT pr so I don't think the failed test is related

urharshitha force-pushed the supportTesting branch 3 times, most recently from 4a244cc to ac007a1 Compare December 8, 2022 07:47

urharshitha force-pushed the supportTesting branch from ac007a1 to b155ab8 Compare December 19, 2022 09:44

urharshitha requested review from Atish-iaf, rajnkamr and jainpulkit22 December 19, 2022 09:45

urharshitha changed the title ~~Adds the unit test for pkg/support/dump_others.go~~ Adds the unit test for pkg/support/dump.go Dec 19, 2022

urharshitha force-pushed the supportTesting branch from b155ab8 to 8b3a157 Compare December 19, 2022 09:52

urharshitha marked this pull request as ready for review December 19, 2022 09:53

jainpulkit22 reviewed Dec 19, 2022

View reviewed changes

urharshitha force-pushed the supportTesting branch from 8b3a157 to 6be01c5 Compare December 19, 2022 11:08

rajnkamr reviewed Dec 20, 2022

View reviewed changes

urharshitha force-pushed the supportTesting branch 2 times, most recently from 050e7a7 to a6416f8 Compare December 22, 2022 21:09

urharshitha changed the title ~~Adds the unit test for pkg/support/dump.go~~ Adds the unit test for pkg/support Dec 22, 2022

urharshitha force-pushed the supportTesting branch from a6416f8 to 3e444ab Compare December 22, 2022 21:13

urharshitha requested review from rajnkamr and jainpulkit22 December 22, 2022 21:13

urharshitha force-pushed the supportTesting branch 2 times, most recently from 7218add to ef4895b Compare December 23, 2022 05:25

urharshitha added this to the Antrea v1.11 release milestone Jan 3, 2023

rajnkamr assigned urharshitha Jan 3, 2023

jainpulkit22 reviewed Jan 3, 2023

View reviewed changes

urharshitha force-pushed the supportTesting branch from ef4895b to e37e346 Compare January 5, 2023 06:23

urharshitha requested review from antoninbas and tnqn January 5, 2023 06:51

urharshitha force-pushed the supportTesting branch 2 times, most recently from 1776982 to 6039202 Compare January 5, 2023 08:21

NamanAg30 force-pushed the supportTesting branch from 97f38f2 to f512ffe Compare June 2, 2023 16:37

NamanAg30 requested a review from antoninbas June 2, 2023 16:52

antoninbas reviewed Jun 2, 2023

View reviewed changes

pkg/support/dump_others_test.go Show resolved Hide resolved

pkg/support/dump_test.go Show resolved Hide resolved

NamanAg30 force-pushed the supportTesting branch from f512ffe to e9e04cb Compare June 2, 2023 20:37

NamanAg30 requested a review from antoninbas June 2, 2023 20:38

antoninbas reviewed Jun 2, 2023

View reviewed changes

pkg/support/dump_test.go Show resolved Hide resolved

pkg/support/dump_others_test.go Outdated Show resolved Hide resolved

pkg/support/dump_test.go Outdated Show resolved Hide resolved

pkg/support/dump_windows_test.go Outdated Show resolved Hide resolved

NamanAg30 force-pushed the supportTesting branch 6 times, most recently from 0fe1a2d to aed06d9 Compare June 5, 2023 17:09

antoninbas reviewed Jun 5, 2023

View reviewed changes

pkg/support/dump_others_test.go Outdated Show resolved Hide resolved

antoninbas reviewed Jun 5, 2023

View reviewed changes

pkg/support/dump_windows_test.go Outdated Show resolved Hide resolved

NamanAg30 force-pushed the supportTesting branch from aed06d9 to 1d5a2b8 Compare June 5, 2023 18:21

NamanAg30 requested a review from antoninbas June 5, 2023 19:26

antoninbas reviewed Jun 5, 2023

View reviewed changes

Adds the unit test for pkg/support/dump.go and pkg/support/dump_windo…

e170c6f

…ws.go Signed-off-by: Harshitha U R <harshithaur1611@gmail.com>

NamanAg30 force-pushed the supportTesting branch from 1d5a2b8 to e170c6f Compare June 5, 2023 19:32

antoninbas approved these changes Jun 5, 2023

View reviewed changes

NamanAg30 requested a review from antoninbas June 6, 2023 16:24

antoninbas approved these changes Jun 6, 2023

View reviewed changes

antoninbas merged commit 85917f5 into antrea-io:main Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the unit test for pkg/support #4452

Adds the unit test for pkg/support #4452

urharshitha commented Dec 7, 2022

codecov bot commented Dec 7, 2022 •

edited

Loading

jainpulkit22 left a comment

jainpulkit22 Dec 19, 2022

rajnkamr Dec 20, 2022

jainpulkit22 Dec 19, 2022

rajnkamr Dec 20, 2022

jainpulkit22 Dec 19, 2022

urharshitha Dec 19, 2022

rajnkamr Dec 20, 2022

urharshitha Dec 20, 2022

antoninbas Jan 6, 2023

jainpulkit22 Dec 19, 2022

jainpulkit22 Dec 19, 2022

rajnkamr Dec 20, 2022 •

edited

Loading

jainpulkit22 left a comment

jainpulkit22 Jan 3, 2023

jainpulkit22 Jan 3, 2023

jainpulkit22 Jan 3, 2023

jainpulkit22 Jan 3, 2023

jainpulkit22 Jan 3, 2023

jainpulkit22 Jan 3, 2023

jainpulkit22 Jan 3, 2023

jainpulkit22 Jan 3, 2023

antoninbas left a comment

antoninbas left a comment

antoninbas Jun 5, 2023

antoninbas left a comment

antoninbas commented Jun 5, 2023

antoninbas commented Jun 6, 2023

NamanAg30 commented Jun 6, 2023

antoninbas commented Jun 6, 2023

XinShuYang commented Jun 7, 2023

XinShuYang commented Jun 7, 2023

NamanAg30 commented Jun 7, 2023

Adds the unit test for pkg/support #4452

Adds the unit test for pkg/support #4452

Conversation

urharshitha commented Dec 7, 2022

codecov bot commented Dec 7, 2022 • edited Loading

Codecov Report

jainpulkit22 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

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 Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

jainpulkit22 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Jun 5, 2023

antoninbas commented Jun 6, 2023

NamanAg30 commented Jun 6, 2023

antoninbas commented Jun 6, 2023

XinShuYang commented Jun 7, 2023

XinShuYang commented Jun 7, 2023

NamanAg30 commented Jun 7, 2023

codecov bot commented Dec 7, 2022 •

edited

Loading

rajnkamr Dec 20, 2022 •

edited

Loading