gzhao9 opened a new issue, #15164:
URL: https://github.com/apache/druid/issues/15164
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?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
