Copilot commented on code in PR #6229:
URL: https://github.com/apache/shenyu/pull/6229#discussion_r2532866777
##########
shenyu-admin/src/test/java/org/apache/shenyu/admin/listener/websocket/WebsocketDataChangedListenerTest.java:
##########
@@ -98,16 +117,29 @@ public void testOnSelectorChanged() {
+
"\\\\\\\"weight\\\\\\\":\\\\\\\"49\\\\\\\"}]\",\"conditionList\":[{\"paramType\":\"uri\","
+
"\"operator\":\"match\",\"paramName\":\"/\",\"paramValue\":\"/http/**\"}],\"id\":\"1336329408516136960\","
+
"\"name\":\"/http\",\"enabled\":true,\"sort\":1,\"namespaceId\":\"649330b6-c2d7-4edc-be8e-8a54df9eb385\"}]}";
- MockedStatic.Verification verification = () ->
WebsocketCollector.send(Constants.SYS_DEFAULT_NAMESPACE_ID, message,
DataEventTypeEnum.UPDATE);
try (MockedStatic<WebsocketCollector> mockedStatic =
mockStatic(WebsocketCollector.class)) {
- mockedStatic.when(verification).thenAnswer((Answer<Void>)
invocation -> null);
+ mockedStatic.when(() -> WebsocketCollector.send(anyString(),
anyString(), any()))
+ .thenAnswer(invocation -> null);
// 调用被测试的方法
websocketDataChangedListener.onSelectorChanged(selectorDataList,
DataEventTypeEnum.UPDATE);
// 验证
Review Comment:
Chinese comments should be replaced with English for consistency with the
rest of the codebase.
- Line 125: "调用被测试的方法" should be "Call the method under test"
- Line 128: "验证" should be "Verify"
```suggestion
// Call the method under test
websocketDataChangedListener.onSelectorChanged(selectorDataList,
DataEventTypeEnum.UPDATE);
// Verify
```
##########
shenyu-admin/src/test/java/org/apache/shenyu/admin/listener/websocket/WebsocketDataChangedListenerTest.java:
##########
@@ -38,6 +38,12 @@
import java.util.List;
import static org.mockito.Mockito.mockStatic;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.ArgumentMatchers.eq;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
Review Comment:
Import statements should follow Java conventions. Static imports should come
after regular imports. Move the static import statements (lines 41-44) before
the Jackson imports (lines 45-46).
```suggestion
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
```
##########
shenyu-admin/src/test/java/org/apache/shenyu/admin/listener/websocket/WebsocketDataChangedListenerTest.java:
##########
@@ -140,11 +185,24 @@ public void testOnAppAuthChanged() {
+
"\"D9FD95F496C9495DB5604778A13C3D08\",\"appSecret\":\"02D25048AA1E466F8920E68B08E668DE\","
+
"\"enabled\":true,\"paramDataList\":[{\"appName\":\"axiba\",\"appParam\":\"123\"}]"
+
",\"pathDataList\":[{\"appName\":\"alibaba\",\"path\":\"/1\",\"enabled\":true}],\"namespaceId\":\"649330b6-c2d7-4edc-be8e-8a54df9eb385\"}]}";
- MockedStatic.Verification verification = () ->
WebsocketCollector.send(Constants.SYS_DEFAULT_NAMESPACE_ID, message,
DataEventTypeEnum.UPDATE);
try (MockedStatic<WebsocketCollector> mockedStatic =
mockStatic(WebsocketCollector.class)) {
- mockedStatic.when(verification).thenAnswer((Answer<Void>)
invocation -> null);
+ mockedStatic.when(() ->
WebsocketCollector.send(Constants.SYS_DEFAULT_NAMESPACE_ID, message,
DataEventTypeEnum.UPDATE))
+ .thenAnswer((Answer<Void>) invocation -> null);
Review Comment:
The stubbing approach in this test is inconsistent with the other tests.
Lines 189-190 use a specific method signature match, while the other tests
(lines 84-85, 122-123, 159-160, 218-219) use flexible matchers (`anyString()`,
`any()`). For consistency and to align with the fix's purpose (handling
non-deterministic JSON ordering), this should use the same pattern:
```java
mockedStatic.when(() -> WebsocketCollector.send(anyString(), anyString(),
any()))
.thenAnswer(invocation -> null);
```
```suggestion
mockedStatic.when(() -> WebsocketCollector.send(anyString(),
anyString(), any()))
.thenAnswer(invocation -> null);
```
##########
shenyu-admin/src/test/java/org/apache/shenyu/admin/listener/websocket/WebsocketDataChangedListenerTest.java:
##########
@@ -74,11 +80,24 @@ public void before() {
public void testOnPluginChanged() {
String message =
"{\"groupType\":\"PLUGIN\",\"eventType\":\"UPDATE\",\"data\":[{\"config\":\"{\\\\\\\"model\\\\\\\":\\\\\\\"black\\\\\\\"}\","
+
"\"role\":\"1\",\"id\":\"2\",\"name\":\"waf\",\"enabled\":true,\"namespaceId\":\"649330b6-c2d7-4edc-be8e-8a54df9eb385\"}]}";
- MockedStatic.Verification verification = () ->
WebsocketCollector.send(Constants.SYS_DEFAULT_NAMESPACE_ID, message,
DataEventTypeEnum.UPDATE);
try (MockedStatic<WebsocketCollector> mockedStatic =
mockStatic(WebsocketCollector.class)) {
- mockedStatic.when(verification).thenAnswer((Answer<Void>)
invocation -> null);
+ mockedStatic.when(() -> WebsocketCollector.send(anyString(),
anyString(), any()))
+ .thenAnswer(invocation -> null);
websocketDataChangedListener.onPluginChanged(pluginDataList,
DataEventTypeEnum.UPDATE);
- mockedStatic.verify(verification);
+ mockedStatic.verify(() -> WebsocketCollector.send(
+ eq(Constants.SYS_DEFAULT_NAMESPACE_ID),
+ argThat(actualMsg -> {
+ try {
+ ObjectMapper mapper = new ObjectMapper();
+ JsonNode expectedJson = mapper.readTree(message);
+ JsonNode actualJson = mapper.readTree(actualMsg);
+ return expectedJson.equals(actualJson);
+ } catch (Exception e) {
+ return false;
Review Comment:
The catch-all exception handler silently returns `false`, which makes test
failures difficult to debug. When JSON parsing fails or comparison throws an
exception, the test will fail with an unhelpful "argument matcher returned
false" message instead of showing the actual parsing error. Consider logging
the exception or using a more specific assertion:
```java
argThat(actualMsg -> {
try {
ObjectMapper mapper = new ObjectMapper();
JsonNode expectedJson = mapper.readTree(message);
JsonNode actualJson = mapper.readTree(actualMsg);
return expectedJson.equals(actualJson);
} catch (Exception e) {
throw new AssertionError("Failed to parse or compare JSON: " +
e.getMessage(), e);
}
})
```
```suggestion
throw new AssertionError("Failed to parse or compare
JSON: " + e.getMessage(), e);
```
##########
shenyu-admin/src/test/java/org/apache/shenyu/admin/listener/websocket/WebsocketDataChangedListenerTest.java:
##########
@@ -74,11 +80,24 @@ public void before() {
public void testOnPluginChanged() {
String message =
"{\"groupType\":\"PLUGIN\",\"eventType\":\"UPDATE\",\"data\":[{\"config\":\"{\\\\\\\"model\\\\\\\":\\\\\\\"black\\\\\\\"}\","
+
"\"role\":\"1\",\"id\":\"2\",\"name\":\"waf\",\"enabled\":true,\"namespaceId\":\"649330b6-c2d7-4edc-be8e-8a54df9eb385\"}]}";
- MockedStatic.Verification verification = () ->
WebsocketCollector.send(Constants.SYS_DEFAULT_NAMESPACE_ID, message,
DataEventTypeEnum.UPDATE);
try (MockedStatic<WebsocketCollector> mockedStatic =
mockStatic(WebsocketCollector.class)) {
- mockedStatic.when(verification).thenAnswer((Answer<Void>)
invocation -> null);
+ mockedStatic.when(() -> WebsocketCollector.send(anyString(),
anyString(), any()))
+ .thenAnswer(invocation -> null);
websocketDataChangedListener.onPluginChanged(pluginDataList,
DataEventTypeEnum.UPDATE);
- mockedStatic.verify(verification);
+ mockedStatic.verify(() -> WebsocketCollector.send(
+ eq(Constants.SYS_DEFAULT_NAMESPACE_ID),
+ argThat(actualMsg -> {
+ try {
+ ObjectMapper mapper = new ObjectMapper();
+ JsonNode expectedJson = mapper.readTree(message);
+ JsonNode actualJson = mapper.readTree(actualMsg);
+ return expectedJson.equals(actualJson);
+ } catch (Exception e) {
+ return false;
+ }
+ }),
Review Comment:
The JSON comparison logic is duplicated across all five test methods.
Consider extracting this into a reusable helper method or custom
ArgumentMatcher to improve maintainability. For example:
```java
private ArgumentMatcher<String> jsonEquals(String expectedJson) {
return actualMsg -> {
try {
ObjectMapper mapper = new ObjectMapper();
JsonNode expectedNode = mapper.readTree(expectedJson);
JsonNode actualNode = mapper.readTree(actualMsg);
return expectedNode.equals(actualNode);
} catch (Exception e) {
return false;
}
};
}
```
Then use it as: `argThat(jsonEquals(message))`
--
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]
