ISSUE-2428 refactor: improve test for EnvsClustersTenantsControllerService #2517
Conversation
EnvsClustersTenantsControllerService .deleteTenant .addTenantId Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getUpdateEnvStatus .deleteEnvironment .addNewCluster .getSyncEnvs Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getEnvDetails Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getClusters Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getMyTenantInfo Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getAllTenants Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getEnvsForRequestTopicsCluster Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .updateTenant Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .deleteCluster Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getClustersPaginated Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getClustersPaginated Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService Signed-off-by: Luke Zhou <[email protected]>
|
@luke-zhou Thanks a million for your contribution 🎉 , I am a little busy at the moment but I will try to start the review this evening or tomorrow morning. |
There was a problem hiding this comment.
Thank you @luke-zhou for this contribution.
I had some stylistic comments, but nothing else just yet.
I need to do some testing on the change but wanted to give you the head up on the mainly import related request.
Thanks you again for your contribution!
| import io.aiven.klaw.model.enums.EntityType; | ||
| import io.aiven.klaw.model.enums.KafkaClustersType; | ||
| import io.aiven.klaw.model.enums.RequestStatus; | ||
| import io.aiven.klaw.model.enums.*; |
There was a problem hiding this comment.
Can you check your IDE settings, we try to avoid using .* on imports, but a lot of them will try to automatically use .* after about 5 entries. We normally set them to 20 before using .*
There is a nice answer from stack overflow on how to set this on Intellij https://stackoverflow.com/a/3348855
| import io.aiven.klaw.model.enums.RequestMode; | ||
| import io.aiven.klaw.model.enums.RequestOperationType; | ||
| import io.aiven.klaw.model.enums.RequestStatus; | ||
| import io.aiven.klaw.model.enums.*; |
There was a problem hiding this comment.
Same here on the imports. Thanks!
| import io.aiven.klaw.model.enums.RequestMode; | ||
| import io.aiven.klaw.model.enums.RequestOperationType; | ||
| import io.aiven.klaw.model.enums.RequestStatus; | ||
| import io.aiven.klaw.model.enums.*; |
There was a problem hiding this comment.
Imports here too please
| import io.aiven.klaw.model.enums.KafkaSupportedProtocol; | ||
| import io.aiven.klaw.model.enums.MetadataOperationType; | ||
| import io.aiven.klaw.model.enums.PermissionType; | ||
| import io.aiven.klaw.model.enums.*; |
There was a problem hiding this comment.
Imports here as well.
| @@ -1103,12 +981,9 @@ public ClusterInfo getClusterInfoFromEnv(String envSelected, String clusterType) | |||
| ClusterInfo clusterInfo = new ClusterInfo(); | |||
| log.debug("getEnvDetails {}", envSelected); | |||
There was a problem hiding this comment.
This is a pre existing log stmt but could you update it to include the method name "getClusterInfoFromEnv"?
| import static org.assertj.core.api.Assertions.assertThatExceptionOfType; | ||
| import static io.aiven.klaw.error.KlawErrorMessages.*; | ||
| import static io.aiven.klaw.helpers.KwConstants.*; | ||
| import static org.assertj.core.api.Assertions.*; |
There was a problem hiding this comment.
just imports here as well. Thanks
Signed-off-by: Luke Zhou <[email protected]>
Signed-off-by: Luke Zhou <[email protected]>
|
Hey @luke-zhou I'm almost finished testing this PR I am sorry for the delay, I've set aside tomorrow to finish it off. Sorry again for the delay! |
EnvsClustersTenantsControllerService .deleteTenant .addTenantId Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getUpdateEnvStatus .deleteEnvironment .addNewCluster .getSyncEnvs Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getEnvDetails Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getClusters Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getMyTenantInfo Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getAllTenants Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getEnvsForRequestTopicsCluster Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .updateTenant Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .deleteCluster Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getClustersPaginated Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService .getClustersPaginated Signed-off-by: Luke Zhou <[email protected]>
EnvsClustersTenantsControllerService Signed-off-by: Luke Zhou <[email protected]>
Signed-off-by: Luke Zhou <[email protected]>
Signed-off-by: Luke Zhou <[email protected]>
core/src/main/java/io/aiven/klaw/service/EnvsClustersTenantsControllerService.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/aiven/klaw/service/EnvsClustersTenantsControllerService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Hey @luke-zhou just finished testing the changes, they look great I even managed to find a pre-existing bug during testing 😓
Aside from Muralis nits I think this PR is good to merge.
Thank you so much for your contribution!
…ntrollerService.java Co-authored-by: Murali Basani <[email protected]>
…ntrollerService.java Co-authored-by: Murali Basani <[email protected]>
|
Thanks, committed suggestion @muralibasani |
Hey @luke-zhou I just noticed the last commit I think needs to be signed, would you be able to sign off your last commit? |
Signed-off-by: Luke Zhou <[email protected]>
Hi @aindriu-aiven I have signed the last commit, and pushed. However I am having issues to sign the following two commits, which i made directly on the PR page: |
Ah I've seen this before let me check if there is a way to do this and I'll let you know asap. Thanks for checking for me! |
Hey I had a chat with some colleagues in this space and apparently we can use This will sign off all the commits you made until HEAD. If you don't mind giving that a go it should let us pass the DCO and merge the PR. Thanks a million! |
Hey @luke-zhou just wanted to see if you think you would be able to give the above command a try to sign off those commits, if it doesn't work just let us know and we can see if we can work around it. Thanks! |
Resolves: ISSUE-2428
#2428
What kind of change does this PR introduce?
What is the current behavior?
Test coverage of EnvsClustersTenantsControllerService class is only 30%
What is the new behavior?
Line coverage of EnvsClustersTenantsControllerService class is increased to 87%
Other information
Refactor some of the code in EnvsClustersTenantsControllerService class
Requirements (all must be checked before review)
mainbranch have been pulledpnpm linthas been run successfully