test(v2v-helper): un-skip TestLiveReplicateDisks via VMOperations refactor#1947
Open
valentin-pf9 wants to merge 2 commits into
Open
test(v2v-helper): un-skip TestLiveReplicateDisks via VMOperations refactor#1947valentin-pf9 wants to merge 2 commits into
valentin-pf9 wants to merge 2 commits into
Conversation
The v2v-helper package fails to build on main after recent merges (platform9#1927, platform9#1932). This commit restores 'make test-v2v-helper' to passing: - openstackops_mock.go: add espDiskIndex int parameter to CreateVM mock to match the interface signature in openstackops.go (PR platform9#1932 added the parameter to the interface but did not regenerate the mock). - migrate_test.go: add 11th gomock.Any() to the three EXPECT().CreateVM call sites for the same reason. - main.go: replace %s with %v throughout the logMigrationParams format string. PR platform9#1927 introduced bool fields (DisconnectSourceNetwork, FallbackToDHCP, etc.) but kept the existing %s verbs, which go vet rejects. - migrate_test.go: skip TestLiveReplicateDisks. The test panics at vmops.GetVMObj().PowerState() because the post-poweroff verification block added in platform9#1927/platform9#1932 wasn't covered when the test was written. This is a separate concern from the build fix; will track in a follow-up issue. - deploy/installer.yaml + deploy/00crds.yaml: regenerated via the pre-commit 'make build-installer' hook.
…actor PR platform9#1945 skipped TestLiveReplicateDisks because vmops.GetVMObj().PowerState(ctx) panicked with a nil pointer dereference — the mock returned an empty *object.VirtualMachine whose property collector was unset. Three call sites in migrate.go's LiveReplicateDisks (lines ~745, ~853, ~953) exercised this path after the post-poweroff verification block landed in platform9#1927/platform9#1932. Refactor to expose GetVmPowerState on the VMOperations interface (the impl already existed on *VMOps at vmops.go:130, just wasn't declared on the interface — main.go:187 already called it on the concrete type). The mock becomes trivially stubbable: mockVMOps.EXPECT().GetVmPowerState().Return(types.VirtualMachinePowerStatePoweredOff, nil).AnyTimes() Changes: - v2v-helper/vm/vmops.go: add GetVmPowerState to VMOperations interface. - v2v-helper/vm/vmops_mock.go: add mock pair for GetVmPowerState. - v2v-helper/migrate/migrate.go: replace 3 vmops.GetVMObj().PowerState(ctx) call sites with vmops.GetVmPowerState() (uses the internal vmops.ctx, same pattern as main.go:187). - v2v-helper/migrate/migrate_test.go: remove t.Skip() added in platform9#1945, add the GetVmPowerState EXPECT to the test setup. Test plan: 'make test-v2v-helper' — TestLiveReplicateDisks now passes (--- PASS: TestLiveReplicateDisks (23.02s)), no FAIL anywhere. Stacks on top of platform9#1945 (build fix); should rebase cleanly once that merges.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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.
Summary
Restores
TestLiveReplicateDisksto running. The test was skipped in #1945 with a TODO becausevmops.GetVMObj().PowerState(ctx)panicked with a nil-pointer dereference — the mock returned an empty*object.VirtualMachinewhose property collector was unset. Three call sites inmigrate.go'sLiveReplicateDisks(lines ~745, ~853, ~953, all in the post-power-off verification block added in #1927/#1932) hit the panic path.The fix
Surfaces
GetVmPowerState()on theVMOperationsinterface. The implementation already existed on*VMOpsatvmops.go:130— it just wasn't declared on the interface.main.go:187already calls it on the concrete type, so this is purely an interface surface expansion plus call-site swap.The mock becomes trivially stubbable:
Changes
v2v-helper/vm/vmops.go— addGetVmPowerState() (types.VirtualMachinePowerState, error)to theVMOperationsinterface.v2v-helper/vm/vmops_mock.go— hand-added mock pair matching the existing pattern.v2v-helper/migrate/migrate.go— replace 3vmops.GetVMObj().PowerState(ctx)call sites withvmops.GetVmPowerState(). Uses the internalvmops.ctx, same asmain.go:187.v2v-helper/migrate/migrate_test.go— remove thet.Skip()from fix(v2v-helper): repair broken build on main #1945, add theGetVmPowerStateexpectation to the test setup.Test plan
make test-v2v-helper:--- PASS: TestLiveReplicateDisks (23.02s)— no skip, no fail.Note on stacking
This PR branches on top of #1945 (build fix). Until that merges, this PR's diff will show #1945's changes too. Should rebase cleanly onto
mainonce #1945 lands — happy to do that as soon as it merges.