Add unit tests for pkg/antctl/raw/set package. #4307

jainpulkit22 · 2022-10-18T05:47:56Z

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

codecov · 2022-10-18T06:28:38Z

Codecov Report

Merging #4307 (e422c57) into main (986754c) will decrease coverage by 3.52%.
The diff coverage is n/a.

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

@@            Coverage Diff             @@
##             main    #4307      +/-   ##
==========================================
- Coverage   64.41%   60.88%   -3.53%     
==========================================
  Files         393      378      -15     
  Lines       55542    54721     -821     
==========================================
- Hits        35775    33318    -2457     
- Misses      17147    18868    +1721     
+ Partials     2620     2535      -85

Flag	Coverage Δ
integration-tests	`34.52% <0.00%> (-0.01%)`	⬇️
kind-e2e-tests	`48.65% <0.00%> (-0.25%)`	⬇️
unit-tests	`45.99% <0.00%> (-2.14%)`	⬇️

Impacted Files	Coverage Δ
...trollers/multicluster/label_identity_controller.go	`0.00% <0.00%> (-80.73%)`	⬇️
pkg/cni/client.go	`0.00% <0.00%> (-77.78%)`	⬇️
...s/multicluster/label_identity_export_controller.go	`0.00% <0.00%> (-72.93%)`	⬇️
...kg/agent/flowexporter/connections/conntrack_ovs.go	`0.00% <0.00%> (-70.91%)`	⬇️
...uster/commonarea/acnp_resourceimport_controller.go	`0.00% <0.00%> (-69.75%)`	⬇️
...rs/multicluster/commonarea/clusterinfo_importer.go	`0.00% <0.00%> (-69.65%)`	⬇️
...luster/controllers/multicluster/node_controller.go	`0.00% <0.00%> (-68.79%)`	⬇️
...s/multicluster/memberclusterannounce_controller.go	`0.00% <0.00%> (-67.75%)`	⬇️
...trollers/multicluster/clusterinfoexport_handler.go	`0.00% <0.00%> (-67.40%)`	⬇️
...ter/controllers/multicluster/gateway_controller.go	`1.47% <0.00%> (-61.77%)`	⬇️
... and 78 more

pkg/antctl/raw/set/flow-aggregator/command_test.go

pkg/antctl/raw/set/flowaggregator/command_test.go

pkg/antctl/raw/set/flowaggregator/command.go

pkg/antctl/raw/set/flowaggregator/command_test.go

luolanzone · 2022-11-07T08:36:53Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+			if cmd.Flag("kubeconfig") != nil {
+				cmd.Flag("kubeconfig").Value.Set(fakeKubeconfig.Name())
+			} else {
+				cmd.Flags().String("kubeconfig", fakeKubeconfig.Name(), "path of kubeconfig")
+			}


Why is the check needed? I feel you just need to assign the kubeconfig instead of checking cmd.Flag("kubeconfig") != nil. Please check if this is redundant or not.

Since it iterates two times, it is necessary to do a nil check because in first case if it is nil it means that the flag is not present and we need to add that flag, and in the second case when it is nil it means that the flag is already there we just need to update or set the value. If we try to set the value when the flag is not present it will generate an error in that case.

The test should initialize the cmd variable inside the loop to avoid side effect caused by previous subtests and such unnecessary check.

luolanzone · 2022-11-07T08:42:06Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+		expectedErr   string
+	}{
+		{
+			name:          "valid configMap and nil args",


I didn't see you check output of the command here when it's successful, I suppose it will print some flow aggregator information?

no it does not print any flow aggregator information, that's why i just checked for the error

luolanzone · 2022-11-07T08:50:04Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+		name        string
+		b           bool
+		value       string
+		parsedValue bool


better to name parsedValue to expectedValue

luolanzone · 2022-11-07T08:52:00Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+
+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			err := setBoolOrFail(&tc.b, tc.value)


I think you can remove field b on the tcs, and just create a new variable before this.

var actualValue bool

luolanzone · 2022-11-07T08:53:51Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+		},
+	}
+
+	_ = setStringOrFail(&tc[0].b, tc[0].value)


b is too simple for a field name, if there is only one case, you may just remove table defined tests.

