De-normalize tests and ensure clean state before each `It()` run by afritzler · Pull Request #516 · ironcore-dev/metal-operator

afritzler · 2025-11-03T08:21:13Z

Proposed Changes

Use RunnableFunc to start registry in manager
Denormalize tests by ensuring each It() block leaves the API server clean

In future we should run EnsureCleanState after each It() block for new tests to ensure that we have a clean state to start the tests. The problem in our tests are ClusterScoped objects which might cause side effects when left around.

For that reason we are having now a:

AfterEach(func(ctx SpecContext) {
        ...
	EnsureCleanState()
})

Additionally: We should postpone the introduction of complex helper methods for now even when that means that we have code duplication in our tests.

nagadeesh-nagaraja

took a brief look, looks like this is still not yet ready?

nagadeesh-nagaraja · 2025-11-03T12:57:22Z

internal/controller/biossettings_controller_test.go

+		By("Ensuring that the Server is in available state")
+		Eventually(UpdateStatus(server, func() {
+			server.Status.State = metalv1alpha1.ServerStateAvailable
+		})).To(Succeed())


are we going back to patching the state of server and not allowing it to transition to Available?

I have seen that this might cause flakiness in server state later. and cause the server Status to not be populated..

I would do the transitioning via agent and registry setup in the BMC and Server reconciler as this is their core responsibility. Repeating this test in other controllers seems arbitrary. Ideally the BIOS/BMCSettings/Version controller only rely on the contract that a Server has to be available.

But I think we are trying to divert the issue. what is the problem allowing it to transition to Available state?
is the helper doing something wrong, then lets fix it. is it flaky do to some other reason, lets root cause it and fix it.

taking a shortcut will only solve the problem for so long.

The overall idea is as mentioned in the PR message is to reduce the helper function code and make each It block more explicit.

nagadeesh-nagaraja · 2025-11-03T12:58:36Z

internal/controller/biossettings_controller_test.go

+
+		Expect(k8sClient.Delete(ctx, bmcSecret)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, server)).To(Succeed())
+		EnsureCleanState()


I think we need to come up with a better plan to clean up?
example:
server is not allowed to delete in Maintenance state..
biosSettings and other maintenance resource also do not go well with delete when inProgress

we need to make sure its deleted properly? else rest of the tests will hit issue nevertheless..

Well the idea of that PR is, that each It block is responsible for leaving a clean state. Yes that will be tricky in the beginning but we can think about later how we can fix that issue by improving the controller coding. In this PR I didn't want to touch any reconciler logic at all.

I think it will be beneficial to have a common helper which can be called which will make sure all the resources are deleted..
just need to make sure that the helper handles all cases.. force delete the resources if necessary.
and we can also make sure that the test in the beginning will clean by calling the same function :)

But force deleting means, that somehow there is a problem in the deletion flow. I think we should rather ensure that we catch problems in a deletion flow if a graceful removal is not possible.

I think cleaning up after each It block sounds good and we would be checking Delete reconciles as well.

nagadeesh-nagaraja · 2025-11-03T13:00:20Z

internal/controller/biossettings_controller_test.go

-		TransitionServerToReservedState(ctx, k8sClient, serverClaim, server, ns.Name)
+		By("Creating an Ignition secret")
+		ignitionSecret := &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "test-",
+			},
+			Data: nil,
+		}
+		Expect(k8sClient.Create(ctx, ignitionSecret)).To(Succeed())
+
+		By("Creating a ServerClaim")
+		serverClaim := &metalv1alpha1.ServerClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "test-",
+			},
+			Spec: metalv1alpha1.ServerClaimSpec{
+				Power:             metalv1alpha1.PowerOff,
+				ServerRef:         &v1.LocalObjectReference{Name: server.Name},
+				IgnitionSecretRef: &v1.LocalObjectReference{Name: ignitionSecret.Name},
+				Image:             "foo:bar",
+			},
+		}
+		Expect(k8sClient.Create(ctx, serverClaim)).To(Succeed())
+
+		By("Ensuring that the Server has been claimed")
+		Eventually(Object(server)).Should(
+			HaveField("Status.State", metalv1alpha1.ServerStateReserved),
+		)


why this revert? can update the common helper function itself?

many tests use it and is very helpful.

