test(scheduler): add tests for LoadConfig by magic-peach · Pull Request #2993 · Project-HAMi/HAMi

magic-peach · 2026-09-09T15:17:04Z

What this PR does / why we need it:
LoadConfig reads and parses the scheduler's YAML config file but had no test
coverage: not for a missing file, not for invalid YAML, not for the success
path every scheduler component depends on at startup. Adds TestLoadConfig
with four subtests: file not found, invalid YAML, an empty file (valid YAML,
zero-value config), and a valid config asserting the parsed nvidia and hygon
fields.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Test-only change, no production code touched. Ran go build, go vet, and the
full pkg/scheduler/config test suite locally, all pass.

Does this PR introduce a user-facing change?
No

This PR was written primarily by Claude Code, an AI coding assistant, under
my direction and review. I have read, tested, and take responsibility for
this change.

Summary by CodeRabbit

Tests
- Expanded configuration loading coverage for missing files, invalid YAML, empty configurations, and valid Nvidia and Hygon settings.
- Added verification that configuration parsing returns appropriate errors and correctly preserves supported hardware settings.

LoadConfig had no test coverage at all despite two distinct error paths, a missing file and invalid YAML, plus the success path that every scheduler component depends on at startup. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>

hami-robot · 2026-09-09T15:17:07Z

@magic-peach: The label(s) kind/test cannot be applied, because the repository doesn't have them.

Details

In response to this:

/kind test

What this PR does / why we need it:
LoadConfig reads and parses the scheduler's YAML config file but had no test
coverage: not for a missing file, not for invalid YAML, not for the success
path every scheduler component depends on at startup. Adds TestLoadConfig
with four subtests: file not found, invalid YAML, an empty file (valid YAML,
zero-value config), and a valid config asserting the parsed nvidia and hygon
fields.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Test-only change, no production code touched. Ran go build, go vet, and the
full pkg/scheduler/config test suite locally, all pass.

Does this PR introduce a user-facing change?
No

This PR was written primarily by Claude Code, an AI coding assistant, under
my direction and review. I have read, tested, and take responsibility for
this change.

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.

hami-robot · 2026-09-09T15:17:10Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: magic-peach
Once this PR has been reviewed and has the lgtm label, please assign shouren 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

coderabbitai · 2026-09-09T15:17:22Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 02abc985-8a9b-4400-910c-89f2fd4ecd7a

📥 Commits

Reviewing files that changed from the base of the PR and between b5ec6b1 and be24912.

📒 Files selected for processing (1)

pkg/scheduler/config/config_test.go

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

📝 Walkthrough

Walkthrough

The change adds TestLoadConfig coverage for missing files, invalid YAML, empty files, and valid Nvidia and Hygon configuration data.

Changes

Configuration loading validation

Layer / File(s)	Summary
LoadConfig scenario coverage `pkg/scheduler/config/config_test.go`	The test imports `os` and verifies `LoadConfig` error handling and successful parsing for empty and valid configuration files.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: ⚪ Minimal · up to be249

This change adds scheduler configuration loader tests without changing production behavior. The added cases cover missing files, invalid YAML, and empty and populated configurations, with no current merge-readiness risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding tests for the scheduler's LoadConfig function.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{A rabbit checks each config door
Missing paths knock, YAML storms roar
Empty files rest without a sound
Nvidia and Hygon fields are found
LoadConfig hops through every round}

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

codecov · 2026-09-09T15:28:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

