You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "gzhao9 (via GitHub)" <gi...@apache.org> on 2023/10/12 19:57:36 UTC
[I] Refactoring the Duplicated/Redundant Mock Objects in test of org.apache.cloudstack.network.tungsten.service [cloudstack]
gzhao9 opened a new issue, #8087:
URL: https://github.com/apache/cloudstack/issues/8087
<!--
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release and main branch are affected too.
Always add information AFTER of these HTML comments, but no need to delete the comments.
-->
##### ISSUE TYPE
<!-- Pick one below and delete the rest -->
* Improvement Request
* Enhancement Request
##### COMPONENT NAME
<!--
Categorize the issue, e.g. API, VR, VPN, UI, etc.
-->
~~~
Testing
~~~
##### CLOUDSTACK VERSION
<!--
New line separated list of affected versions, commit ID for issues on main branch.
-->
[4.18-reflections-update](https://github.com/apache/cloudstack/tree/4.18-reflections-update)
~~~
4.18-reflections-update
~~~
---------
Hi CloudStack developers,
I have noticed that a dependency `TungstenAnswer` is mocked a total 151 times, with similar/identical behavior across three different test suites:
- In class [TungstenElementTest.java](https://github.com/apache/cloudstack/blob/8a34afa8ab8c310f24038c0069fd1ce53b4b81d5/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenElementTest.java), there were 41 identical duplicate creations in 22 test cases.
- In class [TungstenFabricUtilsTest.java](https://github.com/apache/cloudstack/blob/8a34afa8ab8c310f24038c0069fd1ce53b4b81d5/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenFabricUtilsTest.java), there were 2 identical duplicate creations in 2 test cases.
- In class [TungstenServiceImplTest.java](https://github.com/apache/cloudstack/blob/8a34afa8ab8c310f24038c0069fd1ce53b4b81d5/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenServiceImplTest.java), there were 53 identical duplicate creations and 55 similar duplicate creations in 68 test cases.
The following is the duplicate code snippet I just mentioned. It has been repeated 96 times with completely identical logic in the 3 testclasses:
```java
TungstenAnswer answer=mock(TungstenAnswer.class);
when(answer.getResult()).thenReturn(Result);
```
We can eliminate this duplication by creating a [CreateMockTungstenAnswer]((https://github.com/gzhao9/cloudstack/blob/refactoring-org.apache.cloudstack.network.tungsten.service/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/service/CreateMockTungstenAnswer.java)) class.
The code snippet of `CreateMockTungstenAnswer`:
```java
public class CreateMockTungstenAnswer {
//Attribute is for reuse mock creation.
TungstenAnswer answer;
//Implement duplicate mock and stub behavior in the constructor.
CreateMockTungstenAnswer(boolean Result){
answer=mock(TungstenAnswer.class);
when(answer.getResult()).thenReturn(Result);
}
TungstenAnswer get(){return this.answer;}
}
```
When a mock needs to be created in a test case, we can create it with this class like:
```java
TungstenAnswer answer=new CreateMockTungstenAnswer(true).get();
```
Also, in `TungstenServiceImplTest.java`, the creation of mock for `TungstenAnswer` is repeated another 55 times, but with a slightly different stub method in mock creation. For example, in test case `addTungstenDefaultNetworkPolicyTest()`:
```java
TungstenAnswer createTungstenPolicyAnswer=mock(TungstenAnswer.class);
when(createTungstenPolicyAnswer.getResult()).thenReturn(true);
// following are additional different stub from other mock creation
when(createTungstenPolicyAnswer.getApiObjectBase()).thenReturn(networkPolicy);
```
In comparison, in the test `addTungstenDefaultNetworkPolicyTest()`, the stub for the 1st method is same, but there will be an additional 2nd or 3rd method compared to the 96 identical duplicate mock creations.
We can still eliminate repetition by creating a reusable, "basic" mock object, but at each mock creation, we can customize the stub of the other methods.
```java
TungstenAnswer createTungstenPolicyAnswer=new CreateMockTungstenAnswer(true).get();
when(createTungstenPolicyAnswer.getApiObjectBase()).thenReturn(networkPolicy);// Add a different stub behavior.
```
To be more intuitive, the following shows the comparison of how a test case looks before and after the refactoring, with the code snippet of `syncTungstenDbWithCloudstackProjectsAndDomainsTest()` in test suit [TungstenServiceImplTest](https://github.com/apache/cloudstack/blob/8a34afa8ab8c310f24038c0069fd1ce53b4b81d5/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenServiceImplTest.java#L654) as an example:
Code before refactoring:
```java
@Test
public void syncTungstenDbWithCloudstackProjectsAndDomainsTest() {
// The code before this comment is the same
TungstenAnswer createTungstenDomainAnswer = mock(TungstenAnswer.class);
TungstenAnswer createTungstenProjectAnswer = mock(TungstenAnswer.class);
TungstenAnswer createTungstenDefaultProjectAnswer = mock(TungstenAnswer.class);
TungstenAnswer updateTungstenDefaultSecurityGroupAnswer = mock(TungstenAnswer.class);
when(createTungstenDomainAnswer.getResult()).thenReturn(true);
when(createTungstenProjectAnswer.getResult()).thenReturn(true);
when(updateTungstenDefaultSecurityGroupAnswer.getResult()).thenReturn(true);
when(createTungstenDefaultProjectAnswer.getResult()).thenReturn(true);
//The rest of the sentence is exactly the same before and after the refactoring.
}
```
Code after refactoring:
```java
@Test
public void syncTungstenDbWithCloudstackProjectsAndDomainsTest() {
// The code before this comment is the same
TungstenAnswer createTungstenDomainAnswer = new CreateMockTungstenAnswer(true).get();
TungstenAnswer createTungstenProjectAnswer = new CreateMockTungstenAnswer(true).get();
TungstenAnswer createTungstenDefaultProjectAnswer = new CreateMockTungstenAnswer(true).get();
TungstenAnswer updateTungstenDefaultSecurityGroupAnswer = new CreateMockTungstenAnswer(true).get();
//The rest of the sentence is exactly the same before and after the refactoring.
}
```
We've applied a similar refactoring approach to three test suites, [TungstenServiceImplTest.java](https://github.com/gzhao9/cloudstack/blob/refactoring-org.apache.cloudstack.network.tungsten.service/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenServiceImplTest.java), [TungstenFabricUtilsTest.java]() and [TungstenElementTest.java ](https://github.com/gzhao9/cloudstack/blob/refactoring-org.apache.cloudstack.network.tungsten.service/plugins/network-elements/tungsten/src/test/java/org/apache/cloudstack/network/tungsten/service/TungstenElementTest.java). **As a result, we can reduce 321 lines of code, which makes the test cases smaller, easier to understand, and better to maintain.**
**After refactoring, if the `TungstenAnswer` mock creation needs to be modified in the future, the developer only needs to modify one time in `CreateMockTungstenAnswer` instead of 151 places of duplicate mock creation in 3 testclasses.**
#### Benefits of the Proposed Refactoring:
- **Modularity:** The code is now more modular with specific class for creating mock object.
- **Readability:** Test cases are more readable, focusing on specific test logic rather than the setup code.
- **Maintainability:** Easier maintenance due to the reduction of redundancy and a cleaner, more structured codebase.
- **Scalability:** Facilitates the addition of new test cases and makes modifications to existing ones more efficient.
I'd love to hear your thoughts on this proposal! Do you agree with any of these changes, or do you have other insights or preferences?
--
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] Refactoring the Duplicated/Redundant Mock Objects in test of org.apache.cloudstack.network.tungsten.service [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr closed issue #8087: Refactoring the Duplicated/Redundant Mock Objects in test of org.apache.cloudstack.network.tungsten.service
URL: https://github.com/apache/cloudstack/issues/8087
--
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
Re: [I] Refactoring the Duplicated/Redundant Mock Objects in test of org.apache.cloudstack.network.tungsten.service [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on issue #8087:
URL: https://github.com/apache/cloudstack/issues/8087#issuecomment-1761136146
@gzhao9 great initiative, especially if you are interested in improving the tungsten support. But also if your sole intention is to improve the test code, please be our guest. I read nothing in your proposals that rings alarm bells so If you whish to submit a PR go ahead.
thanks for the effort whatever may come of it.
--
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
Re: [I] Refactoring the Duplicated/Redundant Mock Objects in test of org.apache.cloudstack.network.tungsten.service [cloudstack]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on issue #8087:
URL: https://github.com/apache/cloudstack/issues/8087#issuecomment-1760283402
Thanks for opening your first issue here! Be sure to follow the issue template!
--
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
Re: [I] Refactoring the Duplicated/Redundant Mock Objects in test of org.apache.cloudstack.network.tungsten.service [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on issue #8087:
URL: https://github.com/apache/cloudstack/issues/8087#issuecomment-1803242402
@gzhao9 related PRs are merged therefore I'm closing this. Please re-open if anything remaining
--
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