luolanzone · 2022-11-07T08:54:10Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+func TestSetCommitIntervalOrFail(t *testing.T) {
+	tcs := []struct {
+		name        string
+		b           string


pkg/antctl/raw/set/flowaggregator/command_test.go

luolanzone

LGTM overall, a few nits.

luolanzone · 2022-11-09T12:35:44Z

pkg/antctl/raw/set/command.go

@@ -17,7 +17,7 @@ package set
 import (
 	"github.com/spf13/cobra"

-	flow_aggregator "antrea.io/antrea/pkg/antctl/raw/set/flow-aggregator"
+	flow_aggregator "antrea.io/antrea/pkg/antctl/raw/set/flowaggregator"


Suggested change

flow_aggregator "antrea.io/antrea/pkg/antctl/raw/set/flowaggregator"

flowaggregator "antrea.io/antrea/pkg/antctl/raw/set/flowaggregator"

luolanzone · 2022-11-09T12:41:10Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+			buf := new(bytes.Buffer)
+			cmd.SetOutput(buf)
+			cmd.SetOut(buf)
+			cmd.SetErr(buf)


I don't think you need this if you won't use buf to compare the command output with expected result.

luolanzone · 2022-11-09T12:44:16Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+		expectedOutput map[string]string
+	}{
+		{
+			name:           "valid configMap and nil args",


Suggested change

name: "valid configMap and nil args",

name: "valid ConfigMap and nil args",

Can you check all K8s resources and update them with upper case? For example pod to Pod, namespace to Namespace

luolanzone

LGTM

luolanzone · 2022-11-09T12:58:22Z

@tnqn @jianjuns @antoninbas Could you take another look? thanks.

pkg/antctl/raw/set/flowaggregator/command_test.go

tnqn · 2022-11-10T12:23:18Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+			if cmd.Flag("kubeconfig") != nil {
+				cmd.Flag("kubeconfig").Value.Set(fakeKubeconfig.Name())
+			} else {
+				cmd.Flags().String("kubeconfig", fakeKubeconfig.Name(), "path of kubeconfig")
+			}


The test should initialize the cmd variable inside the loop to avoid side effect caused by previous subtests and such unnecessary check.

pkg/antctl/raw/set/flowaggregator/command_test.go

tnqn · 2022-11-10T12:38:09Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			var actualValue string
+			err := setCommitIntervalOrFail(&actualValue, tc.value)
+			if err != nil {


tnqn · 2022-11-10T12:38:51Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+	for _, tc := range tcs {
+		t.Run(tc.name, func(t *testing.T) {
+			cm, err := GetFAConfigMap(k8sClient, tc.configMapName)
+			if err != nil {


pkg/antctl/raw/set/flowaggregator/command_test.go

tnqn · 2022-11-14T16:06:02Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+			if tc.expectedErr != "" {
+				assert.Contains(t, err.Error(), tc.expectedErr)
+			} else {
+				assert.Equal(t, tc.value, actualValue)


tnqn · 2022-11-14T16:06:33Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+			if tc.expectedErr != "" {
+				assert.Contains(t, err.Error(), tc.expectedErr)
+			} else {
+				assert.Equal(t, fakeConfigMap, cm)


tnqn · 2022-11-14T16:09:53Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+			if tc.expectedErr == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.Contains(t, err.Error(), tc.expectedErr)


The code would panic when err is nil.
assert.ErrorContains takes care of nil check and is more suitable here.

tnqn · 2022-11-14T16:10:07Z

pkg/antctl/raw/set/flowaggregator/command_test.go

+			defer os.Unsetenv("FA_CONFIG_MAP_NAME")
+			err := updateRunE(cmd, tc.args)
+			if tc.expectedErr != "" {
+				assert.Contains(t, err.Error(), tc.expectedErr)


Added unit tests to imporove the UT coverage for pkg/antctl/raw/set package. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

tnqn

LGTM

tnqn · 2022-11-15T14:20:54Z

/skip-all

tnqn · 2022-11-15T14:23:12Z

/skip-all

Added unit tests to improve the UT coverage for pkg/antctl/raw/set package. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

jainpulkit22 force-pushed the antctl-set-unit-test branch from e7ea898 to 9855dad Compare October 18, 2022 12:31

jainpulkit22 mentioned this pull request Oct 18, 2022

Improve unit test coverage #4142

Open

37 tasks

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

jainpulkit22 force-pushed the antctl-set-unit-test branch from 9855dad to 8ca8de4 Compare October 18, 2022 12:42

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

jainpulkit22 changed the title ~~Improving the ut coverage for antctl set package.~~ Improving the UT coverage for antctl set package. Oct 18, 2022

jainpulkit22 changed the title ~~Improving the UT coverage for antctl set package.~~ Add unit tests for pkg/antctl/raw/set package. Oct 18, 2022

luolanzone reviewed Oct 18, 2022

View reviewed changes

pkg/antctl/raw/set/flow-aggregator/command_test.go Outdated Show resolved Hide resolved

pkg/antctl/raw/set/flow-aggregator/command_test.go Outdated Show resolved Hide resolved

pkg/antctl/raw/set/flow-aggregator/command_test.go Outdated Show resolved Hide resolved

jainpulkit22 force-pushed the antctl-set-unit-test branch 3 times, most recently from acbc9bf to 487fd38 Compare October 19, 2022 14:49

jainpulkit22 requested a review from luolanzone October 20, 2022 08:57

jainpulkit22 force-pushed the antctl-set-unit-test branch from 487fd38 to 8c27bc3 Compare October 20, 2022 09:40

luolanzone reviewed Oct 21, 2022

View reviewed changes

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

jainpulkit22 force-pushed the antctl-set-unit-test branch from 8c27bc3 to da16c60 Compare November 4, 2022 07:55

jainpulkit22 requested a review from luolanzone November 4, 2022 09:04

luolanzone reviewed Nov 7, 2022

View reviewed changes

jainpulkit22 force-pushed the antctl-set-unit-test branch 2 times, most recently from 9abb930 to a60d0d5 Compare November 8, 2022 12:11

jainpulkit22 requested a review from luolanzone November 8, 2022 16:09

luolanzone reviewed Nov 9, 2022

View reviewed changes

jainpulkit22 force-pushed the antctl-set-unit-test branch from a60d0d5 to 2d8df12 Compare November 9, 2022 12:54

luolanzone previously approved these changes Nov 9, 2022

View reviewed changes

tnqn reviewed Nov 10, 2022

View reviewed changes

jainpulkit22 dismissed luolanzone’s stale review via 7ccad52 November 14, 2022 04:55

jainpulkit22 force-pushed the antctl-set-unit-test branch 2 times, most recently from 7ccad52 to e97a32e Compare November 14, 2022 04:59

jainpulkit22 requested a review from tnqn November 14, 2022 10:23

tnqn reviewed Nov 14, 2022

View reviewed changes

Added Unit Tests

12aca79

Added unit tests to imporove the UT coverage for pkg/antctl/raw/set package. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

jainpulkit22 force-pushed the antctl-set-unit-test branch from e97a32e to 12aca79 Compare November 15, 2022 05:36

jainpulkit22 requested a review from tnqn November 15, 2022 10:09

tnqn approved these changes Nov 15, 2022

View reviewed changes

tnqn merged commit 0582f64 into antrea-io:main Nov 15, 2022

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

Add unit tests for pkg/antctl/raw/set package (antrea-io#4307)

2bfbe91

Added unit tests to improve the UT coverage for pkg/antctl/raw/set package. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

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

Add unit tests for pkg/antctl/raw/set package (antrea-io#4307)

5d9a9b5

Added unit tests to improve the UT coverage for pkg/antctl/raw/set package. Signed-off-by: Pulkit Jain <jainpu@vmware.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for pkg/antctl/raw/set package. #4307

Add unit tests for pkg/antctl/raw/set package. #4307

jainpulkit22 commented Oct 18, 2022

codecov bot commented Oct 18, 2022 •

edited

Loading

luolanzone Nov 7, 2022

jainpulkit22 Nov 7, 2022

tnqn Nov 10, 2022

luolanzone Nov 7, 2022

jainpulkit22 Nov 7, 2022

luolanzone Nov 7, 2022

luolanzone Nov 7, 2022

luolanzone Nov 7, 2022

luolanzone Nov 7, 2022

luolanzone left a comment

luolanzone Nov 9, 2022

luolanzone Nov 9, 2022

luolanzone Nov 9, 2022

jainpulkit22 Nov 9, 2022

luolanzone left a comment

luolanzone commented Nov 9, 2022

tnqn Nov 10, 2022

tnqn Nov 10, 2022

tnqn Nov 10, 2022

tnqn Nov 14, 2022

tnqn Nov 14, 2022

tnqn Nov 14, 2022

tnqn Nov 14, 2022

tnqn left a comment

tnqn commented Nov 15, 2022

tnqn commented Nov 15, 2022

	flow_aggregator "antrea.io/antrea/pkg/antctl/raw/set/flowaggregator"
	flowaggregator "antrea.io/antrea/pkg/antctl/raw/set/flowaggregator"

	name: "valid configMap and nil args",
	name: "valid ConfigMap and nil args",

Add unit tests for pkg/antctl/raw/set package. #4307

Add unit tests for pkg/antctl/raw/set package. #4307

Conversation

jainpulkit22 commented Oct 18, 2022

codecov bot commented Oct 18, 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

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

luolanzone left a comment

Choose a reason for hiding this comment

luolanzone commented Nov 9, 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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Nov 15, 2022

tnqn commented Nov 15, 2022

codecov bot commented Oct 18, 2022 •

edited

Loading