You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/08/22 19:04:01 UTC
[GitHub] [cloudstack] Codegass opened a new issue, #6665: Refactor testCRUDAcl to improve the test experience
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: commits-unsubscribe@cloudstack.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Refactor testCRUDAcl to improve the test experience [cloudstack]
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud closed issue #6665: Refactor testCRUDAcl to improve the test experience
URL: https://github.com/apache/cloudstack/issues/6665
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Codegass commented on issue #6665: Refactor testCRUDAcl to improve the test experience
Posted by GitBox <gi...@apache.org>.
Codegass commented on issue #6665:
URL: https://github.com/apache/cloudstack/issues/6665#issuecomment-1227922368
@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.
I will try to submit the PR this week.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on issue #6665: Refactor testCRUDAcl to improve the test experience
Posted by GitBox <gi...@apache.org>.
rohityadavcloud commented on issue #6665:
URL: https://github.com/apache/cloudstack/issues/6665#issuecomment-1227301215
@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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on issue #6665: Refactor testCRUDAcl to improve the test experience
Posted by GitBox <gi...@apache.org>.
DaanHoogland commented on issue #6665:
URL: https://github.com/apache/cloudstack/issues/6665#issuecomment-1223648978
@Codegass are you planning to do this yourself? any milestone to add?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on issue #6665: Refactor testCRUDAcl to improve the test experience
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on issue #6665:
URL: https://github.com/apache/cloudstack/issues/6665#issuecomment-1603851158
@Codegass can we close this issue, or are you still considering this? I like your sugestion but doubt anybody else will take the time in the short run.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on issue #6665: Refactor testCRUDAcl to improve the test experience
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on issue #6665:
URL: https://github.com/apache/cloudstack/issues/6665#issuecomment-1537905000
@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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] Codegass commented on issue #6665: Refactor testCRUDAcl to improve the test experience
Posted by "Codegass (via GitHub)" <gi...@apache.org>.
Codegass commented on issue #6665:
URL: https://github.com/apache/cloudstack/issues/6665#issuecomment-1604511012
Hi @DaanHoogland , Thank you so much for the the follow up. I apologize for the long waiting of the PR submission, I was working on my other accounts and missed the notification.
I will submit PR this weekends(no later than June 25th).
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org