gzhao9 opened a new issue, #13153: URL: https://github.com/apache/dubbo/issues/13153
Hi Dubbo developers, I noticed that in the 5 test cases of - [StubInvocationUtilTest.java](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java), namely: [unaryCall()](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L45), [unaryCall2()](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L77) , [testUnaryCall()](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L120) , [biOrClientStreamCall()](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/ dubbo/rpc/stub/StubInvocationUtilTest.java#L170) and [serverStreamCall()](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L245) there are quite some duplicated/redundant mock objects created repeatedly for 5 different dependencies in each test case. For example, the mock for `MethodDescriptor` has been repeated 5 times with completely identical logic in the 5 test cases (see [unaryCall(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L65-L70) Line 65-70; [unaryCall2(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L99-L103) Line 99-103; [testUnaryCall(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L141-L145) Line 141-145; [biOrClientStreamCall(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/du bbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L216-L218) Line 213-218; and [serverStreamCall()](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L273-L277) Line 273-277), which looks like: ```java MethodDescriptor method = Mockito.mock(MethodDescriptor.class); when(method.getParameterClasses()) .thenReturn(new Class[]{String.class}); when(method.getMethodName()) .thenReturn("sayHello"); ``` We can eliminate this duplication by creating a `createMockMethodDescriptor()` method to just handle the mock of `MethodDescriptor`. As such, whenever a test case needs to use this mock object, it just needs to call this method without duplicating the mock logic: ```java private MethodDescriptor createMockMethodDescriptor() { MethodDescriptor method = Mockito.mock(MethodDescriptor.class); when(method.getParameterClasses()).thenReturn(new Class[]{String.class}); when(method.getMethodName()).thenReturn("sayHello"); return method; } ``` After this change, the test cases will be smaller and simpler (10 fewer lines of code). More importantly, the logic of the mock for `MethodDescriptor` becomes reusable and easier to maintain if something needs to be changed in the future. For example, if more test cases are added and need this mock object, just call this method; and if the mock logic changes, just need to change this one method. The mock for the other three dependencies `URL`, `ConsumerModel`, and `Result` have similar issues and can be fixed in a similar way. For example, `URL` is also mocked repeatedly with the same logic in the 5 test cases (see [unaryCall(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L47-L58) Line 47-58; [unaryCall2(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L79-L88) Line 79-88; [testUnaryCall(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L122-L131) Line 122-131; [biOrClientStreamCall(),](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/ test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L172-L181) Line 172-181; [serverStreamCall()](https://github.com/apache/dubbo/blob/dc62adb612bfeee254e2af304fc508a9d4eb5d7f/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java#L247-L256) Line 247-256), as ```java ConsumerModel consumerModel = Mockito.mock(ConsumerModel.class); URL url = Mockito.mock(URL.class); when(url.getServiceModel()).thenReturn(consumerModel); when(url.getServiceInterface()).thenReturn(IGreeter.class.getName()); when(url.getProtocolServiceKey()).thenReturn(IGreeter.class.getName()); ``` The only difference is that the stub for `getServiceModel()` will return a different `ConsumerModel`, which is determined by each test case. So we can pass `ConsumerModel` as a parameter to the mock creation method, and whenever the mock for `URL` is needed, just call createMockURL(ConsumerModel consumerModel) and passing the right parameter: ```java private URL createMockURL(ConsumerModel consumerModel) { URL url = Mockito.mock(URL.class); when(url.getServiceModel()).thenReturn(consumerModel); when(url.getServiceInterface()).thenReturn(IGreeter.class.getName()); when(url.getProtocolServiceKey()).thenReturn(IGreeter.class.getName()); return url; } ``` Fixing the repeated mocks with the three dependencies, `URL`, `ConsumerModel`, and `Result`, can further reduce 43 lines of code. The mock for `Invoker<IGreeter>` is also repeated in 5 test cases but with slight variations in one of the stubbed methods in different test cases. For example, in test case `unaryCall()`, ```java Invoker<IGreeter> invoker = Mockito.mock(Invoker.class); when(invoker.getUrl()).thenReturn(url); when(invoker.getInterface()).thenReturn(IGreeter.class); when(invoker.invoke(any(Invocation.class))).thenReturn(result); // this is different from other test cases ``` In comparison, in the test case `unaryCall2()`, the stub for the first two methods are same, with the stub of the 3rd method differing: ```java Invoker<IGreeter> invoker = Mockito.mock(Invoker.class); when(invoker.getUrl()).thenReturn(url); when(invoker.getInterface()).thenReturn(IGreeter.class); when(invoker.invoke(any(Invocation.class))) .thenThrow(new RuntimeException("a")) .thenThrow(new Error("b")); // this is different from other test cases ``` We can still eliminate repetition by creating a reusable, "basic" mock object, but inside each test case, we can further customize the stub of the 3rd method. ```java //Providing a basic version of the mock for Invoker, with only two common methods stubbed: private Invoker<IGreeter> createMockInvoker(URL url) { Invoker<IGreeter> invoker = Mockito.mock(Invoker.class); when(invoker.getUrl()).thenReturn(url); when(invoker.getInter@B-face()).thenReturn(IGreeter.class); return invoker; } ``` Whenever we need to create a mock of `Invoker<IGreeter>`, we just need to call `createMockInvoker(URL url)` first. And then do the custom stub. Like below: ```java Invoker<IGreeter> invoker(URL url)=createMockInvoker(url); //Create a mock with createMockInvoker. when(invoker.invoke(any(Invocation.class))).thenReturn(result); // Add a different stub behavior. ``` Given that the 5 test cases all share the five mock objects, we can make them attributes of the test class and create them in a `@BeforeEach` method. As such, each test case can access and reuse the five mock objects: ```java private Invoker<IGreeter> invoker; private MethodDescriptor method; private String request = "request"; private String response = "response"; private Result result; @BeforeEach void init() throws Throwable { ServiceDescriptor serviceDescriptor = Mockito.mock(ServiceDescriptor.class); ConsumerModel consumerModel = createMockConsumerModel(serviceDescriptor); URL url = createMockURL(consumerModel); invoker = createMockInvoker(url); result = createMockResult(response); method = createMockMethodDescriptor(); } ``` To be more intuitive, the following shows the comparison of how a test case looks before and after the refactoring, with unaryCall() as an example: Before refactoring ```java @Test void unaryCall() throws Throwable { Invoker<IGreeter> invoker = Mockito.mock(Invoker.class); URL url = Mockito.mock(URL.class); ConsumerModel consumerModel = Mockito.mock(ConsumerModel.class); ServiceDescriptor serviceDescriptor = Mockito.mock(ServiceDescriptor.class); when(consumerModel.getServiceModel()).thenReturn(serviceDescriptor); when(url.getServiceModel()) .thenReturn(consumerModel); when(url.getServiceInterface()) .thenReturn(IGreeter.class.getName()); when(url.getProtocolServiceKey()) .thenReturn(IGreeter.class.getName()); when(invoker.getUrl()) .thenReturn(url); when(invoker.getInterface()) .thenReturn(IGreeter.class); Result result = Mockito.mock(Result.class); when(invoker.invoke(any(Invocation.class))) .thenReturn(result); String response = "response"; when(result.recreate()).thenReturn(response); MethodDescriptor method = Mockito.mock(MethodDescriptor.class); when(method.getParameterClasses()) .thenReturn(new Class[]{String.class}); when(method.getMethodName()) .thenReturn("sayHello"); String request = "request"; Object ret = StubInvocationUtil.unaryCall(invoker, method, request); Assertions.assertEquals(response, ret); } ``` After refactoring ```java @Test void unaryCall() { when(invoker.invoke(any(Invocation.class))).thenReturn(result); Object ret = StubInvocationUtil.unaryCall(invoker, method, request); Assertions.assertEquals(response, ret); } ``` Overall, after fixing the repeated mocks for the above five dependencies, we can reduce 76 lines of code, which makes the test cases smaller, easier to understand, and better to maintain. You can review the entire refactored code via this [link](https://github.com/gzhao9/dubbo/blob/refactor-StubInvocationUtilTest/dubbo-rpc/dubbo-rpc-triple/src/test/java/org/apache/dubbo/rpc/stub/StubInvocationUtilTest.java). #### Benefits of the Proposed Refactoring: - **Modularity:** The code is now more modular with specific methods for creating each type of 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: [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]
