cloudstack
cloudstack copied to clipboard
apache
Assert Missing in Test Case testCreateSuccess
ISSUE TYPE
- Improvement Request
COMPONENT NAME
unit-test
OS / ENVIRONMENT
N/A
SUMMARY
I noticed that testCreateSuccess does not contain assert statements. This is confusing to me since I do not know whether this test case pass or fail. In my understanding, testCreateSuccess is testing scaleVMCmd.execution() function to make sure the VM is created successfully. But it just ends there, not checking any of the running results.
The scaleVMCmd.execution() function has defined multiple exceptions and mocked all the values getting functions, but the test case does not have throw Exception or try-catch structure for the scaleVMCmd.execution() . Also, how many times the mocked functions are called are still not checked.
I would suggest refactoring the cases by adding execution time verifications with Mockito. The goal is to make the test condition more explicit and reusable.
if you think this makes sense, I would be more than happy to try the refactoring and submit a PR.
@Codegass I think you can leave this. if the creation is not successful it would have thrown an exception. On the other hand, this is one that is tested implicitely a lot of times as well.
@Codegass thanks for the issue - are you going to work on them? You can directly raise pull requests for your ideas, no need to open issues if you've any pull requests in coming.
@rohityadavcloud Thank you for the reply! I used to think submitting the PR directly is too strong and may disturbing the community. After read you reply, I realized that PR is a capable option for this kind of suggestions.
Based on DaanHoogland 's explanation, I understood the rationale of the design.
@Codegass we had agreed in the community, that an explicit issue isn't required if an author is ready with the PR. For support building, we encourage community to start a [DISCUSS] thread on the community dev@ ML https://cloudstack.apache.org/mailing-lists.html
Is this issue still relevant or could be closed? Thanks.
@Codegass I am closing this issue. If you feel this is invalid please reopen or create a new one