Skip to content

test: address MaxPowerWatts review comments from #55 #57

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bbrown-cw merged 1 commit into from
Oct 19, 2026
Merged

test: address MaxPowerWatts review comments from #55 #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions oem/smc/chassis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,10 @@ func TestChassis(t *testing.T) {
if result.BoardID != "0x1d14" {
t.Errorf("Unexpected board ID: %s", result.BoardID)
}

if result.MaxPowerWatts == nil {

@c4milo c4milo Oct 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: The setup steps in this test report errors with t.Errorf and keep going, so a setup failure panics before it reaches this check.

If the fixture stops decoding, FromChassis returns nil, err (oem/smc/chassis_test.go:94), and result.ID at line 97 panics with a nil dereference. The decode check at oem/smc/chassis_test.go:89 has the same problem. The new redfish test in this PR uses t.Fatalf for the same step.

Please switch both setup checks to t.Fatalf.

t.Error("Expected max power watts to be set")
} else if *result.MaxPowerWatts != 18000 {
t.Errorf("Unexpected max power watts: %v", *result.MaxPowerWatts)
}
}
21 changes: 19 additions & 2 deletions redfish/chassis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,10 @@ func TestChassis(t *testing.T) {
t.Errorf("Received invalid chassis type: %s", result.ChassisType)
}

if result.MaxPowerWatts == nil || *result.MaxPowerWatts != 1200 {
t.Errorf("Received invalid max power watts: %v", result.MaxPowerWatts)
if result.MaxPowerWatts == nil {

@c4milo c4milo Oct 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not blocking: MinPowerWatts is still a float64 (redfish/chassis.go:299), and the comment about it on #55 has no reply.

It has the same missing-vs-zero problem that this PR tests for MaxPowerWatts. 0 W is a valid lower bound, so a caller can't read 0 as "not reported", and a BMC that sends only "MaxPowerWatts": 1200 decodes to a min of 0. Converting it later is a second type change, with its own follow-up PR in each consumer.

Please change MinPowerWatts to *float64 with the same tests, or reply on #55 if it should wait.

t.Error("Expected max power watts to be set")
} else if *result.MaxPowerWatts != 1200 {
t.Errorf("Received invalid max power watts: %v", *result.MaxPowerWatts)
}

if result.Status.Health != common.OKHealth {
Expand Down Expand Up @@ -296,6 +298,21 @@ func TestMinimumChassis(t *testing.T) {
}
}

// TestChassisZeroMaxPowerWatts ensures a reported 0 is distinguishable from absent.
func TestChassisZeroMaxPowerWatts(t *testing.T) {
var result Chassis
err := json.NewDecoder(strings.NewReader(`{"Id": "1", "MaxPowerWatts": 0}`)).Decode(&result)
Comment on lines +301 to +304

@c4milo c4milo Oct 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: This test checks only the reported-0 case, and no test covers "MaxPowerWatts": null.

The doc comment says 0 is distinguishable from absent, but the absent check lives in TestMinimumChassis on the Supermicro RAID fixture. A change to that fixture could drop it, and nothing here would notice.

The schema also types the field as ["number", "null"], and a BMC that sends null should decode to nil like an absent field. Both cases pass today through encoding/json. A later vendor fallback on this field (like toFloat32 in redfish/power.go) could break either one without failing a test.

Please make this a small table with absent, null and 0 cases, each checking for nil or the value.

if err != nil {
t.Fatalf("Error decoding JSON: %s", err)
}

if result.MaxPowerWatts == nil {
t.Error("Expected max power watts to be set when reported as 0")
} else if *result.MaxPowerWatts != 0 {
t.Errorf("Received invalid max power watts: %v", *result.MaxPowerWatts)
}
}

// TestLinkedDriveChassis tests getting drives from versions supporting the older
// Chassis.Links.Drives location.
func TestLinkedDriveChassis(t *testing.T) {
Expand Down
Loading