Improve test reliability by waiting for resources to be fully deleted - #472
Merged
Conversation
Added `Eventually(IsNotFound)` waits after deleting the primary resource and before deleting the Device in `AfterEach`. Without the wait, the Device could be deleted before the controller had a chance to finalize the resource — causing `GetDeviceByName` to fail, the finalizer to never be removed, and the provider state to never be cleaned up. Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Signed-off-by: Adam Trizuljak <adam.trizuljak@sap.com>
Merging this branch will decrease overall coverage
Coverage by fileChanged unit test files
|
Contributor
Author
|
Extracting just the test fixes from the Configured condition PR does not fully fix the tests. The fix must depend on some other code and logic changes that have been introduced in that PR. I would like to merge this as-is and keep building upon it. |
felix-kaestner
approved these changes
Aug 3, 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
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.
While working on #426 I noticed that the tests are unreliable. Typical rate was 1-2 fails out of 10 runs of the test suite. Different tests would fail each time and it was hard to reproduce the failures reliably.
The root cause seems to be this. Envtest doesn't run any controllers, therefore the tests need to delete the created resources manually. This is already handled in the existing
AfterEachblocks. However the deletion could happen too quickly, before the controller could finalize the resource(s). This would then trigger a cascade failure, where resources that should have been deleted are still present (or vice versa) and other related conditions.For example in the Device tests, the added
Eventually(IsNotFound)waits after deleting the primary resource and before deleting the Device inAfterEach. Without the wait, the Device could be deleted before the controller had a chance to finalize the resource — causingGetDeviceByNameto fail, the finalizer to never be removed, and the provider state to never be cleaned up.Also the
EthernetSegmentControllerwas registered insuite_test.gowithoutRequeueInterval, meaning it would always run only 1 round of reconciliation and never get polled again.This PR consists of the extracted test fixes from #426 + several small additional fixes that were required to make this PR pass the tests on its own.
Also adding the generated
cover.htmlfile to gitignore