:seedling: Fix unit-tests on v1.1.x. by guettli · Pull Request #1958 · syself/cluster-api-provider-hetzner

guettli · 2026-04-13T14:42:41Z

Fix unit-tests on v1.1.x.

Failed before:

Summarizing 2 Failures:
[FAIL] HCloudMachineReconciler Basic hcloudmachine test wrong server [It] checks that ImageNotFound is visible in conditions if image does not exist
/home/guettli/syself/caph-fix-unit-tests/controllers/hcloudmachine_controller_test.go:518
[FAIL] HCloudMachineReconciler various specs without placement groups, but with placement group in hcloudMachine spec [It] should show the expected reason for server not created
/home/guettli/syself/caph-fix-unit-tests/controllers/hcloudmachine_controller_test.go:625

Ran 119 of 119 Specs in 60.768 seconds
FAIL! -- 117 Passed | 2 Failed | 0 Pending | 0 Skipped

Fixed the test, by not overwriting the condition.
Updated the test to provide a better err msg, when the test fails.
Provide better err msg, when running envtest fails. For example, when running like DEBUG=1 ginkgo run --focus "without placement groups" ./controllers/... | ./hack/filter-caph-controller-manager-logs.py -

janiskemper · 2026-04-14T09:14:34Z

-			conditions.MarkFalse(hm, infrav1.ServerCreateSucceededCondition,
-				"ServerCreateNotPossible", clusterv1.ConditionSeverityWarning,
-				"%s", err.Error())
+			if !conditions.IsFalse(hm, infrav1.ServerCreateSucceededCondition) {


is this relevant? To me it sounds like we should stick to one version (in the utility function) or here, but not both

@janiskemper what is the desired result?

Currently (pre PR):

reason="ServerCreateNotPossible" message="createServerFromImageNameOrURL failed: create server from imageName ("my-control-plane-2"): no image found with name my-control-plane-2: server create not possible - need action"

But test expected reason="ImageNotFound".

I slightly prefer ImageNotFound, but the more general reason is fine, too.

I also do that. So we should not overwrite it. But my point is that we should find a better solution ideally, not this if statement. @Dhairya-Arora01 can also look into this if you want.

@janiskemper what about merging it like it is now, and make it nice in a second PR. The CI fails for other PRs now, and I would like to fix v1.1.x soon.

@abdullah599 then plz look into that. If we have another issue, we can merge this

6d2a932

I did this (i'll revert it). I did not know we fixed it here

reason:
ServerCreateSucceededCondition are already set with the specific reason by the inner function that returned errServerCreateNotPossible. imo we must not overwrite them here.

The guard isn't needed imo. Every code path that returns errServerCreateNotPossible already sets ServerCreateSucceededCondition to False with a specific reason before returning:

createServer → InstanceHasNonExistingPlacementGroupReason

getSSHKeys → SSHKeyNotFoundReason

getServerImage → ServerTypeNotFoundReason

getServerImage → ImageAmbiguousReason

getServerImage → ImageNotFoundReason

So conditions.IsFalse(hm, infrav1.ServerCreateSucceededCondition) is always true when this handler runs.

so we can just remove this right?

@abdullah599 yes, maybe it works for now to remove that block completely. But who know if the future paths all set a condition. I recommend to leave it like this.

@guettli I wouldn't leave it like this, because the problem shouldn't exist in the first place, if this code part is removed.

janiskemper · 2026-04-14T09:15:43Z

-				Eventually(func() bool {
-					return isPresentAndFalseWithReason(key, hcloudMachine, infrav1.ServerCreateSucceededCondition, infrav1.ImageNotFoundReason)
-				}, timeout, interval).Should(BeTrue())
+				Eventually(func() error {


why do we do this? If the utility function has some issues, we oculd also solve it there probably no? I'm not able to judge on first glance what changed.

@janiskemper before we got an meaningless "false is not true" error. We get much better error now, when the test fails:

expected ServerCreateSucceeded to be False with reason "ImageNotFound",

got status="False" reason="ServerCreateNotPossible" message="createServerFromImageNameOrURL failed: create server from imageName ("my-control-plane-2"): no image found with name my-control-plane-2: server create not possible - need action"

For new code I prefer Eventually(func() error to Eventually(func() bool

But I can revert that, too. Or we write a helper which makes getting a good error message easier.

well this applies to all cases where we use this utility function. Do we have an idea how to improve that?

Added a reusable helper in test/helpers/conditions.go and switched only this Eventually to use it.

I reverted the broader rollout so this PR stays scoped to the original failing case. If we want to apply the same pattern to the other isPresentAndFalseWithReason usages, I can do that in a separate follow-up PR.

@guettli can you plz write down properly what your problem is with the isPresentAndFalseWithReason utility and then why you wanna change it and what you propose? I think that we either properly change the original utility function, we introduce a separate utility function, or we do it differently in this one case.

If no change is needed here for this PR, I would leave the topic, revert the changes, and create an issue for it.

@janiskemper I extracted that to an own PR: 🌱 test: use ConditionFalseWithReasonAtKey in controllers by guettli · Pull Request #1962 · syself/cluster-api-provider-hetzner

This reverts commit d93a20c.

janiskemper

LGTM from my side. @abdullah599 is that what you had in mind as well?

abdullah599

LGTM from my side. @abdullah599 is that what you had in mind as well?

Yes :)

🌱 Fix unit-tests on v1.1.x.

46dc5d8

github-actions Bot added size/XS Denotes a PR that changes 0-20 lines, ignoring generated files. area/code Changes made in the code directory labels Apr 13, 2026

guettli requested a review from Dhairya-Arora01 April 13, 2026 14:50

guettli marked this pull request as ready for review April 13, 2026 14:50

guettli added 2 commits April 14, 2026 09:11

Merge remote-tracking branch 'origin/v1.1.x' into tg/fix-unit-tests

6016f01

provide hint, when env test is missing binaries.

cfdf538

github-actions Bot added size/S Denotes a PR that changes 20-50 lines, ignoring generated files. area/test Changes made in the test directory and removed size/XS Denotes a PR that changes 0-20 lines, ignoring generated files. labels Apr 14, 2026

better fix.

de68006

github-actions Bot added size/M Denotes a PR that changes 50-200 lines, ignoring generated files. and removed size/S Denotes a PR that changes 20-50 lines, ignoring generated files. labels Apr 14, 2026

guettli added 3 commits April 14, 2026 10:25

Preserve specific server create error reasons

4dea907

Simplify server create condition handling

7e69a75

linting.

3736da1

Dhairya-Arora01 reviewed Apr 14, 2026

View reviewed changes

Comment thread test/helpers/envtest.go Outdated

Fix envtest error comment wording

4f3040b

guettli requested a review from Dhairya-Arora01 April 14, 2026 09:04

janiskemper reviewed Apr 14, 2026

View reviewed changes

guettli requested a review from janiskemper April 14, 2026 09:55

guettli mentioned this pull request Apr 14, 2026

🌱 Add linter to show nil panic. #1960

Merged

guettli added 3 commits April 14, 2026 13:36

test: add condition Eventually helper

d93a20c

Revert "test: add condition Eventually helper"

5e10227

This reverts commit d93a20c.

reverted changes to tests which are not needed.

94d2bd5

github-actions Bot added size/S Denotes a PR that changes 20-50 lines, ignoring generated files. and removed size/M Denotes a PR that changes 50-200 lines, ignoring generated files. labels Apr 14, 2026

guettli requested a review from abdullah599 April 14, 2026 14:14

guettli added 2 commits April 14, 2026 16:21

remove not needed code.

8ab0d33

added this to ensure last typo does not happen again.

91c28c1

github-actions Bot added size/M Denotes a PR that changes 50-200 lines, ignoring generated files. and removed size/S Denotes a PR that changes 20-50 lines, ignoring generated files. labels Apr 14, 2026

janiskemper reviewed Apr 14, 2026

View reviewed changes

abdullah599 approved these changes Apr 14, 2026

View reviewed changes

guettli merged commit aaf44ce into v1.1.x Apr 14, 2026
7 checks passed

guettli deleted the tg/fix-unit-tests branch April 14, 2026 15:07

Conversation

guettli commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

guettli Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

janiskemper left a comment

Choose a reason for hiding this comment

Uh oh!

abdullah599 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guettli commented Apr 13, 2026 •

edited

Loading

guettli Apr 14, 2026 •

edited

Loading