test: add and cleanup vm-console-proxy operand

[codingben]

What this PR does / why we need it:

Add and cleanup operand unit tests.

Release note:

Add and cleanup vm-console-proxy operand unit tests

[codingben]

@akrejcir FYI, it can be reviewed except an issue in should create vm-console-proxy resources.

[akrejcir]

Can you also cleanup the names of functions?

[codingben]

I'll rename that PR to add only new tests, and open another PR to have refactored functions.

[codingben]

This PR is ready for review, except some issue with the commented lines:

It("should remove cluster resources on cleanup", func() {
...
		// ExpectResourceNotExists(bundle.serviceAccount, request)
		ExpectResourceNotExists(bundle.clusterRole, request)
		ExpectResourceNotExists(bundle.clusterRoleBinding, request)
		// ExpectResourceNotExists(bundle.configMap, request)
		// ExpectResourceNotExists(bundle.service, request)
		// ExpectResourceNotExists(bundle.deployment, request)
		ExpectResourceNotExists(newRoute(namespace, "vm-console-proxy"), request)
}

It throws nil error when I try to test it.

/cc @akrejcir @0xFelix

[akrejcir]

A partial review.
In general, unit tests don't need to test every single private function. Private functions can change. Tests should cover visible behavior of a module:

• Exported functions and methods
• Exported structs
• Exported fields of structs

In my opinion, an exception to this guideline is if a private function has complex implementation and we want to make sure that it works.

[codingben]

@akrejcir Ah okay :) I'm so sorry. I never wrote before operator tests. I thought we should test all operand functions because it plays critical role in deploying vm-console-proxy, it's not just another library. Thanks!

[codingben]

@akrejcir @0xFelix Thanks a lot for your reviews, really appreciated. It should be okay now. :)

[codingben]

Can you also cleanup the names of functions?

@akrejcir Sorry, what do you mean?

[codingben]

/retest

[akrejcir]

Can you also cleanup the names of functions?

@akrejcir Sorry, what do you mean?

I meant that function names in reconcile.go can be made clearer. For example reconcileServiceAccountsFuncs() can just be reconcileServiceAccount(). But now that you've split this PR into 2, that comment is relevant to the other PR.

[akrejcir]

Can you add unit tests for the annotations functionality? To test these cases:

• proxy is deployed to the namespace specified by the vm-console-proxy-namespace annotation
• when vm-console-proxy-enabled annotation is removed, the proxy resources are deleted

[codingben]

proxy is deployed to the namespace specified by the vm-console-proxy-namespace annotation

Done, please review.

when vm-console-proxy-enabled annotation is removed, the proxy resources are deleted

@akrejcir Done, please review as well.

But, what is the purpose of that test? Isn't similar to should remove cluster resources on cleanup? I mean, it sounds like it's the same test as should remove cluster resources on cleanup and we just need to remove annotation by:

request.Instance.Annotations = map[string]string{
    VmConsoleProxyNamespaceAnnotation: namespace,
}

Before we call to Cleanup?

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[codingben]

/retest

[akrejcir]

/lgtm

[codingben]

@akrejcir Seems like approval is missing and it wasn't merged because of it.

[akrejcir]

/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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 [akrejcir]

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