Skip to content

Fix ginkgo import chain in manager cmd#591

Merged
afritzler merged 1 commit intomainfrom
fix/ginkgoimports
Jan 14, 2026
Merged

Fix ginkgo import chain in manager cmd#591
afritzler merged 1 commit intomainfrom
fix/ginkgoimports

Conversation

@afritzler
Copy link
Member

@afritzler afritzler commented Jan 14, 2026

Proposed Changes

Fix ginkgo import chain in manager cmd.

Summary by CodeRabbit

  • Tests
    • Refactored test cleanup infrastructure by consolidating state validation logic, improving test organization and maintainability across multiple test suites.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 14, 2026

Walkthrough

The change relocates the EnsureCleanState() test helper function from a dedicated test helper file to the controller suite test file and removes explicit calls to this function from multiple test file teardown hooks.

Changes

Cohort / File(s) Summary
Controller test suite reorganization
internal/controller/suite_test.go, internal/controller/test_helper.go
Moved EnsureCleanState() function from test_helper.go (deleted) to suite_test.go (added). Function validates clean API server state by asserting zero items for 12 Metal3 CRD resource types.
Test teardown cleanup removal
internal/cmd/console/console_test.go
Removed import of internal/controller and removed controller.EnsureCleanState() call with accompanying By message from AfterEach.
Webhook v1alpha1 test cleanup removal
internal/webhook/v1alpha1/biossettings_webhook_test.go, internal/webhook/v1alpha1/biosversion_webhook_test.go, internal/webhook/v1alpha1/bmcsettings_webhook_test.go, internal/webhook/v1alpha1/bmcversion_webhook_test.go, internal/webhook/v1alpha1/endpoint_webhook_test.go, internal/webhook/v1alpha1/server_webhook_test.go
Removed import of internal/controller and removed controller.EnsureCleanState() calls with accompanying By messages from AfterEach hooks across all webhook test files.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested labels

area/metal-automation

Suggested reviewers

  • Nuckal777
🚥 Pre-merge checks | ❌ 3
❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description lacks detail required by template. It repeats the title without explaining what was changed, why, or which issues are fixed. Expand description with bullet points listing the key changes (moving EnsureCleanState function, removing imports from test files), rationale, and any issue numbers being fixed.
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive Title mentions fixing 'ginkgo' import chain in 'manager' cmd, but actual changes show EnsureCleanState function relocation across test files with no visible 'manager' cmd or ginkgo import changes. Clarify if the title should reflect the actual changes (EnsureCleanState refactoring) or if the changeset is incomplete relative to the stated objective.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
internal/controller/suite_test.go (1)

330-373: Simplify the function signature by removing unnecessary error return.

The func(g Gomega) error signature with return nil is misleading. Gomega assertions propagate failures via panics that the outer Eventually intercepts—not via error return. The return nil is always reached when assertions pass, making the error type unnecessary.

Additionally, nesting g.Eventually calls inside an outer Eventually creates double-polling, which can lead to unexpectedly long timeouts on failure.

