[WIP] implemented test cases for network package #677

anandpulakala · 2025-03-05T05:51:26Z

added test cases for network package

JIRA Ticket:
https://issues.redhat.com/browse/OSD-28425

openshift-ci · 2025-03-05T05:51:30Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-03-05T05:51:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anandpulakala
Once this PR has been reviewed and has the lgtm label, please assign fahlmant for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

christophermancini · 2025-03-05T13:56:52Z

cmd/network/verification_test.go

 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/ec2"
+	hivev1 "github.com/openshift/hive/apis/hive/v1"
+
+	"reflect"


nit: std lib packages should always be together

Yeah, using tooling like goimports will help with that.

@christophermancini , @btoll ,
incorporated the changes mentioned above.

christophermancini · 2025-03-05T14:00:37Z

cmd/network/packet-capture_test.go

+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"


nit: non-stdlib imports should be grouped, sorted

@christophermancini ,
incorporated the changes mentioned above.

joshbranham · 2025-03-05T16:00:07Z

cmd/network/cmd_test.go

+	ginkgo.Context("When running the network command", func() {
+
+		ginkgo.It("should create the network command", func() {
+			gomega.Expect(cmd.Use).To(gomega.Equal("network"))
+			gomega.Expect(cmd.Short).To(gomega.Equal("network related utilities"))
+			gomega.Expect(len(cmd.Commands())).To(gomega.BeNumerically(">", 0))
+		})
+
+		ginkgo.It("should add the correct subcommands", func() {
+			gomega.Expect(len(cmd.Commands())).To(gomega.BeNumerically(">", 1))
+			subCmd := cmd.Commands()[0]
+			gomega.Expect(subCmd.Use).To(gomega.Equal("packet-capture"))
+		})
+
+		ginkgo.It("should execute the network command without errors", func() {
+			cmd.SetArgs([]string{})
+			err := cmd.Execute()
+			gomega.Expect(err).To(gomega.BeNil())
+		})
+	})


Can you share a ticket for this work? I am curious the motivation for this work (more testing is good!).

However, I would question the benefit of testing things like cobra command setup. You are essentially testing that it compiles, which is already validated. I would also prefer to see basic testing done without using ginkgo, but I realize a lot of our projects have already pulled it in for some cases.

We are trying to achieve 50% of the code coverage.

The tickets for this task are below:
https://issues.redhat.com/browse/SDE-2844
https://issues.redhat.com/browse/OSD-27869
https://issues.redhat.com/browse/OSD-27870

As part of 50% code coverage, we are testing cobra command setup as well, also as part of the tickets the recommendation was to use ginkgo for testing and hence we went ahead and created the test cases based on the direction provided.

I agree with @joshbranham, we don't want to be testing the prerogatives of the cobra package, which I believe these tests are doing. We need to trust that the cobra team has already done their due diligence, leaving us free to test what we've done.

I am inclined to agree with Ben and Josh.

I will review this PR but I still would ask you remove testing for basic cobra behavior here. As far as "meeting 50% coverage" I personally will not hold your PR up for that, assuming we have added tests for our business logic 👍

Hi @joshbranham,
As per your comment, I have removed the cmd_test.go file as it only has test case for cobra command.
Below is the commit id for your reference:
0c3d955

btoll · 2025-03-05T20:45:05Z

cmd/network/verification_test.go

+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := generateServiceLog(tc.output, tc.clusterId)
+			if !reflect.DeepEqual(got, tc.want) {


I was recently looking into comparing objects and the reflect package and came across this that you may find interesting:
https://github.com/google/go-cmp

In my view, we should be using go-cmp instead of reflect.DeepEqual. There's good information in that link I provided, and the package is by the Go team.

+1 for go-cmp package

@christophermancini , @btoll

On going through the code, I found multiple references to the reflect package, one of them being the below file

https://github.com/openshift/osdctl/blob/e049bffecfb689c003fb4750ee58653afd4b8b57/cmd/account/mgmt/account-list_test.go

Hence, the decision was taken to use the reflect package.

Also, using go-cmp package will result in dependencies being installed which we would want to ideally avoid.

My thoughts on this is that even though the reflect package is used in other areas, that doesn't mean that we can't update that to use a better alternative. The commit that introduced the reflect package in account-list_test.go is four years old.

When using caching, the next build will be significantly faster. I don't know if a run is able to use the cache, though.

Also, since you brought in the assert library, why not do:

for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { got := generateServiceLog(tc.output, tc.clusterId) assert.Equal(t, tc.want.Template, got.Template) assert.Equal(t, tc.want.ClusterId, got.ClusterId) assert.Equal(t, tc.want.TemplateParams, got.TemplateParams) }) }

changed as suggested

Also, go-cmp is a readily used and respected dependency with low overhead. It would only be included in the test binary, I feel confident it would be acceptable to add. In fact, it appears to be used heavily within the org already: https://github.com/search?type=code&q=org%3Aopenshift+go-cmp

btoll · 2025-03-07T00:03:12Z

cmd/network/verification_test.go

