[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user asfgit closed the pull request at: https://github.com/apache/cloudstack/pull/1350 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rhtyd commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-216423449 @swill yes that's what I meant, therefore commented that it's ready for merge. Thanks. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-216308956 @rafaelweingartner thanks for the review. :) I think what @rhtyd meant is anything outstanding is only cosmetic, I don't think he meant the entire discussion. I think this one is ready to merge... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-216307184 @swill, I did a review on the comments and discussions to get me up to date (we had a discussion about some code duplications). Those discussions were pretty good, I believe that there was some block of code being repeated, and also some blocks (or lines I do not recall) that were never used or not needed. I did a code walkthrough and I cannot say anything about the code. It is very good now. I can for sure give an LGMT here ;) Sorry guys to say this, but, those discussions were not merely about cosmetics! --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user swill commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-216300372 @rafaelweingartner This one seems to be in OK shape. I am missing one code review, given that you have actively reviewed this code, can I get your final status? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rhtyd commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-216221000 LGTM, there are some comments on cosmetics but in general we should get this CI tested and merged cc @swill tag:easypr --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user bvbharatk commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-200186297 ### ACS CI BVT Run **Sumarry:** Build Number 121 Hypervisor xenserver NetworkType Advanced Passed=104 Failed=14 Skipped=4 _Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 **Failed tests:** * :setup Failed * integration.smoke.test_privategw_acl.TestPrivateGwACL * test_01_vpc_privategw_acl Failed **Skipped tests:** test_vm_nic_adapter_vmxnet3 test_deploy_vgpu_enabled_vm test_06_copy_template test_06_copy_iso **Passed test suits:** integration.smoke.test_deploy_vm_with_userdata.TestDeployVmWithUserData integration.smoke.test_affinity_groups_projects.TestDeployVmWithAffinityGroup integration.smoke.test_portable_publicip.TestPortablePublicIPAcquire integration.smoke.test_over_provisioning.TestUpdateOverProvision integration.smoke.test_global_settings.TestUpdateConfigWithScope integration.smoke.test_scale_vm.TestScaleVm integration.smoke.test_service_offerings.TestCreateServiceOffering integration.smoke.test_loadbalance.TestLoadBalance integration.smoke.test_routers.TestRouterServices integration.smoke.test_reset_vm_on_reboot.TestResetVmOnReboot integration.smoke.test_snapshots.TestSnapshotRootDisk integration.smoke.test_deploy_vms_with_varied_deploymentplanners.TestDeployVmWithVariedPlanners integration.smoke.test_network.TestDeleteAccount integration.smoke.test_non_contigiousvlan.TestUpdatePhysicalNetwork integration.smoke.test_deploy_vm_iso.TestDeployVMFromISO integration.smoke.test_public_ip_range.TestDedicatePublicIPRange integration.smoke.test_multipleips_per_nic.TestDeployVM integration.smoke.test_regions.TestRegions integration.smoke.test_affinity_groups.TestDeployVmWithAffinityGroup integration.smoke.test_network_acl.TestNetworkACL integration.smoke.test_pvlan.TestPVLAN integration.smoke.test_volumes.TestCreateVolume integration.smoke.test_ssvm.TestSSVMs integration.smoke.test_nic.TestNic integration.smoke.test_deploy_vm_root_resize.TestDeployVM integration.smoke.test_resource_detail.TestResourceDetail integration.smoke.test_secondary_storage.TestSecStorageServices integration.smoke.test_vm_life_cycle.TestDeployVM integration.smoke.test_disk_offerings.TestCreateDiskOffering --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-176082402 @DaanHoogland I agree there should be some guideline on code style. If these are there I think most of us will be willing to follow these. I guess it will not be difficult to adapt something that is already out there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user DaanHoogland commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-176080946 @agneya2001 Sounds like you want to set the style guide. you and @rafaelweingartner should take this discussion to a higher level and take it to a discuss thread. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175931632
This code that was suggested:
private AccountVO createAccountVoForType(short type) {
AccountVO accountVO = new AccountVO();
accountVO.setType(type);
return accountVO;
}
@Test
public void testAdminLockableAccount() {
AccountVO accountVO =
createAccountVoForType(Account.ACCOUNT_TYPE_ADMIN);
assertFalse(quotaManager.isLockable(accountVO));
}
@Test
public void testAdminLockableAccount() {
AccountVO accountVO =
createAccountVoForType(Account.ACCOUNT_TYPE_ADMIN);
assertFalse(quotaManager.isLockable(accountVO));
}
and this code that will otherwise be:
@Test
public void testResourceDomainAdminLockableAccount() {
AccountVO accountVO = new AccountVO();
accountVO.setType(Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN);
assertFalse(quotaManager.isLockable(accountVO));
}
@Test
public void testProjectLockableAccount() {
AccountVO accountVO = new AccountVO();
accountVO.setType(Account.ACCOUNT_TYPE_PROJECT);
assertFalse(quotaManager.isLockable(accountVO));
}
@rafaelweingartner To me the code below looks simple and more readable.
(In general if this was not the test code some of this code will be in
constructor. Coming to constructors, there is much more repeatable code there
in many cloudstack classes.) Now this might not look good to you and some
others too. I suggest if that is the case take some effort and put up a code
style guideline or at least adopt a guideline that already exists so that we
donot have to go thru this. This will also be food for further cleanup as
findbug and code style check already are CC: @bhaisaab @remibergsma
@DaanHoogland @cristofolini. Till then this stands as it is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175702905 @bhaisaab I think the test cases in different methods, each one for a single and unit test is the right way to go. What I disagree is with the creation of âAccountVOâ object. Anyone that looks at lines 202 â 204, can see that they are the same as 211- 214. Additionally, those "Id" and "DomainId" parameters that are being set are not used by the âisLockableâ method. On top of everything, can you please look at lines 221-222, 231-232 and so forth; that is a clear example of code duplication without even thinking. ``` AccountVO accountVO = new AccountVO(); accountVO = new AccountVO(); ``` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1350#discussion_r51005837
--- Diff:
framework/quota/test/org/apache/cloudstack/quota/QuotaManagerImplTest.java ---
@@ -197,4 +197,58 @@ public void testProcessQuotaBalanceForAccount() {
Mockito.verify(quotaAcc,
Mockito.times(1)).persistQuotaAccount(Mockito.any(QuotaAccountVO.class));
}
+@Test
+public void testAdminLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(2L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_ADMIN);
+assertFalse(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testNormalLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(3L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_NORMAL);
+assertTrue(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void tesDomainAdmingLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(5L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_DOMAIN_ADMIN);
+assertTrue(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testReadOnlyAdminLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(6L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_READ_ONLY_ADMIN);
+assertFalse(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testResourceDomainAdminLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(7L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN);
+assertFalse(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testProjectLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(7L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_PROJECT);
+assertFalse(quotaManager.isLockable(accountVO));
--- End diff --
@abhinandanprateek We can actually remove the setId and setDomainId in all
the testxxxLockableAccounts; create like a single class leve AccountVO and then
in each of the testxxxLockableAccounts methods simply call the setType method
and then call assert on them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1350#discussion_r51006727
--- Diff:
framework/quota/test/org/apache/cloudstack/quota/QuotaManagerImplTest.java ---
@@ -197,4 +197,58 @@ public void testProcessQuotaBalanceForAccount() {
Mockito.verify(quotaAcc,
Mockito.times(1)).persistQuotaAccount(Mockito.any(QuotaAccountVO.class));
}
+@Test
+public void testAdminLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(2L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_ADMIN);
+assertFalse(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testNormalLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(3L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_NORMAL);
+assertTrue(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void tesDomainAdmingLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(5L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_DOMAIN_ADMIN);
+assertTrue(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testReadOnlyAdminLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(6L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_READ_ONLY_ADMIN);
+assertFalse(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testResourceDomainAdminLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(7L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN);
+assertFalse(quotaManager.isLockable(accountVO));
+}
+
+@Test
+public void testProjectLockableAccount() {
+AccountVO accountVO = new AccountVO();
+accountVO.setId(7L);
+accountVO.setDomainId(1L);
+accountVO.setType(Account.ACCOUNT_TYPE_PROJECT);
+assertFalse(quotaManager.isLockable(accountVO));
--- End diff --
@bhaisaab I don't agree with the change as it impacts simplicity and
quality. All the three lines that you are asking me have a data element that
will become a function parameter, I am not sure how it will simplify or result
in enhanced quality.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175693496 @rafaelweingartner fair points, but it would make sense if you can comment on the diff in the context of the change. Specific to the PR I see several testXXXLockableAccount which tries to test account types separately. Are you suggesting to get them all refactored, say put in a single testLockableAccount() and use a single Account/User object to test the lockability instead of explicitly separate test methods? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175708121 @agneya2001 how about you have a look at the test tomorrow and see if you can incorporate some of the suggestions like pull out the account from the method as a class variable? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175710627 @rafaelweingartner @bhaisaab I have taken out the duplicate account initialisation. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175712977 @cristofolini your changes are incorporated can you review these. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1350#issuecomment-175816413
I disagree that it would make the code more complex.
That is what I meant:
```
private AccountVO createAccountVoForType(short type) {
AccountVO accountVO = new AccountVO();
accountVO.setType(type);
return accountVO;
}
@Test
public void testAdminLockableAccount() {
AccountVO accountVO =
createAccountVoForType(Account.ACCOUNT_TYPE_ADMIN);
assertFalse(quotaManager.isLockable(accountVO));
}
...
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175563484 @agneya2001 LGTM again, need one more review to get this merged along with some testing (though in this case, we've some unit tests for the change) --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175656788 @rafaelweingartner It is only about realistic test data that is being passed to the method tested. You should differentiate between the tested code and the code that is testing. The logic/code that is being tested should have all the characteristics that you are talking about ie any data that is not being used should not be there. But when it comes to writing tests and creating mocks you do it in a more realistic manner without deducing what could be happening inside the method as you only test what the method is supposed to be doing. That makes it error proof in future and that was what i was talking about. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175634392 @rafaelweingartner If it is a black box test then the person testing will obviously try to pass a realistic data set, that is what I have done. It is possible that tomorrow there are some changes to the tested method and the data that looks superficial may come into play and expose some erratic behaviour. The dataset passed mocks the dataset that will go thru the method in real runtime condition. Now offcourse we can see that the data is not being used in this method but does that mean I should change the dataset that mocks the real value ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175637289 If in a remotely distant future someone that we do not even know yet changes that method and make use of some extra data (forgetting to check the test cases), the test cases should catch that and then the person that is doing that change should fix the tests. That is why we write test cases, for the code of today, not for the code of tomorrow. IMO we should prepare the architecture of software to the future, but the code itself should not be developed waiting for some futuristic hypothetical condition that may not ever happen. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175599430 Hi @agneya2001, there are a lot of duplicated lines at âQuotaManagerImplTestâ, would you mind removing them? Additionally, the lines you use to set the domainId and the id at the AccountVO object, you do not need to execute them, the method that you want to test does not need them. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175630573 It is cultural; we have to stop duplicating lines; no matter where and no matter how simple those blocks may seem. We also have to stop coding things that are not needed as you did in the test. If a code is not needed, why bother writing it down? I am referring to the âaccountVO.setIdâ and accountVO.setDomainIdâ at the tests you wrote. I do not know if ACS has a guideline for that, for me, it looks like common sense. That was my opinion, but if others here are ok with the duplication of lines, feel free to merge the PR. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175609511 @agneya2001 that is exactly what I meant. I disagree with you, letting this as they are, it is not simple. Why duplicate code, when we can do better? Especially if we use a good and descriptive method name. About the initialization, I also disagree with you, at that point those values are not used, so they are needed and you do not need to code those lines. If those attributes are needed somewhere else, they should be used only at the point in which they are needed. Please, keep in mind the less the better. We need quality in our code, not quantity. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175624327 @rafaelweingartner quality is what we are discussing and I simply do not think that putting 3 lines of object initialisation in a method will make it high quality. Are you referring to some guidelines that I can see too ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175601093 @rafaelweingartner Can you comment on the duplicated lines. There is a way you can get to the code and comment there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175601814 I know that. I just did not want to go one by one and tell you that they are duplicated, I believe you can easily see that. Those duplicated lines I mentioned are in the test methods you created. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175603740 @rafaelweingartner if you mean initialisation of AccountVO to be moved into a method, and replacing it with a method call. I guess I would prefer it this way as this keeps things simple. The initialisation for domainId and id is done as the method will receive similar object, so no harm in passing a object that is close to the one passed at runtime, though as you said the function tested is not using it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user rafaelweingartner commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175667113 I do not agree. If people are comfortable with it, so let it be. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user agneya2001 commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-175408926 @cristofolini @bhaisaab @remibergsma test code split into simpler methods. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user cristofolini commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-174211441 I see you've put a bunch of different test cases into a single `testLockableAccount` method. I'd strongly suggest you put each of those different cases into their own methods, so that if one of them fails in a build, we can easily determine what is failing. As it stands, that single test would fail if any of those cases goes wrong, leaving us to guess what failed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Quota: consolidated lockable account chec...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1350#issuecomment-173218775 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
