refactor (crc/machine) : Provide a dummy implementation for virtualMachine object for writing unit tests (#4407) #4423
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @rohanKanojia. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
rohanKanojia
force-pushed
the
pr/issue4407
branch
5 times, most recently
from
0e36ad5
to
988b812
Compare
| config: config, | ||
| diskDetails: memoize.NewMemoizer(time.Minute, 5*time.Minute), | ||
| ramDetails: memoize.NewMemoizer(30*time.Second, 2*time.Minute), | ||
| virtualMachine: vm, |
There was a problem hiding this comment.
I'd separate the commit adding this virtualMachine: vm field from the commit introducing the VirtualMachine interface. This commit is huge because of the introduction of this interface, but the changes needed for the interface changes are very mechanical (vm.bundle -> vm.Bundle) and such. If they are put in a commit doing only this, it's easier to review as we can focus on one thing, these mechanical changes.
Then in a 2nd commit come the "logic" changes which add a way to mock/test the virtual machine, and it's going to be a lot smaller, and easier to understand/review.
There was a problem hiding this comment.
I have split this PR into multiple commits, please review if it's as per your expectations. I can adjust if additional changes are needed.
There was a problem hiding this comment.
Hmm https://github.com/cfergeau/crc/commits/pr/issue4407/ is closer to what I had in mind. One commit doing only the VirtualMachine interface introduction and making use of its methods (could be 2 commits if you prefer, but I think it's fine as one). I added 2 follow-up commits which are meant to be squashed in the first one.
Then one commit adds newClientWithVirtualMachine, loadVirtualMachineLazily so that we can override it, and the rest of your series, I haven't touched it.
There was a problem hiding this comment.
By the way, the content in Solutions/Idea and Proposed Changes would be nice to have in the individual commit logs, as one line commit logs are very terse when you'll get back to them in 6 months trying to remember what the commit was about, but also too long for a short log, they overflow quite a bit in github UI, git log, ... They typically should be about 60 characters in length.
Better to have a short 1line log, followed by more details in the log explaining why you are making the change, and giving a high level overview of what you are doing if it's a big change.
rohanKanojia
force-pushed
the
pr/issue4407
branch
from
988b812
to
4d2f859
Compare
| @@ -43,7 +43,7 @@ func NewClient(name string, debug bool, config crcConfig.Storage) Client { | |||
| return newClientWithVirtualMachine(name, debug, config, nil) | |||
| } | |||
|
|
|||
| // newClientWithVirtualMachine creates a Client instance with a overridden VirtualMachine implementation. | |||
| // newClientWithVirtualMachine creates a Client instance with an overridden VirtualMachine implementation. | |||
There was a problem hiding this comment.
You could squash this in the commit where it was added.
| config: config, | ||
| diskDetails: memoize.NewMemoizer(time.Minute, 5*time.Minute), | ||
| ramDetails: memoize.NewMemoizer(30*time.Second, 2*time.Minute), | ||
| virtualMachine: vm, |
There was a problem hiding this comment.
Hmm https://github.com/cfergeau/crc/commits/pr/issue4407/ is closer to what I had in mind. One commit doing only the VirtualMachine interface introduction and making use of its methods (could be 2 commits if you prefer, but I think it's fine as one). I added 2 follow-up commits which are meant to be squashed in the first one.
Then one commit adds newClientWithVirtualMachine, loadVirtualMachineLazily so that we can override it, and the rest of your series, I haven't touched it.
| config: config, | ||
| diskDetails: memoize.NewMemoizer(time.Minute, 5*time.Minute), | ||
| ramDetails: memoize.NewMemoizer(30*time.Second, 2*time.Minute), | ||
| virtualMachine: vm, |
There was a problem hiding this comment.
By the way, the content in Solutions/Idea and Proposed Changes would be nice to have in the individual commit logs, as one line commit logs are very terse when you'll get back to them in 6 months trying to remember what the commit was about, but also too long for a short log, they overflow quite a bit in github UI, git log, ... They typically should be about 60 characters in length.
Better to have a short 1line log, followed by more details in the log explaining why you are making the change, and giving a high level overview of what you are doing if it's a big change.
…raction
Add a VirtualMachine interface and make the CRC `machine` package client use the VirtualMachine interface instead of a concrete implementation. This way we can inject a dummy test FakeVirtualMachine implementation into client tests that can ease writing tests for this package.
- Add some additional methods in VirtualMachine interface so that we can replace direct usage of struct fields with interface methods
- `Bundle()`
- `Driver()`
- `API()`
- `Host()`
- `Kill()`
Introduce a new constructor method `newClientWithVirtualMachine` in machine client that would have an additional VirtualMachine argument, this would be kept package private so that it's used only by tests in the same package.
rohanKanojia
force-pushed
the
pr/issue4407
branch
from
4d2f859
to
ada0fc6
Compare
Add FakeVirtualMachine sturct in `fakemachine` for adding dummy implementation for `VirtualMachine` interface. Currently, I've only completed methods used by `stop_test.go`, I'll add more in small increments as we implement more unit tests using this implementation. Signed-off-by: Rohan Kumar <[email protected]>
…rtualMachine implementation Add additional tests in `stop_test.go` to verify that client.Stop() updates the state of virtual machine and unexposes exposed ports as expected. Use the fake vm implementation added previously to test vm state modification behavior on stop. Signed-off-by: Rohan Kumar <[email protected]>
…rface Make VirtualMachine implement vsock methods so that vsock interaction is also done via VirtualMachine interface. This would help in writing tests, we can override the behavior of expose/unexpose in fake vm implementation. Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia
force-pushed
the
pr/issue4407
branch
from
ada0fc6
to
fac1ad5
Compare
Description
Fixes: Issue #4407
Relates to: Issue #4407, PR #4400
Type of change
test, version modification, documentation, etc.)
Checklist
Solution/Idea
machinepackage client use the VirtualMachine interface instead of a concrete implementation. This way we can inject a dummy test FakeVirtualMachine implementation into client tests that can ease writing tests for this package.Bundle()Driver()API()GetHost()VirtualMachinein the client so we can avoid creating it if it's already created.newClientWithVirtualMachinein machine client that would have an additional VirtualMachine argument, this would be kept package private so that it's used only by tests in the same package.fakemachinefor adding dummy implementation forVirtualMachineinterface. Currently, I've only completed methods used bystop_test.go, I'll add more in small increments as I get more familiar with the project.Proposed changes
VirtualMachineinterface and make client member functions use it instead ofvirtualMachinestruct.VirtualMachineimplementation from testsVirtualMachineinterface instead ofvirtualMachinestruct (these changes are only related to changing use ofvirtualMachinestruct toVirtualMachineinterface:pkg/crc/machine/ip.gopkg/crc/machine/poweroff.gopkg/crc/machine/start.gopkg/crc/machine/status.gopkg/crc/machine/stop.goTesting
It's a small refactor. I've only run E2E tests locally to verify if these changes don't introduce any kind of regression.