🌱 Add test coverage for VimMachineService

[laozc]

What this PR does / why we need it:
Add test coverage for VimMachineService in pkg/services
Address #2308

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (919c03f) 63.33% compared to head (6cdadaa) 63.89%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2444      +/-   ##
==========================================
+ Coverage   63.33%   63.89%   +0.55%     
==========================================
  Files         119      119              
  Lines        8592     8596       +4     
==========================================
+ Hits         5442     5492      +50     
+ Misses       2729     2682      -47     
- Partials      421      422       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[laozc]

/retest-required

[killianmuldoon]

Question on this @laozc - do you have a way to run these tests individually locally? Especially through an IDE?

I have a problem running tests formatted in this way i.e. using Describe instead of a Test.* function in Intellij/Goland, makes it hard to write tests and hard to review them.

[laozc]

Question on this @laozc - do you have a way to run these tests individually locally? Especially through an IDE?

I have a problem running tests formatted in this way i.e. using Describe instead of a Test.* function in Intellij/Goland, makes it hard to write tests and hard to review them.

I wrote the test and tested it with the help of Goland which could tell me the coverage in details.
Only one drawback that these tests could not be run independently.

What did you meet when you were running these tests?

[chrischdi]

Only one drawback that these tests could not be run independently.

I think that's the main point Killian thought of.
Maybe we can converge to the pattern we use in CAPI to have a TestMain func and by that are able to run the tests independently, at least for the new tests we are introducing.
With that said: I don't know how much we would need to change for that 🤔

[laozc]

I believe a plugin Ginkgo needs to be installed to support running the test separately in Goland/IntelliJ IDEA.
We have a few tests written in Ginkgo at the moment.

[killianmuldoon]

The Ginkgo plugin does work which is good for existing tests. Going forward though I think we should conform to the upstream CAPI policy described here.

In Cluster API you MUST use ginkgo only for E2E tests, where it is required to leverage the support for running specs in parallel; in any case, developers MUST NOT use the table driven extension DSL (DescribeTable, Entry commands) which is considered unintuitive.

I'll make a comment to this effect on the umbrella issue too.

[laozc]

Thanks for the info.
I took some time to move all the tests in this file away from Ginkgo.

[sbueringer]

Sorry, I'll get back to this in a bit. I want to get one bigger change merged first (#2527). Then we can rebase / review / merge this PR

[sbueringer]

@laozc #2527 will merge in a bit. Do you have time to rebase this PR once #2527 is merged?

[laozc]

Sure. Let me rebase it.

[sbueringer]

Thank you very much!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 20bf5f88212315683841005af9a7e06aeb3ef7ae