Add unit tests for util/util.go by bromivipo · Pull Request #995 · GoogleCloudPlatform/osconfig

bromivipo · 2026-05-26T15:00:43Z

This PR contains unit tests for util.go
Initial coverage: 3.3%, current coverage 78.7%

google-oss-prow · 2026-05-26T15:01:12Z

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-05-28T19:25:42Z

+			}
+			if err == nil {
+				utiltest.AssertFileContents(t, tt.input, string(tt.content))
+			}


can we use here

utiltest.AssertErrorMatch(t, err, tt.wantErr) utiltest.AssertFileContents(t, tt.input, string(tt.content))

iliatsuprik · 2026-05-28T19:26:53Z

+
+			if err == nil {
+				utiltest.AssertFileContents(t, tt.input, tt.content)
+			}


can we use here AssertErrorMatch, AssertEquals and AssertFileContents without conditions? these functions should handle empty input

AssertErrorMatch could be used without conditions, but AssertFileContents fails if a file does not exist. So I moved error cases to separate tests.

AssertErrorMatch can be customized to accept options than custom comparer can help you to archive the thing you want, you can create custom comparer to compare errors by return value of Error() only. Keep in mind that you might need to craft custom filter as well to ensure it does not use exact type, but compatibility with error interface.

iliatsuprik · 2026-05-28T19:28:25Z

+		os.Exit(1)
+	}
+	fmt.Fprint(os.Stdout, "success msg")
+	os.Exit(0)


not sure if this test validates anything useful, probably we don't need it

it does not validate anything itself. But it is used as a separate process to validate DefaultRunner.

I agree with Ilia, there is no reason to keep the test that is not clear what it test to

google-oss-prow · 2026-06-01T12:25:51Z

New changes are detected. LGTM label has been removed.

burov · 2026-07-02T12:50:08Z

/gcbrun

burov · 2026-07-02T12:51:40Z

/gcbrun

burov

I see there differen

burov · 2026-07-02T12:46:20Z

+		os.Exit(1)
+	}
+	fmt.Fprint(os.Stdout, "success msg")
+	os.Exit(0)


I agree with Ilia, there is no reason to keep the test that is not clear what it test to

burov · 2026-07-02T12:49:10Z