nagadeesh-nagaraja · 2025-11-03T13:00:36Z

internal/controller/biossettings_controller_test.go

+		By("Creating an Ignition secret")
+		ignitionSecret := &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "test-",
+			},
+			Data: nil,
+		}
+		Expect(k8sClient.Create(ctx, ignitionSecret)).To(Succeed())
+
+		By("Creating a ServerClaim")
+		serverClaim := &metalv1alpha1.ServerClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace:    ns.Name,
+				GenerateName: "test-",
+			},
+			Spec: metalv1alpha1.ServerClaimSpec{
+				Power:             metalv1alpha1.PowerOff,
+				ServerRef:         &v1.LocalObjectReference{Name: server.Name},
+				IgnitionSecretRef: &v1.LocalObjectReference{Name: ignitionSecret.Name},
+				Image:             "foo:bar",
+			},
+		}
+		Expect(k8sClient.Create(ctx, serverClaim)).To(Succeed())
+
+		By("Ensuring that the Server has been claimed")
+		Eventually(Object(server)).Should(
+			HaveField("Status.State", metalv1alpha1.ServerStateReserved),
+		)


same as above?

nagadeesh-nagaraja · 2025-11-03T13:05:18Z

internal/controller/biossettingsset_controller_test.go

+		Expect(k8sClient.Delete(ctx, server01)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, server02)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, server03)).To(Succeed())
+		Expect(k8sClient.Delete(ctx, bmcSecret)).To(Succeed())


how is this different from the DeleteAllMetalResources(ctx, ns.Name) funtion helper?

- Use `RunnableFunc` to start registry in `manager` - Denormalize tests by ensuring each `It()` block leaves the API server clean

Nuckal777

Thanks. 👍

Nuckal777 · 2025-11-07T14:40:12Z

internal/controller/bmcversionset_controller_test.go

+			Expect(k8sClient.Delete(ctx, &maintenance)).To(Succeed())
+		}
+
+		server01 := &metalv1alpha1.Server{


I would argue that the BMC and Server cleanup could be moved in the AfterEach block for deduplication, because the BMCs are setup in the BeforeEach block.

Thanks for pointing this out. Moved the server cleanup for both It blocks to the AfterEach.

Nuckal777 · 2025-11-07T14:40:26Z

internal/controller/bmcversionset_controller_test.go

+			Expect(k8sClient.Delete(ctx, &maintenance)).To(Succeed())
+		}
+
+		server01 := &metalv1alpha1.Server{


I would argue that the BMC and Server cleanup could be moved in the AfterEach block for deduplication, because the BMCs are setup in the BeforeEach block.

github-project-automation bot added this to Roadmap Nov 3, 2025

github-actions bot added size/XXL enhancement New feature or request labels Nov 3, 2025

afritzler force-pushed the enh/tests branch 2 times, most recently from 7348d54 to 4f32a62 Compare November 3, 2025 08:32

nagadeesh-nagaraja reviewed Nov 3, 2025

View reviewed changes

hardikdr added the area/metal-automation label Nov 4, 2025

afritzler force-pushed the enh/tests branch 9 times, most recently from be88bbf to 19eba2d Compare November 5, 2025 16:11

afritzler mentioned this pull request Nov 6, 2025

Fix lldp tests on darwin #521

Merged

afritzler force-pushed the enh/tests branch 4 times, most recently from 2ef792d to ef4b7c1 Compare November 6, 2025 12:05

De-normalize tests and ensure clean state before each It() run

5d1726b

- Use `RunnableFunc` to start registry in `manager` - Denormalize tests by ensuring each `It()` block leaves the API server clean

afritzler force-pushed the enh/tests branch from ef4b7c1 to 5d1726b Compare November 6, 2025 12:50

Nuckal777 reviewed Nov 7, 2025

View reviewed changes

Update bmc version set test

ebd5a08

Nuckal777 approved these changes Nov 7, 2025

View reviewed changes

afritzler merged commit fe4f215 into main Nov 7, 2025
15 checks passed

afritzler deleted the enh/tests branch November 7, 2025 15:46

github-project-automation bot moved this to Done in Roadmap Nov 7, 2025

Conversation

afritzler commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

nagadeesh-nagaraja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Nuckal777 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

afritzler commented Nov 3, 2025 •

edited

Loading