+			want:    "",
+			wantErr: false,
+		},
+		// Add more test cases for different scenarios


Is this something that can be done now? :)

If we don't know what those scenarios are yet and creating them is out of scope for the current ticket, then lets remove the comment. It would be better to add a JIRA ticket to the backlog to define and add more test scenarios, than merge the comment to the main branch.

As requested, the extra comment has been removed.

I'm looking again at this, and it's in scope to create more unit tests for this particular method. For instance, the getCaBundleFromManagementCluster and getCaBundleFromHive could possibly have their paths tested.

The logic is somewhat confusing, too. Passing it an empty string won't return an error, so why have a wantErr field in the struct?

Hi @btoll ,
I have removed all unnecessary cases and only added case for nill error,
If we go inside fetchCluster() it requires utils.createconnection mocking and as of now we are skipping it because to mock utils we needs update its implementation from tightly coupled struct to interface based.

anandpulakala · 2025-03-10T06:19:17Z

/test all

openshift-ci · 2025-03-10T08:08:32Z

@anandpulakala: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

btoll · 2025-03-20T00:05:31Z

I think there are problems with the Test_egressVerificationGetPlatform test, specifically the following, which I believe is confusing why it is not raising an error:

        {                                                                                                                                                                                       
            name: "invalid",                                                                                                                                                                    
            e: &EgressVerification{                                                                                                                                                             
                cluster: newTestCluster(t, cmv1.NewCluster().CloudProvider(cmv1.NewCloudProvider().ID("foo"))),                                                                                 
                log:     newTestLogger(t),                                                                                                                                                      
            },        
            expectErr: true,                                                                                                                                                                          
        },

I think it should be more explicit that it is expected to fail.

btoll · 2025-03-20T00:25:25Z

cmd/network/verification_test.go

+	cmd := NewCmdValidateEgress()
+	assert.NotNil(t, cmd)
+	assert.Equal(t, "verify-egress", cmd.Use)
+}


This can be removed, it's just testing cobra.

removed as suggested

btoll · 2025-03-20T01:10:50Z

cmd/network/verification_test.go

+	}
+}
+
+func TestEgressVerification_GetPlatform(t *testing.T) {


I think it would be valuable to separate these into two distinct tests, one that tests .platformName and one that tests .cluster.

seperated to two distinct tests as suggested

btoll · 2025-03-20T02:08:45Z

pkg/k8s/client.go

+	return &LazyClient{
+		client: c,
+	}
+}


Since this is only be used by a test suite, let's move it into that suite.

client is private to k8s.LazyClient, that's why I have put LazyClientMock() in K8s.

In that case, we can use an interface which the mock would implement. We can then remove these additions from that package.

we can't mock that in packetcaptureOptions k8s.LazyClient is struct, to use mock we have to change it to interface. If we change K8s.LazyClient to interface then we have to make changes at all the places where lazyClient is being called.
if we don't want to change in client.go, then shall we skip this Test case ?

btoll · 2025-03-20T02:17:38Z

cmd/network/verification_test.go

+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ctx := context.Background()
+			err := test.e.fetchCluster(ctx)


This isn't really testing much, because when a ClusterId isn't provided it's just short-circuiting the fetchCluster method and returning nil. There's never a situation where an error would be returned.

I have removed the extra cases, now it is testing only for negative case, positive cases cannot be tested as they are using utils.createConnection(), for this we need to mock utils package.

btoll · 2025-03-20T02:40:36Z

cmd/network/packet-capture_test.go

+				reason:  tt.reason,
+				kubeCli: &k8s.LazyClient{},
+			}
+			err := ops.complete(nil, nil)


