Add unit tests for zypper.go by bromivipo · Pull Request #1014 · GoogleCloudPlatform/osconfig

bromivipo · 2026-06-23T11:49:24Z

This PR contains unit tests for packages/zypper.go
Initial coverage 73.8%. Current coverage: 100%

google-oss-prow · 2026-06-23T11:49:51Z

Hi @bromivipo. Thanks for your PR.

I'm waiting for a GoogleCloudPlatform member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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/test-infra repository.

iliatsuprik · 2026-06-23T12:08:58Z

+			defer mockCtrl.Finish()
+
+			mockCommandRunner := utilmocks.NewMockCommandRunner(mockCtrl)
+			runner = mockCommandRunner


I suppose we can move lines 639-643 out of the test loop (that's done like this in most of the tests), so testing part would be shorter

iliatsuprik · 2026-06-23T12:09:28Z

+			defer mockCtrl.Finish()
+
+			mockCommandRunner := utilmocks.NewMockCommandRunner(mockCtrl)
+			runner = mockCommandRunner


same, can we move mockCommandRunner creation out of the loop. Can we also do this for other tests in this file?

google-oss-prow · 2026-06-23T12:10:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

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

google-oss-prow · 2026-06-23T13:19:24Z

New changes are detected. LGTM label has been removed.

burov · 2026-07-17T15:01:55Z

/gcbrun

burov · 2026-07-17T15:13:53Z

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.expectedCmd != nil {


I guess can be replaced with https://github.com/GoogleCloudPlatform/osconfig/blob/master/util/utiltest/utiltest.go#L208

burov · 2026-07-17T15:14:30Z

+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockCommandRunner.EXPECT().
+				Run(testCtx, utilmocks.EqCmd(expectedCmd)).


testCtx -> ctx, see other cases as well

Removed global testCtx usage across the file and initialized local context in each test function instead

burov · 2026-07-17T15:20:44Z

+			name:    "generic error, want wrapped error",
+			stdout:  []byte("stdout"),
+			mockErr: errors.New("generic error"),
+			wantErr: errors.New(`error running /usr/bin/zypper with args ["--gpg-auto-import-keys" "--non-interactive" "install" "--auto-agree-with-licenses" "patch:patch1" "package:pkg1"]: generic error, stdout: "stdout", stderr: "stderr"`),


Extract error to variable and give it reasonable name

Fixed both in this and one other test

burov · 2026-07-17T15:21:24Z

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockCommandRunner.EXPECT().


Same comment about util

burov · 2026-07-17T15:21:37Z

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockCommandRunner.EXPECT().


Same comment about util

burov · 2026-07-17T15:24:04Z

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


Avoid defining separate tests fro handling only negative cases, test should cover both success/fail case.

Merged TestParseZypperPatchInfo_Errors with two existing functions TestParsePatchInfo and TestParsePatchInfo_differentFormatsOfConflictPkgsVersions into TestParseZypperPatchInfo. Changed the way mappings are verified and also shortened patchInfo mocks without losing coverage for edge cases.

burov · 2026-07-28T09:48:24Z

/gcbrun

burov · 2026-07-28T10:09:25Z

+		t.Run(tt.name, func(t *testing.T) {
+			utiltest.SetExpectedCommands(ctx, mockCommandRunner, tt.expectedCommands)
+
+			_, _ = ZypperPatches(ctx, tt.options...)


Why we're not testing return parameters ?

Added the return parameter checks. Initially, I didn't test the return values here because I viewed this test primarily as a check for proper command construction with various options.

burov · 2026-08-19T09:03:32Z

/gcbrun

add unit tests for zypper.go

e5d3e81

google-oss-prow Bot added the size/L label Jun 23, 2026

google-oss-prow Bot requested review from paulinakania and savija-tv June 23, 2026 11:49

google-oss-prow Bot added the needs-ok-to-test label Jun 23, 2026

rename err -> gotErr

33a0f0a

iliatsuprik approved these changes Jun 23, 2026

View reviewed changes

google-oss-prow Bot assigned iliatsuprik Jun 23, 2026

google-oss-prow Bot added the lgtm label Jun 23, 2026

move mocks outside of the loop

0a9103d

google-oss-prow Bot removed the lgtm label Jun 23, 2026

bromivipo marked this pull request as ready for review June 24, 2026 09:01

google-oss-prow Bot requested review from burov and zoltak-g June 24, 2026 09:02

Merge branch 'master' into zypper

cec50ff

burov reviewed Jul 17, 2026

View reviewed changes

bromivipo and others added 3 commits July 20, 2026 13:21

addressed comments and refactoredparseZypperPatchInfo tests

7a3e9c4

Merge branch 'master' into zypper

e7d06bd

Merge branch 'master' into zypper

516c6e8

burov and others added 4 commits July 28, 2026 15:32

Merge branch 'master' into zypper

ac45547

Merge branch 'master' into zypper

f6e10b3

Merge branch 'master' into zypper

f3b9ebb

Merge branch 'master' into zypper

52fb043

burov reviewed Aug 19, 2026

View reviewed changes

Merge branch 'master' into zypper

f85bc61

resolve

7f18b02

google-oss-prow Bot added size/XL and removed size/L labels Aug 20, 2026

bromivipo added 3 commits August 25, 2026 10:33

Merge branch 'master' into zypper

721e106

Merge branch 'master' into zypper

ac50852

Merge branch 'master' into zypper

f245d5d

Uh oh!

Conversation

bromivipo commented Jun 23, 2026

Uh oh!

google-oss-prow Bot commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-prow Bot commented Jun 23, 2026

Uh oh!

google-oss-prow Bot commented Jun 23, 2026

Uh oh!

burov commented Jul 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bromivipo Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burov commented Jul 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

burov commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bromivipo Jul 20, 2026 •

edited

Loading