Repository navigation
test: address MaxPowerWatts review comments from #55 - #57
Conversation
| // 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) |
There was a problem hiding this comment.
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.
| t.Errorf("Unexpected board ID: %s", result.BoardID) | ||
| } | ||
|
|
||
| if result.MaxPowerWatts == nil { |
There was a problem hiding this comment.
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.
|
|
||
| if result.MaxPowerWatts == nil || *result.MaxPowerWatts != 1200 { | ||
| t.Errorf("Received invalid max power watts: %v", result.MaxPowerWatts) | ||
| if result.MaxPowerWatts == nil { |
There was a problem hiding this comment.
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.
c4milo
left a comment
c4milo
left a comment
There was a problem hiding this comment.
LGTM, left some nits and 1 non-blocking comment.
follow up on comments from #55