You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2022/08/19 13:10:02 UTC
[GitHub] [dubbo] Codegass opened a new issue, #10486: Simplify `MockClusterInvokerTest.testMockInvokerFromOverride_Invoke_Fock_WithOutDefault()` to improve the Readability
Codegass opened a new issue, #10486:
URL: https://github.com/apache/dubbo/issues/10486
I noticed that the test case, [testMockInvokerFromOverride_Invoke_Fock_WithOutDefault](https://github.com/apache/dubbo/blob/dcb2d703fbc5c53bbd7e5be52b1b7554566a9481/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/wrapper/MockClusterInvokerTest.java#L224), tests the invoke() function with two different mock setup scenarios.
The first scenario is testing the predefined fail mock(line [233-242](https://github.com/apache/dubbo/blob/dcb2d703fbc5c53bbd7e5be52b1b7554566a9481/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/wrapper/MockClusterInvokerTest.java#L233-L242)). It tests the invoke() function when “getSomething” and “getSomething2” are properly set up in the URL parameters.The return values are asserted to be “x” and “y” respectively.
```java
@Test
public void testMockInvokerFromOverride_Invoke_Fock_WithOutDefault() {
...
RpcInvocation invocation = new RpcInvocation();
invocation.setMethodName("getSomething");
Result ret = cluster.invoke(invocation);
Assertions.assertEquals("x", ret.getValue());
// If no mock was configured, return null directly
invocation = new RpcInvocation();
invocation.setMethodName("getSomething2");
ret = cluster.invoke(invocation);
Assertions.assertEquals("y", ret.getValue());
...
```
The second scenario is testing the not-configured mock([245-252](https://github.com/apache/dubbo/blob/dcb2d703fbc5c53bbd7e5be52b1b7554566a9481/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/wrapper/MockClusterInvokerTest.java#L245-L252)), when “getSomething3” is not properly set as a URL parameter. In this scenario, the test case is expected to fail if no exception is raised. This is currently expressed as Assertions.fail inside a Try block, followed by Catch the expected RpcException. The expected behavior under this scenario could be made more explicit by removing the Try-Catch, and use assert.Throws function to verify the expected exception directly.
```java
@Test
public void testMockInvokerFromOverride_Invoke_Fock_WithOutDefault() {
...
try {
ret = cluster.invoke(invocation);
Assertions.fail();
} catch (RpcException e) {
}
```
To make the test case simpler and easier to understand, I would like to propose two changes:
1. Separate the two scenarios into two test cases, `testMockInvokerFromOverride_Invoke_Fock_WithOutDefault_getSomething` and `testMockInvokerFromOverride_Invoke_Fock_WithOutDefault_noConfigure` respectively. The benefit is that each test case has a dedicated focus and could fail only due to one scenario.
2. Remove the Try-Catch block in the second test scenario, and use assert.Throws to make the expected behavior more straightforward and explicit.
Following, I describe the proposed solution in more detail:
* Extract the setup from line [225-231](https://github.com/apache/dubbo/blob/dcb2d703fbc5c53bbd7e5be52b1b7554566a9481/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/wrapper/MockClusterInvokerTest.java#L225-L231) as the s`etupMockInvokerFromOverride_Invoke_Fock_WithOutDefault()`for arranging the URL. This setup function can be reused by both scenarios mentioned above.
* Extract the line [233-242](https://github.com/apache/dubbo/blob/dcb2d703fbc5c53bbd7e5be52b1b7554566a9481/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/wrapper/MockClusterInvokerTest.java#L233-L242) as the first test case, `testMockInvokerFromOverride_Invoke_Fock_WithOutDefault_getSomething`, to test the `invoke()` with predefined inputs, getSomething and getSomething2.
* Extract the line [245-252](https://github.com/apache/dubbo/blob/dcb2d703fbc5c53bbd7e5be52b1b7554566a9481/dubbo-cluster/src/test/java/org/apache/dubbo/rpc/cluster/support/wrapper/MockClusterInvokerTest.java#L245-L252) as the second test case, `testMockInvokerFromOverride_Invoke_Fock_WithOutDefault_notConfigure`, to test the `invoke()` with getSomething3, which is not configured. Specifically, remove the try-catch block and use the assertThrows to assert the expected exception.
After refactoring:
Setup
```java
public Invoker<IHelloService> setupMockInvokerFromOverride_Invoke_Fock_WithOutDefault(){
URL url = URL.valueOf("remote://1.2.3.4/" + IHelloService.class.getName())
.addParameter(REFER_KEY,
URL.encode(PATH_KEY + "=" + IHelloService.class.getName()
+ "&" + "getSomething.mock=fail:return x"
+ "&" + "getSomething2.mock=fail:return y"))
.addParameter("invoke_return_error", "true");
Invoker<IHelloService> cluster = getClusterInvoker(url);
}
```
Test Cases
```java
@Test
public void testMockInvokerFromOverride_Invoke_Fock_WithOutDefault_getSomething() {
Invoker<IHelloService> cluster = setupMockInvokerFromOverride_Invoke_Fock_WithOutDefault();
RpcInvocation invocation = new RpcInvocation();
RpcInvocation invocation2 = new RpcInvocation();
invocation.setMethodName("getSomething");
invocation2.setMethodName("getSomething2");
Result ret = cluster.invoke(invocation);
Result ret2 = cluster.invoke(invocation2);
Assertions.assertEquals("x", ret.getValue());
Assertions.assertEquals("y", ret2.getValue());
}
```
```java
@Test
public void testMockInvokerFromOverride_Invoke_Fock_WithOutDefault_notConfigure() {
Invoker<IHelloService> cluster = setupMockInvokerFromOverride_Invoke_Fock_WithOutDefault();
RpcInvocation invocation = new RpcInvocation();
invocation.setMethodName("getSomething3");
Assertions.assertThrows(RpcException.class,()->{cluster.invoke(invocation);});
}
```
This refactoring will make it easier to understand the test target from the test case name. In addition, putting multiple testing scenarios in one test case makes it harder for us to locate bugs in one CI\CD cycle.
--
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: notifications-unsubscribe@dubbo.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] chickenlj commented on issue #10486: Simplify `MockClusterInvokerTest.testMockInvokerFromOverride_Invoke_Fock_WithOutDefault()` to improve the Readability
Posted by GitBox <gi...@apache.org>.
chickenlj commented on issue #10486:
URL: https://github.com/apache/dubbo/issues/10486#issuecomment-1234024555
That‘s a very detailed analysis, we really appreciate the efforts that you have put into this. It would be great if you can submit the code directly by filing a pull request.
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org