🌱 e2e: Convert Eventually closures to Gomega callbacks - #1904
Draft
bryanv wants to merge 1 commit into
Draft
Conversation
Many Eventually() closures in lib/vmoperator/vmoperator.go returned a
bare bool, discarding the underlying error or field mismatch. On
timeout Ginkgo could only report "expected false to be true", giving
no clue whether the API server was unreachable, the object didn't
exist, or a specific field was wrong.
Convert these to func(g Gomega) closures with Should(Succeed()), so a
timeout reports the actual failing sub-assertion. This covers the
functions called out in VMSVC-3940 plus the same conversion applied
consistently to every other Wait*/Verify* helper in the file that had
the same shape, since leaving some helpers converted and others not
would make the file inconsistent and reintroduce the same silent
failure mode piecemeal.
Along the way:
- Replace reflect.DeepEqual with ConsistOf for condition-changed
logging in VerifyVirtualMachineGroupPublishRequestCompleted, per
VMSVC-3940.
- Replace the Expect(false).To(BeTrue()) anti-pattern in
WaitForVirtualMachineImageCacheReady and
WaitForSubnetOrSubnetSetToBeDeleted with proper nil/condition
checks and Fail(), and fix the vm.Status.Conditions vs
vm.GetConditions() accessor inconsistency this touched.
- Drop redundant fmt.Sprintf() calls where Gomega already accepts a
format string and args.
- Update Go doc comments on several Wait*/Verify* functions to name
the function per convention instead of a generic "Utility function
to ..." lead-in, and add a missing timeout message to two
Eventually calls that previously failed with no description.
- Fix two typos: a doubled negative ("cannot not be fetched") and a
duplicated space in a log format string.
- Extract the VDS/NSX/VPC network-provider lookup logic that
GetVirtualMachineNetworkProviderIP and the WaitForVMNetworkProviderInfo
helpers each implemented separately into three shared
list*ProviderInfo functions, used by both the single-shot and the
retrying callers.
Behavior change: GetVirtualMachineNetworkProviderIP previously
returned "" if the topology-appropriate network-provider object
existed but had no owner reference matching the VM; it now fails the
spec immediately with a descriptive error, matching how it already
handled an empty list. The VDS/NSX branches cannot both apply to a
single config, so no other fall-through path is affected. Otherwise,
no behavior change is intended: retry semantics, timeouts, and
pass/fail outcomes for currently-passing specs are preserved.
Verified with go build and go vet across the test/e2e module; not
yet run against a live WCP cluster.
Fixes VMSVC-3940
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Minimum allowed line rate is |
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
1 participant
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.
What does this PR do, and why is it needed?
Many Eventually() closures in lib/vmoperator/vmoperator.go returned a bare bool, discarding the underlying error or field mismatch. On timeout Ginkgo could only report "expected false to be true", giving no clue whether the API server was unreachable, the object didn't exist, or a specific field was wrong.
Convert these to func(g Gomega) closures with Should(Succeed()), so a timeout reports the actual failing sub-assertion. This covers the functions called out in VMSVC-3940 plus the same conversion applied consistently to every other Wait*/Verify* helper in the file that had the same shape, since leaving some helpers converted and others not would make the file inconsistent and reintroduce the same silent failure mode piecemeal.
Along the way:
Behavior change: GetVirtualMachineNetworkProviderIP previously returned "" if the topology-appropriate network-provider object existed but had no owner reference matching the VM; it now fails the spec immediately with a descriptive error, matching how it already handled an empty list. The VDS/NSX branches cannot both apply to a single config, so no other fall-through path is affected. Otherwise, no behavior change is intended: retry semantics, timeouts, and pass/fail outcomes for currently-passing specs are preserved.
Which issue(s) is/are addressed by this PR? (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # vmop-3940
Are there any special notes for your reviewer:
Please add a release note if necessary: