Refactor `QemuImgTest.testCreateAndInfo()` to avoid the "Eager Test"

[chenhao-work]

ISSUE TYPE

• Improvement Request

COMPONENT NAME

unit-test

OS / ENVIRONMENT

N/A

SUMMARY

I noticed that all the test cases in the the QemuImgTest are having qemu.create() with one another function as the testing targets. Here I will use the testCreateAndInfoWithOptions case as example.

...
        QemuImgFile file = new QemuImgFile(filename, size, PhysicalDiskFormat.QCOW2);
        String clusterSize = "131072";
        Map<String, String> options = new HashMap<String, String>();

        options.put("cluster_size", clusterSize);

        QemuImg qemu = new QemuImg(0);
        qemu.create(file, options);
        Map<String, String> info = qemu.info(file);
...

The method qemu.create(file, options) and method qemu.info(file) are the two testing targets of the test case, which makes the testing goal of the test case obscure. And when the first method qemu.create(file, options) fails, the the second method will not be tested in the CI\CD process. It will take developer sometime to make sure which method is the actual buggy one.

So I suggest split the test case into two separate case:

1. testCreateWithOptions: Extract the line 68 to 79 and add the file existing check assertTrue(file.exist()) in the end as the test case to test qemu.create(file, options) .
2. testInfoWithOptions: Extract the line line 80 to line 89 and add the sequence of methods related to qemu.create(file, options) in the beginning to create the file with option as the test case to test qemu.info (file). Here the qemu.create(file, options) is a test setup.3.

By Splitting the test case, the checking for the qemu.create(file, options) will be more detailed and make the testing target more clear and easy to locate the issue by the test case name. This will also improve the readability of the test cases, making developers easy to understand the test target from the test case name.

Operational impact

The testing refactoring won’t affect any part of the production code behavior. Also, only the original cases are replaced with unit tests.

NOTE

Please let me know if you think this proposal makes sense, If so I can submit a PR accordingly, If not, comments are appreciated.

[DaanHoogland]

@chenhao-work go ahead, as you say this won't affect production code and if the cleanup is good it is a nice enhancement to the QA of cloudstack. One concern is that if one test uses the functionality of the other as a prerequisite, and the order of execution is not controlled, the possibility that the create() fails in the info() test is still there. it would be better if the create can be mocked somehow and the info executed on a mock. In this case that might not be possible, or hard to achieve. a possibility would be to add a mock-file to test against.

please go ahead and create your PR, looking forward to it.

[rohityadavcloud]

Thanks @chenhao-work for the issue and suggestion. For the proposed changes you may go ahead and propose a PR, an issue isn't always needed or necessary for proposing a new PR. For now, I'm closing this issue as this isn't a bug.