apache / apache/dubbo

Simplify `MockClusterInvokerTest.testMockInvokerFromOverride_Invoke_Fock_WithOutDefault()` to improve the Readability

Open
#10,486 1 comment 0 reactions 0 assignees View on GitHub
type/proposal
Dominant language
Java
Stars
41.6k
Forks
26.4k
Avg merge
15h 13m
Merged PRs (30d)
4

Description

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 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 cluster = getClusterInvoker(url);
}
```
Test Cases
```java
@Test
public void testMockInvokerFromOverride_Invoke_Fock_WithOutDefault_getSomething() {
Invoker 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 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.