🌱 Fix flaky unit-tests 🎉🎉🎉 #1723
Merged
Conversation
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
Before the test did not realy test something because the state is none at the beginning (before provisioning), too.
github-actions
bot
added
size/M
Dhairya-Arora01
previously approved these changes
Nov 21, 2025
janiskemper
requested changes
Nov 21, 2025
Contributor
janiskemper
left a comment
janiskemper
left a comment
There was a problem hiding this comment.
the test doesn't test deletion anymore, but deprovisioning of the host when a hetznerbaremetalmachine is deleted. Can you explain why you change the test? Also, if you wanna do that change, you should update the test description
Background: All tests share the same reconciler and mock objects. This means if test-1 changes the mocks, then test-2 could fail. The method CreateNamespace() was changed to ResetAndCreateNamespace(). Flakiness should be reduced, because every test resets the mocks now.
github-actions
bot
added
size/L
janiskemper
requested changes
Nov 22, 2025
janiskemper
reviewed
Nov 25, 2025
janiskemper
previously approved these changes
Nov 26, 2025
use hack/filter-caph-controller-manager-logs.py to filter noise.
Contributor
|
I don't think it is a good idea to change the base branch.. Do you want to merge it to the other one? |
Collaborator
Author
I need the changes from the other branch to see the output of test failures clearly. I hope the other branch gets approved and merged soon. Then this PR goes into main. |
janiskemper
previously approved these changes
Nov 26, 2025
Contributor
janiskemper
left a comment
janiskemper
left a comment
There was a problem hiding this comment.
well the last change of the base branch dismissed my review ;)
Dhairya-Arora01
approved these changes
Nov 27, 2025
guettli
added a commit
that referenced
this pull request
Dec 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Before this PR roughly 3 of 10
make test-unitcalls failed because of a flaky test. Many tests already contained the string "(flaky)" in the name.The root-cause is that mocks and reconcilers are shared between tests.
All tests run sequentially (no concurrency). But changes done in one test could influence the second test.
Running tests in isolation (for example via
FIt()) always succeeded.This PR updates CreateNamespace() to ResetAndCreateNamespace().
Mocks get re-created via a Reset() method.
On my local device the tests were successful 70 times in a row (still running).
Introduces a Resetter/ResetAndCreateNamespace pattern for tests, to:
Resetting alone did not remove the flakiness. This PR ensures that mocks which have a state (like HCloudClient) do not get used, when a test has finished.
The background is explained in the kubebuilder docs: envTest, Namespace usage Limitations: Namespaces can't be deleted in envTests. This means objects from test-1 might still exist and getting reconciled while test-2 runs. When test-1 uses the stateful hcloudClient, although this test has already passed, it modifies the environment of test-2. Resetting does not resolve this.
This PR uses the pattern suggested by the kubebuilder docs: FooReconciler.Namespace. At the top of the Reconcile method the namespace gets checked. If set and different, reconcile is stopped.
The test for hbmm deletion was updated. It checks that the host gets deprovisioned. And the same test gets done in phase "ensure-provisioned".
HetznerBareMetalHostReconciler in "avoid conflict errors": Ignore NotFound error.
Closes #1685 #1506 #1435