Codegass opened a new issue, #6665: URL: https://github.com/apache/cloudstack/issues/6665
##### ISSUE TYPE * Improvement Request ##### COMPONENT NAME unit-test ##### OS / ENVIRONMENT N/A ##### SUMMARY The case [ testCRUDAcl](https://github.com/apache/cloudstack/blob/6125886f3d0b64ff3d0a743d00bf414774e7e2e3/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/nicira/NiciraNvpApiIT.java#L107) in the project is testing `api.findAcl()` function multiple times with 2 different scenarios (`testCRUDAclReadAll`and `testCRUDAclReadOne`) in the test case `testCRUDAcl`. The mixing of these 2 testing scenarios' arrangement make the test case's structure complicated. In short, putting multiple testing scenarios in one test case makes it hard to locate bugs in one CI\CD cycle. I suggest refactoring this case into 2 simple unit tests with one testing scenario for each unit test case. This will also improve the readability of the test cases, making developers easy to understand the test target from the test case name. ##### Proposed changes [ testCRUDAcl ](https://github.com/apache/cloudstack/blob/6125886f3d0b64ff3d0a743d00bf414774e7e2e3/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/nicira/NiciraNvpApiIT.java#L107) case is aiming to test acl curd behavior. It doesn’t have strong data dependency between different testing scenarios inside the case. So for this case, I suggest to : 1. Extract the universal arrangement section into a setup function for the test case: the arrangement from [line 108 to line 127](https://github.com/apache/cloudstack/blob/6125886f3d0b64ff3d0a743d00bf414774e7e2e3/plugins/network-elements/nicira-nvp/src/test/java/com/cloud/network/nicira/NiciraNvpApiIT.java#L108-L127) can be extracted as a function named as CRUDAclRulesAndTagsSetup with Javadoc description. (So it won't bother the developer to understand the details of this test setup) 2. Split the test scenarios with their specific arrangements into separate unit tests: each testing scenario (here is “read all” and “read one”) with their special arrangement will be split into one unit test case with the CRUDAclSetup function. 3. Rename the new unit test case with their testing scenarios and testing target: the name of new cases will be re-named by combining the original test case name `testCRUDAcl` and condition name, like `testCRUDAclReadAll`and `testCRUDAclReadOne`. Also, since this case is also asserting the exception, the try-catch structure will be carried to each unit test case. 4. (This one I am not quite sure) Since the test case is CURD, shall we also test the creation, update, and Delete with value assertion? Right now, the case seems only to add the assertion for `api.findAcl()`. After this refactoring, all the test scenarios will belong to one unit test case and can be evaluated in one cycle of testing. ###### Rationale Although we know that this refactor will bring in some redundancy in the test cases, but then it will significantly save time for the developer to identify the issue in just one test cycle, and every case will have a clear description just by their names. ###### 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
