Move BMC and BIOS Version CRDs unit test to go mock server#829
Merged
Conversation
nagadeesh-nagaraja
changed the title
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds mock Redfish firmware-upgrade task resources and implements asynchronous upgrade progression in the Go mock server, refactors RedfishLocalBMC upgrade/version methods to shared helpers and updates task parsing, and migrates test suite to use a slice of mock servers with per-server reset hooks. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MockServer
participant TaskSvc as Task Service
participant Resources as System/Manager Resources
Client->>MockServer: POST SimpleUpdate (ImageURI, Targets)
activate MockServer
MockServer->>TaskSvc: Load initial task template (`index.json`)
MockServer->>Client: 202 + Location (task monitor URI)
deactivate MockServer
note over MockServer: launch goroutine (generation guarded)
loop async progression
MockServer->>TaskSvc: Merge next step from `steps.json` / `steps-fail.json`
MockServer->>MockServer: Update stored override
end
alt Completed terminal step
MockServer->>Resources: Resolve RelatedItem targets
MockServer->>Resources: Write ImageURI to BIOS/BMC version fields
MockServer->>MockServer: Record updated resources
else Exception terminal step
note over MockServer: No resource version update
end
Client->>MockServer: GET task monitor URI (poll)
activate MockServer
MockServer->>Client: Return current task state JSON
deactivate MockServer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/controller/bmcsettings_controller_test.go (1)
86-95:⚠️ Potential issue | 🟡 MinorReset BMC mock state after cleanup, not before it.
Line 87 clears the mock while the BMC/server and any failed-spec leftovers may still reconcile. A later reconcile can reapply settings after the reset, leaving stale mock state for the next test. Move this after
EnsureCleanState().Suggested teardown ordering
- mockServers[0].ResetBMCSettings("BMC") - Expect(k8sClient.Delete(ctx, bmc)).To(Succeed()) Eventually(UpdateStatus(server, func() { server.Status.State = metalv1alpha1.ServerStateAvailable })).To(Succeed()) Expect(k8sClient.Delete(ctx, server)).To(Succeed()) Expect(k8sClient.Delete(ctx, bmcSecret)).To(Succeed()) EnsureCleanState() + mockServers[0].ResetBMCSettings("BMC")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/bmcsettings_controller_test.go` around lines 86 - 95, The ResetBMCSettings call is happening too early and can be undone by later reconciles; move mockServers[0].ResetBMCSettings("BMC") to the end of the AfterEach teardown, after EnsureCleanState() so the mock is reset only once all Kubernetes deletes and the EnsureCleanState() check complete; update the AfterEach block that contains ResetBMCSettings, EnsureCleanState, UpdateStatus, and k8sClient.Delete calls accordingly.internal/controller/biossettings_controller_test.go (1)
78-83:⚠️ Potential issue | 🟡 MinorMove the mock BIOS reset after Kubernetes cleanup.
These
AfterEachblocks reset the Redfish mock before deleting resources and beforeEnsureCleanState(). If a spec fails early, remaining BIOSSettings/controllers can reconcile after the reset and write state back into the mock, leaking into the next spec. Move the reset afterEnsureCleanState().Suggested teardown ordering
- mockServers[0].ResetBIOSSettings(path.Base(server.Spec.SystemURI)) - Expect(k8sClient.Delete(ctx, bmcSecret)).To(Succeed()) Expect(k8sClient.Delete(ctx, server)).To(Succeed()) EnsureCleanState() + mockServers[0].ResetBIOSSettings(path.Base(server.Spec.SystemURI))Apply the same ordering to the other two
AfterEachblocks in this file.Also applies to: 1058-1064, 1387-1392
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/biossettings_controller_test.go` around lines 78 - 83, The teardown resets the Redfish mock (mockServers[0].ResetBIOSSettings(...)) before cleaning up k8s resources and calling EnsureCleanState(), which can allow controllers to reconcile and write back into the mock when a spec fails; move the mockServers[0].ResetBIOSSettings(...) call to after EnsureCleanState() and after deleting bmcSecret and server (the k8sClient.Delete calls) inside the AfterEach blocks (including the other two AfterEach blocks that mirror this pattern) so the cluster is fully cleaned before the mock is reset.
🧹 Nitpick comments (1)
internal/controller/biosversion_controller_test.go (1)
76-81: Prefer a full upgrade-task reset in teardown.These
AfterEachblocks depend on the localserver.Spec.SystemURIbeing populated. If a spec fails before the server object is refreshed, the scoped reset may miss the in-flight task. Since set tests already use the no-arg reset, consider usingResetUpgradeTask()here too.Suggested teardown reset
- mockServers[0].ResetUpgradeTask(server.Spec.SystemURI) + mockServers[0].ResetUpgradeTask()Also applies to: 476-481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/biosversion_controller_test.go` around lines 76 - 81, In the AfterEach teardown in biosversion_controller_test.go replace the scoped reset call that uses server.Spec.SystemURI with the no-arg reset to ensure any in-flight upgrade tasks are cleared even if server.Spec wasn't populated; specifically update the mockServers[0].ResetUpgradeTask(server.Spec.SystemURI) calls (present in the AfterEach blocks around the test and again at lines ~476-481) to mockServers[0].ResetUpgradeTask() so the teardown performs a full reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bmc/mock/server/server.go`:
- Around line 369-374: handleSimpleUpdate currently ignores json.Unmarshal
errors, allowing malformed payloads to be treated as valid; update the function
(handleSimpleUpdate) to check the error returned from json.Unmarshal(body, &req)
and if non-nil write an appropriate HTTP error (e.g., http.Error with status 400
Bad Request) and return instead of proceeding to accept the request; ensure the
error path logs or includes the unmarshal error context so tests and callers see
the failure.
---
Outside diff comments:
In `@internal/controller/biossettings_controller_test.go`:
- Around line 78-83: The teardown resets the Redfish mock
(mockServers[0].ResetBIOSSettings(...)) before cleaning up k8s resources and
calling EnsureCleanState(), which can allow controllers to reconcile and write
back into the mock when a spec fails; move the
mockServers[0].ResetBIOSSettings(...) call to after EnsureCleanState() and after
deleting bmcSecret and server (the k8sClient.Delete calls) inside the AfterEach
blocks (including the other two AfterEach blocks that mirror this pattern) so
the cluster is fully cleaned before the mock is reset.
In `@internal/controller/bmcsettings_controller_test.go`:
- Around line 86-95: The ResetBMCSettings call is happening too early and can be
undone by later reconciles; move mockServers[0].ResetBMCSettings("BMC") to the
end of the AfterEach teardown, after EnsureCleanState() so the mock is reset
only once all Kubernetes deletes and the EnsureCleanState() check complete;
update the AfterEach block that contains ResetBMCSettings, EnsureCleanState,
UpdateStatus, and k8sClient.Delete calls accordingly.
---
Nitpick comments:
In `@internal/controller/biosversion_controller_test.go`:
- Around line 76-81: In the AfterEach teardown in biosversion_controller_test.go
replace the scoped reset call that uses server.Spec.SystemURI with the no-arg
reset to ensure any in-flight upgrade tasks are cleared even if server.Spec
wasn't populated; specifically update the
mockServers[0].ResetUpgradeTask(server.Spec.SystemURI) calls (present in the
AfterEach blocks around the test and again at lines ~476-481) to
mockServers[0].ResetUpgradeTask() so the teardown performs a full reset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7b462d3-6431-4ab4-92e0-6268433ad604
📒 Files selected for processing (14)
bmc/mock/server/data/TaskService/Tasks/upgrade/index.jsonbmc/mock/server/data/TaskService/Tasks/upgrade/steps-fail.jsonbmc/mock/server/data/TaskService/Tasks/upgrade/steps.jsonbmc/mock/server/server.gobmc/redfish_local.gointernal/controller/biossettings_controller_test.gointernal/controller/biossettingsset_controller_test.gointernal/controller/biosversion_controller_test.gointernal/controller/biosversionset_controller_test.gointernal/controller/bmcsettings_controller_test.gointernal/controller/bmcsettingsset_controller_test.gointernal/controller/bmcversion_controller_test.gointernal/controller/bmcversionset_controller_test.gointernal/controller/suite_test.go
afritzler
approved these changes
Apr 24, 2026
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #828
Summary by CodeRabbit
New Features
Chores