Refactoring the Duplicated/Redundant Mock Objects in test of org.apache.druid.segment.loading;
- Dominant language
- Java
- Stars
- 14.1k
- Forks
- 3.8k
- Avg merge
- 2d 58m
- Merged PRs (30d)
- 233
Description
Hi Druid developers,
I noticed that the dependency `DataSegment` was repeatedly mocked a total of 12 times in three different test suites.
There are a total of two mock behaviors of mocking `DataSegment`.
The code for the first behavior is as follows:
```java
DataSegment segment = mock(DataSegment.class);
when(segment.getLoadSpec()).thenReturn(ImmutableMap.of("type", "sane"));
```
It has been repeated 8 times with completely identical logic in the 3 test classes:
- In class [OmniDataSegmentMoverTest.java](https://github.com/apache/druid/blob/master/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentMoverTest.java), there were 3 identical duplicate creations in 3 test cases.
- In class [OmniDataSegmentArchiverTest.java](https://github.com/apache/druid/blob/master/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentArchiverTest.java), there were 3 identical duplicate creations in 3 test cases.
- In class [OmniDataSegmentKillerTest.java](https://github.com/apache/druid/blob/master/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentKillerTest.java), there were 2 identical duplicate creations in 2 test cases.
Example code for another behavior is as follows:
```java
DataSegment segment = mock(DataSegment.class);
when(segment.getLoadSpec()).thenReturn(ImmutableMap.of("type", "sane"));
when(segment.isTombstone()).thenReturn(false);//one more stub method over the first behavior.
```
Compared to the first behavior. It stubs an extra `isTombstone()` method. It has been repeated 4 times with completely identical logic in 2 test cases of [OmniDataSegmentKillerTest.java](https://github.com/apache/druid/blob/master/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentKillerTest.java).
We can create a [CreateMockDataSegment](https://github.com/gzhao9/druid/blob/Refactoring-org.apache.druid.segment.loading/server/src/test/java/org/apache/druid/segment/loading/CreateMockDataSegment.java) class to eliminate duplicates.
The code snippet of `CreateMockTungstenAnswer`:
```java
public class CreateMockDataSegment {
//Attribute is for reuse mock creation.
private DataSegment segment;
//Implement the first behavior of mock creation.
CreateMockDataSegment(String key, String value) {
segment = mock(DataSegment.class);
when(segment.getLoadSpec()).thenReturn(ImmutableMap.of(key, value));
}
//Implement the second behavior of mock creation.
CreateMockDataSegment(String key, String value, boolean isTombstone) {
segment=new CreateMockDataSegment(key, value).getSegment();//Reuse the first creation behavior.
when(segment.isTombstone()).thenReturn(isTombstone);
}
public DataSegment getSegment() {return segment;}
}
```
When we need to reuse the first mock creation.Just call this class with two parameters to complete the mock creation. It is as follows:
```java
final DataSegment segment = new CreateMockDataSegment("type", "sane").getSegment();
```
Similarly, when we need to reuse the second mock creation behavior. Just call this class with three parameters to complete the mock creation. It is as follows:
```java
final DataSegment segment = new CreateMockDataSegment("type", "sane", false).getSegment();
```
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 `testKillMultipleSegmentsWithType()` in test suit `OmniDataSegmentKillerTest.java` as an example:
Code [before refactoring](https://github.com/apache/druid/blob/master/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentKillerTest.java#L142C1-L165C4):
```java
@Test
public void testKillMultipleSegmentsWithType() throws SegmentLoadingException{
// The code before this comment is the same
final DataSegment segment1 = Mockito.mock(DataSegment.class);
final DataSegment segment2 = Mockito.mock(DataSegment.class);
final DataSegment segment3 = Mockito.mock(DataSegment.class);
Mockito.when(segment1.isTombstone()).thenReturn(false);
Mockito.when(segment1.getLoadSpec()).thenReturn(ImmutableMap.of("type", "sane"));
Mockito.when(segment2.isTombstone()).thenReturn(false);
Mockito.when(segment2.getLoadSpec()).thenReturn(ImmutableMap.of("type", "sane"));
Mockito.when(segment3.isTombstone()).thenReturn(false);
Mockito.when(segment3.getLoadSpec()).thenReturn(ImmutableMap.of("type", "sane_2"));
//The rest of the sentence is exactly the same before and after the refactoring.
}
```
Code [after refactoring](https://github.com/gzhao9/druid/blob/Refactoring-org.apache.druid.segment.loading/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentKillerTest.java#L138C1-L154C4):
```java
@Test
public void testKillMultipleSegmentsWithType() throws SegmentLoadingException{
// The code before this comment is the same
final DataSegment segment1 = new CreateMockDataSegment("type", "sane", false).getSegment();
final DataSegment segment2 = new CreateMockDataSegment("type", "sane", false).getSegment();
final DataSegment segment3 = new CreateMockDataSegment("type", "sane_2", false).getSegment();
//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, [OmniDataSegmentMoverTest.java](https://github.com/gzhao9/druid/blob/Refactoring-org.apache.druid.segment.loading/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentMoverTest.java), [OmniDataSegmentArchiverTest.java](https://github.com/gzhao9/druid/blob/Refactoring-org.apache.druid.segment.loading/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentArchiverTest.java) and [OmniDataSegmentKillerTest.java](https://github.com/gzhao9/druid/blob/Refactoring-org.apache.druid.segment.loading/server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentKillerTest.java). As a result, we can reduce 16 lines of code.
**With refactoring, if the `DataSegment` mock creation needs to be modified in the future, the developer only needs to modify one time in `CreateMockDataSegment` instead of 12 places of duplicate mock creation in 3 test classes.**
#### Benefits of the Proposed Refactoring:
- **Modularity:** The code is now more modular with a specific class for creating mock objects.
- **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?
Contributor guide
Research direction
Start with server/src/test/java/org/apache/druid/segment/loading/OmniDataSegmentMoverTest.java, OmniDataSegmentArchiverTest.java, and OmniDataSegmentKillerTest.java, then review the proposed CreateMockDataSegment.java helper. Run the three corresponding test classes and verify that the duplicated DataSegment setup is centralized while existing test behavior remains unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- testing
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100