Skip to content

add unit tests for getRuntimeString - #2831

Merged
tariq1890 merged 1 commit into
NVIDIA:mainfrom
yashrajshuklaaa:add-clusterinfo-unit-tests
Sep 3, 2026
Merged

add unit tests for getRuntimeString#2831
tariq1890 merged 1 commit into
NVIDIA:mainfrom
yashrajshuklaaa:add-clusterinfo-unit-tests

Conversation

@yashrajshuklaaa

Copy link
Copy Markdown
Contributor

Description

Adds unit tests for getRuntimeString (docker, containerd, cri-o and error cases). First test file for the clusterinfo package
which had 0% coverage. Now at 5.4%.

Testing

go test ./controllers/clusterinfo/... -v -cover. All 5 subtests pass.

@copy-pr-bot

copy-pr-bot Bot commented Aug 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Added clusterinfo unit tests for getRuntimeString. The table-driven tests cover Docker, containerd, CRI-O, unknown runtime values, and empty runtime values. Valid inputs must return the expected runtime constants. Invalid inputs must return errors.

Merge Risk: ⚪ Minimal · up to d8b1d

This PR only adds unit tests and does not change production behavior. An error-path assertion could be strengthened, but no actionable merge-blocking risk remains.


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
controllers/clusterinfo/clusterinfo_test.go-74-76 (1)

74-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the returned value for error cases.

The error branch checks only err and returns. If getRuntimeString returns a non-empty runtime with an error, these subtests still pass. Assert that result is empty before returning.

Proposed fix
			if tc.expectError {
				require.Error(t, err)
+				require.Empty(t, result)
				return
			}

Source: Path instructions


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: c075bd47-7526-4e5d-929b-dc62cee21e25

📥 Commits

Reviewing files that changed from the base of the PR and between fa49ea6 and d8b1d0b.

📒 Files selected for processing (1)
  • controllers/clusterinfo/clusterinfo_test.go

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

@kvalliyurnatt

Copy link
Copy Markdown
Contributor

/ok to test d8b1d0b

@tariq1890

Copy link
Copy Markdown
Contributor

Thanks for your contribution @yashrajshuklaaa ! Please ensure that your commits are signed. See here

@yashrajshuklaaa

Copy link
Copy Markdown
Contributor Author

@tariq1890 DCO signed already.

@kvalliyurnatt

Copy link
Copy Markdown
Contributor

@yashrajshuklaaa could you fold in the comment removal changes from your other PR here. #2832

@yashrajshuklaaa

Copy link
Copy Markdown
Contributor Author

@kvalliyurnatt sure

@yashrajshuklaaa
yashrajshuklaaa force-pushed the add-clusterinfo-unit-tests branch from d8b1d0b to 8e690a3 Compare September 3, 2026 14:30
@yashrajshuklaaa

yashrajshuklaaa commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

@kvalliyurnatt done!

@tariq1890

Copy link
Copy Markdown
Contributor

@yashrajshuklaaa commit sign off and commit signing are two different things. Very easy to confuse the two, so I don't blame you!

I am referring to signing your commits with a GPG signature. After you do this, the your commit will also show up as Verified.

Screenshot 2026-09-03 at 8 07 36 AM

Please refer to this link to get started with signed commits.

Signed-off-by: Yashraj Shukla <shuklayashraj68@gmail.com>
@yashrajshuklaaa
yashrajshuklaaa force-pushed the add-clusterinfo-unit-tests branch from 8e690a3 to 316161a Compare September 3, 2026 16:04
@yashrajshuklaaa

Copy link
Copy Markdown
Contributor Author

@tariq1890 done! I was a bit confused between DCO and commit signing. Thanks for pointing me in the right direction!

@tariq1890

Copy link
Copy Markdown
Contributor

/ok to test 316161a

@tariq1890
tariq1890 merged commit acfeab6 into NVIDIA:main Sep 3, 2026
21 checks passed
@yashrajshuklaaa

Copy link
Copy Markdown
Contributor Author

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants