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:58:56 UTC
[I] Refactoring the Duplicated/Redundant Mock Objects in SwiftUtilTest [cloudstack]
gzhao9 opened a new issue, #8088:
URL: https://github.com/apache/cloudstack/issues/8088
<!--
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
~~~
---------
Hello CloudStack Developers,
I've observed that the `SwiftClientCfg` dependency is repeatedly mocked with identical behavior in [SwiftUtilTest.java](https://github.com/apache/cloudstack/blob/8a34afa8ab8c310f24038c0069fd1ce53b4b81d5/utils/src/test/java/com/cloud/utils/SwiftUtilTest.java)
The following is the duplicate code snippet I just mentioned. The creation of mock objects was repeated a total of 9 times across 9 different test case.
```java
SwiftClientCfg cfg = mock(SwiftClientCfg.class);//
given(cfg.getEndPoint()).willReturn("swift.endpoint");
given(cfg.getAccount()).willReturn("cs");
given(cfg.getUserName()).willReturn("sec-storage");
given(cfg.getKey()).willReturn("mypassword");
// the last stub may willReturn() different values in different tests.
given(cfg.getStoragePolicy()).willReturn('policy');
```
The last method, `getStoragePolicy()` will be stubbed to return different values in each test cases. But the rest of the stub behavior is exactly the same.
We can eliminate this duplication by creating a method [CreateMockSwiftClientCfg(String policy)](https://github.com/gzhao9/cloudstack/blob/Refactoring-SwiftUtilTest/utils/src/test/java/com/cloud/utils/SwiftUtilTest.java#L184C1-L192C6) in `SwiftUtilTest.java`.
The code snippet of the method is as follows:
```java
private SwiftClientCfg CreateMockSwiftClientCfg(String policy){
SwiftClientCfg cfg = mock(SwiftClientCfg.class);//
given(cfg.getEndPoint()).willReturn("swift.endpoint");
given(cfg.getAccount()).willReturn("cs");
given(cfg.getUserName()).willReturn("sec-storage");
given(cfg.getKey()).willReturn("mypassword");
given(cfg.getStoragePolicy()).willReturn(policy);
return cfg;
}
```
When a mock needs to be created in a test case, we can create it with this method and take different stub return values in `getStoragePolicy()` as parameter:
```java
SwiftClientCfg cfg = CreateMockSwiftClientCfg("policy1");
```
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 `testGetUploadCmd()` as an example:
[Code before refactoring](https://github.com/apache/cloudstack/blob/8a34afa8ab8c310f24038c0069fd1ce53b4b81d5/utils/src/test/java/com/cloud/utils/SwiftUtilTest.java#L146C2-L159C1):
```java
@Test
public void testGetUploadCmd() {
SwiftClientCfg cfg = mock(SwiftClientCfg.class);//
given(cfg.getEndPoint()).willReturn("swift.endpoint");
given(cfg.getAccount()).willReturn("cs");
given(cfg.getUserName()).willReturn("sec-storage");
given(cfg.getKey()).willReturn("mypassword");
given(cfg.getStoragePolicy()).willReturn(null);
String uploadCmd = SwiftUtil.getUploadObjectCommand(cfg, "swift", "T-1", "template.vhd", 1024);
String expected = "/usr/bin/python swift -A swift.endpoint -U cs:sec-storage -K mypassword upload T-1 template.vhd";
assertThat(uploadCmd, is(equalTo(expected)));
}
```
[Code after refactoring](https://github.com/gzhao9/cloudstack/blob/Refactoring-SwiftUtilTest/utils/src/test/java/com/cloud/utils/SwiftUtilTest.java#L130C1-L137C6):
```java
@Test
public void testGetUploadCmd() {
SwiftClientCfg cfg = CreateMockSwiftClientCfg(null);
String uploadCmd = SwiftUtil.getUploadObjectCommand(cfg, "swift", "T-1", "template.vhd", 1024);
String expected = "/usr/bin/python swift -A swift.endpoint -U cs:sec-storage -K mypassword upload T-1 template.vhd";
assertThat(uploadCmd, is(equalTo(expected)));
}
```
We have applied the same refactoring approach to the other 8 test cases, you can see the refactored code in [SwiftUtilTest.java](https://github.com/gzhao9/cloudstack/blob/Refactoring-SwiftUtilTest/utils/src/test/java/com/cloud/utils/SwiftUtilTest.java). **As a result, we can reduce 45 lines of code, which makes the test cases smaller, easier to understand, and better to maintain.**
**After the refactoring, if a developer needs to modify the mock creation of `SwiftClientCfg`, they only need to modify `CreateMockSwiftClientCfg` instead of modifying it in 9 different test cases before the refactoring.**
#### Benefits of the Proposed Refactoring:
- **Modularity:** The code is now more modular with a 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 SwiftUtilTest [cloudstack]
Posted by "gzhao9 (via GitHub)" <gi...@apache.org>.
gzhao9 commented on issue #8088:
URL: https://github.com/apache/cloudstack/issues/8088#issuecomment-1864113288
> @gzhao9 is anything else remaining here? I see the related PRs have been merged?
I think it's all right. I notice that the PR submitted has been approved.
--
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 SwiftUtilTest [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on issue #8088:
URL: https://github.com/apache/cloudstack/issues/8088#issuecomment-1855821793
@gzhao9 is anything else remaining here? I see the related PRs have been merged?
--
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 SwiftUtilTest [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr closed issue #8088: Refactoring the Duplicated/Redundant Mock Objects in SwiftUtilTest
URL: https://github.com/apache/cloudstack/issues/8088
--
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 SwiftUtilTest [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on issue #8088:
URL: https://github.com/apache/cloudstack/issues/8088#issuecomment-1761139276
As with the tungsten proposal (which is basically the same overall design) please go ahead, I'll review your code.
--
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