Flag	Coverage Δ
unittests	`71.35% <ø> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Eshiv-Pandey

imo, this adds coverage for a thin os.ReadFile + yaml.Unmarshal wrapper whose parsing is already covered by Test_LoadConfig in the same file, with no linked issue. idts this pr will survive. don't close it tho.. wait for the maintainers to review this

maishivamhoo123 · 2026-09-10T15:29:11Z

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


There is already a existing Test_LoadConfig func i think it would be very confusing to add a new TestLoadConfig func ,
It would be better if you change the func name and in new function call only error case.
What do you think?

maishivamhoo123 · 2026-09-10T15:30:46Z

+func TestLoadConfig(t *testing.T) {
+	t.Run("file does not exist", func(t *testing.T) {
+		_, err := LoadConfig("/nonexistent/path/config.yaml")
+		assert.ErrorContains(t, err, "no such file or directory")


I think it would be better if we use the existing errors.Is(err, fs.ErrNotExist) instead "no such file or directory" will be better am i correct?.

moezdil · 2026-09-13T15:27:21Z

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


did you search for existing LoadConfig tests first?

moezdil · 2026-09-13T15:27:21Z

+func TestLoadConfig(t *testing.T) {
+	t.Run("file does not exist", func(t *testing.T) {
+		_, err := LoadConfig("/nonexistent/path/config.yaml")
+		assert.ErrorContains(t, err, "no such file or directory")


why does this break on windows machines?

Shouren · 2026-09-15T07:25:32Z

@magic-peach Could we consolidate these into one TestLoadConfig? The existing Test_LoadConfig calls yaml.Unmarshal directly. We could write its existing fixture to a temporary file, call LoadConfig, and retain all its field assertions, then add subtests for missing files, invalid YAML, and empty files. This would cover the actual loading path while avoiding overlapping tests and confusing names.

moezdil · 2026-09-25T21:36:52Z

Bcs of the inactivity of PR owners, it was closed. It can be re-opened if needed anytime.

Add tests for LoadConfig

be24912

LoadConfig had no test coverage at all despite two distinct error paths, a missing file and invalid YAML, plus the success path that every scheduler component depends on at startup. Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>

hami-robot Bot requested review from archlitchi and moezdil September 9, 2026 15:17

magic-peach temporarily deployed to nvidia September 9, 2026 15:34 — with GitHub Actions Inactive

Eshiv-Pandey reviewed Sep 9, 2026

View reviewed changes

maishivamhoo123 suggested changes Sep 10, 2026

View reviewed changes

moezdil reviewed Sep 13, 2026

View reviewed changes

moezdil closed this Sep 25, 2026

Navigation Menu

test(scheduler): add tests for LoadConfig - #2993

test(scheduler): add tests for LoadConfig#2993
magic-peach wants to merge 1 commit into
Project-HAMi:masterfrom
magic-peach:test-loadconfig-coverage

magic-peach commented Sep 9, 2026 •

edited

Loading

Uh oh!

hami-robot Bot commented Sep 9, 2026

Uh oh!

hami-robot Bot commented Sep 9, 2026

Uh oh!

coderabbitai Bot commented Sep 9, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented Sep 9, 2026

Uh oh!

Eshiv-Pandey left a comment

Uh oh!

maishivamhoo123 Sep 10, 2026

Uh oh!

maishivamhoo123 Sep 10, 2026

Uh oh!

moezdil Sep 13, 2026

Uh oh!

moezdil Sep 13, 2026

Uh oh!

Shouren commented Sep 15, 2026

Uh oh!

moezdil commented Sep 25, 2026

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

magic-peach commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

hami-robot Bot commented Sep 9, 2026

Uh oh!

hami-robot Bot commented Sep 9, 2026

Uh oh!

coderabbitai Bot commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented Sep 9, 2026

Codecov Report

Uh oh!

Eshiv-Pandey left a comment

Choose a reason for hiding this comment

Uh oh!

maishivamhoo123 Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

maishivamhoo123 Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

moezdil Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

moezdil Sep 13, 2026

Choose a reason for hiding this comment

Uh oh!

Shouren commented Sep 15, 2026

Uh oh!

moezdil commented Sep 25, 2026

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

magic-peach commented Sep 9, 2026 •

edited

Loading

coderabbitai Bot commented Sep 9, 2026 •

edited

Loading