This isn't really testing much. When a reason is given, it should test the impersonated 'userName' and 'elevationReasons`.

Also, the wantErr pattern isn't useful here. The complete method will never return a legitimate error.

I have removed wantErr field, but we cannot compare userName & elevationReasons as they are private to LazyClient.

btoll · 2025-03-20T03:02:00Z

cmd/network/packet-capture_test.go

+	assert.Equal(t, 1, len(pod.Spec.Containers))
+}
+
+func TestNewPacketCaptureOptionsOne(t *testing.T) {


This looks like a duplicate of TestNewPacketCaptureOptions.

I have removed the duplicate TestNewPacketCaptureOptions as suggested.

btoll · 2025-03-20T03:03:25Z

cmd/network/packet-capture_test.go

+	assert.Equal(t, 1, len(ds.Spec.Template.Spec.Containers))
+}
+
+func TestDesiredPacketCapturePodExist(t *testing.T) {


Is this significantly different from TestDesiredPacketCapturePod?

removed as it is not required

btoll · 2025-03-20T03:04:43Z

cmd/network/packet-capture_test.go

+	assert.Equal(t, lazyClient, ops.kubeCli)
+}
+
+func TestDesiredPacketCaptureDaemonSetOne(t *testing.T) {


How is this different from TestDesiredPacketCaptureDaemonSet? Should they be grouped?

removed as it is not required

christophermancini

Several nits, but otherwise it is looking good.

christophermancini · 2025-03-21T13:04:26Z

cmd/network/packet-capture_test.go

+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	"github.com/openshift/osdctl/pkg/k8s"


nit: should be grouped at minimum with other github.com packages

reordered with github.com

christophermancini · 2025-03-21T13:08:38Z

cmd/network/packet-capture_test.go

 )

+func TestHelperProcess(t *testing.T) {


A comment describing this pattern and its value, or a link to a document describing the pattern would be helpful.

removed as it is not needed

christophermancini · 2025-03-21T13:09:57Z

cmd/network/packet-capture_test.go

+			err := ops.complete(nil, nil)
+			if tt.wantErr {
+				assert.Error(t, err)
+			} else {


nit: would prefer use of return within if conditional instead of using an else statement.

christophermancini · 2025-03-21T13:10:52Z

cmd/network/packet-capture_test.go

+			if tt.wantError {
+				assert.Error(t, err)
+				expectedErr := fmt.Errorf("failed to delete Pod %s/%s: %v", tt.pod.Namespace, tt.pod.Name, tt.mockError)
+				assert.Equal(t, expectedErr.Error(), err.Error())


nit: same as above, use return instead of else.

cmd/network/packet-capture_test.go

christophermancini · 2025-03-21T13:13:32Z

cmd/network/packet-capture_test.go

+		wantErr bool
+	}{
+		{
+			name:    "without reason",


nit: use snake_case for subtest names. Go will automatically add them, by using them in the code, a developer can copy paste the test name from terminal and paste it into CTRL+F in their IDE to find the exact test quicker.

changed to snake_case as suggested

christophermancini · 2025-03-21T13:15:27Z

cmd/network/verification_test.go

+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/runtime"
+


nit: remove newline and sort

removed extra line

christophermancini · 2025-03-21T13:15:34Z

cmd/network/verification_test.go

 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/ec2"
 	"github.com/aws/aws-sdk-go-v2/service/ec2/types"
+


nit: remove newline and sort

removed extra line

christophermancini · 2025-03-21T13:15:40Z

cmd/network/verification_test.go

 	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	"github.com/openshift-online/ocm-sdk-go/logging"
 	hivev1 "github.com/openshift/hive/apis/hive/v1"
+


nit: remove newline and sort

removed extra line

christophermancini · 2025-03-21T13:15:50Z

cmd/network/verification_test.go

 	onv "github.com/openshift/osd-network-verifier/pkg/verifier"
+


nit: remove newline and sort

removed extra line

implemented test cases for network package

5f0c643

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 5, 2025

christophermancini reviewed Mar 5, 2025

View reviewed changes

joshbranham reviewed Mar 5, 2025

View reviewed changes

btoll reviewed Mar 5, 2025

View reviewed changes

btoll reviewed Mar 7, 2025

View reviewed changes

anandpulakala and others added 2 commits March 10, 2025 11:57

Merge branch 'openshift:master' into OSD/network-unit-tests

faf70a8

Update test cases based on internal review feedback

a56c80e

anandpulakala marked this pull request as ready for review March 10, 2025 07:06

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2025

openshift-ci bot requested review from clcollins and fahlmant March 10, 2025 07:06

Anand Pulakala added 3 commits March 10, 2025 12:53

updated go.mod

2f4b3d0

used reflect instead of go-cmp

260d06e

updated go.mod

e049bff

anandpulakala marked this pull request as draft March 18, 2025 07:01

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 18, 2025

anandpulakala changed the title ~~implemented test cases for network package~~ [WIP] implemented test cases for network package Mar 18, 2025

anandpulakala and others added 2 commits March 19, 2025 09:51

Merge branch 'openshift:master' into OSD/network-unit-tests

ae73390

removed cobra tests for cmd.go

0c3d955

btoll reviewed Mar 20, 2025

View reviewed changes

changed as per the review comments

2186a44

christophermancini reviewed Mar 21, 2025

View reviewed changes

changes as per the review comments

0ab4987


		"github.com/stretchr/testify/assert"
		"k8s.io/apimachinery/pkg/runtime"

		onv "github.com/openshift/osd-network-verifier/pkg/verifier"

[WIP] implemented test cases for network package #677

Are you sure you want to change the base?

[WIP] implemented test cases for network package #677

Conversation

anandpulakala commented Mar 5, 2025 • edited Loading

openshift-ci bot commented Mar 5, 2025

openshift-ci bot commented Mar 5, 2025

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

anandpulakala Mar 6, 2025 • edited Loading

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

anandpulakala Mar 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophermancini Mar 7, 2025 • edited Loading

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

anandpulakala commented Mar 10, 2025

openshift-ci bot commented Mar 10, 2025

btoll commented Mar 20, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophermancini 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

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

anandpulakala commented Mar 5, 2025 •

edited

Loading

anandpulakala Mar 6, 2025 •

edited

Loading

anandpulakala Mar 21, 2025 •

edited

Loading

christophermancini Mar 7, 2025 •

edited

Loading

btoll commented Mar 20, 2025 •

edited

Loading