♻️ Suggested simplification
 func EnsureCleanState() {
 	GinkgoHelper()
 
-	Eventually(func(g Gomega) error {
+	Eventually(func(g Gomega) {
 		endpoints := &metalv1alpha1.EndpointList{}
-		g.Eventually(ObjectList(endpoints)).Should(HaveField("Items", HaveLen(0)))
+		g.Expect(k8sClient.List(context.Background(), endpoints)).To(Succeed())
+		g.Expect(endpoints.Items).To(BeEmpty())
 
 		bmcs := &metalv1alpha1.BMCList{}
-		g.Eventually(ObjectList(bmcs)).Should(HaveField("Items", HaveLen(0)))
+		g.Expect(k8sClient.List(context.Background(), bmcs)).To(Succeed())
+		g.Expect(bmcs.Items).To(BeEmpty())
 
-		bmcSecrets := &metalv1alpha1.BMCSecretList{}
-		g.Eventually(ObjectList(bmcSecrets)).Should(HaveField("Items", HaveLen(0)))
-		// ... similar for remaining types ...
-
-		return nil
+		// ... apply similar pattern to remaining resource types ...
 	}).Should(Succeed())
 }

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7337ee4 and 2eb7e69.

📒 Files selected for processing (9)
  • internal/cmd/console/console_test.go
  • internal/controller/suite_test.go
  • internal/controller/test_helper.go
  • internal/webhook/v1alpha1/biossettings_webhook_test.go
  • internal/webhook/v1alpha1/biosversion_webhook_test.go
  • internal/webhook/v1alpha1/bmcsettings_webhook_test.go
  • internal/webhook/v1alpha1/bmcversion_webhook_test.go
  • internal/webhook/v1alpha1/endpoint_webhook_test.go
  • internal/webhook/v1alpha1/server_webhook_test.go
💤 Files with no reviewable changes (8)
  • internal/cmd/console/console_test.go
  • internal/webhook/v1alpha1/server_webhook_test.go
  • internal/webhook/v1alpha1/bmcversion_webhook_test.go
  • internal/webhook/v1alpha1/endpoint_webhook_test.go
  • internal/webhook/v1alpha1/biossettings_webhook_test.go
  • internal/webhook/v1alpha1/biosversion_webhook_test.go
  • internal/controller/test_helper.go
  • internal/webhook/v1alpha1/bmcsettings_webhook_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Code must follow standard Go formatting and idioms; use clear, explicit error handling with minimal global state
Use small, focused functions; introduce interfaces only when justified

Files:

  • internal/controller/suite_test.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Tests must be deterministic and not rely on timing assumptions

Files:

  • internal/controller/suite_test.go
🧠 Learnings (2)
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Use Envtest-based tests for controller behavior and API interactions

Applied to files:

  • internal/controller/suite_test.go
📚 Learning: 2026-01-09T15:31:43.862Z
Learnt from: CR
Repo: ironcore-dev/metal-operator PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-09T15:31:43.862Z
Learning: Applies to **/controllers/**/*_controller_test.go : Write unit tests for reconcile logic and pure functions

Applied to files:

  • internal/controller/suite_test.go
🧬 Code graph analysis (1)
internal/controller/suite_test.go (12)
api/v1alpha1/endpoint_types.go (1)
  • EndpointList (46-50)
api/v1alpha1/bmc_types.go (1)
  • BMCList (259-263)
api/v1alpha1/bmcsecret_types.go (1)
  • BMCSecretList (62-66)
api/v1alpha1/serverclaim_types.go (1)
  • ServerClaimList (80-84)
api/v1alpha1/bmcsettings_types.go (1)
  • BMCSettingsList (91-95)
api/v1alpha1/bmcversionset_types.go (1)
  • BMCVersionSetList (60-64)
api/v1alpha1/bmcversion_types.go (1)
  • BMCVersionList (94-98)
api/v1alpha1/biosversion_types.go (1)
  • BIOSVersionList (141-145)
api/v1alpha1/biossettingsset_types.go (1)
  • BIOSSettingsSetList (60-64)
api/v1alpha1/biossettings_types.go (1)
  • BIOSSettingsList (151-155)
api/v1alpha1/servermaintenance_types.go (1)
  • ServerMaintenanceList (99-103)
api/v1alpha1/server_types.go (1)
  • ServerList (439-443)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Run test chart
  • GitHub Check: Run linter
  • GitHub Check: Run tests
  • GitHub Check: Run test e2e

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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

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

@github-actions github-actions bot added bug Something isn't working size/L labels Jan 14, 2026
@afritzler afritzler marked this pull request as ready for review January 14, 2026 09:33
@afritzler afritzler requested a review from a team as a code owner January 14, 2026 09:33
@afritzler afritzler merged commit 9bb0509 into main Jan 14, 2026
16 of 17 checks passed
@github-project-automation github-project-automation bot moved this to Done in Roadmap Jan 14, 2026
@afritzler afritzler deleted the fix/ginkgoimports branch January 14, 2026 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants