Add unit tests for wrapped.go

[iholder101]

What this PR does / why we need it:
This PR will increase our test value and test coverage and reach our goal of 73% coverage.

Release note:

NONE

[iholder101]

/cc @stu-gott @dhiller

[iholder101]

/retest

[iholder101]

/retest

[EdDev]

Adding tests is always a good thing, thank you for that.

I do have some issues with tests that use global variables and perform the check against unexported functions/methods.
Testable code requires a level of modularity that benefits tests but also future changes to the production code. Using global functions does not contribute to the code modularity, nor to the ability to maintain it during future refactoring.

Golang provides light interfaces to assist and make code both modular and testable.
I think we should try and use that and avoid function pointers that are replaced by tests. It is unsafe and hard to maintain.

The same goes with testing internal functions. These functions are expected to change as refactoring happens or functionality expands. But if tests depend on the internal implementation, they will break frequently and make it hard to safely change code which is not affecting the external API of the package.
If it does make sense to tests internal functions, it is probably a sign they need to appear in a separate package with a proper API and a small scoped tests to cover them.

• I am aware that the current codebase is not agreeing with me, but it is worth raising this from time to time, trying to change the current norm.

[dhiller]

Adding tests is always a good thing, thank you for that.

I do have some issues with tests that use global variables and perform the check against unexported functions/methods. Testable code requires a level of modularity that benefits tests but also future changes to the production code. Using global functions does not contribute to the code modularity, nor to the ability to maintain it during future refactoring.

Can you elaborate and be more concrete on this please? I can't follow.

Golang provides light interfaces to assist and make code both modular and testable. I think we should try and use that and avoid function pointers that are replaced by tests. It is unsafe and hard to maintain.

The same goes with testing internal functions. These functions are expected to change as refactoring happens or functionality expands. But if tests depend on the internal implementation, they will break frequently and make it hard to safely change code which is not affecting the external API of the package. If it does make sense to tests internal functions, it is probably a sign they need to appear in a separate package with a proper API and a small scoped tests to cover them.

So you are saying that we shouldn't write test for code that is expected to change? I think the opposite is true? In general a test would verify that the refactoring didn't break anything, as that is exactly the definition of refactoring, so this would indicate that the tests are bad? Can you elaborate?

* I am aware that the current codebase is not agreeing with me, but it is worth raising this from time to time, trying to change the current norm.

[dhiller]

In general I think that breaking up isolation to make code testable is a sacrifice well worth to be made. While I agree that global functions and variables are better avoided in general, when this is done for testing, I'd say this is ok.

[dhiller]

/approve

/hold waiting for the discussion to settle.

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhiller

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

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

[EdDev]

Testable code requires a level of modularity that benefits tests but also future changes to the production code. Using global functions does not contribute to the code modularity, nor to the ability to maintain it during future refactoring.

Can you elaborate and be more concrete on this please? I can't follow.

Using a global variable to hold a function, just because you need to replace it in a test is a hack. It does not make the production code modular or serve it in any other way, it actually makes it harder to understand, harder to debug, fragile in testing because changes may leak between tests and even has performance penalty (not that we care much in this case, but still).
It also has dependencies on how tests are ran, e.g. if tests are executed in parallel using threads or goroutines, it will cause a mess.

The goal is to write testable code that not only serves tests, but serves the production code in making it modular, easy to change and readable. The test job (as I see it) is also to show how to use the portion of code it tests, it is after all another user of it.

The same goes with testing internal functions. These functions are expected to change as refactoring happens or functionality expands. But if tests depend on the internal implementation, they will break frequently and make it hard to safely change code which is not affecting the external API of the package. If it does make sense to tests internal functions, it is probably a sign they need to appear in a separate package with a proper API and a small scoped tests to cover them.

So you are saying that we shouldn't write test for code that is expected to change? I think the opposite is true? In general a test would verify that the refactoring didn't break anything, as that is exactly the definition of refactoring, so this would indicate that the tests are bad? Can you elaborate?

It is expected that all code paths are covered through the package API, leaving you room to change the internal details without touching the tests. If you trigger an internal function/method directly, you most likely will break the test when you refactor and will not be able to trust the existing tests to protect you.
If you cannot cover a path through the API (the exported package entities), it means the code is not testable and it requires some work to make it so.

I am experiencing this on every line of code I change these days, which is both painful and risky.

/hold waiting for the discussion to settle

By approving, you have made your decision already. I have only commented and not voted intentionally as I know this is the existing standard in this project. I have no intention in blocking contributions in areas I do not maintain, I just noticed the change when the bot sent me a review request and felt the need to point out the issues I see with this.

No need to hold it because of me.

[iholder101]

/retest

[iholder101]

/retest

[iholder101]

/retest
@dhiller ping

[iholder101]

/retest

[dhiller]

/unhold

[iholder101]

@dhiller rebased

[iholder101]

/retest

[Barakmor1]

/lgtm

[iholder101]

@dhiller seems like pull-kubevirt-e2e-k8s-1.20-cgroupsv2 is stuck?

[dhiller]

/test pull-kubevirt-e2e-k8s-1.20-cgroupsv2

[kubevirt-commenter-bot]

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

[iholder101]

/test pull-kubevirt-e2e-k8s-1.20-cgroupsv2

[dhiller]

/hold

Disabling retest-bot due to high cluster load.

[dhiller]

/unhold