+func TestAtomicWrite_Success(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	tests := []struct {
+		name    string
+		input   string
+		content []byte
+		mode    os.FileMode
+	}{
+		{
+			name:    "valid file path, expect success",
+			input:   filepath.Join(tmpDir, "test-write-1.txt"),
+			content: []byte("test content"),
+			mode:    0644,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := AtomicWrite(tt.input, tt.content, tt.mode)
+			utiltest.AssertErrorMatch(t, err, nil)
+			utiltest.AssertFileContents(t, tt.input, string(tt.content))
+		})
+	}
+}
+
+func TestAtomicWrite_Error(t *testing.T) {
+	tmpDir := t.TempDir()
+
+	tests := []struct {
+		name            string
+		input           string


Please refactor TestAtomicWrite to the single tests, there is no reason to have two table tests with only one case, testing the same function. It is complete against the purpose of the approach.

burov · 2026-07-02T12:49:28Z

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


Same comment as below, please refactor tests to have one test and table testing should cover all success and failure cases

burov · 2026-07-02T12:53:44Z

+	os.Exit(0)
+}
+
+func TestDefaultRunnerRun(t *testing.T) {


What this test is actually testing?

It verifies that DefaultRunner.Run captures stdout and stderr from the executed command and propagates execution errors. I've replaced the custom helper process with standard system commands to make it clearer.
Is there anything else you think this test should be checking?

burov · 2026-07-09T13:41:16Z

+			cmd:        testSuccessCmd(),
+			wantStdout: "success msg",
+			wantStderr: "",
+			wantErr:    "^<nil>$",


wantErr by convention should be an error rather then regexp, we should avoid doing regexp comparison for errors as much as possible.

burov · 2026-07-15T13:04:30Z

+
+			if err == nil {
+				utiltest.AssertFileContents(t, tt.input, tt.content)
+			}


AssertErrorMatch can be customized to accept options than custom comparer can help you to archive the thing you want, you can create custom comparer to compare errors by return value of Error() only. Keep in mind that you might need to craft custom filter as well to ensure it does not use exact type, but compatibility with error interface.

google-oss-prow · 2026-07-15T15:41:01Z

[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 ask for approval from burov. 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

bromivipo · 2026-07-15T16:23:01Z

AssertErrorMatch now accepts custom cmp.Option parameters. Added utiltest.EquateErrorMessage for exact string equality and utiltest.EquateErrorMessagePrefix for matching errors with dynamic suffixes (such as temporary filenames or path errors), so we no longer use regexps for this.

Hopefully, I understood the intent correctly.

bromivipo · 2026-07-15T16:26:08Z

+// EquateErrorMessagePrefix is a cmp.Option that compares errors by checking if either error string is a prefix of the other.
+var EquateErrorMessagePrefix = cmp.FilterValues(filterErrors, cmp.Comparer(func(got, want any) bool {
+	gotErr, wantErr := got.(error).Error(), want.(error).Error()
+	return strings.HasPrefix(gotErr, wantErr) || strings.HasPrefix(wantErr, gotErr)


Both directions are checked to satisfy cmp.Diff's symmetry requirement.

burov · 2026-07-20T14:02:23Z

+}))
+
+// EquateErrorMessagePrefix is a cmp.Option that compares errors by checking if either error string is a prefix of the other.
+var EquateErrorMessagePrefix = cmp.FilterValues(filterErrors, cmp.Comparer(func(got, want any) bool {


I suggest avoiding using Prefix matching for errors, that bring us a risk of hiding problems rather than showing the exact failure reason.

Could you explain why we cannot check exact error here ?

AtomicWrite functions append a randomized integer suffix based on time.Now() to the tmp file path and the errors contain those paths in .Error(), so the error messages are not predetermined. I could not find a way to compare exact errors without using regexp or prefix matching, so I think it's better to remove those two cases to avoid breaking the testing guidelines. I don't think we'll lose much coverage here, as those test cases were essentially just checking error propagation from syscall failures. Additionally, I've added a positive edge case to both tests instead.

burov · 2026-07-20T14:04:23Z

I guess we need to refactor usage of AssertError.* to avoid duplicating check, just add required options where it is required.

I've moved base assertion logic into an option which is now used by default when no other options are provided. Hope I understood your point correctly

burov · 2026-07-20T14:06:00Z

+			reader := strings.NewReader(tt.content)
+			gotChecksum, gotErr := AtomicWriteFileStream(reader, tt.checksum, tt.input, tt.mode)
+
+			utiltest.AssertErrorMatchAndSkip(t, gotErr, tt.wantErr, utiltest.EquateErrorMessagePrefix)


I guess that can be refactored to use exact error match, if not please explain why

replied above

burov · 2026-07-21T15:43:40Z

/gcbrun

burov · 2026-08-11T08:50:33Z

/gcbrun

burov · 2026-08-19T09:01:12Z

/gcbrun

google-oss-prow Bot added the size/L label May 26, 2026

google-oss-prow Bot requested review from burov and ekremenetskii May 26, 2026 15:01

google-oss-prow Bot added the needs-ok-to-test label May 26, 2026

bromivipo marked this pull request as ready for review May 27, 2026 11:29

google-oss-prow Bot requested a review from savija-tv May 27, 2026 11:29

iliatsuprik approved these changes May 28, 2026

View reviewed changes

google-oss-prow Bot assigned iliatsuprik May 28, 2026

google-oss-prow Bot added lgtm and removed lgtm labels May 28, 2026

bromivipo force-pushed the util branch from 0b9a4de to 22c35bf Compare June 30, 2026 16:00

burov reviewed Jul 2, 2026

View reviewed changes

burov reviewed Jul 15, 2026

View reviewed changes

burov requested changes Jul 15, 2026

View reviewed changes

google-oss-prow Bot assigned burov Jul 15, 2026

bromivipo added 5 commits July 15, 2026 13:19

add unit tests

32fabb2

resolve

6932858

prettify

c7e95b3

resolve

9741375

add options for error comparison

f721061

bromivipo force-pushed the util branch from ad3fdae to f721061 Compare July 15, 2026 15:40

bromivipo requested a review from burov July 15, 2026 16:21

bromivipo commented Jul 15, 2026

View reviewed changes

Merge branch 'master' into util

6329a59

burov requested changes Jul 20, 2026

View reviewed changes

remove cases and refactor utiltest more

5d0835b

bromivipo requested a review from burov July 21, 2026 11:29

burov and others added 6 commits July 21, 2026 17:43

Merge branch 'master' into util

b2c6b69

Merge branch 'master' into util

e3b7430

Merge branch 'master' into util

6c4b37b

Merge branch 'master' into util

508d3e2

Merge branch 'master' into util

b6f1875

Merge branch 'master' into util

196d3e6

Merge branch 'master' into util

eb9ce44

bromivipo added 3 commits August 25, 2026 10:32

Merge branch 'master' into util

0b62a00

Merge branch 'master' into util

0c65b9e

Merge branch 'master' into util

d290eb0

Uh oh!

Conversation

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

Uh oh!

Uh oh!

google-oss-prow Bot commented May 26, 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!

google-oss-prow Bot commented Jun 1, 2026

Uh oh!

burov commented Jul 2, 2026

Uh oh!

burov commented Jul 2, 2026

Uh oh!

burov left a comment

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!

google-oss-prow Bot commented Jul 15, 2026

Uh oh!

bromivipo commented Jul 15, 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!

burov commented Jul 21, 2026

Uh oh!

burov commented Aug 11, 2026

Uh oh!

burov commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

bromivipo commented May 26, 2026 •

edited

Loading