Copilot commented on code in PR #7796:
URL: https://github.com/apache/incubator-seata/pull/7796#discussion_r2541677625
##########
config/seata-config-consul/src/test/java/org/apache/seata/config/consul/ConsulConfigurationTest.java:
##########
@@ -135,26 +135,36 @@ void testInitSeataConfig() throws Exception {
@Test
void testOnChangeEvent_skipWhenValueIsBlank() throws InterruptedException {
-
String dataId = "seata.properties";
- ConsulConfiguration.ConsulListener listener = new
ConsulConfiguration.ConsulListener(dataId, null);
+ // Mock the initial call in ConsulListener constructor (2-arg version)
+ GetValue initValue = mock(GetValue.class);
+ when(initValue.getDecodedValue()).thenReturn("dummy");
+ Response<GetValue> initResponse = new Response<>(initValue, 1L, false,
1L);
+ when(mockConsulClient.getKVValue(eq(dataId), (String)
isNull())).thenReturn(initResponse);
+
+ // Mock the watch call in onChangeEvent loop (3-arg version)
GetValue blankValue = mock(GetValue.class);
when(blankValue.getDecodedValue()).thenReturn("");
-
Response<GetValue> blankResponse = new Response<>(blankValue, 2L,
false, 2L);
- when(mockConsulClient.getKVValue(eq(dataId), isNull(),
any(QueryParams.class)))
+ when(mockConsulClient.getKVValue(eq(dataId), (String) isNull(),
any(QueryParams.class)))
.thenReturn(blankResponse);
+ ConsulConfiguration.ConsulListener listener = new
ConsulConfiguration.ConsulListener(dataId, null);
+
// Run onChangeEvent in a separate thread since it loops indefinitely
- Thread thread = new Thread(() -> listener.onChangeEvent(new
ConfigurationChangeEvent()));
+ Thread thread = new Thread(() -> {
+ try {
+ listener.onChangeEvent(new ConfigurationChangeEvent());
+ } catch (Exception e) {
+ // ignore
Review Comment:
The catch block silently swallows all exceptions without any logging or
documentation of expected exceptions. This makes debugging test failures
difficult. Consider either: (1) catching only the expected
`InterruptedException` and rethrowing other exceptions, or (2) at minimum,
adding a comment explaining why all exceptions are safely ignored in this test
context.
```suggestion
} catch (InterruptedException e) {
// Expected interruption during test; safe to ignore.
} catch (Exception e) {
// Log unexpected exceptions to aid debugging.
e.printStackTrace();
```
##########
config/seata-config-consul/src/test/java/org/apache/seata/config/consul/ConsulConfigurationTest.java:
##########
@@ -135,26 +135,36 @@ void testInitSeataConfig() throws Exception {
@Test
void testOnChangeEvent_skipWhenValueIsBlank() throws InterruptedException {
-
String dataId = "seata.properties";
- ConsulConfiguration.ConsulListener listener = new
ConsulConfiguration.ConsulListener(dataId, null);
+ // Mock the initial call in ConsulListener constructor (2-arg version)
+ GetValue initValue = mock(GetValue.class);
+ when(initValue.getDecodedValue()).thenReturn("dummy");
+ Response<GetValue> initResponse = new Response<>(initValue, 1L, false,
1L);
+ when(mockConsulClient.getKVValue(eq(dataId), (String)
isNull())).thenReturn(initResponse);
+
+ // Mock the watch call in onChangeEvent loop (3-arg version)
GetValue blankValue = mock(GetValue.class);
when(blankValue.getDecodedValue()).thenReturn("");
-
Response<GetValue> blankResponse = new Response<>(blankValue, 2L,
false, 2L);
- when(mockConsulClient.getKVValue(eq(dataId), isNull(),
any(QueryParams.class)))
+ when(mockConsulClient.getKVValue(eq(dataId), (String) isNull(),
any(QueryParams.class)))
.thenReturn(blankResponse);
+ ConsulConfiguration.ConsulListener listener = new
ConsulConfiguration.ConsulListener(dataId, null);
+
// Run onChangeEvent in a separate thread since it loops indefinitely
- Thread thread = new Thread(() -> listener.onChangeEvent(new
ConfigurationChangeEvent()));
+ Thread thread = new Thread(() -> {
+ try {
+ listener.onChangeEvent(new ConfigurationChangeEvent());
+ } catch (Exception e) {
+ // ignore
+ }
+ });
thread.start();
Thread.sleep(100);
- // Interrupt to break the loop
thread.interrupt();
thread.join(500);
- // Assert: Test passes as long as no exceptions are thrown
assertTrue(true);
Review Comment:
This assertion is meaningless and doesn't verify the actual test behavior.
The test should verify that the blank value was properly skipped (e.g., by
checking that no exception was thrown, or by verifying the mock interactions).
Consider using `verify(mockConsulClient, times(1)).getKVValue(eq(dataId),
(String) isNull())` to confirm the constructor call was made, or simply remove
the assertion if the test passing without exceptions is sufficient.
```suggestion
// Verify that the initial call in the constructor was made once
org.mockito.Mockito.verify(mockConsulClient,
org.mockito.Mockito.times(1))
.getKVValue(eq(dataId), (String) isNull());
// Verify that the watch call in onChangeEvent loop was made at
least once
org.mockito.Mockito.verify(mockConsulClient,
org.mockito.Mockito.atLeastOnce())
.getKVValue(eq(dataId), (String) isNull(),
any(QueryParams.class));
```
--
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]
